After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 628429 - Add support for DivX XSUB subtitles
Add support for DivX XSUB subtitles
Status: RESOLVED OBSOLETE
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
unspecified
Other Linux
: Normal enhancement
: git master
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2010-08-31 15:39 UTC by Bastien Nocera
Modified: 2018-11-03 13:06 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
test.divx (950.00 KB, application/octet-stream)
2010-08-31 15:39 UTC, Bastien Nocera
  Details
Preliminary patch adding XSUB support (32.07 KB, patch)
2010-12-01 13:41 UTC, Reynaldo H. Verdejo Pinochet
needs-work Details | Review
xsub decoder and mixer (34.64 KB, patch)
2013-07-11 18:09 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
Patch updated (34.71 KB, patch)
2013-07-11 20:41 UTC, Reynaldo H. Verdejo Pinochet
needs-work Details | Review
Sebastian's review changes (35.05 KB, patch)
2013-07-12 21:33 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
Latest changes (35.14 KB, patch)
2013-07-13 01:28 UTC, Reynaldo H. Verdejo Pinochet
none Details | Review
Latest changes (35.12 KB, patch)
2013-07-13 02:10 UTC, Reynaldo H. Verdejo Pinochet
needs-work Details | Review

Description Bastien Nocera 2010-08-31 15:39:52 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)
Comment 2 Reynaldo H. Verdejo Pinochet 2010-12-01 13:41:24 UTC
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
Comment 3 Stefan Sauer (gstreamer, gtkdoc dev) 2011-03-02 11:28:10 UTC
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.
Comment 4 Stefan Sauer (gstreamer, gtkdoc dev) 2011-03-02 11:29:12 UTC
Forget my comment regarding test conntent. Missed the attachment :/
Comment 5 bcxa.sz 2011-08-25 05:41:52 UTC
Just one question: is it working now? Or still some effort to be done?
Comment 6 bcxa.sz 2011-08-25 06:50:55 UTC
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.
Comment 7 Edward Hervey 2012-06-18 09:54:54 UTC
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 ?
Comment 8 ausante.teamglobal 2012-06-25 05:56:58 UTC
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..
Comment 9 ausante.teamglobal 2012-06-25 07:38:14 UTC
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...
Comment 10 Tim-Philipp Müller 2012-06-26 13:11:13 UTC
> 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.
Comment 11 Reynaldo H. Verdejo Pinochet 2013-07-11 18:09:52 UTC
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 ;)
Comment 12 Reynaldo H. Verdejo Pinochet 2013-07-11 18:12:25 UTC
For individual commits you can take a look at:

http://cgit.collabora.com/git/user/reynaldo/gst-plugins-bad/log/?h=xsub
Comment 13 Reynaldo H. Verdejo Pinochet 2013-07-11 20:41:40 UTC
Created attachment 248965 [details] [review]
Patch updated

And now one that actually builds :P (sry, previous patch had a 1 off changeset)
Comment 14 Sebastian Dröge (slomo) 2013-07-12 08:44:46 UTC
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)
Comment 15 Reynaldo H. Verdejo Pinochet 2013-07-12 21:32:04 UTC
(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!
Comment 16 Reynaldo H. Verdejo Pinochet 2013-07-12 21:33:42 UTC
Created attachment 249051 [details] [review]
 Sebastian's review changes
Comment 17 Reynaldo H. Verdejo Pinochet 2013-07-13 01:22:11 UTC
(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!
Comment 18 Reynaldo H. Verdejo Pinochet 2013-07-13 01:28:50 UTC
Created attachment 249057 [details] [review]
Latest changes
Comment 19 Reynaldo H. Verdejo Pinochet 2013-07-13 02:10:55 UTC
Created attachment 249058 [details] [review]
Latest changes
Comment 20 Reynaldo H. Verdejo Pinochet 2013-07-13 02:13:26 UTC
(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.
Comment 21 Sebastian Dröge (slomo) 2013-07-15 08:14:36 UTC
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
Comment 22 Sun 2013-10-19 11:28:56 UTC
Is there a way we can use this element with playbin2 ?
Comment 23 Sebastian Dröge (slomo) 2013-10-30 18:24:40 UTC
Reynaldo, any news on this?
Comment 24 Reynaldo H. Verdejo Pinochet 2013-11-02 23:13:25 UTC
(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.
Comment 25 Alan Pater 2014-03-12 01:58:12 UTC
Is this the same as https://bugzilla.gnome.org/show_bug.cgi?id=583267
Comment 26 Sebastian Dröge (slomo) 2014-03-12 08:12:05 UTC
(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.
Comment 27 Alan Pater 2014-10-22 19:05:39 UTC
test.avi does not display subtitles on Ubuntu 14.04
Comment 28 GStreamer system administrator 2018-11-03 13:06:34 UTC
-- 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.