[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

--- Comment #17 from Reynaldo H. Verdejo Pinochet <reynaldo at opendot.cl> 2013-07-13 01:22:11 UTC ---
(In reply to comment #3)

Hi Stefan. Took me a while ;)

> Review of attachment 175622 [details]:
> 
> 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!

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