Bug 34372 – [GTK] Position queries are lagging

• Component: Media
• Status: RESOLVED FIXED

[Sebastian Dröge (slomo)]

Hi,
when trying the "introduction to html 5" video on youtube with the HTML5 beta stuff enabled, the position queries are not performed very often unless the video is only partially visible or completely hidden. It looks like the main loop is quite busy or the position queries have a too low priority.

This can be made visible by adding a printf() in the position querying function or simply by looking at the position of the video on the website.

[Philippe Normand]

If I play the video there http://htmlfive.appspot.com/static/video.html

currentTime is called about 30 times per second... This is quite a lot indeed :(

[Philippe Normand]

(In reply to comment #1)
> If I play the video there http://htmlfive.appspot.com/static/video.html
> 
> currentTime is called about 30 times per second... This is quite a lot indeed
> :(

This is when the mouse is over the video and the controls visible. When the controls are not displayed currentTime is much less called

[Sebastian Dröge (slomo)]

The problem is not that it's called too often... but not often enough!

Just try it with http://www.youtube.com/watch?v=siOHh0uzcuY (make sure that the browser window is maximized and that the video is large).

[Philippe Normand]

(In reply to comment #3)
> The problem is not that it's called too often... but not often enough!
> 
> Just try it with http://www.youtube.com/watch?v=siOHh0uzcuY (make sure that the
> browser window is maximized and that the video is large).

ok I reproduce the issue on my laptop, 1 core is almost always at 100%... everything is fine if i use the i7... but this is not the average computer configuration ;)

[Philippe Normand]

Created attachment 49285 [details]
proposed patch

This patch fixes the issue on my laptop. Can you try it please? Not
flagging for review yet.

[Philippe Normand]

(In reply to comment #5)
> Created an attachment (id=49285) [details]
> proposed patch
> 
> This patch fixes the issue on my laptop. Can you try it please? Not
> flagging for review yet.

This patch would be a (not so good) workaround for https://bugzilla.gnome.org/show_bug.cgi?id=610830

[Philippe Normand]

Created attachment 49296 [details]
proposed patch

[Philippe Normand]

Comment on attachment 49296 [details]
proposed patch

Unmarking for review, I found that this patch breaks the video-collapse test

[Gustavo Noronha (kov)]

(In reply to comment #8)
> (From update of attachment 49296 [details])
> Unmarking for review, I found that this patch breaks the video-collapse test

This test doesn't pass for me. I believe your proposed change may just be exposing a problem that already exists.

[Gustavo Noronha (kov)]

Created attachment 50433 [details]
normalize SharedTimer timeout priorities

This is part of the fix. It has the nice effect of getting media/video-played-reset.html passing consistently for me (the 100 ms timeout used in the js code was not being called enough).

[Gustavo Noronha (kov)]

Created attachment 50435 [details]
do the same to our g_idle usage for parseDataUrl

[WebKit Review Bot]

Attachment 50433 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/gtk/SharedTimerGtk.cpp:66:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Gustavo Noronha (kov)]

Created attachment 50436 [details]
normalize priorities throughout the media player

[Philippe Normand]

Comment on attachment 50433 [details]
normalize SharedTimer timeout priorities


>--- a/WebCore/platform/gtk/SharedTimerGtk.cpp
>+++ b/WebCore/platform/gtk/SharedTimerGtk.cpp
>@@ -63,10 +63,7 @@ void setSharedTimerFireTime(double fireTime)
>     }
> 
>     stopSharedTimer();
>-    if (intervalInMS == 0)
>-        sharedTimer = g_idle_add(timeout_cb, NULL);
>-    else
>-        sharedTimer = g_timeout_add_full(G_PRIORITY_DEFAULT, intervalInMS, timeout_cb, NULL, NULL);
>+    sharedTimer = g_timeout_add(intervalInMS, timeout_cb, NULL);

What about switching to g_timeout_add_seconds?
http://live.gnome.org/GnomeGoals/UseTimeoutAddSeconds

The word on the streets is that it is much more efficient than g_timeout_add. The Timer API uses seconds anyway.

[Philippe Normand]

(In reply to comment #14)

> What about switching to g_timeout_add_seconds?
> http://live.gnome.org/GnomeGoals/UseTimeoutAddSeconds
> 
> The word on the streets is that it is much more efficient than g_timeout_add.
> The Timer API uses seconds anyway.

Err, wouldn't work as the Timer seconds are double, like 0.25... g_timeout_add_seconds requires unsigned int

[Benjamin Otte]

I think we should come up with an order of priorities and then apply these priorities to all the idles and timeouts properly. (People here seem to just swap g_timeout_add() with g_idle_add() randomly and hope stuff works.)

http://mail.gnome.org/archives/gtk-devel-list/2008-March/msg00184.html has a longer elaboration about that topic and how I solved it in Swfdec. (The Swfdec solution is ok, but far from perfect.)
I'd also be interested in how other browsers prioritize events.

There's a bunch of possible event sources and all of them can take nontrivial amount of time and starve a CPU:
1) user events (can trigger lots of JS that toggles a lot of the document)
2) JS timeouts (can trigger JS, and we all know what that means)
3) reads from data (can trigger extensive relayouting)
4) the media engine (can trigger expensive repaints)
5) repaint events (shadows!)
Repaints can be triggered by the first 4 as well as the window system.

The most important goal you want to achieve IMO is that user input is handled before anything else. This includes actually handling the event in gtk and the browser (GDK_PRIORITY_EVENTS) and repainting the parts of the screen that need a repaint due to the user input (GDK_PRIORITY_REDRAW). Those are defined in http://git.gnome.org/browse/gtk+/tree/gdk/gdkevents.h#n43 and then you gotta stuf the rest of the events in there to guarantee that this happens but that repaints do not starve important stuff.

No idea how to best achieve this though. Would need to think more about this code to have a clear opinion.

[Eric Seidel (no email)]

Comment on attachment 50433 [details]
normalize SharedTimer timeout priorities

OK.

[Eric Seidel (no email)]

Comment on attachment 50435 [details]
do the same to our g_idle usage for parseDataUrl

Sounds sane to me.

[Eric Seidel (no email)]

Comment on attachment 50436 [details]
normalize priorities throughout the media player

Do we have a Gtk bug on this?  That we would revert these changes after the Gtk fix goes in?

These reviews are mostly just rubber-stamps, as I'm definitely not a Gtk expert.  I trust you to be. :)

[Gustavo Noronha (kov)]

Committed as 560053 through 560055. Given the expected failure of collapsed happened, I skipped it, and opened https://bugs.webkit.org/show_bug.cgi?id=36165.

Answering Eric's question about having a bug in GTK+, yes, here's the relevant comment:

-    // Use HIGH_IDLE+20 priority, like Gtk+ for redrawing operations.
-    priv->timeout_id = g_timeout_add_full(G_PRIORITY_HIGH_IDLE + 20, 0,
+    // This should likely use a lower priority, but glib currently starves
+    // lower priority sources.
+    // See: https://bugzilla.gnome.org/show_bug.cgi?id=610830.

Thanks!