[Bug 628429] Add support for DivX XSUB subtitles

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=628429
  GStreamer | gst-plugins-bad | unspecified

Sebastian Dröge <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #248965|none                        |needs-work
             status|                            |

--- Comment #14 from Sebastian Dröge <slomo at circular-chaos.org> 2013-07-12 08:44:46 UTC ---
Review of attachment 248965:
 --> (https://bugzilla.gnome.org/review?bug=628429&attachment=248965)

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)

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.