The demos at the WebGL repository run at hundreds of FPS on Mac. They use requestAnimationFrame, which is not throttled and should be.
<rdar://problem/10054266>
We should also throttle to 1 fps or below in inactive tabs.
(In reply to comment #2) > We should also throttle to 1 fps or below in inactive tabs. Please read the discussion on public-web-perf@w3.org about this. Currently in Chrome we throttle inactive tabs completely, but Firefox fires at a very low rate. We've considered moving to a 0.1Hz throttle for inactive tabs, but haven't done this yet.
Created attachment 106946 [details] Patch
Comment on attachment 106946 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106946&action=review > Source/WebCore/dom/ScriptedAnimationController.cpp:40 > +#define MaximumAnimationInterval 0.015 This is a minimum interval, not a maximum > Source/WebCore/dom/ScriptedAnimationController.cpp:142 > + double scheduleTime = MaximumAnimationInterval - (currentTime() - m_timeSinceLastAnimationFrame); scheduleTime is not an absolute time, it's an interval. The name should reflect this. Call it 'delay' maybe. Is currentTime() the right thing to use? I thought we had a "time for all the current animations" value. I find this hard to read. Is m_timeSinceLastAnimationFrame just there for clamping? If so, what's MaximumAnimationInterval for? m_timeSinceLastAnimationFrame is also poorly named. It's not an interval, it's an absolute time. > Source/WebCore/dom/ScriptedAnimationController.cpp:155 > + m_timeSinceLastAnimationFrame = currentTime(); Is currentTime() the right thing to use again?
(In reply to comment #5) > (From update of attachment 106946 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106946&action=review > > > Source/WebCore/dom/ScriptedAnimationController.cpp:40 > > +#define MaximumAnimationInterval 0.015 > > This is a minimum interval, not a maximum > > > Source/WebCore/dom/ScriptedAnimationController.cpp:142 > > + double scheduleTime = MaximumAnimationInterval - (currentTime() - m_timeSinceLastAnimationFrame); > > scheduleTime is not an absolute time, it's an interval. The name should reflect this. Call it 'delay' maybe. > > Is currentTime() the right thing to use? I thought we had a "time for all the current animations" value. > > I find this hard to read. Is m_timeSinceLastAnimationFrame just there for clamping? If so, what's MaximumAnimationInterval for? > > m_timeSinceLastAnimationFrame is also poorly named. It's not an interval, it's an absolute time. Everything above has been changed... > > > Source/WebCore/dom/ScriptedAnimationController.cpp:155 > > + m_timeSinceLastAnimationFrame = currentTime(); > > Is currentTime() the right thing to use again? This is what we were using with the old implementation, so I think we should keep it, at least until we get the CSS Animations to fire off rAF.
Created attachment 106949 [details] Patch
Comment on attachment 106949 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106949&action=review > Source/WebCore/dom/ScriptedAnimationController.cpp:145 > + double scheduleDelay = max(MinimumAnimationInterval - (currentTime() - m_lastAnimationFrameTime), 0.0); You can use max<double> to avoid the "0.0". > Source/WebCore/dom/ScriptedAnimationController.h:76 > + Timer<ScriptedAnimationController> m_animationTimer;\ I don't think this compiled.
(In reply to comment #8) > (From update of attachment 106949 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106949&action=review > > > Source/WebCore/dom/ScriptedAnimationController.cpp:145 > > + double scheduleDelay = max(MinimumAnimationInterval - (currentTime() - m_lastAnimationFrameTime), 0.0); Done > > You can use max<double> to avoid the "0.0". > > > Source/WebCore/dom/ScriptedAnimationController.h:76 > > + Timer<ScriptedAnimationController> m_animationTimer;\ > > I don't think this compiled. Yeah, I don't know where that came from. Gone now.
Committed r94908: <http://trac.webkit.org/changeset/94908>
Is there any particular reason why you picked 15ms instead of 16ms (or possibly a repeating pattern of 16,17,17,16,17,17,... to try to match up with 60Hz displays)?
(In reply to comment #11) > Is there any particular reason why you picked 15ms instead of 16ms (or possibly a repeating pattern of 16,17,17,16,17,17,... to try to match up with 60Hz displays)? Well, there's no "matching up" going on. So the best we could do it tune the number to 60hz so we don't, on average, render more times that is being displayed. The problem with setting the number to 16.67ms is that in many cases that will cause you to miss a 60Hz rate due to the overhead of firing the timer and servicing the event. So 15ms mostly avoid renders that will never be seen, but will prevent the overhead from causing a framerate miss.
I disagree this patch. I think each platform (ex> webview) should throttle FPS, not WebCore. I have the counter example. If 2d canvas has heavy animation, it is possible that requestAnimationFrame is called several times per one update of WebView.