Bug 171060 – REGRESSION (r215469): [ios-simulator-wk2] LayoutTest compositing/animation/animation-backing.html is a flaky failure

• Status: RESOLVED
• Resolution: FIXED
• Component: Animations

[Ryan Haddad]

[ios-simulator] LayoutTest compositing/animation/animation-backing.html is a flaky failure

https://build.webkit.org/results/Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/r215557%20(805)/results.html

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=compositing%2Fanimation%2Fanimation-backing.html

--- /Volumes/Data/slave/ios-simulator-10-release-tests-wk2/build/layout-test-results/compositing/animation/animation-backing-expected.txt
+++ /Volumes/Data/slave/ios-simulator-10-release-tests-wk2/build/layout-test-results/compositing/animation/animation-backing-actual.txt
@@ -1 +1 @@
-Saw layer flushes during animation: PASS
+No layer flushes during animation: FAIL

[Ryan Haddad]

It looks like this regressed with "Enable optimized layer flushes on iOS" https://trac.webkit.org/changeset/215469/webkit

[Antti Koivisto]

I'll take a look.

[Simon Fraser (smfr)]

The flush needs to travers tiled layers under animating layers, because we have to recompute the coverage rect as the animation runs.

After your changes, how do we ensure the recomputation of coverage rects on all layers with scrolling etc, which is used to drop backing store outside the viewport, and compute the right tiling for tiled layers?

[Antti Koivisto]

(In reply to Simon Fraser (smfr) from comment #3)
> The flush needs to travers tiled layers under animating layers, because we
> have to recompute the coverage rect as the animation runs.
> 
> After your changes, how do we ensure the recomputation of coverage rects on
> all layers with scrolling etc, which is used to drop backing store outside
> the viewport, and compute the right tiling for tiled layers?

We traverse all descendants if coverage rect changes. For scrolling we take care to invalidate the scrolling layers so coverage gets recomputed. I assume we are missing or misoptimizing some invalidation with animations.

[Antti Koivisto]

We can probably just invalidate all layers with accelerated animations in progress.

[Antti Koivisto]

Created attachment 307750 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 307750 [details]
patch

I don't think you need all this. We already have CommitState.ancestorHasTransformAnimation, why can't you key off of that?

[Antti Koivisto]

Possibly. What I need is answer to "Does this layer have any descendants with accelerated transform animations?" Do we have this somewhere already? 

Cleary I could just traverse the entire layer tree but that is much slower than what the patch does (just traverses the active animations).

[Radar WebKit Bug Importer]

<rdar://problem/31771174>

[Simon Fraser (smfr)]

Should be easy to add "descendant has accelerated transform animation" as a post-traversal bit set. I would prefer to keep this code in GraphicsLayerCA.

[Antti Koivisto]

(In reply to Simon Fraser (smfr) from comment #10)
> Should be easy to add "descendant has accelerated transform animation" as a
> post-traversal bit set. I would prefer to keep this code in GraphicsLayerCA.

Yeah, should be doable.

[Antti Koivisto]

Created attachment 307968 [details]
patch

GraphicsLayerCA only (almost) patch

[Simon Fraser (smfr)]

Comment on attachment 307968 [details]
patch

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

> Source/WebCore/rendering/RenderLayerBacking.cpp:961
> +    bool hasAcceleratedTransformAnimation = renderer().animation().isRunningAcceleratedAnimationOnRenderer(renderer(), CSSPropertyTransform, AnimationBase::Running | AnimationBase::Paused);
> +    m_graphicsLayer->setHasAcceleratedTransformAnimation(hasAcceleratedTransformAnimation);
> +    if (!hasAcceleratedTransformAnimation)

I don't think you need to set this from outside of GraphicsLayer(CA). GraphicsLayerCA::isRunningTransformAnimation() knows this, and GraphicsLayerCA::updateAnimations() can set or clear the state.

[Antti Koivisto]

Created attachment 308117 [details]
patch

[WebKit Commit Bot]

Comment on attachment 308117 [details]
patch

Clearing flags on attachment: 308117

Committed r215750: <http://trac.webkit.org/changeset/215750>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.