Bug 46283 – Allow CSS animations to run at more than 40FPS

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Layout and Rendering

[Noam Rosenthal]

Currently the timer that controls CSS animations is set to pop every 25 milliseconds, which caps CSS animations at 40 FPS.
Instead, the hardcoded 25ms should just be a default, that can be overridden by the platform in build-time.

[Noam Rosenthal]

Created attachment 68402 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 68402 [details]
Patch

I think a better approach would be to adjust this through settings (like DOMTimer, see bug 45362).

Even better, longer-term, would be to tie CSS animation to screen refresh (60Hz is about 17ms).

[Noam Rosenthal]

Yes, the 16ms I put in the Qt build is based on 60Hz. But I'll let each platform decide what to do with its own port;
Can we just let this patch go in, change it to just be 16ms for everyone, or should I add a settings value for this now? I don't really mind :)

[Simon Fraser (smfr)]

I'd prefer Settings; I don't like proliferating #ifdefs. Comments in the DOMTimer bug may be relevant.

[Noam Rosenthal]

Comment on attachment 68402 [details]
Patch

To smfr's request, will put this into settings.

[Noam Rosenthal]

Created attachment 69599 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 69599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69599&action=review

> WebCore/page/Settings.h:204
> +        static void setMinDOMTimerInterval(double); // Interval specified in seconds.        
> +        static void setAnimationTimerDelay(double); // Timer delay specified in seconds.

Comments are strictly worse than code.  Please include the units in the code, and then you don't need the "seconds" comment.

Example:
void setAnimationTimerDelay(double seconds)
would be better, but:

void setAnimationTimerDelaySeconds(double); woudl be even better!

> WebKit/qt/Api/qgraphicswebview.cpp:250
> +    Settings::setAnimationTimerDelay(0.0167);

This constant would be much clearer if documented, and ideally computed from values more human readable.

Also, this method would be much clearer if it mentioned the units in question.

[Simon Fraser (smfr)]

You may also be interested in bug 47447.

[Noam Rosenthal]

> void setAnimationTimerDelaySeconds(double); woudl be even better!
> 
I agree, but I tried to be consistent with setMinDOMTimerInterval. Should I just change both to setMinDOMTimerIntervalSeconds?

[Noam Rosenthal]

Created attachment 75616 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 75616 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75616&action=review

> WebCore/page/animation/AnimationController.cpp:46
> +double AnimationController::s_animationTimerDelay = 0.025;

Please document what this value means.  It's not even clear what the units are (although I'm assuming seconds).

> WebKit/qt/Api/qgraphicswebview.cpp:252
> +    Settings::setAnimationTimerDelay(0.0167);

Please make it clearer what this value means.

[Noam Rosenthal]

(In reply to comment #11)
> (From update of attachment 75616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75616&action=review
> 
> > WebCore/page/animation/AnimationController.cpp:46
> > +double AnimationController::s_animationTimerDelay = 0.025;
> 
> Please document what this value means.  It's not even clear what the units are (although I'm assuming seconds).
> 
> > WebKit/qt/Api/qgraphicswebview.cpp:252
> > +    Settings::setAnimationTimerDelay(0.0167);
> 
> Please make it clearer what this value means.


This is a webkit-wide problem that this patch didn't create; (In reply to comment #11)
> (From update of attachment 75616 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75616&action=review
> 
> > WebCore/page/animation/AnimationController.cpp:46
> > +double AnimationController::s_animationTimerDelay = 0.025;
> 
> Please document what this value means.  It's not even clear what the units are (although I'm assuming seconds).
> 
> > WebKit/qt/Api/qgraphicswebview.cpp:252
> > +    Settings::setAnimationTimerDelay(0.0167);
> 
> Please make it clearer what this value means.

This is a webkit-wide problem that this patch didn't create; Adding the word "seconds" everywhere, which I agree would make things clearer, would make this the only place in the codebase where such notion exists... It's easy for me to change this patch to include 'Seconds', but should we go for clarity or consistency?

[Noam Rosenthal]

*** This bug has been marked as a duplicate of bug 56151 ***