GNOME Bugzilla – Bug 628429
Add support for DivX XSUB subtitles
Last modified: 2018-11-03 13:06:34 UTC
Created attachment 169171 [details] test.divx $ gst-launch playbin2 'uri=file:///home/hadess/Desktop/test.avi' Setting pipeline to PAUSED ... Pipeline is PREROLLING ... WARNING: from element /GstPlayBin2:playbin20/GstURIDecodeBin:uridecodebin0: No decoder available for type 'video/x-avi-unknown, fourcc=(fourcc)DXSB'. Additional debug info: gsturidecodebin.c(712): unknown_type_cb (): /GstPlayBin2:playbin20/GstURIDecodeBin:uridecodebin0 Pipeline is PREROLLED ... Setting pipeline to PLAYING ... New clock: GstPulseSinkClock The file was created with a "normal" avi file, a corresponding srt file, and AviAddXSUBs[1]. This is the "supported" way to embed subtitles in DIVX files for use on the PS3, and other "DivX certified" players. (If the given file isn't good enough, feel free to ask me for another test one) [1]: http://www.calcitapp.com/AVIAddXSubs.php (Windows app, works in WINE)
http://heisenbugs.blogspot.com/2010/11/adding-xsub-support-to-gstreamer.html
Created attachment 175622 [details] [review] Preliminary patch adding XSUB support For those who might want to try it out, current development of the plugin is taking place here: http://git.collabora.co.uk/?p=user/reynaldo/gst-plugins-bad;a=shortlog;h=refs/heads/bgo_628429
Review of attachment 175622 [details] [review]: Having some pointer how to try it would be nice too. Like where to fine a suitable file. ::: gst/xsub/gstxsub.c @@ +51,3 @@ + * <title>Example launch line</title> + * |[ + * gst-launch -v -m fakesrc ! xsub ! fakesink show-background=FALSE It would be nice to have a proper example. E.g. fakesink has no show-background, but xsub seems to have. @@ +80,3 @@ +{ + PROP_0, + PROP_SHOWBG just spell it out -> PROP_SHOW_BACKGROUND @@ +342,3 @@ + } + + g_static_mutex_unlock (&filter->lock); shoudl the unlock go up before the preceding "if (spu) {". The lock is protecting the queue and there is no queue access in the "if (spu) {" block. @@ +381,3 @@ + parsed->coords[0], parsed->coords[1], parsed->coords[2], + parsed->coords[3], parsed->buf->size); + g_static_mutex_unlock (&filter->lock); I guess the unlock could go before the logging. ::: gst/xsub/gstxsub.h @@ +75,3 @@ + GQueue *spu_queue; + + GStaticMutex lock; Add a comment that the lock protects the queue.
Forget my comment regarding test conntent. Missed the attachment :/
Just one question: is it working now? Or still some effort to be done?
I patched the as below: [root@fedora14 gst-plugins-bad-0.10.21]# patch -p1 < ../0001-First-working-version-of-XSUB-decoding-and-bliting-p.patch patching file configure.ac Hunk #1 FAILED at 345. Hunk #2 FAILED at 1770. 2 out of 2 hunks FAILED -- saving rejects to file configure.ac.rej patching file gst/xsub/Makefile.am patching file gst/xsub/TODO patching file gst/xsub/gstxsub.c patching file gst/xsub/gstxsub.h patching file gst/xsub/xsub.c patching file gst/xsub/xsub.h It seemed that this patch has been obsolete on gst-plugins-bad-0.10.21.
Reynaldo, looks like the overlay branch in your repo is more recent that the patch attached here. It rebases quite cleanly on current 0.10. Do you have any plans in pushing this ? What's the difference between the two branches in your repo ?
Hi Reynaldo & Edward Kindly share the patch to support 'xsub' subtitle codec that can be applied over 'gst-plugins-bad' (0.10.23 and above) In the official distribution there is no 'xsub' plugin in 'gst-plugins-bad' category (0.10.23). But there do exist one more element 'divx' but this element is not installed because there are dependencies for this element (unsure what those dependencies are and if 'divx' contains support for xsub also) ? Thanks to clarify our understanding..
One more observation that the patch when applied reports HUNK failure. After applying patch if run ./configure, it still doesn't list xsub in the list of plugins to be built or not to be built. make and make install still builds the original set of plugins before applying patch. Kindly suggest...
> Reynaldo, looks like the overlay branch in your repo is more recent that the > patch attached here. > > It rebases quite cleanly on current 0.10. Do you have any plans in pushing this > ? What's the difference between the two branches in your repo ? Reynaldo was at some point working on something akin to the blending functionality that's now in libgstvideo as part of the GstVideoOverlayComposition API. I believe the overlay branch is the xsub element plus an experimental prototype of such an API. The element should be ported to the now-existing API for that ideally.
Created attachment 248955 [details] [review] xsub decoder and mixer Attaching a new 1.0 version ready for review. There's still work to be done. Mainly on the spu bliting itself that should be using the libgstvideo api but element as it is right now works and I'd like to push it to -bad for once and for all. It's being a while ;)
For individual commits you can take a look at: http://cgit.collabora.com/git/user/reynaldo/gst-plugins-bad/log/?h=xsub
Created attachment 248965 [details] [review] Patch updated And now one that actually builds :P (sry, previous patch had a 1 off changeset)
Review of attachment 248965 [details] [review]: Thanks for updating this to 1.0 :) In general, compare this to the assrender code and take a lot from there, specific comments below: ::: gst/xsub/Makefile.am @@ +26,3 @@ + $(GST_PLUGINS_BASE_LIBS) -lgstvideo-$(GST_API_VERSION) +libgstxsub_la_LDFLAGS = $(GST_PLUGIN_LDFLAGS) +libgstxsub_la_LIBTOOLFLAGS = --tag=disable-static $(GST_PLUGIN_LIBTOOLFLAGS) instead @@ +29,3 @@ + +# headers we need but don't want installed +# noinst_HEADERS = gstplugin.h Put xsub.h and gstxsub.h here instead of in SOURCES and remove gstplugin.h ::: gst/xsub/gstxsub.c @@ +133,3 @@ + gst_element_class_set_metadata (element_class, + "XSub", + "FIXME:Generic", Mixer/Video/Overlay/Subtitle @@ +148,3 @@ + + g_object_class_install_property (gobject_class, PROP_SHOWBG, + g_param_spec_boolean ("show-background", "Showbg", Showbg -> Show Background @@ +149,3 @@ + g_object_class_install_property (gobject_class, PROP_SHOWBG, + g_param_spec_boolean ("show-background", "Showbg", + "Show SPU Background ?", DEFAULT_SHOWBG, G_PARAM_READWRITE)); Add G_PARAM_STATIC_STRINGS @@ +167,3 @@ + filter->video_sink = gst_pad_new_from_static_template (&sink_factory_pict, + "video_sink"); + GST_PAD_SET_PROXY_CAPS (filter->video_sink); You probably also want GST_PAD_FLAG_PROXY_ALLOCATION and GST_PAD_FLAG_PROXY_SCHEDULING for the video_sink @@ +193,3 @@ +xsub_sink_event_spu (GstPad * pad, GstObject * parent, GstEvent * event) +{ + return gst_pad_event_default (pad, parent, event); This is not dropping everything btw, by default many events will be send to the srcpad Look at assrender for the event handling @@ +206,3 @@ + gst_event_parse_caps (event, &caps); + if (!gst_xsub_set_caps (pad, caps)) + return FALSE; You leak the event here on failures @@ +251,3 @@ +/* This function handles the link with other elements */ +static gboolean +gst_xsub_set_caps (GstPad * pad, GstCaps * caps) Maybe call this a bit different as it's only for the video pad. Also the comment above is wrong @@ +256,3 @@ + GstStructure *structure = gst_caps_get_structure (caps, 0); + + filter = GST_XSUB (gst_pad_get_parent (pad)); You already had the parent in the event function, just pass it through here @@ +259,3 @@ + + gst_structure_get_int (structure, "width", &filter->width); + gst_structure_get_int (structure, "height", &filter->height); Maybe use gst_video_info_from_caps() here and store a GstVideoInfo in your instance struct @@ +291,3 @@ + + if (GST_BUFFER_TIMESTAMP (buf) >= GST_BUFFER_TIMESTAMP (spu->buf)) { + if (GST_BUFFER_TIMESTAMP (buf) <= GST_BUFFER_TIMESTAMP (spu->buf) + spu->buf could also be after buf and should be rendered here too if timestamp+duration is smaller than the timestamp+duration of the video buffer. Take a look at assrender @@ +310,3 @@ + GST_DEBUG_OBJECT (pad, "Have SPU data for this frame"); + buf = gst_buffer_make_writable (buf); + if (!gst_buffer_map (buf, &pict_map, GST_MAP_WRITE)) { You want GST_MAP_READWRITE here and better use the GstVideoFrame API @@ +339,3 @@ + } + + /* FIXME: This should use the new overlay API */ Yes :) @@ +407,3 @@ + parsed->coords[3], gst_buffer_get_size (parsed->buf)); + g_mutex_unlock (&filter->lock); + } You need to implement some waiting/synchronization here, otherwise you'll just accept the complete stream and use a lot of memory. See assrender code ::: gst/xsub/gstxsub.h @@ +75,3 @@ + gboolean show_bg; + + GQueue *spu_queue; You can also store the GQueue directly (not as a pointer)
(In reply to comment #14) > Review of attachment 248965 [details] [review]: > > Thanks for updating this to 1.0 :) In general, compare this to the assrender > code and take a lot from there, specific comments below: Will do. Thanks for the hint. > ::: gst/xsub/Makefile.am > @@ +26,3 @@ > + $(GST_PLUGINS_BASE_LIBS) -lgstvideo-$(GST_API_VERSION) > +libgstxsub_la_LDFLAGS = $(GST_PLUGIN_LDFLAGS) > +libgstxsub_la_LIBTOOLFLAGS = --tag=disable-static > > $(GST_PLUGIN_LIBTOOLFLAGS) instead Done > @@ +29,3 @@ > + > +# headers we need but don't want installed > +# noinst_HEADERS = gstplugin.h > > Put xsub.h and gstxsub.h here instead of in SOURCES and remove gstplugin.h Done > ::: gst/xsub/gstxsub.c > @@ +133,3 @@ > + gst_element_class_set_metadata (element_class, > + "XSub", > + "FIXME:Generic", > > Mixer/Video/Overlay/Subtitle Done > @@ +148,3 @@ > + > + g_object_class_install_property (gobject_class, PROP_SHOWBG, > + g_param_spec_boolean ("show-background", "Showbg", > > Showbg -> Show Background Done > @@ +149,3 @@ > + g_object_class_install_property (gobject_class, PROP_SHOWBG, > + g_param_spec_boolean ("show-background", "Showbg", > + "Show SPU Background ?", DEFAULT_SHOWBG, G_PARAM_READWRITE)); > > Add G_PARAM_STATIC_STRINGS Done > @@ +167,3 @@ > + filter->video_sink = gst_pad_new_from_static_template (&sink_factory_pict, > + "video_sink"); > + GST_PAD_SET_PROXY_CAPS (filter->video_sink); > > You probably also want GST_PAD_FLAG_PROXY_ALLOCATION and > GST_PAD_FLAG_PROXY_SCHEDULING for the video_sink Yes. Thanks. Done. > @@ +193,3 @@ > +xsub_sink_event_spu (GstPad * pad, GstObject * parent, GstEvent * event) > +{ > + return gst_pad_event_default (pad, parent, event); > > This is not dropping everything btw, by default many events will be send to the > srcpad Yeah, dropping the misleading comment for now. I actually might need to handle some of these events after I implement the missing synchronization to not parse all spu buffers at once. > > Look at assrender for the event handling > > @@ +206,3 @@ > + gst_event_parse_caps (event, &caps); > + if (!gst_xsub_set_caps (pad, caps)) > + return FALSE; > > You leak the event here on failures Fixed. Thanks. > @@ +251,3 @@ > +/* This function handles the link with other elements */ > +static gboolean > +gst_xsub_set_caps (GstPad * pad, GstCaps * caps) > > Maybe call this a bit different as it's only for the video pad. Also the > comment above is wrong Misleading comment dropped. Name changed. > @@ +256,3 @@ > + GstStructure *structure = gst_caps_get_structure (caps, 0); > + > + filter = GST_XSUB (gst_pad_get_parent (pad)); > > You already had the parent in the event function, just pass it through here Assrender seems to do the same. I's there a particular justification for this change? > @@ +259,3 @@ > + > + gst_structure_get_int (structure, "width", &filter->width); > + gst_structure_get_int (structure, "height", &filter->height); > > Maybe use gst_video_info_from_caps() here and store a GstVideoInfo in your > instance struct Makes sense but will make the switch latter as an improvement if you don't mind. > @@ +291,3 @@ > + > + if (GST_BUFFER_TIMESTAMP (buf) >= GST_BUFFER_TIMESTAMP (spu->buf)) { > + if (GST_BUFFER_TIMESTAMP (buf) <= GST_BUFFER_TIMESTAMP (spu->buf) + > > spu->buf could also be after buf and should be rendered here too if > timestamp+duration is smaller than the timestamp+duration of the video buffer. > Take a look at assrender Done. Please check the logic again though just in case. I will probably add a few tests for timestamps validity too. > @@ +310,3 @@ > + GST_DEBUG_OBJECT (pad, "Have SPU data for this frame"); > + buf = gst_buffer_make_writable (buf); > + if (!gst_buffer_map (buf, &pict_map, GST_MAP_WRITE)) { > > You want GST_MAP_READWRITE here and better use the GstVideoFrame API Went for _READWRITE as suggested but will leave the change to use GstVideoFrame for latter if you don't mind. > @@ +339,3 @@ > + } > + > + /* FIXME: This should use the new overlay API */ > > Yes :) Sure. Plan is to merge this once it passes review and then work on improving it as time allows. This particular change (to make sense) will also end up in me adding support for other pixel formats so there's some work to do beyond just using the new API. > @@ +407,3 @@ > + parsed->coords[3], gst_buffer_get_size (parsed->buf)); > + g_mutex_unlock (&filter->lock); > + } > > You need to implement some waiting/synchronization here, otherwise you'll just > accept the complete stream and use a lot of memory. See assrender code Yes. I'd like to handle this latter as an improvement though if you're OK with it. > ::: gst/xsub/gstxsub.h > @@ +75,3 @@ > + gboolean show_bg; > + > + GQueue *spu_queue; > > You can also store the GQueue directly (not as a pointer) Yup. do you see any benefit from it though? Most of the GQueue funcs I'm using take a *GQueue as param so I went for it to avoid littering the code &s. I have no strong opinion on it though. I'm attaching an updated patch. Please check the repo I just pointed to for the individual commits (if needed). Thanks a lot for your review!
Created attachment 249051 [details] [review] Sebastian's review changes
(In reply to comment #3) Hi Stefan. Took me a while ;) > Review of attachment 175622 [details] [review]: > > Having some pointer how to try it would be nice too. Like where to fine a > suitable file. Included, yes ;) > ::: gst/xsub/gstxsub.c > @@ +51,3 @@ > + * <title>Example launch line</title> > + * |[ > + * gst-launch -v -m fakesrc ! xsub ! fakesink show-background=FALSE > > It would be nice to have a proper example. E.g. fakesink has no > show-background, but xsub seems to have. Done. > @@ +80,3 @@ > +{ > + PROP_0, > + PROP_SHOWBG > > just spell it out -> PROP_SHOW_BACKGROUND Done. > @@ +342,3 @@ > + } > + > + g_static_mutex_unlock (&filter->lock); > > shoudl the unlock go up before the preceding "if (spu) {". The lock is > protecting the queue and there is no queue access in the "if (spu) {" block. There's queue data access through spu. Can you take another look though? code has changed quite a bit. > @@ +381,3 @@ > + parsed->coords[0], parsed->coords[1], parsed->coords[2], > + parsed->coords[3], parsed->buf->size); > + g_static_mutex_unlock (&filter->lock); > > I guess the unlock could go before the logging. Please take another look as per the above comment ^ > ::: gst/xsub/gstxsub.h > @@ +75,3 @@ > + GQueue *spu_queue; > + > + GStaticMutex lock; > > Add a comment that the lock protects the queue. Ended up changing the var name to make it clear. Works for you? Thanks for your review!
Created attachment 249057 [details] [review] Latest changes
Created attachment 249058 [details] [review] Latest changes
(In reply to comment #15) > (In reply to comment #14) > > Review of attachment 248965 [details] [review] [details]: > [...] > > > @@ +256,3 @@ > > + GstStructure *structure = gst_caps_get_structure (caps, 0); > > + > > + filter = GST_XSUB (gst_pad_get_parent (pad)); > > > > You already had the parent in the event function, just pass it through here > > Assrender seems to do the same. I's there a particular > justification for this change? Done. > [...] > > ::: gst/xsub/gstxsub.h > > @@ +75,3 @@ > > + gboolean show_bg; > > + > > + GQueue *spu_queue; > > > > You can also store the GQueue directly (not as a pointer) Done.
Review of attachment 249058 [details] [review]: ::: gst/xsub/gstxsub.c @@ +287,3 @@ + + frame_start = GST_BUFFER_TIMESTAMP (buf); + frame_stop = frame_start + GST_BUFFER_DURATION (buf); You assume here that all video buffers have timestamps and a duration. You need to check and fail if they are not there @@ +304,3 @@ + + if (frame_start <= spu_stop) + break; You can make the check a bit more simple I guess. The spu is to be displayed if (spu_start < frame_stop && spu_stop > frame_start). It is to be dropped if (spu_stop <= frame_start) and you have to wait if (spu_start >= frame_stop). Also please implement synchronization between spu and video stream here, look at assrender for an example. @@ +406,3 @@ + GST_ERROR_OBJECT (pad, "Parsing of header/spu data failed!"); + gst_buffer_unref (parsed->buf); + g_slice_free (GstXSubData, parsed); Shouldn't this return GST_FLOW_ERROR after a few errors in a row then? @@ +411,3 @@ + parsed->bgless_row = g_slice_alloc (parsed->width * XSUB_RGB_BPP); + g_mutex_lock (&filter->spu_queue_lock); + g_queue_push_tail (&filter->spu_queue, parsed); Synchronization with video chain missing here
Is there a way we can use this element with playbin2 ?
Reynaldo, any news on this?
(In reply to comment #23) > Reynaldo, any news on this? Still on my TODO. Should be able to provide a more complete version in a few days.
Is this the same as https://bugzilla.gnome.org/show_bug.cgi?id=583267
(In reply to comment #25) > Is this the same as https://bugzilla.gnome.org/show_bug.cgi?id=583267 No, that bug is about vobsub DVD subtitles.
test.avi does not display subtitles on Ubuntu 14.04
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/25.