You need to
before you can comment on or make changes to this bug.
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.
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 :(
(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
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).
(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 ;)
Created an attachment (id=49285) [details]
This patch fixes the issue on my laptop. Can you try it please? Not
flagging for review yet.
(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
Created an attachment (id=49296) [details]
(From update of attachment 49296 [details])
Unmarking for review, I found that this patch breaks the video-collapse test
(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.
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).
Created an attachment (id=50435) [details]
do the same to our g_idle usage for parseDataUrl
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] 
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created an attachment (id=50436) [details]
normalize priorities throughout the media player
(From update of attachment 50433 [details])
>@@ -63,10 +63,7 @@ void setSharedTimerFireTime(double fireTime)
>- if (intervalInMS == 0)
>- sharedTimer = g_idle_add(timeout_cb, NULL);
>- 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?
The word on the streets is that it is much more efficient than g_timeout_add. The Timer API uses seconds anyway.
(In reply to comment #14)
> What about switching to g_timeout_add_seconds?
> 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
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.
(From update of attachment 50433 [details])
(From update of attachment 50435 [details])
Sounds sane to me.
(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. :)
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.