Summary: | [GTK] Position queries are lagging | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sebastian Dröge (slomo) <slomo> | ||||||||||||
Component: | Media | Assignee: | Nobody <webkit-unassigned> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | eric, evan, gustavo, otte, pnormand, webkit.review.bot, xan.lopez | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | OS X 10.5 | ||||||||||||||
Attachments: |
|
Description
Sebastian Dröge (slomo)
2010-01-30 01:22:36 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 :( (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 attachment 49285 [details]
proposed patch
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] > 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 attachment 49296 [details]
proposed patch
Comment on attachment 49296 [details]
proposed patch
Unmarking for review, I found that this patch breaks the video-collapse test
(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. 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).
Created attachment 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] [5]
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 50436 [details]
normalize priorities throughout the media player
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. (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 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 on attachment 50433 [details]
normalize SharedTimer timeout priorities
OK.
Comment on attachment 50435 [details]
do the same to our g_idle usage for parseDataUrl
Sounds sane to me.
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. :)
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! |