Make Composited Animations Invalidate Paint Directly [chromium/src : main]
Claire Chambers (Gerrit)
Claire Chambers voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Kevin Ellis
- Olga Gerchikov
- Robert Flack
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Ellis (Gerrit)
Kevin Ellis added 4 comments![]( "Open in Gerrit")
Looks like your stress test just failed when run in the virtual context.
CONSOLE ERROR: Uncaught ReferenceError: promise_test is not defined
This could suggest that you might have to switch to relative paths to load the test harness scripts. You should be able to experiment with this locally.
CompositedPaintStatus::kNeedsRepaintOrNoAnimation)),Why are we changing background color status? I don't think you should be touching background color at all.
void ElementAnimations::InvalidatePaintForCompositedAnimationsIfNecessary(Your change is triggering multiple crashes in CSS-transitions within this method.
SetCompositedBackgroundColorStatus(CompositedPaintStatus::kNoAnimation);Why is this patch changing behavior for background color?
Related details
- Claire Chambers
- Olga Gerchikov
- Robert Flack
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/45409.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Olga Gerchikov
- Robert Flack
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Claire Chambers (Gerrit)
Claire Chambers voted and added 3 comments![]( "Open in Gerrit")
Votes added by Claire Chambers
| Commit-Queue | +1 |
Looks like your stress test just failed when run in the virtual context.
CONSOLE ERROR: Uncaught ReferenceError: promise_test is not defined
This could suggest that you might have to switch to relative paths to load the test harness scripts. You should be able to experiment with this locally.
Thanks, I had tried this with a prior version of the test but it didn't seem to fail expectedly without the fix. I've implemented this.
CompositedPaintStatus::kNeedsRepaintOrNoAnimation)),Why are we changing background color status? I don't think you should be touching background color at all.
Because bgcolor animations rely on CompositablePaintAnimationChanged, same as clip path animations. Since this change makes CompositablePaintAnimationChanged unnecessary, it seemed a good idea to do so for both kinds of animations. They both need paint invalidated for the same reasons.
I'm curious why we'd want to keep CompositablePaintAnimationChanged for bgcolor but eliminate it for clip paths.
void ElementAnimations::InvalidatePaintForCompositedAnimationsIfNecessary(Your change is triggering multiple crashes in CSS-transitions within this method.
- Kevin Ellis
- Olga Gerchikov
- Robert Flack
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Ellis (Gerrit)
Kevin Ellis added 9 comments![]( "Open in Gerrit")
ScopedCompositeBGColorAnimationForTest scoped_feature(true);Rather than deleting the test, can you update it to reflect the changes to ElementAnimations.
KeyframeEffect* effect = DynamicTo<KeyframeEffect>(entry.key->effect());```
if (entry.key->CalculateAnimationPlayState() == Animation::kIdle) {
continue;
}
KeyframeEffect* effect = ...
```
and drop effect->HasPlayingAnimation. Rationale, a paused animation can still be active, though it is cancelled on the compositor and falls back to main. We are effectively grouping the kNoAnimation and kNotComposited states in the Set...Status() calls below.
SetCompositedBackgroundColorStatus(CompositedPaintStatus::kNoAnimation);Kevin EllisWhy is this patch changing behavior for background color?
The comment does not seem to fully align with the logic in HasAnimationForProperty. The comment suggests it is testing for the case of a single cancelled animation yet the method iterates through all animations on an element to see if any are running and affecting the relevant property. A cancelled animation has playState = idle, and is not updated. The HasPlayingAnimation method will return false if there is a paused animation. This might still need to be repainted. If we see that there is an animation, but it is not running, it should probably fall back to the not-composited state.
A bit of background on the design. With composite after paint, we have set up paint before deciding if we can composite the animation. Both the paint code and the animation code have to agree on the decision. We had numerous painting bugs related to failing to paint the background color under various edge cases and want to ensure we don't regress. The kNeedsRepaintOrNoAnimation case was to ensure that paint updated the state correctly, as it was in charge of making the definitive compositing decision.
It appears that the underlying issue is that a cancelled animation can remain in element animations longer than expected. The entry is pruned via a call to the KeyframeEffect::DetachTarget method. It seems risky to add a detach call when entering the idle state since the animation can be reactivated and would need to be reattached. For now, it is OK to simply skip over the idle animations as suggested in a previous comment. Perhaps with a TODO to investigate if cancelled animations should be synchronously pruned from the list.
// If the animation has been canceled, we need to revert back toNo need to repeat the same comment.
element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(Not sure if this is the right place for this call. Deferring to andruud@.
The recommended option in https://issues.chromium.org/issues/40789479 is:
2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).
Strictly speaking we are still during style update at this stage.
// Test that style->CompositablePaintAnimationChanged() should be true in theRather than deleting these tests due to an obsolete method they could potentially be updated to test the new behavior (i.e. Paint invalidation got triggered).
function swapAnimation() {Simpler would be to toggle a class and add the class to the style section above.
await delay(100);This test looks susceptible to Timeouts given that it has a total of 5s wait time and the Timeout is 6s.
anim.ready.then(() => { return waitForAnimationTime(300); }).then(takeScreenshot);Susceptible to test flakes. The waitForAnimationTime method spams requestAnimationFrame. Instead use a long duration and a negative start delay.
You can then trigger the screenshot as soon as the animation is running and has been rendered.
await raf();
anim = ...;
await anim.ready;
await raf();
takeScreenshot();
Related details
- Claire Chambers
- Olga Gerchikov
- Robert Flack
Robert Flack (Gerrit)
Robert Flack added 1 comment![]( "Open in Gerrit")
Moving myself to cc, re-add if/when needed. @kev...@chromium.org has the context on this one.
Related details
- Claire Chambers
- Olga Gerchikov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Claire Chambers (Gerrit)
Claire Chambers added 9 comments![]( "Open in Gerrit")
ScopedCompositeBGColorAnimationForTest scoped_feature(true);Rather than deleting the test, can you update it to reflect the changes to ElementAnimations.
Done
KeyframeEffect* effect = DynamicTo<KeyframeEffect>(entry.key->effect());```
if (entry.key->CalculateAnimationPlayState() == Animation::kIdle) {
continue;
}
KeyframeEffect* effect = ...
```and drop effect->HasPlayingAnimation. Rationale, a paused animation can still be active, though it is cancelled on the compositor and falls back to main. We are effectively grouping the kNoAnimation and kNotComposited states in the Set...Status() calls below.
I altered this to check for idleness rather than whether it is playing, in accordance with your other comment.
SetCompositedBackgroundColorStatus(CompositedPaintStatus::kNoAnimation);Kevin EllisWhy is this patch changing behavior for background color?
The comment does not seem to fully align with the logic in HasAnimationForProperty. The comment suggests it is testing for the case of a single cancelled animation yet the method iterates through all animations on an element to see if any are running and affecting the relevant property. A cancelled animation has playState = idle, and is not updated. The HasPlayingAnimation method will return false if there is a paused animation. This might still need to be repainted. If we see that there is an animation, but it is not running, it should probably fall back to the not-composited state.
A bit of background on the design. With composite after paint, we have set up paint before deciding if we can composite the animation. Both the paint code and the animation code have to agree on the decision. We had numerous painting bugs related to failing to paint the background color under various edge cases and want to ensure we don't regress. The kNeedsRepaintOrNoAnimation case was to ensure that paint updated the state correctly, as it was in charge of making the definitive compositing decision.
It appears that the underlying issue is that a cancelled animation can remain in element animations longer than expected. The entry is pruned via a call to the KeyframeEffect::DetachTarget method. It seems risky to add a detach call when entering the idle state since the animation can be reactivated and would need to be reattached. For now, it is OK to simply skip over the idle animations as suggested in a previous comment. Perhaps with a TODO to investigate if cancelled animations should be synchronously pruned from the list.
Added TODO as per suggestion.
// If the animation has been canceled, we need to revert back toNo need to repeat the same comment.
Done
element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(Not sure if this is the right place for this call. Deferring to andruud@.
The recommended option in https://issues.chromium.org/issues/40789479 is:
2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).Strictly speaking we are still during style update at this stage.
I'm absolutely open to moving it elsewhere - anywhere after applying the pending animation update should work.
I do think putting it here is very convenient as we get to piggyback off of the list of animations with pending updates, which is cleared during this function.
// Test that style->CompositablePaintAnimationChanged() should be true in theRather than deleting these tests due to an obsolete method they could potentially be updated to test the new behavior (i.e. Paint invalidation got triggered).
Done, though certain test had to be further amended. The test that changes the keyframes now uses SetKeyframes(), as is used by the Web Animations API, that way the animation is properly set pending. The test that changes the start time has had its second half removed, as adding the start time the first time cancels the animation effect as it is no longer current/in effect. Changing it the second time does not change its state, and thus does not cause a style recalc and paint invalidation. I believe the only scenario in which this is relevant is animations with positive delays, which are known not to be supported at the moment, so I don't believe this issue needs resolving now.
Simpler would be to toggle a class and add the class to the style section above.
Done
This test looks susceptible to Timeouts given that it has a total of 5s wait time and the Timeout is 6s.
I've cut it down to 20 loops, and this seems to catch the crash just as well. This should avoid running so close to the timeout.
anim.ready.then(() => { return waitForAnimationTime(300); }).then(takeScreenshot);Susceptible to test flakes. The waitForAnimationTime method spams requestAnimationFrame. Instead use a long duration and a negative start delay.
You can then trigger the screenshot as soon as the animation is running and has been rendered.await raf();
anim = ...;
await anim.ready;
await raf();
takeScreenshot();
I think my comment may not have clearly explained what this test is testing for. Specifically, the case where there is a running, valid cc animation with a curve such that [0,t]-> static frame, (t, animation end) -> changing frame. Without the presence of this animation, the clip path will begin changing at time t. However, if you add a second animation *before* time t, such that the animation has a curve [0,t+n] -> static frame, then this animation should prevent the clip path from changing at time t as this new animation's curve takes precedence.
The reason for the static frame is such that any change in the clip path will cause paint invalidation independent of any animation change. As a result, having the test set up this way ensures that *just* a new animation with the web animations API will cause the running cc animation to be invalidated. In earlier prototypes of this change, this was not the case. This test would fail as the old animation would still be running despite the addition of a new animation.
I don't believe your test will achieve the same effect, as adding a second animation *after* time t will alter the clip path property which causes paint invalidation irrespective of any animation due to the change in the clip path property.
I completely agree that this test methodology is a little janky, and I'm interested in improving it, but I don't want to sacrifice the essential discriminatory element of this test, which is that it should fail when a change to a composited animation does not invalidate paint independently to changes in style.
Related details
- Kevin Ellis
- Olga Gerchikov
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Ellis (Gerrit)
Kevin Ellis added 2 comments![]( "Open in Gerrit")
Getting very close. Just one comment left on the test.
anim.ready.then(() => { return waitForAnimationTime(300); }).then(takeScreenshot);Claire ChambersSusceptible to test flakes. The waitForAnimationTime method spams requestAnimationFrame. Instead use a long duration and a negative start delay.
You can then trigger the screenshot as soon as the animation is running and has been rendered.await raf();
anim = ...;
await anim.ready;
await raf();
takeScreenshot();
I think my comment may not have clearly explained what this test is testing for. Specifically, the case where there is a running, valid cc animation with a curve such that [0,t]-> static frame, (t, animation end) -> changing frame. Without the presence of this animation, the clip path will begin changing at time t. However, if you add a second animation *before* time t, such that the animation has a curve [0,t+n] -> static frame, then this animation should prevent the clip path from changing at time t as this new animation's curve takes precedence.
The reason for the static frame is such that any change in the clip path will cause paint invalidation independent of any animation change. As a result, having the test set up this way ensures that *just* a new animation with the web animations API will cause the running cc animation to be invalidated. In earlier prototypes of this change, this was not the case. This test would fail as the old animation would still be running despite the addition of a new animation.
I don't believe your test will achieve the same effect, as adding a second animation *after* time t will alter the clip path property which causes paint invalidation irrespective of any animation due to the change in the clip path property.
I completely agree that this test methodology is a little janky, and I'm interested in improving it, but I don't want to sacrifice the essential discriminatory element of this test, which is that it should fail when a change to a composited animation does not invalidate paint independently to changes in style.
My only concern here is that if the test is flaky it gets disabled and we lose test coverage. If you run the test without the rest of your patch, does it reliably fail? Can you get the same failure with a double raf in place of a rafs for 300ms?
If you run the test 1000 times with your patch (--repeat-each=1000) does it ever TIMEOUT?
Related details
- Claire Chambers
- Olga Gerchikov
Kevin Ellis (Gerrit)
Kevin Ellis added 2 comments![]( "Open in Gerrit")
CompositedPaintStatus::kNeedsRepaintOrNoAnimation)),Why are we changing background color status? I don't think you should be touching background color at all.
Because bgcolor animations rely on CompositablePaintAnimationChanged, same as clip path animations. Since this change makes CompositablePaintAnimationChanged unnecessary, it seemed a good idea to do so for both kinds of animations. They both need paint invalidated for the same reasons.
I'm curious why we'd want to keep CompositablePaintAnimationChanged for bgcolor but eliminate it for clip paths.
Acknowledged
element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(Claire ChambersNot sure if this is the right place for this call. Deferring to andruud@.
The recommended option in https://issues.chromium.org/issues/40789479 is:
2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).Strictly speaking we are still during style update at this stage.
I'm absolutely open to moving it elsewhere - anywhere after applying the pending animation update should work.
I do think putting it here is very convenient as we get to piggyback off of the list of animations with pending updates, which is cleared during this function.
Acknowledged
Claire Chambers (Gerrit)
Claire Chambers added 1 comment![]( "Open in Gerrit")
anim.ready.then(() => { return waitForAnimationTime(300); }).then(takeScreenshot);Claire ChambersSusceptible to test flakes. The waitForAnimationTime method spams requestAnimationFrame. Instead use a long duration and a negative start delay.
You can then trigger the screenshot as soon as the animation is running and has been rendered.await raf();
anim = ...;
await anim.ready;
await raf();
takeScreenshot();
Kevin EllisI think my comment may not have clearly explained what this test is testing for. Specifically, the case where there is a running, valid cc animation with a curve such that [0,t]-> static frame, (t, animation end) -> changing frame. Without the presence of this animation, the clip path will begin changing at time t. However, if you add a second animation *before* time t, such that the animation has a curve [0,t+n] -> static frame, then this animation should prevent the clip path from changing at time t as this new animation's curve takes precedence.
The reason for the static frame is such that any change in the clip path will cause paint invalidation independent of any animation change. As a result, having the test set up this way ensures that *just* a new animation with the web animations API will cause the running cc animation to be invalidated. In earlier prototypes of this change, this was not the case. This test would fail as the old animation would still be running despite the addition of a new animation.
I don't believe your test will achieve the same effect, as adding a second animation *after* time t will alter the clip path property which causes paint invalidation irrespective of any animation due to the change in the clip path property.
I completely agree that this test methodology is a little janky, and I'm interested in improving it, but I don't want to sacrifice the essential discriminatory element of this test, which is that it should fail when a change to a composited animation does not invalidate paint independently to changes in style.
My only concern here is that if the test is flaky it gets disabled and we lose test coverage. If you run the test without the rest of your patch, does it reliably fail? Can you get the same failure with a double raf in place of a rafs for 300ms?
If you run the test 1000 times with your patch (--repeat-each=1000) does it ever TIMEOUT?
It worked fine to 2000x iters before, however I've edited per request.
Related details
- Kevin Ellis
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Claire Chambers (Gerrit)
Claire Chambers added 1 comment![]( "Open in Gerrit")
element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(Claire ChambersNot sure if this is the right place for this call. Deferring to andruud@.
The recommended option in https://issues.chromium.org/issues/40789479 is:
2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).Strictly speaking we are still during style update at this stage.
Kevin EllisI'm absolutely open to moving it elsewhere - anywhere after applying the pending animation update should work.
I do think putting it here is very convenient as we get to piggyback off of the list of animations with pending updates, which is cleared during this function.
Acknowledged
- Anders Hartvoll Ruud
- Kevin Ellis
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau removed dpr...@google.com from this change![]( "Open in Gerrit")
Related details
- Anders Hartvoll Ruud
- Kevin Ellis
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Ellis (Gerrit)
Kevin Ellis voted and added 2 comments![]( "Open in Gerrit")
Votes added by Kevin Ellis
| Code-Review | +1 |
2 comments
// TODO(crbug.com/1245806): We could invalidate the keyframes by using e.g.Nit: This issue is marked as fixed and might no longer be relevant. OK with artificially invalidating, but we should consider removing the comment if misleading.
Related details
- Anders Hartvoll Ruud
- Claire Chambers
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Anders Hartvoll Ruud (Gerrit)
Anders Hartvoll Ruud voted and added 2 comments![]( "Open in Gerrit")
Votes added by Anders Hartvoll Ruud
| Code-Review | +1 |
2 comments
lgtm for VirtualTestSuites, defer to kevers@ for everything else.
element_animations->InvalidatePaintForCompositedAnimationsIfNecessary(Claire ChambersNot sure if this is the right place for this call. Deferring to andruud@.
The recommended option in https://issues.chromium.org/issues/40789479 is:
2. After the style/layout lifecycle, go through compositor-pending animations and invalidate paint directly (somewhere on the LocalFrameView-level).Strictly speaking we are still during style update at this stage.
Kevin EllisI'm absolutely open to moving it elsewhere - anywhere after applying the pending animation update should work.
I do think putting it here is very convenient as we get to piggyback off of the list of animations with pending updates, which is cleared during this function.
Claire ChambersAcknowledged
Adding them as reviewer.
Sorry for the delay. This call site seems fine, thanks for tackling this issue! :-)
Related details
- Claire Chambers
Claire Chambers (Gerrit)
Claire Chambers added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/1245806): We could invalidate the keyframes by using e.g.Nit: This issue is marked as fixed and might no longer be relevant. OK with artificially invalidating, but we should consider removing the comment if misleading.
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Claire Chambers (Gerrit)
Claire Chambers voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
13 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/animation/css/css_animations_test.cc
Insertions: 0, Deletions: 5.
@@ -648,11 +648,6 @@
EXPECT_TRUE(element->ComputedStyleRef().HasCurrentBackgroundColorAnimation());
// Change compositor snapshot values:
- //
- // TODO(crbug.com/1245806): We could invalidate the keyframes by using e.g.
- // var()-references, and changing them in the base style, but it does
- // currently not work for composited animations, hence the snapshot is
- // invalidated artificially.
InvalidateCompositorKeyframesSnapshot(animation);
// Also do an "unrelated" change, to avoid IsAnimationStyleChange()==true.
element->classList().toggle(AtomicString("unrelated"), ASSERT_NO_EXCEPTION);
```
Change information
Make Composited Animations Invalidate Paint Directly
The previous system of relying on synthetic style diffs to produce paint invalidation proved too unwieldly, and created issues where animation changes in the pending animation update would cause crashes in certain situations.
This CL completely removes CompositablePaintAnimationChanged and replaces it with an extra hook in post style update scope. Any animations that are compositor pending or have been canceled have their corresponding layout objects invalidated.
Bug: 1246826
- M third_party/blink/renderer/core/animation/animation.cc
- M third_party/blink/renderer/core/animation/css/css_animations.cc
- M third_party/blink/renderer/core/animation/css/css_animations_test.cc
- M third_party/blink/renderer/core/animation/element_animations.cc
- M third_party/blink/renderer/core/animation/element_animations.h
- M third_party/blink/renderer/core/css/post_style_update_scope.cc
- M third_party/blink/renderer/core/dom/element.cc
- M third_party/blink/renderer/core/layout/layout_object.cc
- M third_party/blink/renderer/core/paint/box_painter_base.cc
- M third_party/blink/renderer/core/style/computed_style.cc
- M third_party/blink/renderer/core/style/computed_style_extra_fields.json5
- M third_party/blink/renderer/modules/csspaint/nativepaint/background_color_paint_definition_test.cc
- M third_party/blink/renderer/modules/csspaint/nativepaint/clip_path_paint_definition_test.cc
- M third_party/blink/web_tests/VirtualTestSuites
- A third_party/blink/web_tests/css3/masking/clip-path-animation-stress.html
- A third_party/blink/web_tests/external/wpt/css/css-masking/clip-path/animations/two-clip-path-animation-diff-length4.html
Code-Review: +1 by Kevin Ellis, +1 by Anders Hartvoll Ruud
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/45409
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |