Bug 167720 – CSS animation should use DisplayRefreshMonitor when possible to time the frame rate

• Component: Animations
• Status: RESOLVED INVALID

[Said Abou-Hallawa]

Timing the animation frame rate by using DisplayRefreshMonitor is more accurate than using the system timer.

[Said Abou-Hallawa]

Created attachment 300378 [details]
Patch

[Said Abou-Hallawa]

Created attachment 300380 [details]
Patch

[Said Abou-Hallawa]

<rdar://problem/29516928>

[Simon Fraser (smfr)]

Comment on attachment 300378 [details]
Patch

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

If the original code is in place, why do we need so much new code?

> Source/WebCore/ChangeLog:8
> +        Instead of creating a repeating time for the CSS animation, we should use

time->timer

> Source/WebCore/page/FrameView.h:68
> +// Allow a little more than 60fps to make sure we can at least hit that frame rate.
> +#define MinimumAnimationInterval 1.0 / 6.0 

The value doesn't match the comment.  Also 1/6 is 0.1666...s which is 167ms.

[Tim Horton]

Comment on attachment 300380 [details]
Patch

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

> Source/WebCore/platform/graphics/DisplayRefreshMonitorClient.cpp:29
> -#if USE(REQUEST_ANIMATION_FRAME_DISPLAY_MONITOR)
> +#if USE(DISPLAY_REFRESH_MONITOR)

Maybe we can just leave the USE() alone and change it in a different patch? It makes this one way bigger than it needs to be.

> Source/WebKit2/WebProcess/WebPage/Cocoa/RemoteLayerTreeDisplayRefreshMonitor.mm:-57
> -    setIsActive(true);

This is somewhat terrifying.

[Said Abou-Hallawa]

Created attachment 300384 [details]
Patch

[Build Bot]

Comment on attachment 300384 [details]
Patch

Attachment 300384 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2989405

New failing tests:
animations/keyframes-dynamic.html

[Build Bot]

Created attachment 300387 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Tim Horton]

Comment on attachment 300384 [details]
Patch

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

> Source/WebCore/page/FrameView.h:68
> +#define MinimumAnimationInterval 0.015

We don't usually use defines like this. Also, why is it here? Do people use it other than AnimationController? FrameView.h is ... an odd place.

> Source/WebCore/page/animation/AnimationController.cpp:41
> +#include "DisplayRefreshMonitor.h"
> +#include "DisplayRefreshMonitorManager.h"

Like smfr, I am a bit confused (but I might have caused some of that confusion). You showed us earlier that old Chromium code, and said that it was all in the tree -- if it is, maybe we should fix it instead of making AnimationController depend on DisplayRefreshMonitor directly? Can you link us to that patch so we can observe/discuss/think about it a bit?

> Source/WebCore/page/animation/AnimationController.cpp:373
> +        windowScreenDidChange();

What? Did the window's screen really change?

> Source/WebCore/page/animation/AnimationControllerPrivate.h:168
> +    void windowScreenDidChange();

We don't interleave members and functions like this.

> Source/WebKit2/WebProcess/WebPage/Cocoa/RemoteLayerTreeDisplayRefreshMonitor.mm:-57
> -    setIsActive(true);

This remains terrifying. Why is it ok? Especially as "unrelated clean-up"?

[Said Abou-Hallawa]

Created attachment 300588 [details]
Patch

[Said Abou-Hallawa]

Created attachment 300589 [details]
Patch

[Said Abou-Hallawa]

Created attachment 300591 [details]
Patch

[Said Abou-Hallawa]

This patch is not relevant anymore. CSS animation is now handled by Page::updateRendering(). So it does not need a separate timer/DisplayRefreshMonitor anymore.