Bug 34372 - [GTK] Position queries are lagging
: [GTK] Position queries are lagging
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Media Elements
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Nobody
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-30 01:22 PST by Sebastian Dröge (slomo)
Modified: 2010-03-16 05:13 PDT (History)
7 users (show)

See Also:


Attachments
proposed patch (1.84 KB, patch)
2010-02-23 06:25 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
proposed patch (2.58 KB, patch)
2010-02-23 08:44 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
normalize SharedTimer timeout priorities (1.96 KB, patch)
2010-03-10 13:55 PST, Gustavo Noronha (kov)
eric: review+
gns: commit‑queue-
Details | Formatted Diff | Diff
do the same to our g_idle usage for parseDataUrl (1.88 KB, patch)
2010-03-10 13:59 PST, Gustavo Noronha (kov)
eric: review+
gns: commit‑queue-
Details | Formatted Diff | Diff
normalize priorities throughout the media player (6.08 KB, patch)
2010-03-10 14:02 PST, Gustavo Noronha (kov)
eric: review+
gns: commit‑queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sebastian Dröge (slomo) 2010-01-30 01:22:36 PST
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.
Comment 1 Philippe Normand 2010-02-23 02:34:34 PST
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 :(
Comment 2 Philippe Normand 2010-02-23 02:55:22 PST
(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
Comment 3 Sebastian Dröge (slomo) 2010-02-23 05:29:58 PST
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).
Comment 4 Philippe Normand 2010-02-23 05:50:39 PST
(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 ;)
Comment 5 Philippe Normand 2010-02-23 06:25:22 PST
Created attachment 49285 [details]
proposed patch

This patch fixes the issue on my laptop. Can you try it please? Not
flagging for review yet.
Comment 6 Philippe Normand 2010-02-23 07:54:05 PST
(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
Comment 7 Philippe Normand 2010-02-23 08:44:35 PST
Created attachment 49296 [details]
proposed patch
Comment 8 Philippe Normand 2010-03-05 00:14:36 PST
Comment on attachment 49296 [details]
proposed patch

Unmarking for review, I found that this patch breaks the video-collapse test
Comment 9 Gustavo Noronha (kov) 2010-03-10 13:13:11 PST
(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.
Comment 10 Gustavo Noronha (kov) 2010-03-10 13:55:43 PST
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).
Comment 11 Gustavo Noronha (kov) 2010-03-10 13:59:21 PST
Created attachment 50435 [details]
do the same to our g_idle usage for parseDataUrl
Comment 12 WebKit Review Bot 2010-03-10 14:00:37 PST
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.
Comment 13 Gustavo Noronha (kov) 2010-03-10 14:02:45 PST
Created attachment 50436 [details]
normalize priorities throughout the media player
Comment 14 Philippe Normand 2010-03-11 00:59:13 PST
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.
Comment 15 Philippe Normand 2010-03-11 01:32:52 PST
(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
Comment 16 Benjamin Otte 2010-03-15 08:51:25 PDT
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.
Comment 17 Eric Seidel 2010-03-15 16:18:22 PDT
Comment on attachment 50433 [details]
normalize SharedTimer timeout priorities

OK.
Comment 18 Eric Seidel 2010-03-15 16:19:09 PDT
Comment on attachment 50435 [details]
do the same to our g_idle usage for parseDataUrl

Sounds sane to me.
Comment 19 Eric Seidel 2010-03-15 16:20:26 PDT
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. :)
Comment 20 Gustavo Noronha (kov) 2010-03-16 05:13:28 PDT
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!