Bug 16356 – [GTK] Integrate GStreamer video with the graphics backend

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Alp Toker]

Right now, videos are played in their own top level windows, a default behaviour provided by GStreamer.

We should instead render video in the right places on the web page.

[Alp Toker]

Created attachment 17847 [details]
GStreamer gfx integration and cleanups

This patch isn't quite ready for review, but does work. Have been a bit busy the last couple of days, so haven't finished it off yet. The Sink boilerplate is taken from Clutter with appropriate licensing headers preserved.

Issues (all easy to fix, will look into it soon):

 * Doesn't call repaint(), so will not always repaint transformed videos.

 * Copies data more times than is necessary.

 * Code isn't properly formatted.

 * Several warnings remain in the code since our compiler flags are stricter.

(In future I see the VideoSink possibly becoming part of the public API and integrating with the proposed DOM binding, but for now I think the best place for it is in WebCore.)

Once we have the basics down, we can look at overlay optimisations/XVideo for untransformed content, but this patch doesn't deal with that.

[Alp Toker]

Created attachment 17906 [details]
Integrate video with the graphics backend

(The code still shows that it's been ported from C in places, but looks clean enough.)

[Maciej Stachowiak]

+    int fps_n, fps_d;
+    int par_n, par_d;

This is not good style for struct field declarations. Please change to declaring each separately.

Otherwise r=me assuming testing is ok.

[Alp Toker]

Landed in r28792 with requested changes.

[Zan Dobersek]

Created attachment 26099 [details]
Implementation of some (unimplemented) functions

This patch implements search for supported types and functions for requiring maximum time loaded, known bytes, total bytes, loaded bytes and handling of "an unknown stream" situation.

[Holger Freyther]

Comment on attachment 26099 [details]
Implementation of some (unimplemented) functions


> +        // Splitting the capability by comma and taking the first part
> +        // as capability can be something like "audio/x-wavpack, framed=(boolean)false"
> +        gchar* type = g_strsplit(gst_caps_to_string(caps), ",", 2)[0];
> +


LEAK, LEAK, LEAK! I suffered from this with hald 0.5.9 on my Neo1973. http://library.gnome.org/devel/glib/stable/glib-String-Utility-Functions.html#g-strsplit. Please redo this and use valgrind or memprof to check that you don't leak.

[Zan Dobersek]

Created attachment 26151 [details]
Fix of leaks in previous patch

Fixes leaking problem from the previous patch.

Now also calls gst_init when getting supported types and GStreamer hasn't been initialized yet.

GStreamer supports types that WebKit supports by default or plugins should (e.g. Flash). We do not want to show these supported as media.

[Zan Dobersek]

Created attachment 26163 [details]
Updated fix of leaks in previous patch

Adds a missing gst_query_unref.

[Zan Dobersek]

Created attachment 26611 [details]
Uses enum values instead of magic numbers

Now uses enum values instead of 'magic numbers'.

[Zan Dobersek]

Created attachment 26612 [details]
Uses enum values and gets rid of fprintfs

Now excludes fprintfs that were used for debugging.

[Holger Freyther]

Comment on attachment 26612 [details]
Uses enum values and gets rid of fprintfs


> +    // These subtypes are already beeing supported by WebKit itself
> +    HashSet<String>* ignoredApplicationSubtypes = new HashSet<String>;

^^^^^ leak, leak, leak. Just omit the  '*' and the "= new..."

> +    ignoredApplicationSubtypes->add(String("javascript"));

and turn this into ignoredApplicationSubtypes.add.

[Zan Dobersek]

Created attachment 27249 [details]
Leakage fix

Fixes leakage mentioned above.

[Zan Dobersek]

Created attachment 28391 [details]
Updated patch

The previous patch, which was actually reviewed but never committed, is now outdated because of the (not-so-)recent changes in the media part of WebKit.

This patch keeps up with these changes. Tested on tests in media/ directory, we succeed at 55/60 tests, with 3 failures not actually being WebKit's fault, one depending on the eventSender and one needing the possibility of loading videos in frames.

[Gustavo Noronha (kov)]

Reopening, so that it shows up in the bug list correctly, since we have a new patch here.

[Gustavo Noronha (kov)]

Comment on attachment 17906 [details]
Integrate video with the graphics backend

clearing flag to not show up on the pending commits list

[Gustavo Noronha (kov)]

Comment on attachment 27249 [details]
Leakage fix

clearing flag to not show up on the pending commits list

[Gustavo Noronha (kov)]

Comment on attachment 28391 [details]
Updated patch

> +        Further implement MediaPlayerPrivate class on Gtk port.

I have read the patch, and tested/tried it. It seems to have a few regressions, such as media/video-currentTime-set2.html now failing, and the poster at http://webkit.org/blog/140/html5-media-support/ not showing 'Loading...' anymore when I click play, nor displaying the video (I think this may be actually a step forward, but I would like to have a smaller context to investigate). I think you could split the patch in smaller ones, and we can work through them one at a time. I suggest the following:

 - Reworking GST initialization
 - Additional error check
 - Supported formats
 - Play/pause fixes
 - Total/current bytes and time

I'll be glad to work with you on this, and review the patches.

[Zan Dobersek]

Created attachment 38455 [details]
WIP patch

This is a work-in-progress patch and is therefor not bisected into smaller (and better) patches. It also includes diff of the Skipped list.

This patch only implements functions and applies modifications necessary for most of the layout tests in LayoutTests/media/ directory to pass. With applied patch, 64 tests apply.

Some crashes still occur in webkit_video_sink_idle_func when priv->async_queue is checked. Between g_idle_add_full call in webkit_video_sink_render and webkit_video_sink_idle_func call, a call to webkit_video_sink_dispose is made. This corrupts async queue, therefor crashes occur. In the patch, g_idle_remove_by_data is called in webkit_video_sink_stop to remove these idle calls, but some still appear to slip by.

In MediaPlayerPrivateGStreamer.cpp, setEndTime() implementation has been removed, as it seems this is not used anywhere in WebCore. Maybe not the best idea, as it could get used?
There are plenty of things still awaiting implementation, such as functions maxTimeLoaded, maxTimeSeekable, maxTimeBuffered, ... There is also a bit of a mess in the updateStates function that could use a bit of a cleanup.
Error handling was a bit improved, now calling loadingFailed to the MediaPlayerPrivate with a different error for different error domain that we get in mediaPlayerPrivateErrorCallback.
Also, the patch now uses playbin2 as the factory name when creating m_playBin. playbin2 is surely better that playbin, however, according to gst base plugins reference manual [1] it is, at this moment, considered unstable, but Rhythmbox is already using it since 0.12.2.[2][3]

That's about it, next step are the http/media tests. Comments, suggestions, anything welcome.

[1] http://gstreamer.freedesktop.org/data/doc/gstreamer/head/gst-plugins-base-plugins/html/gst-plugins-base-plugins-playbin2.html
[2] http://bugzilla.gnome.org/show_bug.cgi?id=542922
[3] http://git.gnome.org/cgit/rhythmbox/tree/NEWS#n100

[Gustavo Noronha (kov)]

Comment on attachment 38455 [details]
WIP patch

I like it, keep rocking =)

 222     if (m_errorOccured)
 223         return 0;
 224 
 225     float ret = 0.0;

Since you're initializing it here, you don't need to reset this in the else block bellow this.

Actually, why not do an early return here instead:

225228     if (gst_element_query(m_playBin, query)) {

Make that check to !, print the error log, and return, after unref'ing the object.

> -    GstElement* audioSink = gst_element_factory_make("gconfaudiosink", 0);
> +//    GstElement* audioSink = gst_element_factory_make("gconfaudiosink", 0);
>      m_videoSink = webkit_video_sink_new(m_surface);
>  
> -    g_object_set(m_playBin, "audio-sink", audioSink, NULL);
> +//    g_object_set(m_playBin, "audio-sink", audioSink, NULL);
>      g_object_set(m_playBin, "video-sink", m_videoSink, NULL);

I think this can be removed, indeed, instead of just being commented. We only need to override the video sink. Except for these, I don't see a reason not to r+ this. Anything specific keeping you from setting r? apart from a missing ChangeLog, and lack of 'divinding' the patch into smaller patches? I believe this patch has actually a good size, tbh.

[Zan Dobersek]

Created attachment 38474 [details]
Implementations, modifications, improvements.

Basically the last WIP patch with ChangeLogs, an early return used in MediaPlayerPrivate::currentTime() and audio sink creation and application to the playbin removed, as recommended by kov. Also puts some style back to the media section of the Skipped list (was a mess in WIP patch).

[Gustavo Noronha (kov)]

Comment on attachment 38474 [details]
Implementations, modifications, improvements.

> +            // Splitting the capability by comma and taking the first part
> +            // as capability can be something like "audio/x-wavpack, framed=(boolean)false"
> +            gchar** capability = g_strsplit(gst_caps_to_string(caps), ",", 2);

There's a small leak here - gst_caps_to_string returns a string that needs to be freed. I will turn this into a GOwnPtr when landing.

Other than that, r=me. I'll land it.

[Gustavo Noronha (kov)]

Comment on attachment 38474 [details]
Implementations, modifications, improvements.

Landed as r47710. Is there any reason for this bug to remain open, Zan? If you feel not, please close it =). Clearing flags.