Bug 215229 – Step animations invalidate style on every rendering update whether or not they need to

• Component: Animations
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

DocumentTimelinesController::updateAnimationsAndSendEvents() is called on every rendering update. It calls DeclarativeAnimation::tick(), which calls down to KeyframeEffect::invalidate() for every animation that is not suspended, unconditionally.

That means that every rendering update (which may be triggered by something other than animations) invalidates style, thus causing a style update, which is a power hit.


  * frame #0: 0x000000069a53549c WebCore`WebCore::KeyframeEffect::invalidate(this=0x00000006bbcc6ae0) at KeyframeEffect.cpp:1241:23
    frame #1: 0x000000069a53608d WebCore`WebCore::KeyframeEffect::animationDidTick(this=0x00000006bbcc6ae0) at KeyframeEffect.cpp:1547:5
    frame #2: 0x000000069a53ec3d WebCore`WebCore::WebAnimation::tick(this=0x00000006bbbd3c50) at WebAnimation.cpp:1238:19
    frame #3: 0x000000069a500c65 WebCore`WebCore::DeclarativeAnimation::tick(this=0x00000006bbbd3c50) at DeclarativeAnimation.cpp:68:19
    frame #4: 0x000000069a52b768 WebCore`WebCore::DocumentTimelinesController::updateAnimationsAndSendEvents(this=0x00000006bbc16550, timestamp=(m_value = 24.493000000000002)) at DocumentTimelinesController.cpp:117:24
    frame #5: 0x000000069b8ef2cc WebCore`WebCore::Page::updateRendering(this=0x00000006bbc72a58, document={ origin = file://, url = file:///Volumes/Data/Development/system/webkit/testcontent/scrolling/scrolling-tree/fixed-node-order.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache })::$_23::operator()(WebCore::Document&) const at Page.cpp:1479:34
    frame #6: 0x000000069b8ef233 WebCore`WTF::Detail::CallableWrapper<WebCore::Page::updateRendering()::$_23, void, WebCore::Document&>::call(this=0x00000006bbc72a50, in={ origin = file://, url = file:///Volumes/Data/Development/system/webkit/testcontent/scrolling/scrolling-tree/fixed-node-order.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }) at Function.h:52:39
    frame #7: 0x000000069b8b826a WebCore`WTF::Function<void (WebCore::Document&)>::operator(this=0x00007ffee2261028, in={ origin = file://, url = file:///Volumes/Data/Development/system/webkit/testcontent/scrolling/scrolling-tree/fixed-node-order.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache })(WebCore::Document&) const at Function.h:83:35
    frame #8: 0x000000069b8aa9dc WebCore`WebCore::Page::forEachDocument(this=0x00000006bd9fa000, functor=0x00007ffee2261028)> const&) const at Page.cpp:3109:9
    frame #9: 0x000000069b8b115d WebCore`WebCore::Page::updateRendering(this=0x00000006bd9fa000) at Page.cpp:1474:5
    frame #10: 0x000000010f36a056 WebKit`WebKit::WebPage::updateRendering(this=0x00007f8839009008) at WebPage.cpp:3864:13
    frame #11: 0x000000010edbb4e0 WebKit`WebKit::TiledCoreAnimationDrawingArea::updateRendering(this=0x00000006bbced880, flushType=Normal) at TiledCoreAnimationDrawingArea.mm:459:19
    frame #12: 0x000000010edbfded WebKit`WebKit::TiledCoreAnimationDrawingArea::updateRenderingRunLoopCallback(this=0x00000006bbced880) at TiledCoreAnimationDrawingArea.mm:911:5
    frame #13: 0x000000010edd0168 WebKit`WebKit::TiledCoreAnimationDrawingArea::TiledCoreAnimationDrawingArea(this=0x00000006bbcfafb8)::$_0::operator()() const at TiledCoreAnimationDrawingArea.mm:88:15

[Radar WebKit Bug Importer]

<rdar://problem/66636153>

[Simon Fraser (smfr)]

This is tested by the (failing) animations/steps-transform-rendering-updates.html

[Antti Koivisto]

Created attachment 407395 [details]
wip

[Antti Koivisto]

This bug is specific to step timing functions. Rendering updates will tick animations only if DocumentTimeline::m_animationResolutionScheduled bit is set which correctly doesn't happen during accelerated animations.

Step timing functions with transforms try and fail to start accelerated which causes them to repeatedly schedule unnecessary rendering updates.

[Antti Koivisto]

Created attachment 407397 [details]
patch

[EWS]

Committed r266232: <https://trac.webkit.org/changeset/266232>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407397 [details].

[Simon Fraser (smfr)]

Comment on attachment 407397 [details]
patch

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

> Source/WebCore/animation/KeyframeEffect.cpp:1498
> +    if (m_acceleratedPropertiesState == AcceleratedProperties::None || m_someKeyframesUseStepsTimingFunction || is<StepsTimingFunction>(timingFunction()))

It's GraphicsLayerCA that decides that it can't accelerate animations with steps() timing functions, so adding this code here is spreading that logic around into two places. Not great.

[Antti Koivisto]

GraphicsLayerCA is a totally wrong place for that sort of logic. Duplication can be removed after the legacy animation code is removed.

[Simon Fraser (smfr)]

But the idea is that some native platforms might be able to support steps() natively. You're hard-coding Core Animation limitations into cross-platform code.

[Antti Koivisto]

We should have a better way to communicate that sort of information to the animation engine (when we actually have a reason to abstract it).

[Antoine Quint]

We can probably have this logic sit somewhere in between GraphicsLayerCA and KeyframeEffect that would allow for some platform-specific logic.

[Antti Koivisto]

Specific accelerated properties are already hardcoded in animation code. This seems like a much bigger deal if we want to be (unnecessarily?) generic.