Bug 29959

Summary: [GTK] QoS support in the video sink
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, slomo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
proposed patch
none
updated patch
gustavo: review-
updated patch
none
updated patch none

Description Philippe Normand 2009-10-01 06:08:19 PDT
As slomo suggested, VideoSinkGStreamer.cpp should really implement QoS. If the idle
callback is called too late it should simply drop the frame. ("too late"
can be calculated from buffer timestamp, the sinks clock and the buffer
duration or framerate).
Comment 1 Sebastian Dröge (slomo) 2009-10-01 06:17:36 PDT
Too late is (IIRC)

element's base time + buffer time + buffer duration >= element's clock time
Comment 2 Philippe Normand 2009-10-05 04:25:55 PDT
Created attachment 40621 [details]
proposed patch
Comment 3 Sebastian Dröge (slomo) 2009-10-05 04:28:59 PDT
Patch looks good to me but as said on IRC it's only a start, on top of this QoS as I've explained on IRC.

This for now, will make playback go smoother and make sure that audio/video are displayed in sync.
Comment 4 Philippe Normand 2009-10-05 10:39:28 PDT
Created attachment 40640 [details]
updated patch
Comment 5 Gustavo Noronha (kov) 2009-10-08 16:27:52 PDT
Comment on attachment 40640 [details]
updated patch

Awesome stuff, and I'm happy to see the real fix for the race condition happening! What do you think of moving this sink to WebKit/gtk/webkit/ like we did with soup-dialog, by the way? The synchronization and object life time handling look OK to me, I have just a couple nitpicks:

> +    // use HIGH_IDLE+20 priority, like Gtk+ for redrawing operations
> +    priv->timeout_id = g_timeout_add_full(G_PRIORITY_HIGH_IDLE + 20, 0,
> +                                          webkit_video_sink_timeout_func,
> +                                          gst_object_ref(sink),
> +                                          (GDestroyNotify) gst_object_unref);

Additional space on the cast, and I suggest a normal sentence in the comment, with capital U and period =).

>  
>  static void
> -webkit_video_sink_finalize(GObject* object)
> +_unlock_buffer_mutex(WebKitVideoSinkPrivate* priv)
> +{
> +
> +    g_mutex_lock(priv->buffer_mutex);
> +

There seems to be an unnecessary line at the start of this function. Also, I don't think we use this _name pattern anywhere else, so I'd suggest just dropping the underscore here. You are already declaring it static, which should be enough.

Thanks for your work on this!
Comment 6 Philippe Normand 2009-10-09 02:28:06 PDT
Created attachment 40939 [details]
updated patch
Comment 7 Sebastian Dröge (slomo) 2009-10-09 02:41:00 PDT
In the timeout callback, no need to get the timeout id. But set the timeout id to 0 in the callback after taking the mutex!

When unblocking the buffer waiting, you also want to remove the timeout if the id is != 0.
Comment 8 Sebastian Dröge (slomo) 2009-10-09 02:49:21 PDT
Oh, and you want to make the video sink a subclass of GstVideoSink. This will actually set up the QoS/lateness logic in GstBaseSink, suitable for video:

http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-libs/html/gst-plugins-base-libs-gstvideosink.html
Comment 9 Philippe Normand 2009-10-09 03:43:39 PDT
Created attachment 40940 [details]
updated patch
Comment 10 Gustavo Noronha (kov) 2009-10-09 12:15:08 PDT
Comment on attachment 40940 [details]
updated patch

Thanks. And thanks to slomo for the meat of the review =).
Comment 11 WebKit Commit Bot 2009-10-09 12:37:48 PDT
Comment on attachment 40940 [details]
updated patch

Clearing flags on attachment: 40940

Committed r49401: <http://trac.webkit.org/changeset/49401>
Comment 12 WebKit Commit Bot 2009-10-09 12:37:52 PDT
All reviewed patches have been landed.  Closing bug.