Bug 67171 – requestAnimationFrame doesn't throttle on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Chris Marrin]

The demos at the WebGL repository run at hundreds of FPS on Mac. They use requestAnimationFrame, which is not throttled and should be.

[Simon Fraser (smfr)]

<rdar://problem/10054266>

[Dean Jackson]

We should also throttle to 1 fps or below in inactive tabs.

[James Robinson]

(In reply to comment #2)
> We should also throttle to 1 fps or below in inactive tabs.

Please read the discussion on [e-mail] 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.

[Chris Marrin]

Created attachment 106946 [details]
Patch

[Simon Fraser (smfr)]

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?

[Chris Marrin]

(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.

[Chris Marrin]

Created attachment 106949 [details]
Patch

[Simon Fraser (smfr)]

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.

[Chris Marrin]

(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.

[Chris Marrin]

Committed r94908: <http://trac.webkit.org/changeset/94908>

[James Robinson]

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)?

[Chris Marrin]

(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.

[Dongseong Hwang]

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.