Bug 29959 – [GTK] QoS support in the video sink

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Philippe Normand]

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

[Sebastian Dröge (slomo)]

Too late is (IIRC)

element's base time + buffer time + buffer duration >= element's clock time

[Philippe Normand]

Created attachment 40621 [details]
proposed patch

[Sebastian Dröge (slomo)]

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.

[Philippe Normand]

Created attachment 40640 [details]
updated patch

[Gustavo Noronha (kov)]

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!

[Philippe Normand]

Created attachment 40939 [details]
updated patch

[Sebastian Dröge (slomo)]

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.

[Sebastian Dröge (slomo)]

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

[Philippe Normand]

Created attachment 40940 [details]
updated patch

[Gustavo Noronha (kov)]

Comment on attachment 40940 [details]
updated patch

Thanks. And thanks to slomo for the meat of the review =).

[WebKit Commit Bot]

Comment on attachment 40940 [details]
updated patch

Clearing flags on attachment: 40940

Committed r49401: <http://trac.webkit.org/changeset/49401>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.