New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 692310 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: Nov 26
Cc:
█ trchen@chromium.org
pdr@chromium.org
chrishtr@chromium.org
█ wkorman@chromium.org
vmp...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , All
Pri: 2
Type: Task

Blocking:
issue 666986



Sign in to add a comment

Composited animations that paint no content never become ready in SPv2.

Project Member Reported by wkorman@chromium.org, Feb 15 2017 
Breakout summarizing issue found while working on  http://crbug.com/674317#c26  investigating hanging composited animation focused layout tests under SPv2:

SPv2 only creates layers when there's something to paint. Composited animations can today come into existence for an element that paints no content. In this case a ready promise for an animation of such an element will never be resolved.

Proposed solution:
- essentially, don't allow animations as described above to be composited.
- to effect this, (something like, rough sketch):
  + pass all compositor element ids generated for all animations in to PAC.
  + PAC::update does its business, and whenever it sees a composited animation it removes it from that list.
  + what remains in the list are the non-composited animations.
  + make sure after this point in control flow that the animation subsystem treats all of these as non-composited animations.
- note: in order for this to be feasible we need to move Document::updateAnimations() to be after Paint phase in FrameView::updateLifecyclePhasesInternal

Though it seems a little strange to even bother creating a compositor element id in PaintPropertyTreeBuilder given that later we may decide not to make it a composited animation and thus it didn't need one, the way to think about this is that PPTB is prepping info that PAC may need to make its compositing decisions.

There are other possible solutions that we considered and found less savory for example dummy display items or alternate logic elsewhere to artificially create layers, but this seems like it fits best with the overarching design goal of (1) separation of concern w.r.t. animation, compositing, paint data, and (2) PAC being the final arbiter of compositing decisions for Blink content.

A trivial example that (today, in SPv2) creates a composited animation where there is no content, and thus no layer, and will thus never tick compositor-side, become ready, or resolve its promise:

<span id="error"></span>
<script>error.animate({opacity: ['1', '1']}, 1).ready.then(() => { console.log('Promise resolved.'); });</script>

Note the Animation.ready method is still experimental and though we hit this issue while debugging layout tests it looks like WebAnimationsAPI is marked 'experimental' in REF. See also compatibility at:

https://developer.mozilla.org/en-US/docs/Web/API/Animation/ready

Based on experimental state, I'm not sure whether this would block shipping SPv2, but we need to address it at some point.

Comment 1 by alancutter@chromium.org, Feb 16 2017

Cc: dstockwell@chromium.org
If Blink has enough information (in Animation::isCandidateForAnimationOnCompositor()) to not request compositor animations in these cases that sounds like a good solution to me.

Alternatively the compositor side could detect such animations, clean them up but still send a start time back to Blink. That would be acceptable from a Blink standpoint.

Comment 2 by wkorman@chromium.org, Feb 16 2017 

http://crrev.com/2693363002/ in flight to move Document::updateAnimations() to be after Paint phase (see 'note:' in initial bug description). Hit potential snag with SVG, see comments on change. Love will find a way.

Comment 3 by wkorman@chromium.org, Feb 17 2017 

Minor edit to proposed sol'n in original description: have PAC just build up a list of element ids it's seen that actually end up having layers assigned, and provide that as output. So, a positive list rather than a negative list. Animation logic can use this as it iterates through animations.

This doesn't involve setting/adding anything to ObjectPaintProperties or PaintPropertyTree nodes. It does mean we'll have to add a param to pass things down into animation logic.

Open to further revision as we look at strawperson change.
Project Member

Comment 5 by bugdroid1@chromium.org, Feb 27 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6d087c27d71247d697ae2afee12b5a7532551e68

commit 6d087c27d71247d697ae2afee12b5a7532551e68
Author: wkorman <wkorman@chromium.org>
Date: Mon Feb 27 19:48:45 2017

Paint some content for animated elements in composited animation tests.

In SPv2 we won't create a layer for an element that paints no content.
For composited animation focused layout tests we explicitly want to
make sure the element under test is composited. Thus we must paint
some kind of content (what it is doesn't actually matter).

Here we add an 'x' as dummy text content to be painted by any element that is
intended to be animated. We could alternatively set a background color and
ensure the element has some non-empty size, but that involves more styling which
feels more intrusive.

BUG= 692310 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2702483002
Cr-Commit-Position: refs/heads/master@{#453303}

[modify] https://crrev.com/6d087c27d71247d697ae2afee12b5a7532551e68/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/6d087c27d71247d697ae2afee12b5a7532551e68/third_party/WebKit/LayoutTests/animations/compositor-start-event-timing.html
[modify] https://crrev.com/6d087c27d71247d697ae2afee12b5a7532551e68/third_party/WebKit/LayoutTests/animations/resources/composited-animation-test.js
[modify] https://crrev.com/6d087c27d71247d697ae2afee12b5a7532551e68/third_party/WebKit/LayoutTests/virtual/threaded/animations/KeyframeEffectReadOnly-composited-animation.html
[modify] https://crrev.com/6d087c27d71247d697ae2afee12b5a7532551e68/third_party/WebKit/LayoutTests/virtual/threaded/animations/element-animate-positive-delay.html
[modify] https://crrev.com/6d087c27d71247d697ae2afee12b5a7532551e68/third_party/WebKit/LayoutTests/virtual/threaded/animations/img-element-transform.html
[modify] https://crrev.com/6d087c27d71247d697ae2afee12b5a7532551e68/third_party/WebKit/LayoutTests/virtual/threaded/animations/inline-block-transform.html
[modify] https://crrev.com/6d087c27d71247d697ae2afee12b5a7532551e68/third_party/WebKit/LayoutTests/virtual/threaded/animations/transitions-retarget.html

Comment 6 by wkorman@chromium.org, Mar 1 2017 

Brief update, initial exploratory patch at http://crrev.com/2724083002/ currently failing on simple test case due to:

- Animation::isCandidateForAnimationOnCompositor() returning false (as intended) for an animation for which PAC created no layer
- thus Animation::preCommit() does not create an animation player for that animation
- subsequent call in Animation::preCommit() to Animation::maybeStartAnimationOnCompositor() ends up passing all checks
- calls into CompositorAnimations::startAnimationOnCompositor() which then fails DCHECK for non-nullptr animation player

One potential fix I plan to look at is moving the element-id-set-check into Animation::canStartAnimationOnCompositor(). I don't yet fully grok the intended semantic differences between canStartAnimationOnCompositor() and isCandidateForAnimationOnCompositor().

There are also various cleanup/optimization bits still needed in the patch but I wanted to provide some insight and allow for feedback while exploration progresses.

Comment 7 by wkorman@chromium.org, Mar 14 2017 

Notes on compositable animation constraint methods. Rough call graph attached.

isCandidateForAnimationOnCompositor
- Animation::isCandidateForAnimationOnCompositor
  = called by:
    o Animation::preCommit
  + Animation::canStartAnimationOnCompositor (see below)
  + KeyframeEffectReadOnly::isCandidateForAnimationOnCompositor
- KeyframeEffectReadOnly::isCandidateForAnimationOnCompositor
  = called by:
    o Animation::isCandidateForAnimationOnCompositor
    o KeyframeEffectReadOnly::maybeStartAnimationOnCompositor
  + no model/target; style has motion offset; multiple xform properties
  + CompositorAnimations::isCandidateForAnimationOnCompositor
- CompositorAnimations::isCandidateForAnimationOnCompositor
  = called by:
    o CompositorAnimations::startAnimationOnCompositor
    o KeyframeEffectReadOnly::isCandidateForAnimationOnCompositor
  + keyframe effect has properties
  + all properties are css properties (which aren't?)
  + incompatible transform property vs un-transformable LayoutObject
  + keyframe neutrality vs. composite effect model and animatable value
  + transform ops that depend on box size
  + filters that move pixels
  + more than one transform property (has TODO)
  + incompatible animations:
    o target has no animations (sanity check?)
    o considerAnimationAsIncompatible:
      . animation already added
      . current animation already pending/running
      . animation to add has lower sequence number (priority)
    o if anim to add affects same animatable property as already-attached anim

canStartAnimationOnCompositor
- Animation::canStartAnimationOnCompositor
  = called by:
    o Animation::isCandidateForAnimationOnCompositor
    o Animation::maybeStartAnimationOnCompositor
  + composited animations disabled for testing
  + effect suppressed (only used by inspector on anim clone/release)
  + non-compositable playback rates (marked FIXME, edge cases)
  + has timeline, content, isKeyframeEffectReadOnly, and is playing
- CompositorAnimations::canStartAnimationOnCompositor
  = called by:
    o CompositorAnimations::startAnimationOnCompositor
    o CompositorAnimations::cancelAnimationOnCompositor
    o KeyframeEffectReadOnly::maybeStartAnimationOnCompositor
  + threaded animation enabled
  + SPv2: paint property nodes have direct compositing reason
  + SPv1: layout object exists and compositing state=paints into own backing

maybeStartAnimationOnCompositor
- Animation::maybeStartAnimationOnCompositor
  = called by:
    o Animation::preCommit
  + Animation::canStartAnimationOnCompositor
  + KeyframeEffectReadOnly::maybeStartAnimationOnCompositor
- KeyframeEffectReadOnly::maybeStartAnimationOnCompositor
  = called by:
    o Animation::maybeStartAnimationOnCompositor
  + Animation::isCandidateForAnimationOnCompositor
  + CompositorAnimations::canStartAnimationOnCompositor
  + CompositorAnimations::startAnimationOnCompositor
Compositable Animation Constraint Methods.jpg
59.1 KB View Download

Comment 8 by wkorman@chromium.org, Mar 15 2017 

Brief note -- spent time today looking at compositing/animation/hidden-composited.html which came into being for  http://crbug.com/444852  which states that a composited animation, when it or one of its ancestors is hidden, should continue ticking/executing.

The test case paints no content (empty div). In theory if we add content it should start out as a composited animation, but I'm not yet sure what will happen either now at ToT or once we finish with the work planned in this bug. Further investigation required.

Concern is that our current target end state w.r.t. coding plan may be that the animation de-composites when hidden (since it paints nothing), re-composites when shown, essentially leading to stop/start, and so could lose composited animation state.

Also TBD is whether this would really matter from a web animations specification and user-surprise perspective.  http://crbug.com/444852  stance seems like it could be debated.

Comment 9 by wkorman@chromium.org, Mar 16 2017 

http://crrev.com/2724083002 is in review.
Project Member

Comment 10 by bugdroid1@chromium.org, Mar 22 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8951c6c53fdbf06bd999541965c86da70a7999ca

commit 8951c6c53fdbf06bd999541965c86da70a7999ca
Author: chrishtr <chrishtr@chromium.org>
Date: Wed Mar 22 21:11:25 2017

[SPv2] Don't paint invisible PaintLayers with transform animations.

Only an effect animation can cause the PaintLayer to become visible via compositor
thread updates.

BUG= 700176 , 692310 

Review-Url: https://codereview.chromium.org/2770763002
Cr-Commit-Position: refs/heads/master@{#458875}

[modify] https://crrev.com/8951c6c53fdbf06bd999541965c86da70a7999ca/third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp
[modify] https://crrev.com/8951c6c53fdbf06bd999541965c86da70a7999ca/third_party/WebKit/Source/core/paint/PaintLayerPainter.h
[modify] https://crrev.com/8951c6c53fdbf06bd999541965c86da70a7999ca/third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp

Comment 11 by wkorman@chromium.org, Mar 23 2017 

Working through failure of virtual/threaded/animations/composited-animations-rotate-zero-degrees.html with current patch http://crrev.com/2724083002. virtual/threaded/animations/compositor-independent-transform-properties.html may be a similar case.

I believe the first and last frames of a number of the animations tested therein are now being de-composited due to either painting nothing (first frame under certain keyframes) or the animation no longer being active (last frame for some).

Still validating and considering best approach.

It may be that we need to enhance the animations/composited-animation-test.js infra to allow for some frames to be de-composited due to the new-for-SPv2 design philosophy wherein we do not composite if nothing would be painted, or if animation is not active though it may have been active previously (this latter part is just my current guess re: why the last frame is failing).

Comment 12 by wkorman@chromium.org, Mar 23 2017 

See attached expectations of actual vs expected for virtual/threaded/animations/composited-animations-rotate-zero-degrees.html for reference. You can see 8 frames are empty in expected. Those are not planned to ever be composited in SPv2 as they paint nothing.

For now I'm planning to mark this test as failing with a comment re: the 8 frames being de-composited. We can enhance composited-animation.js as I note above to optionally allow specifying some frames as not composited, but this will only be the case for SPv2 and we're not exposing that flag to test JS currently, so we'd either need to expose that, have an SPv2-specific test, or have unsightly errors-present SPv2-specific baseline.
composited-animations-rotate-zero-degrees-expected.png
7.9 KB View Download
composited-animations-rotate-zero-degrees-actual.png
27.1 KB View Download

Comment 13 by wkorman@chromium.org, Mar 24 2017 

These six tests seem to be flakily crashing on the linux SPv2 trybot for http://crrev.com/2724083002. Investigating.

virtual/threaded/animations/element-animate-positive-delay.html
virtual/threaded/animations/composited-animations-rotate-zero-degrees.html
virtual/threaded/animations/transitions-retarget.html
virtual/threaded/animations/composited-animation-independent-transform-cancel.html
virtual/threaded/animations/inline-block-transform.html
virtual/threaded/animations/img-element-transform.html

Comment 14 by wkorman@chromium.org, Mar 28 2017

Labels: Type-Task

Comment 15 by wkorman@chromium.org, Mar 30 2017 

virtual/threaded/animations/transitions-retarget.html is crashing here:

#0  cc::LayerImpl::GetMutatorHost (this=0x0) at ../../cc/layers/layer_impl.cc:112
#1  0x00007fffee04c6e4 in cc::PropertyTrees::GetAnimationScales (this=0x3fd1416d8448, transform_node_id=4, layer_tree_impl=0x3fd1416d8420) at ../../cc/trees/property_tree.cc:1896
#2  0x00007fffedd8b6fb in cc::PictureLayerImpl::RecalculateRasterScales (this=0x3fd13ef03aa0) at ../../cc/layers/picture_layer_impl.cc:1065
#3  0x00007fffedd8a442 in cc::PictureLayerImpl::UpdateTiles (this=0x3fd13ef03aa0) at ../../cc/layers/picture_layer_impl.cc:465
#4  0x00007fffee01db66 in cc::LayerTreeImpl::UpdateDrawProperties (this=0x3fd1416d8420, update_lcd_text=true) at ../../cc/trees/layer_tree_impl.cc:1212
#5  0x00007fffedfe43ad in cc::LayerTreeHostImpl::UpdateSyncTreeAfterCommitOrImplSideInvalidation (this=0x3fd13ee39020) at ../../cc/trees/layer_tree_host_impl.cc:376
#6  0x00007fffedfe42cc in cc::LayerTreeHostImpl::CommitComplete (this=0x3fd13ee39020) at ../../cc/trees/layer_tree_host_impl.cc:342
#7  0x00007fffee08ea06 in cc::ProxyImpl::ScheduledActionCommit (this=0x3fd13ea6a6e0) at ../../cc/trees/proxy_impl.cc:524
#8  0x00007fffedf05038 in cc::Scheduler::ProcessScheduledActions (this=0x3fd13ef15b20) at ../../cc/scheduler/scheduler.cc:635
#9  0x00007fffedf060fe in cc::Scheduler::NotifyReadyToCommit (this=0x3fd13ef15b20) at ../../cc/scheduler/scheduler.cc:166

This preceding line:

LayerImpl* layer_impl = layer_tree_impl->LayerById(node->owning_layer_id);

node is a transform node, and owning_layer_id is -1. So we fail to get a LayerImpl and crash subsequently.
Project Member

Comment 16 by bugdroid1@chromium.org, Apr 7 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa

commit efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa
Author: wkorman <wkorman@chromium.org>
Date: Fri Apr 07 04:43:45 2017

[SPv2] Decomposite otherwise-compositable animations that paint nothing.

In SPv1 we create cc layers for composited animations even if they
paint no content.

In SPv2, we only create cc layers when something is painted,
a.k.a., there is a PaintChunk.

This means that in SPv1 we could be assured that a composited
animation that painted no content would still have a cc layer
with the animation's compositor element id set on it, but in
SPv2 such an animation will have no cc layer and accordingly
it is impossible to run it as a composited animation.

In SPv2, we choose to not composite such animations.

To do otherwise would require:

1. code complexity to identify when this is the case,
through a dummy PaintChunk or similar, and ensure that
we create a cc layer even though there's nothing in need
of painting.

2. compositor resource overhead to maintain a layer
solely for the purpose of executing a composited animation
even though it is not producing any visible content.

BUG= 692310 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2724083002
Cr-Commit-Position: refs/heads/master@{#462748}

[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/LayoutTests/animations/resources/composited-animation-test.js
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/LayoutTests/virtual/threaded/animations/composited-animation-independent-transform-cancel.html
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/LayoutTests/virtual/threaded/animations/compositor-independent-transform-properties.html
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/Animation.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/Animation.h
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/AnimationTest.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/AnimationTimelineTest.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/CompositorAnimationsTest.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/CompositorPendingAnimations.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/CompositorPendingAnimations.h
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/DocumentAnimations.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/DocumentAnimations.h
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/animation/EffectStackTest.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/frame/FrameView.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/frame/FrameView.h
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/layout/compositing/PaintLayerCompositor.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/core/svg/graphics/SVGImage.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/platform/graphics/CompositorElementId.h
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
[modify] https://crrev.com/efeb13b3ba0f7604fe7f61a69d83870d1a5f51fa/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp

Comment 17 by wkorman@chromium.org, Dec 7 2017

Cc: -dstockwell@chromium.org -ajuma@chromium.org
Owner: ----
Status: Available (was: Assigned)

Comment 18 by chrishtr@chromium.org, Dec 7 2017

Cc: wkorman@chromium.org
@wkorman: does anything remain for this bug? Is it actually done perhaps?

Comment 19 by wkorman@chromium.org, Dec 7 2017 

I think this is still an issue, see in particular the test and notes in comment #12. I had a mental note to talk with pdr@ as he was going to do work to create dummy paint chunks for scrolling purposes, and it may be that to solve this we use those for this situation.

Comment 20 by wangxianzhu@chromium.org, Nov 26

Status: Fixed (was: Available)
The test virtual/threaded/animations/composited-animations-rotate-zero-degrees.html is constantly passing, perhaps because we have landed the dummy paint chunks for scrolling.
►

Sign in to add a comment