Bug 67171 - requestAnimationFrame doesn't throttle on Mac
Summary: requestAnimationFrame doesn't throttle on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.7
: P2 Normal
Assignee: Chris Marrin
URL: https://cvs.khronos.org/svn/repos/reg...
Keywords: InRadar
Depends on:
Blocks: 64591
  Show dependency treegraph
 
Reported: 2011-08-29 17:28 PDT by Chris Marrin
Modified: 2012-01-03 17:28 PST (History)
5 users (show)

See Also:


Attachments
Patch (21.37 KB, patch)
2011-09-09 17:32 PDT, Chris Marrin
no flags Details | Formatted Diff | Diff
Patch (21.35 KB, patch)
2011-09-09 18:12 PDT, Chris Marrin
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Marrin 2011-08-29 17:28:17 PDT
The demos at the WebGL repository run at hundreds of FPS on Mac. They use requestAnimationFrame, which is not throttled and should be.
Comment 1 Simon Fraser (smfr) 2011-08-31 11:42:36 PDT
<rdar://problem/10054266>
Comment 2 Dean Jackson 2011-08-31 13:52:39 PDT
We should also throttle to 1 fps or below in inactive tabs.
Comment 3 James Robinson 2011-09-01 11:36:16 PDT
(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.
Comment 4 Chris Marrin 2011-09-09 17:32:59 PDT
Created attachment 106946 [details]
Patch
Comment 5 Simon Fraser (smfr) 2011-09-09 17:43:58 PDT
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?
Comment 6 Chris Marrin 2011-09-09 18:10:24 PDT
(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.
Comment 7 Chris Marrin 2011-09-09 18:12:24 PDT
Created attachment 106949 [details]
Patch
Comment 8 Simon Fraser (smfr) 2011-09-09 18:42:44 PDT
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.
Comment 9 Chris Marrin 2011-09-10 06:58:12 PDT
(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.
Comment 10 Chris Marrin 2011-09-10 07:20:07 PDT
Committed r94908: <http://trac.webkit.org/changeset/94908>
Comment 11 James Robinson 2011-09-12 13:38:19 PDT
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)?
Comment 12 Chris Marrin 2011-09-12 14:23:55 PDT
(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.
Comment 13 Dongseong Hwang 2012-01-03 17:28:58 PST
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.