Bug 107521 – Don't service animations on both the animation frame callback and animation timer at the same time

• Component: Animations
• Status: RESOLVED LATER

Don't service animations on both the animation frame callback and animation timer at the same time

Created attachment 183926 [details]
Patch

Created attachment 183931 [details]
Patch

[Build Bot]

Comment on attachment 183931 [details]
Patch

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

New failing tests:
compositing/reflections/animation-inside-reflection.html
compositing/layer-creation/overlap-animation-clipping.html
fast/css-generated-content/pseudo-element-events.html
css3/filters/custom/custom-filter-transforms-animation.html
animations/3d/matrix-transform-type-animation.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/visible-rect/animated-from-none.html
compositing/repaint/become-overlay-composited-layer.html
css3/masking/clip-path-animation.html
css3/filters/composited-during-animation-layertree.html
animations/animation-border-overflow.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/animation/animation-compositing.html
compositing/layer-creation/overlap-animation.html
animations/3d/change-transform-in-end-event.html
css3/filters/filter-animation-from-none-multi.html
css3/filters/filter-animation-from-none.html
css3/filters/filter-animation-from-none-multi-hw.html
css3/filters/custom/custom-filter-array-blending.html
fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html
css3/filters/filter-animation-from-none-hw.html
css3/filters/composited-during-animation.html
css3/filters/filter-animation-hw.html
compositing/geometry/partial-layout-update.html
compositing/visible-rect/animated.html
compositing/layer-creation/overlap-animation-container.html
compositing/layer-creation/animation-overlap-with-children.html
compositing/reflections/nested-reflection-animated.html

Servicing of animations in the animation frame callback was added in http://trac.webkit.org/changeset/107575, however I don't believe it ever worked as intended.

In the case of accelerated animations every animation frame callback requested another (see http://trac.webkit.org/browser/trunk/Source/WebCore/page/animation/AnimationController.cpp#L232).

In the case of software animations calls to updateAnimations(..) are triggered by both the animation timer and the animation frame callback.


I believe the failures we're seeing with my patch are because DumpRenderTree (at least for chromium, I will test mac tomorrow) doesn't generate frames unless requested (eg. testRunner.display())

By adding the following to the layout tests I can cause them to pass, however I'm not sure whether it's appropriate to do so:

setInterval(function() { testRunner.display() }, 16);

[WebKit Review Bot]

Comment on attachment 183931 [details]
Patch

Attachment 183931 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16011798

New failing tests:
compositing/reflections/animation-inside-reflection.html
compositing/layer-creation/overlap-animation-clipping.html
animations/3d/matrix-transform-type-animation.html
animations/animation-direction.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/repaint/become-overlay-composited-layer.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
compositing/animation/state-at-end-event-transform-layer.html
animations/animation-direction-reverse-fill-mode.html
compositing/animation/animation-compositing.html
compositing/layer-creation/overlap-animation.html
animations/3d/change-transform-in-end-event.html
animations/animation-direction-reverse-hardware.html
animations/animation-direction-reverse-timing-functions.html
http/tests/eventsource/eventsource-cors-with-credentials.html
animations/3d/transform-perspective.html
animations/animation-drt-api-multiple-keyframes.html
animations/animation-direction-reverse-non-hardware.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
animations/animation-direction-reverse.html
compositing/geometry/partial-layout-update.html
animations/animation-direction-reverse-fill-mode-hardware.html
compositing/layer-creation/overlap-animation-container.html
animations/animation-direction-reverse-hardware-opacity.html
animations/animation-direction-reverse-timing-functions-hardware.html

[Simon Fraser (smfr)]

(In reply to comment #4)
> Servicing of animations in the animation frame callback was added in http://trac.webkit.org/changeset/107575, however I don't believe it ever worked as intended.
> 
> In the case of accelerated animations every animation frame callback requested another (see http://trac.webkit.org/browser/trunk/Source/WebCore/page/animation/AnimationController.cpp#L232).
> 
> In the case of software animations calls to updateAnimations(..) are triggered by both the animation timer and the animation frame callback.
> 
> 
> I believe the failures we're seeing with my patch are because DumpRenderTree (at least for chromium, I will test mac tomorrow) doesn't generate frames unless requested (eg. testRunner.display())
> 
> By adding the following to the layout tests I can cause them to pass, however I'm not sure whether it's appropriate to do so:
> 
> setInterval(function() { testRunner.display() }, 16);

I'm still not sure from this comment what the actual bug is, and how you're trying to fix it.

(In reply to comment #6)

Hi Simon,

Thanks for taking a look.

> I'm still not sure from this comment what the actual bug is,

The bug is that animation servicing is being triggered by both the animation frame callback and a timer inside AnimationController. http://trac.webkit.org/changeset/107575 (Use requestAnimationFrame callbacks to pump CSS animations) added the use of request animation frame, but didn’t modifiy the existing timing mechanism.

This manifests in several different ways:

In the case of software animations it results in a style recalc frequently being triggered twice per frame. (on each frame callback in addition to the 40hz animation timer)

In the case of an animation with a start delay it results in frames being generated even while we are still waiting for the start delay to elapse.

In the case of exclusive accelerated animations it results in constant scheduling of request animation frame while the accelerated animation is running -- results in unnecessary use of the main thread.

> and how you're trying to fix it.

My attempted fix is to combine the scheduling logic between request animation frame and the animation timer so that:
 * if servicing of animations is required immediately we use request animation frame (only)
 * in any other case we use the animation timer

[James Robinson]

Do CSS animation events fire with your patch on backgrounded tabs?

(In reply to comment #8)
> Do CSS animation events fire with your patch on backgrounded tabs?

No, they do not. I can see how in the past the timer would have caused them to fire. I will look into addressing this.

However they also do not fire on background tabs in Chrome Canary or WebKit Nightly... any ideas?

(In reply to comment #9)
 
> However they also do not fire on background tabs in Chrome Canary or WebKit Nightly... any ideas?

I bisected all the way to: Suspend CSS animations for background tabs
https://bugs.webkit.org/show_bug.cgi?id=106940

[James Robinson]

Sounds like that's deliberate behavior, then, so no need to worry about firing the CSS events.

[Build Bot]

Comment on attachment 183931 [details]
Patch

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

New failing tests:
compositing/reflections/animation-inside-reflection.html
compositing/layer-creation/overlap-animation-clipping.html
animations/3d/matrix-transform-type-animation.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/visible-rect/animated-from-none.html
compositing/repaint/become-overlay-composited-layer.html
css3/filters/composited-during-animation-layertree.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
compositing/animation/state-at-end-event-transform-layer.html
compositing/animation/animation-compositing.html
compositing/layer-creation/overlap-animation.html
animations/3d/transform-perspective.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
css3/filters/composited-during-animation.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
compositing/geometry/partial-layout-update.html
compositing/visible-rect/animated.html
compositing/layer-creation/overlap-animation-container.html
compositing/layer-creation/animation-overlap-with-children.html
compositing/reflections/nested-reflection-animated.html

(In reply to comment #11)
> Sounds like that's deliberate behavior, then, so no need to worry about firing the CSS events.

Any suggestions on what to do about the issue running under DumpRenderTree?

Created attachment 185628 [details]
Patch

(In reply to comment #14)
> Created an attachment (id=185628) [details]
> Patch

Latest patch respects USE(REQUEST_ANIMATION_FRAME_TIMER) which I expect will resolve the mac failures. Also experimenting with not suspending animations on hidden pages and servicing animations at events.

Still expect failures on Chromium DRT.

[WebKit Review Bot]

Comment on attachment 185628 [details]
Patch

Attachment 185628 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16260060

New failing tests:
compositing/reflections/animation-inside-reflection.html
compositing/layer-creation/overlap-animation-clipping.html
animations/3d/matrix-transform-type-animation.html
animations/animation-direction.html
animations/3d/state-at-end-event-transform.html
animations/animation-add-events-in-handler.html
animations/additive-transform-animations.html
animations/3d/replace-filling-transform.html
compositing/repaint/become-overlay-composited-layer.html
animations/animation-border-overflow.html
animations/animation-direction-alternate-reverse.html
compositing/animation/state-at-end-event-transform-layer.html
animations/animation-direction-reverse-fill-mode.html
compositing/animation/animation-compositing.html
compositing/layer-creation/overlap-animation.html
animations/3d/change-transform-in-end-event.html
animations/animation-direction-reverse-hardware.html
animations/animation-drt-api.html
animations/animation-direction-reverse-timing-functions.html
animations/3d/transform-perspective.html
animations/animation-drt-api-multiple-keyframes.html
animations/animation-direction-reverse-non-hardware.html
animations/animation-direction-normal.html
animations/3d/transform-origin-vs-functions.html
animations/animation-direction-reverse.html
compositing/geometry/partial-layout-update.html
animations/animation-direction-reverse-fill-mode-hardware.html
compositing/layer-creation/overlap-animation-container.html
animations/animation-direction-reverse-hardware-opacity.html
animations/animation-direction-reverse-timing-functions-hardware.html

[Build Bot]

Comment on attachment 185628 [details]
Patch

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

New failing tests:
fast/loader/image-in-page-cache.html
fast/history/timed-refresh-in-cached-frame.html
css3/filters/composited-during-animation-layertree.html
fast/frames/frame-dead-region.html
fast/loader/stateobjects/popstate-fires-with-page-cache.html
plugins/frameset-with-plugin-frame.html
fast/harness/user-preferred-language.html

[Build Bot]

Comment on attachment 185628 [details]
Patch

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

New failing tests:
fast/loader/image-in-page-cache.html
fast/history/timed-refresh-in-cached-frame.html
css3/filters/composited-during-animation-layertree.html
fast/frames/frame-dead-region.html
fast/harness/results.html
plugins/frameset-with-plugin-frame.html
fast/loader/stateobjects/pushstate-clears-forward-history.html
fast/harness/user-preferred-language.html
fast/dom/Geolocation/not-enough-arguments.html

[Ahmad Saleem]

This seems to have landed in Blink via following:

https://bugs.chromium.org/p/chromium/issues/detail?id=227161#c4

Is it something needed in Safari, there is no test case to verify on how this improve performance or web-compat but just wanted to share updated information.

[Ryosuke Niwa]

We rewrote animation code enough that I highly doubt this patch is relevant.