Bug 34372 - [GTK] Position queries are lagging
: [GTK] Position queries are lagging
Status: RESOLVED FIXED
: WebKit
Media Elements
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2010-01-30 01:22 PST by
Modified: 2010-03-16 05:13 PST (History)


Attachments
proposed patch (1.84 KB, patch)
2010-02-23 06:25 PST, Philippe Normand
no flags Review Patch | Details | Formatted Diff | Diff
proposed patch (2.58 KB, patch)
2010-02-23 08:44 PST, Philippe Normand
no flags Review Patch | 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-
Review Patch | 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-
Review Patch | 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-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 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 From 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 From 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 From 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 From 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 From 2010-02-23 06:25:22 PST -------
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.
------- Comment #6 From 2010-02-23 07:54:05 PST -------
(In reply to comment #5)
> Created an attachment (id=49285) [details] [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 From 2010-02-23 08:44:35 PST -------
Created an attachment (id=49296) [details]
proposed patch
------- Comment #8 From 2010-03-05 00:14:36 PST -------
(From update of attachment 49296 [details])
Unmarking for review, I found that this patch breaks the video-collapse test
------- Comment #9 From 2010-03-10 13:13:11 PST -------
(In reply to comment #8)
> (From update of attachment 49296 [details] [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 From 2010-03-10 13:55:43 PST -------
Created an attachment (id=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 From 2010-03-10 13:59:21 PST -------
Created an attachment (id=50435) [details]
do the same to our g_idle usage for parseDataUrl
------- Comment #12 From 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 From 2010-03-10 14:02:45 PST -------
Created an attachment (id=50436) [details]
normalize priorities throughout the media player
------- Comment #14 From 2010-03-11 00:59:13 PST -------
(From update of attachment 50433 [details])

>--- 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 From 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 From 2010-03-15 08:51:25 PST -------
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 From 2010-03-15 16:18:22 PST -------
(From update of attachment 50433 [details])
OK.
------- Comment #18 From 2010-03-15 16:19:09 PST -------
(From update of attachment 50435 [details])
Sounds sane to me.
------- Comment #19 From 2010-03-15 16:20:26 PST -------
(From update of attachment 50436 [details])
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 From 2010-03-16 05:13:28 PST -------
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!