Implement iterationComposite for CSS animations [chromium/src : main]
Felipe Erias (Gerrit)
Felipe Erias added 1 comment![]( "Open in Gerrit")
Hello,
This CL is an initial implementation of the iterationComposite property from Web Animations Level 2, which controls how animation values accumulate across multiple iterations.
https://drafts.csswg.org/web-animations-2/#iteration-composite-operation
I hope that the CL description provides a good starting point, but please don't hesitate to ask me to clarify any point. I have already updated the existing WPT and unit tests, and I am currently working to add additional unit tests for this feature.
Thank you for your review.
Best regards,
Felipe
Related details
- Alan Cutter
- David Awogbemila
- Kevin Ellis
- Robert Flack
- Stephen McGruer
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alan Cutter (Gerrit)
Alan Cutter added 1 comment![]( "Open in Gerrit")
bool InvalidatableInterpolation::ApplyIterationAccumulation() const {Should we be using this return value?
Related details
- David Awogbemila
- Felipe Erias
- Kevin Ellis
- Robert Flack
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. |
Alan Cutter (Gerrit)
Alan Cutter added 2 comments![]( "Open in Gerrit")
I don't count myself as an owner of this code anymore but this looks pretty good to me!
assert_equals: Animated filter list at 50s of the third iteration expected "contrast(4) brightness(4)" but got "contrast(2) brightness(2)"What's going wrong for this test?
Felipe Erias (Gerrit)
Felipe Erias added 1 comment![]( "Open in Gerrit")
bool InvalidatableInterpolation::ApplyIterationAccumulation() const {Should we be using this return value?
Thank you for your review.
Good point. That return value is unused and not particularly informative (it would be false in most cases, for different reasons) so I have removed it:
`void InvalidatableInterpolation::ApplyIterationAccumulation() const`
Related details
- Alan Cutter
- David Awogbemila
- 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. |
Alan Cutter (Gerrit)
Alan Cutter added 1 comment![]( "Open in Gerrit")
@kev...@chromium.org would you be okay with being primary reviewer for this CL?
Related details
- David Awogbemila
- Felipe Erias
- Kevin Ellis
Kevin Ellis (Gerrit)
Kevin Ellis added 7 comments![]( "Open in Gerrit")
// TODO: "grayscale", "invert" and "sepia should use a an initial value of 0. i->Interpolate(0, progress, EffectModel::kIterationCompositeReplace);Optional nit: though preexisting "i" is an awful variable name for anything other than a loop index. Since we are here, how about we give the variable a better name.
0, 2, EffectModel::kIterationCompositeReplace,Suggest we add a unit test for kIterationCompositeAccumulate.
As a sanity check we DCHECK that the feature is enabled after the early return above.
// TODO Assumes that the property is additive (accumulation is addition).What is to be done here? Helpful if we can flush out the remaining details. Transform accumulation won't be additive. Are there other cases?
void KeyframeEffectModelBase::PropertySpecificKeyframeGroup::CheckIfStatic() {This static check should indicate that the result is not static if using itertationComposite accumulate. Some background on the static optimization:
Without static optimization:
element.animate({ background: ['red', 'green'], ...);
animates on the main thread as background-color is currently the only longhand property for background that is supported on the compositor. In this case, we are also animating background-position-x background-image, ... even though the value is not changing over the course of the animation. With static optimization, these constant valued properties are dropped from consideration when making the compositing decision. With iteration-composite accumulate, these properties are no longer constant valued as they would instead result in a staircase effect, jumping at each iteration boundary.
At some point, we can likely extend to support non-replace, but for now, I don't think the added effort is justified, especially given that iterationComposite accumulate is not supported on the compositor. We still need to tweak to CheckIfStatic even though we won't be compositing since a static property does not tick on every frame on either main or the compositor. The idea is that you just need to pop the effect onto the stack at the start of the active phase and remove it at the end of the active phase. With accumulate, we need to tick every iteration (we do this anyways), but also handle the discontinuity where due to floating point quantization, we could end up on the wrong side of the step discontinuity without some fuzzy handling. We have this handling in place for step timing functions, but introduces a bit of extra bookkeeping that is likely not justified at this stage.
A change is landing shortly to this method to mark properties as provisionally static when there are neutral keyframes. This will place you in a merge conflict, but should be simple enough the handle.
EffectModel::IterationCompositeOperation iteration_composite) {New parameters are not used. Add a TODO.
Related details
- David Awogbemila
- Felipe Erias
Kevin Ellis (Gerrit)
Kevin Ellis added 1 comment![]( "Open in Gerrit")
@kev...@chromium.org would you be okay with being primary reviewer for this CL?
- Alan Cutter
- David Awogbemila
- Felipe Erias
Felipe Erias (Gerrit)
Felipe Erias added 8 comments![]( "Open in Gerrit")
Thank you very much for your feedback. I have rebased the CL and implemented the suggested changes.
// TODO: "grayscale", "invert" and "sepia should use a an initial value of 0.The new method `Scale()` uses the correct default value for interpolation.
This bug still affects the existing `Add()` method, used by `animation-composition: accumulate`, so I have opened crbug.com/461845655
i->Interpolate(0, progress, EffectModel::kIterationCompositeReplace);Optional nit: though preexisting "i" is an awful variable name for anything other than a loop index. Since we are here, how about we give the variable a better name.
Replaced with `interpolation`.
0, 2, EffectModel::kIterationCompositeReplace,Suggest we add a unit test for kIterationCompositeAccumulate.
I have added unit tests for this feature.
In doing so, I found out that this implementation is incorrect when we use multiple keyframes. The reason is that three or more keyframes cause the interpolation to be split into separate segments.
With `iterationComposite:accumulate`, each segment should be added the end keyframe of the previous iteration of the overall animation, but currently there isn't a simple way to get that information. I am still working on it.
```css
@keyframes zIndexTest {
0% { z-index: 0; }
50% { z-index: 50; }
100% { z-index: 100; }
}
.element {
animation-name: zIndexTest;
animation-duration: 1s;
animation-iteration-count: 2;
animation-iteration-composite: accumulate;
}// value at t=1250ms should be 125
```
As a sanity check we DCHECK that the feature is enabled after the early return above.
Done
// TODO Assumes that the property is additive (accumulation is addition).What is to be done here? Helpful if we can flush out the remaining details. Transform accumulation won't be additive. Are there other cases?
I've removed the `TODO` because it can be misleading. This code does not really "assume" anything but simply delegates to the subclass of `InterpolableValue` which will carry out the appropriate logic,
void KeyframeEffectModelBase::PropertySpecificKeyframeGroup::CheckIfStatic() {This static check should indicate that the result is not static if using itertationComposite accumulate. Some background on the static optimization:
Without static optimization:
element.animate({ background: ['red', 'green'], ...);
animates on the main thread as background-color is currently the only longhand property for background that is supported on the compositor. In this case, we are also animating background-position-x background-image, ... even though the value is not changing over the course of the animation. With static optimization, these constant valued properties are dropped from consideration when making the compositing decision. With iteration-composite accumulate, these properties are no longer constant valued as they would instead result in a staircase effect, jumping at each iteration boundary.
At some point, we can likely extend to support non-replace, but for now, I don't think the added effort is justified, especially given that iterationComposite accumulate is not supported on the compositor. We still need to tweak to CheckIfStatic even though we won't be compositing since a static property does not tick on every frame on either main or the compositor. The idea is that you just need to pop the effect onto the stack at the start of the active phase and remove it at the end of the active phase. With accumulate, we need to tick every iteration (we do this anyways), but also handle the discontinuity where due to floating point quantization, we could end up on the wrong side of the step discontinuity without some fuzzy handling. We have this handling in place for step timing functions, but introduces a bit of extra bookkeeping that is likely not justified at this stage.
A change is landing shortly to this method to mark properties as provisionally static when there are neutral keyframes. This will place you in a merge conflict, but should be simple enough the handle.
Thank you for the explanation.
Since this code is in an internal class, I have changed the signature of the method so it can take `iterationComposite` into account:
```
void CheckIfStatic(IterationCompositeOperation iteration_composite);
```
EffectModel::IterationCompositeOperation iteration_composite) {New parameters are not used. Add a TODO.
I've added a "note" instead, since this is an intentional decision and not something that needs to be fixed later.
```
// Note: iteration_composite is unused since transitions don't iterate.
// This parameter exists to conform with the interface in Interpolation.
```
Related details
- Alan Cutter
- 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 6 comments![]( "Open in Gerrit")
// TODO: "grayscale", "invert" and "sepia should use a an initial value of 0.Felipe EriasTODO(crbug.com/.....);
The new method `Scale()` uses the correct default value for interpolation.
This bug still affects the existing `Add()` method, used by `animation-composition: accumulate`, so I have opened crbug.com/461845655
Acknowledged
0, 2, EffectModel::kIterationCompositeReplace,Felipe EriasSuggest we add a unit test for kIterationCompositeAccumulate.
I have added unit tests for this feature.
In doing so, I found out that this implementation is incorrect when we use multiple keyframes. The reason is that three or more keyframes cause the interpolation to be split into separate segments.
With `iterationComposite:accumulate`, each segment should be added the end keyframe of the previous iteration of the overall animation, but currently there isn't a simple way to get that information. I am still working on it.
```css
@keyframes zIndexTest {
0% { z-index: 0; }
50% { z-index: 50; }
100% { z-index: 100; }
}.element {
animation-name: zIndexTest;
animation-duration: 1s;
animation-iteration-count: 2;
animation-iteration-composite: accumulate;
}// value at t=1250ms should be 125
```
OK. How about we mark the CL as WIP until the issue is addressed.
// TODO Assumes that the property is additive (accumulation is addition).Felipe EriasWhat is to be done here? Helpful if we can flush out the remaining details. Transform accumulation won't be additive. Are there other cases?
I've removed the `TODO` because it can be misleading. This code does not really "assume" anything but simply delegates to the subclass of `InterpolableValue` which will carry out the appropriate logic,
Acknowledged
entry.value->CheckIfStatic(iteration_composite_);No need to pass in a data member as an argument since CheckIfStatic is not a static method.
if (iteration_composite ==iteration_composite_ and drop the parameter.
EffectModel::IterationCompositeOperation iteration_composite) {Felipe EriasNew parameters are not used. Add a TODO.
I've added a "note" instead, since this is an intentional decision and not something that needs to be fixed later.
```
// Note: iteration_composite is unused since transitions don't iterate.
// This parameter exists to conform with the interface in Interpolation.
```
- Felipe Erias
Felipe Erias (Gerrit)
Felipe Erias added 4 comments![]( "Open in Gerrit")
Thank you for your review. as mentioned below, I will mark the CL as WIP while I implement the missing functionality.
0, 2, EffectModel::kIterationCompositeReplace,Felipe EriasSuggest we add a unit test for kIterationCompositeAccumulate.
Kevin EllisI have added unit tests for this feature.
In doing so, I found out that this implementation is incorrect when we use multiple keyframes. The reason is that three or more keyframes cause the interpolation to be split into separate segments.
With `iterationComposite:accumulate`, each segment should be added the end keyframe of the previous iteration of the overall animation, but currently there isn't a simple way to get that information. I am still working on it.
```css
@keyframes zIndexTest {
0% { z-index: 0; }
50% { z-index: 50; }
100% { z-index: 100; }
}.element {
animation-name: zIndexTest;
animation-duration: 1s;
animation-iteration-count: 2;
animation-iteration-composite: accumulate;
}// value at t=1250ms should be 125
```
OK. How about we mark the CL as WIP until the issue is addressed.
Yes, I will do so.
Do you have any thoughts or suggestions regarding the best approach to add support for multiple keyframes?
entry.value->CheckIfStatic(iteration_composite_);No need to pass in a data member as an argument since CheckIfStatic is not a static method.
The problem is that `PropertySpecificKeyframeGroup` can not access non-static members of its outer class.
This means that there isn't a straightforward way to reach `iteration_composite_` or `IterationComposite()` from within `CheckIfStatic()`.
As fas as I can see, currently our main options are:
- `PropertySpecificKeyframeGroup` keeps a pointer to its `KeyframeEffectModelBase` owner and calls `owner_->IterationComposite()` when needed
- `CheckIfStatic()` receives `iteration_composite` as a parameter
if (iteration_composite ==iteration_composite_ and drop the parameter.
```cpp
if (iteration_composite_ ==
IterationCompositeOperation::kIterationCompositeAccumulate) {
```
fails to compile with
Use of non-static data member 'iteration_composite_' of 'KeyframeEffectModelBase' from nested type 'PropertySpecificKeyframeGroup'clang(nested_non_static_member_use)
See above.
Related details
- Kevin Ellis