[Bug 628429] Add support for DivX XSUB subtitles

Fri Jul 12 14:32:12 PDT 2013

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

--- Comment #15 from Reynaldo H. Verdejo Pinochet <reynaldo at opendot.cl> 2013-07-12 21:32:04 UTC ---
(In reply to comment #14)
> Review of attachment 248965 [details]:
> 
> 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!

-- 
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.