Support multiple properties on NPW-based animations. [chromium/src : main]
Kevin Ellis (Gerrit)
Kevin Ellis added 2 comments![]( "Open in Gerrit")
#if EXPENSIVE_DCHECKS_ARE_ON()This check is not assured if we have a background-color and clip-path animation in the same animation. The background-color animation can force a late fallback to main due to detection of a pixel-moving filter. As far as paint is concerned, the animation is composited, but can potentially fall back to main in Animation::PreCommit, which has the final say.
Related details
- Claire Chambers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Ellis (Gerrit)
Kevin Ellis added 1 comment![]( "Open in Gerrit")
DocumentLifecycle::kInPrePaint) {This change is needed as when we have both a clip-path and background-color change in the same animation we trigger a call for whether enabled on the compositor from pre-paint instead of paint. At the pre-paint stage, the first fragment does not have local border box properties. It is OK to return false early and then true later. It just means we think the animation is supported on the compositor only to find out it isn't later. The graceful fallback to main in PreCommit ensures that things are in check.
Claire Chambers (Gerrit)
Claire Chambers added 5 comments![]( "Open in Gerrit")
npw_reasons != NativePaintWorkletProperties::kNoPaintWorklet) {Could we gate this behind the FF? I am almost certain this code could never cause a problem, but I am paranoid about turning an obscure undiscovered painting bug into a client crash and blocking the experiment as we're running it in stable.
If you like, you can just gate the clip-path specific check.
// We make one exception for the forced reset for background-color.
// The eligibility for compositing is make at paint time. Hold off on the
// rest until the element has been painted.not sure what 'the rest' means here, I think you actually meant to say the opposite (?).
```suggestion
// We only fall back the NPW status if it has received an eligibility
// determination. If it hasn't, this could mean its target is outside the
// paint apron or been made invisible with CSS, and is still potentially
// compositable.
```
if (npw_reasons & NativePaintWorkletProperties::kClipPathPaintWorklet) {
element_animations->SetCompositedClipPathStatus(
ElementAnimations::CompositedPaintStatus::kNotComposited);
}There is actually an obscure case this will change behavior. If you start a clip-path animation with WAAPI while its parent has `content-visibility: hidden`, you will not have yet had a clip-path eligibility calculation, but `PreCommit` still runs. The animation fails to composite as it has no `LayoutObject` and thus you get `kAnimationHasNoVisibleChange`. As soon as you change `content-visibility: visible`, the element gets pre-paint and paint, but there's no eligibility recalc, so if this code were left unchanged you would be leaving a compositable animation as `kNotComposited`.
I am doubtful this case exists in the wild (though I did test it when I was hunting for clip-path bugs back during the PAC clip hierarchy bug investigation), but you can fix it by just applying your same bgcolor check for clip-path.
bool HasNativePaintWorketReason(Should this just be a member of `Animation`? I see why you did it this way, because we assign `npw_reasons` conditionally based on the timeline type.
However, this could be replaced with `const bool timeline_supports_npw = timeline && timeline->IsMonotonicallyIncreasing()`.
You could use this function in `PreCommit` and it would make it slightly more readable.
#if EXPENSIVE_DCHECKS_ARE_ON()
if (animation &&
CompositeClipPathStatus(element) == CompositedPaintStatus::kComposited) {
CHECK(animation->HasActiveAnimationsOnCompositor() ||
animation->CheckCanStartAnimationOnCompositor(
nullptr, StartOnCompositorReason::kGeneric) ==
CompositorAnimations::kNoFailure);
}
#endif // EXPENSIVE_DCHECKS_ARE_ON()
Can we keep this `DCHECK`? You added a lifecycle check in `CompositorMayHaveIncorrectDamageRect`, so this should should pass now? Apologies if I am wrong, I have not tried building your change to verify.
Related details
- Kevin Ellis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| 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")
#if EXPENSIVE_DCHECKS_ARE_ON()
if (animation &&
CompositeClipPathStatus(element) == CompositedPaintStatus::kComposited) {
CHECK(animation->HasActiveAnimationsOnCompositor() ||
animation->CheckCanStartAnimationOnCompositor(
nullptr, StartOnCompositorReason::kGeneric) ==
CompositorAnimations::kNoFailure);
}
#endif // EXPENSIVE_DCHECKS_ARE_ON()
Can we keep this `DCHECK`? You added a lifecycle check in `CompositorMayHaveIncorrectDamageRect`, so this should should pass now? Apologies if I am wrong, I have not tried building your change to verify.
Nevermind, I'm stupid. I see your comment and I understand now. The case this breaks in is when we're in paint and the background has painted but the svg clip hasn't yet, and this will fail with `kUnsupportedCSSProperty` due to bgcolor being `kNotComposited`.
It's ugly, but you can fix this by simply checking the npw reasons of the animation and excluding the case. If that doesn't work in your mind for some reason, ignore this comment.
Kevin Ellis (Gerrit)
Kevin Ellis added 5 comments![]( "Open in Gerrit")
npw_reasons != NativePaintWorkletProperties::kNoPaintWorklet) {Could we gate this behind the FF? I am almost certain this code could never cause a problem, but I am paranoid about turning an obscure undiscovered painting bug into a client crash and blocking the experiment as we're running it in stable.
If you like, you can just gate the clip-path specific check.
Done
// We make one exception for the forced reset for background-color.
// The eligibility for compositing is make at paint time. Hold off on the
// rest until the element has been painted.not sure what 'the rest' means here, I think you actually meant to say the opposite (?).
```suggestion
// We only fall back the NPW status if it has received an eligibility
// determination. If it hasn't, this could mean its target is outside the
// paint apron or been made invisible with CSS, and is still potentially
// compositable.
```
Done
if (npw_reasons & NativePaintWorkletProperties::kClipPathPaintWorklet) {
element_animations->SetCompositedClipPathStatus(
ElementAnimations::CompositedPaintStatus::kNotComposited);
}There is actually an obscure case this will change behavior. If you start a clip-path animation with WAAPI while its parent has `content-visibility: hidden`, you will not have yet had a clip-path eligibility calculation, but `PreCommit` still runs. The animation fails to composite as it has no `LayoutObject` and thus you get `kAnimationHasNoVisibleChange`. As soon as you change `content-visibility: visible`, the element gets pre-paint and paint, but there's no eligibility recalc, so if this code were left unchanged you would be leaving a compositable animation as `kNotComposited`.
I am doubtful this case exists in the wild (though I did test it when I was hunting for clip-path bugs back during the PAC clip hierarchy bug investigation), but you can fix it by just applying your same bgcolor check for clip-path.
Logic now consistent for clip-path and BG-color.
Should this just be a member of `Animation`? I see why you did it this way, because we assign `npw_reasons` conditionally based on the timeline type.
However, this could be replaced with `const bool timeline_supports_npw = timeline && timeline->IsMonotonicallyIncreasing()`.
You could use this function in `PreCommit` and it would make it slightly more readable.
I might be missing something here. This is a temporary method while we have the 2-code paths (with and without multiple property support). If only supporting a single property, we require an exact match, otherwise we set the single bit.
This method is being used downstream of a check for a monotonic timeline, but the method itself has nothing to do with the type of timeline. Failure of NPWs with SDA is a separate issue (likely tied to not having full support for start/end delays).
I don't see an overlap with the PreCommit logic either.
#if EXPENSIVE_DCHECKS_ARE_ON()
if (animation &&
CompositeClipPathStatus(element) == CompositedPaintStatus::kComposited) {
CHECK(animation->HasActiveAnimationsOnCompositor() ||
animation->CheckCanStartAnimationOnCompositor(
nullptr, StartOnCompositorReason::kGeneric) ==
CompositorAnimations::kNoFailure);
}
#endif // EXPENSIVE_DCHECKS_ARE_ON()
Claire ChambersCan we keep this `DCHECK`? You added a lifecycle check in `CompositorMayHaveIncorrectDamageRect`, so this should should pass now? Apologies if I am wrong, I have not tried building your change to verify.
Nevermind, I'm stupid. I see your comment and I understand now. The case this breaks in is when we're in paint and the background has painted but the svg clip hasn't yet, and this will fail with `kUnsupportedCSSProperty` due to bgcolor being `kNotComposited`.
It's ugly, but you can fix this by simply checking the npw reasons of the animation and excluding the case. If that doesn't work in your mind for some reason, ignore this comment.
As we know exactly why this might happen, I'm reluctant to add special case bloat. A fallback is simply a natural result of we make a tentative yes decision upstream of having all of the facts.
In a follow up patch, I'm planning to add fallback in the clip-path only case when the clip itself is blurred (we have the same bug here).Note that clipping the bug is absolutely fine on the compositor. Blurring the clip is problematic when the blur radius applied ot the clip exceeds the clip bounds. I have a demo.
Related details
- Claire Chambers
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Claire Chambers (Gerrit)
Claire Chambers added 4 comments![]( "Open in Gerrit")
Support multiple properties on NPW-based animations.I really appreciate all the work you did to get this to work with clip-paths. :)
LGTM w/tiny nit
if (npw_reasons & NativePaintWorkletProperties::kClipPathPaintWorklet) {
element_animations->SetCompositedClipPathStatus(
ElementAnimations::CompositedPaintStatus::kNotComposited);
}Kevin EllisThere is actually an obscure case this will change behavior. If you start a clip-path animation with WAAPI while its parent has `content-visibility: hidden`, you will not have yet had a clip-path eligibility calculation, but `PreCommit` still runs. The animation fails to composite as it has no `LayoutObject` and thus you get `kAnimationHasNoVisibleChange`. As soon as you change `content-visibility: visible`, the element gets pre-paint and paint, but there's no eligibility recalc, so if this code were left unchanged you would be leaving a compositable animation as `kNotComposited`.
I am doubtful this case exists in the wild (though I did test it when I was hunting for clip-path bugs back during the PAC clip hierarchy bug investigation), but you can fix it by just applying your same bgcolor check for clip-path.
Logic now consistent for clip-path and BG-color.
Thanks 😊
bool HasNativePaintWorketReason(Kevin EllisShould this just be a member of `Animation`? I see why you did it this way, because we assign `npw_reasons` conditionally based on the timeline type.
However, this could be replaced with `const bool timeline_supports_npw = timeline && timeline->IsMonotonicallyIncreasing()`.
You could use this function in `PreCommit` and it would make it slightly more readable.
I might be missing something here. This is a temporary method while we have the 2-code paths (with and without multiple property support). If only supporting a single property, we require an exact match, otherwise we set the single bit.
This method is being used downstream of a check for a monotonic timeline, but the method itself has nothing to do with the type of timeline. Failure of NPWs with SDA is a separate issue (likely tied to not having full support for start/end delays).
I don't see an overlap with the PreCommit logic either.
Sorry, I didn't explain myself properly. In `PreCommit` you have `npw_reasons & NativePaintWorkletProperties::kProperty` checks and here you also have the same.
In principle, you could just `animation->HasNPWProperty(...)`
I mentioned the timeline case because downstream you conditionally assign `npw_reasons` based on the timeline type, and that's how we fall back in that case (`npw_reasons == 0` --> equality in the switch fails --> `generator` left as `nullptr` --> `DefaultToUnsupportedProperty`). However, you could just store this boolean and read it directly, allowing you to delete `npw_reasons` entirely.
However, to your point, this would only save 1 line, and only a few characters vs bitwise algebra, so this is an unimportant nit especially since NPW (probably?) isn't going to be the main method to composite new properties going forward. I'll leave it up to you.
#if EXPENSIVE_DCHECKS_ARE_ON()
if (animation &&
CompositeClipPathStatus(element) == CompositedPaintStatus::kComposited) {
CHECK(animation->HasActiveAnimationsOnCompositor() ||
animation->CheckCanStartAnimationOnCompositor(
nullptr, StartOnCompositorReason::kGeneric) ==
CompositorAnimations::kNoFailure);
}
#endif // EXPENSIVE_DCHECKS_ARE_ON()
Claire ChambersCan we keep this `DCHECK`? You added a lifecycle check in `CompositorMayHaveIncorrectDamageRect`, so this should should pass now? Apologies if I am wrong, I have not tried building your change to verify.
Kevin EllisNevermind, I'm stupid. I see your comment and I understand now. The case this breaks in is when we're in paint and the background has painted but the svg clip hasn't yet, and this will fail with `kUnsupportedCSSProperty` due to bgcolor being `kNotComposited`.
It's ugly, but you can fix this by simply checking the npw reasons of the animation and excluding the case. If that doesn't work in your mind for some reason, ignore this comment.
As we know exactly why this might happen, I'm reluctant to add special case bloat. A fallback is simply a natural result of we make a tentative yes decision upstream of having all of the facts.
In a follow up patch, I'm planning to add fallback in the clip-path only case when the clip itself is blurred (we have the same bug here).Note that clipping the bug is absolutely fine on the compositor. Blurring the clip is problematic when the blur radius applied ot the clip exceeds the clip bounds. I have a demo.
Ack. I'll just apply this to our DWCs in edge. I'll let you know if we get any unexpected hits.
Related details
- Kevin Ellis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Claire Chambers (Gerrit)
Claire Chambers voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Kevin Ellis
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Ellis (Gerrit)
Kevin Ellis added 1 comment![]( "Open in Gerrit")
bool HasNativePaintWorketReason(Kevin EllisShould this just be a member of `Animation`? I see why you did it this way, because we assign `npw_reasons` conditionally based on the timeline type.
However, this could be replaced with `const bool timeline_supports_npw = timeline && timeline->IsMonotonicallyIncreasing()`.
You could use this function in `PreCommit` and it would make it slightly more readable.
Claire ChambersI might be missing something here. This is a temporary method while we have the 2-code paths (with and without multiple property support). If only supporting a single property, we require an exact match, otherwise we set the single bit.
This method is being used downstream of a check for a monotonic timeline, but the method itself has nothing to do with the type of timeline. Failure of NPWs with SDA is a separate issue (likely tied to not having full support for start/end delays).
I don't see an overlap with the PreCommit logic either.
Sorry, I didn't explain myself properly. In `PreCommit` you have `npw_reasons & NativePaintWorkletProperties::kProperty` checks and here you also have the same.
In principle, you could just `animation->HasNPWProperty(...)`
I mentioned the timeline case because downstream you conditionally assign `npw_reasons` based on the timeline type, and that's how we fall back in that case (`npw_reasons == 0` --> equality in the switch fails --> `generator` left as `nullptr` --> `DefaultToUnsupportedProperty`). However, you could just store this boolean and read it directly, allowing you to delete `npw_reasons` entirely.
However, to your point, this would only save 1 line, and only a few characters vs bitwise algebra, so this is an unimportant nit especially since NPW (probably?) isn't going to be the main method to composite new properties going forward. I'll leave it up to you.
In PreCommit we have A & B, whereas here we have A & B or A == B depending on the flag setting. Though with the added Runtime flag check it amounts to A & B in Precommit, that is extra logic to sort though just to save a line or two of code. I'd rather leave as is.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kevin Ellis (Gerrit)
Kevin Ellis added 1 comment![]( "Open in Gerrit")
Related details
- Vladimir Levin
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vladimir Levin (Gerrit)
Vladimir Levin voted Code-Review+1![]( "Open in Gerrit")
- Kevin Ellis
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |