[Bug 628429] Add support for DivX XSUB subtitles
GStreamer (bugzilla.gnome.org)
bugzilla at gnome.org
Fri Jul 12 18:22:18 PDT 2013
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.
More information about the gstreamer-bugs
mailing list