Bug 171060 - REGRESSION (r215469): [ios-simulator-wk2] LayoutTest compositing/animation/animation-backing.html is a flaky failure
Summary: REGRESSION (r215469): [ios-simulator-wk2] LayoutTest compositing/animation/an...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Animations (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-20 09:41 PDT by Ryan Haddad
Modified: 2017-04-25 12:20 PDT (History)
5 users (show)

See Also:


Attachments
patch (11.18 KB, patch)
2017-04-21 10:53 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (9.62 KB, patch)
2017-04-24 06:00 PDT, Antti Koivisto
simon.fraser: review-
Details | Formatted Diff | Diff
patch (6.45 KB, patch)
2017-04-25 11:02 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryan Haddad 2017-04-20 09:41:43 PDT
[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
Comment 1 Ryan Haddad 2017-04-20 09:44:51 PDT
It looks like this regressed with "Enable optimized layer flushes on iOS" https://trac.webkit.org/changeset/215469/webkit
Comment 2 Antti Koivisto 2017-04-20 13:43:38 PDT
I'll take a look.
Comment 3 Simon Fraser (smfr) 2017-04-21 03:56:26 PDT
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?
Comment 4 Antti Koivisto 2017-04-21 05:50:54 PDT
(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.
Comment 5 Antti Koivisto 2017-04-21 05:59:35 PDT
We can probably just invalidate all layers with accelerated animations in progress.
Comment 6 Antti Koivisto 2017-04-21 10:53:18 PDT
Created attachment 307750 [details]
patch
Comment 7 Simon Fraser (smfr) 2017-04-21 14:04:55 PDT
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?
Comment 8 Antti Koivisto 2017-04-21 15:21:52 PDT
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).
Comment 9 Radar WebKit Bug Importer 2017-04-21 19:49:52 PDT
<rdar://problem/31771174>
Comment 10 Simon Fraser (smfr) 2017-04-22 02:17:10 PDT
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.
Comment 11 Antti Koivisto 2017-04-22 02:26:33 PDT
(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.
Comment 12 Antti Koivisto 2017-04-24 06:00:49 PDT
Created attachment 307968 [details]
patch

GraphicsLayerCA only (almost) patch
Comment 13 Simon Fraser (smfr) 2017-04-25 08:46:35 PDT
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.
Comment 14 Antti Koivisto 2017-04-25 11:02:53 PDT
Created attachment 308117 [details]
patch
Comment 15 WebKit Commit Bot 2017-04-25 12:20:21 PDT
Comment on attachment 308117 [details]
patch

Clearing flags on attachment: 308117

Committed r215750: <http://trac.webkit.org/changeset/215750>
Comment 16 WebKit Commit Bot 2017-04-25 12:20:23 PDT
All reviewed patches have been landed.  Closing bug.