Closed Bug 1394284 Opened 7 years ago Closed 7 years ago

stylo: animate transform with singular matrix as discrete

Categories

(Core :: CSS Parsing and Computation, enhancement, P3)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: hiro, Assigned: chenpighead)

References

(Blocks 2 open bugs,
URL
)

Details

Attachments

(3 files, 1 obsolete file)

Bug 1394284 - add wpt test for fallback discrete type of transform animation.
59 bytes, text/x-review-board-request
boris
: review+
hiro
: review+
Details
Bug 1394284 - add fallback discrete procedure for transform animation.
59 bytes, text/x-review-board-request
boris
: review+
hiro
: review+
Details
Servo PR, #18591
41 bytes, text/x-github-pull-request
Details | Review 
Current stylo handles interpolation between normal and singular matrices as if it's discrete type animations.
Whereas gecko handles the decomposition for singular matrices as it's the identity matrix. Neither way is correct. We should fix both of them.
According to the spec,

```
If one of the matrices for interpolation is non-invertible, the used animation function must fall-back to a discrete animation according to the rules of the respective animation specification.
```

Hi hiro, does this mean that stylo's implementation makes more sense?

I haven't went through the code path of stylo's implementation, but I did confirmed that Gecko would fallback to use a identity matrix while decomposing a singular matrix [1].


[1] https://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/layout/style/nsStyleTransformMatrix.cpp#1202
Flags: needinfo?(hikezoe)
We shouldn't do discrete animation inside the Animate trait. If we can't interpolate, we should return an error and then CSS animations or Web Animations will do discrete animation. We need to be able to detect the non-interpolable case for CSS transitions (and the non-additive case for SMIL).
Oh, I did not notice the sentence in the spec.  Yes, Brian is right. we need to return Err(()) from the animate() method for ComputedMatrix, then we can handle the case as discrete for animations and handle it as non-transitionable for transition.
Flags: needinfo?(hikezoe)
(In reply to Brian Birtles (:birtles) from comment #2)
> We shouldn't do discrete animation inside the Animate trait. If we can't
> interpolate, we should return an error and then CSS animations or Web
> Animations will do discrete animation. We need to be able to detect the
> non-interpolable case for CSS transitions (and the non-additive case for
> SMIL).

Oh, I see. This explains why hiro would say "Neither way is correct".

In Gecko, we fallback to use identity matrix and hide the fact of animating from/to a singular matrix. In Stylo, we fallback to do discrete animation inside the Animate trait [1].

The correct behavior would be letting the matrix decomposition method reports error for non-invertible matrice, and propagate the error to CSS animations or Web animations so they can do the fallback discrete animation accordingly.

Brian, Hiroyuki, thanks for the quick response. I'll take a look at this. :)


[1] https://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/servo/components/style/properties/helpers/animated_properties.mako.rs#1270-1274
Assignee: nobody → jeremychen
Status: NEW → ASSIGNED
Yes, there is a bug [1]. We don't propagate the Err(()) to the caller of animate(). Therefore, if we want to fix the bug in comment 2, I think we should fix [1]. (But you have to make sure this fix doesn't affect the interpolation of mismatch transform lists because it seems we cannot distinguish mismatch transform list from the Err(()) of matrix decomposition.)


[1] http://searchfox.org/mozilla-central/rev/f2a1911ad310bf8651f342d719e4f4ca0a7b9bfb/servo/components/style/properties/helpers/animated_properties.mako.rs#2080-2085
Another bug: [1]. In Servo_MatrixTransform_Operate(), we didn't fallback to discrete animation, so I believe you have to update this, too.

[1] http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/servo/ports/geckolib/glue.rs#2151-2153
(In reply to Boris Chiou [:boris] from comment #6)
> Another bug: [1]. In Servo_MatrixTransform_Operate(), we didn't fallback to
> discrete animation, so I believe you have to update this, too.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/servo/ports/geckolib/glue.rs#2151-
> 2153

This is for resolving mismatched transform lists during "layout".
Ok, IIUC there 3 cases that we need to consider:

1. matched transform lists, both have a single matrix(), at least one of the matrix is singular

The shouldn't be a problem. We can still capture the error from the matrix decomposition method, and propagate it to CSS animations or Web animations (though I haven't got through the code where the CSS animations or Web animations handles this).

2. matched transform lists, at least one of the lists has a singular matrix() function

This is what Boris just mentioned in comment 5, which is a bit unclear to me. Should we let the whole transform lists to fall back to discrete animation? Or, we just want the matrix() to do the fallback discrete animation, and leave the rest transform functions animate normally?

3. mismatched transform lists

Here, I guess we should just do the same thing as the 1st case. Maybe put the fallback discrete process in [1]


Hi Brian, any idea about how we should handle 2nd case?


[1] https://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/servo/ports/geckolib/glue.rs#2115-2134
Flags: needinfo?(bbirtles)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #8)
> Ok, IIUC there 3 cases that we need to consider:
> 
> 1. matched transform lists, both have a single matrix(), at least one of the
> matrix is singular
> 
> The shouldn't be a problem. We can still capture the error from the matrix
> decomposition method, and propagate it to CSS animations or Web animations
> (though I haven't got through the code where the CSS animations or Web
> animations handles this).

It's this code here:

  http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/servo/ports/geckolib/glue.rs#552-558

This applies to all animations, but for transitions we do a preliminary check that the values are interpolable before creating the transition:

  http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/layout/style/nsTransitionManager.cpp#895

The implementation for which is here:

  http://searchfox.org/mozilla-central/rev/67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/servo/ports/geckolib/glue.rs#329-335

> 2. matched transform lists, at least one of the lists has a singular
> matrix() function
> 
> This is what Boris just mentioned in comment 5, which is a bit unclear to
> me. Should we let the whole transform lists to fall back to discrete
> animation? Or, we just want the matrix() to do the fallback discrete
> animation, and leave the rest transform functions animate normally?

In the case where we have a singular matrix() then yes, I believe we should fall back to discrete animation.

I believe Boris was concerned that this doesn't make mismatched transform lists *without* a singular matrix also fall back to discrete animation, i.e. case 3.

> 3. mismatched transform lists
> 
> Here, I guess we should just do the same thing as the 1st case. Maybe put
> the fallback discrete process in [1]

I believe for mismatched transform list we fall back to matrix interpolation, right? i.e. This is not the same as the first case which is discrete animation.

Perhaps I am misunderstanding something?
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles) from comment #9)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #8)
> > Ok, IIUC there 3 cases that we need to consider:
> > 
> > 1. matched transform lists, both have a single matrix(), at least one of the
> > matrix is singular
> > 
> > The shouldn't be a problem. We can still capture the error from the matrix
> > decomposition method, and propagate it to CSS animations or Web animations
> > (though I haven't got through the code where the CSS animations or Web
> > animations handles this).
> 
> It's this code here:
> 
>  
> http://searchfox.org/mozilla-central/rev/
> 67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/servo/ports/geckolib/glue.rs#552-558
> 
> This applies to all animations, but for transitions we do a preliminary
> check that the values are interpolable before creating the transition:
> 
>  
> http://searchfox.org/mozilla-central/rev/
> 67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/layout/style/nsTransitionManager.
> cpp#895
> 
> The implementation for which is here:
> 
>  
> http://searchfox.org/mozilla-central/rev/
> 67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/servo/ports/geckolib/glue.rs#329-335
> 

Nice!! Thanks for pointing them out. :)

> > 2. matched transform lists, at least one of the lists has a singular
> > matrix() function
> > 
> > This is what Boris just mentioned in comment 5, which is a bit unclear to
> > me. Should we let the whole transform lists to fall back to discrete
> > animation? Or, we just want the matrix() to do the fallback discrete
> > animation, and leave the rest transform functions animate normally?
> 
> In the case where we have a singular matrix() then yes, I believe we should
> fall back to discrete animation.

I still don't understand. Probably I should use an example...

ex.
from_list: [ Scale(2), Matrix(1,0,0,0) ]
to_list: [ Scale(3), Matrix(1,2,3,4) ]

In the current implementation, we'll let Matrix() part do the discrete animation, and Scale() part do the normal animation. However, it's unclear to me, should we discretely animate the whole from_list to to_list?
Or, current implementation is correct and fixing this bug should not regress the current behavior?

> I believe Boris was concerned that this doesn't make mismatched transform
> lists *without* a singular matrix also fall back to discrete animation, i.e.
> case 3.
> 
> > 3. mismatched transform lists
> > 
> > Here, I guess we should just do the same thing as the 1st case. Maybe put
> > the fallback discrete process in [1]
> 
> I believe for mismatched transform list we fall back to matrix
> interpolation, right? i.e. This is not the same as the first case which is
> discrete animation.
> 
> Perhaps I am misunderstanding something?

Yes, we fall back to matrix interpolation.
I discussed a bit with Boris, it turns out that the InterpolateMatrix will keep as it is in the style system. Once our layout engine is ready for rendering, we'll send it back to here [1] and do the interpolation. So, fallback mechanism would probably lay in here [1]. I think propagating the err() to [1] and fixing in [1] should just work.


[1] https://searchfox.org/mozilla-central/rev/4d8e389498a08668cce9ebf6232cc96be178c3e4/servo/ports/geckolib/glue.rs#2115-2134
Flags: needinfo?(bbirtles)
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #10)
> (In reply to Brian Birtles (:birtles) from comment #9)
> > (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #8)
> > > Ok, IIUC there 3 cases that we need to consider:
> > > 
> > > 1. matched transform lists, both have a single matrix(), at least one of the
> > > matrix is singular
> > > 
> > > The shouldn't be a problem. We can still capture the error from the matrix
> > > decomposition method, and propagate it to CSS animations or Web animations
> > > (though I haven't got through the code where the CSS animations or Web
> > > animations handles this).
> > 
> > It's this code here:
> > 
> >  
> > http://searchfox.org/mozilla-central/rev/
> > 67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/servo/ports/geckolib/glue.rs#552-558
> > 
> > This applies to all animations, but for transitions we do a preliminary
> > check that the values are interpolable before creating the transition:
> > 
> >  
> > http://searchfox.org/mozilla-central/rev/
> > 67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/layout/style/nsTransitionManager.
> > cpp#895
> > 
> > The implementation for which is here:
> > 
> >  
> > http://searchfox.org/mozilla-central/rev/
> > 67f38de2443e6b613d874fcf4d2cd1f2fc3d5e97/servo/ports/geckolib/glue.rs#329-335
> > 
> 
> Nice!! Thanks for pointing them out. :)
> 
> > > 2. matched transform lists, at least one of the lists has a singular
> > > matrix() function
> > > 
> > > This is what Boris just mentioned in comment 5, which is a bit unclear to
> > > me. Should we let the whole transform lists to fall back to discrete
> > > animation? Or, we just want the matrix() to do the fallback discrete
> > > animation, and leave the rest transform functions animate normally?
> > 
> > In the case where we have a singular matrix() then yes, I believe we should
> > fall back to discrete animation.
> 
> I still don't understand. Probably I should use an example...
> 
> ex.
> from_list: [ Scale(2), Matrix(1,0,0,0) ]
> to_list: [ Scale(3), Matrix(1,2,3,4) ]
> 
> In the current implementation, we'll let Matrix() part do the discrete
> animation, and Scale() part do the normal animation. However, it's unclear
> to me, should we discretely animate the whole from_list to to_list?
> Or, current implementation is correct and fixing this bug should not regress
> the current behavior?

As far as I understand, the current implementation is incorrect and we should discretely animate the whole from_list to to_list.

The spec says,

  "If one of the matrices for interpolation is non-invertible, the used animation function must
   fall-back to a discrete animation according to the rules of the respective animation specification."

which isn't entirely clear (especially since it is in the context of interpolating a single matrix) but I think it means that the whole thing falls back to discrete animation.

If other browsers just discretely animate the matrix then we can update the spec, otherwise we should fall back to discrete animation for the whole function.
Flags: needinfo?(bbirtles)
(In reply to Brian Birtles (:birtles, away until 19 Sep) from comment #11)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #10)
> > (In reply to Brian Birtles (:birtles) from comment #9)
> > I still don't understand. Probably I should use an example...
> > 
> > ex.
> > from_list: [ Scale(2), Matrix(1,0,0,0) ]
> > to_list: [ Scale(3), Matrix(1,2,3,4) ]
> > 
> > In the current implementation, we'll let Matrix() part do the discrete
> > animation, and Scale() part do the normal animation. However, it's unclear
> > to me, should we discretely animate the whole from_list to to_list?
> > Or, current implementation is correct and fixing this bug should not regress
> > the current behavior?
> 
> As far as I understand, the current implementation is incorrect and we
> should discretely animate the whole from_list to to_list.
> 
> The spec says,
> 
>   "If one of the matrices for interpolation is non-invertible, the used
> animation function must
>    fall-back to a discrete animation according to the rules of the
> respective animation specification."
> 
> which isn't entirely clear (especially since it is in the context of
> interpolating a single matrix) but I think it means that the whole thing
> falls back to discrete animation.
> 
> If other browsers just discretely animate the matrix then we can update the
> spec, otherwise we should fall back to discrete animation for the whole
> function.

Ok, thank you for clarifying this for me. :)
Hmm, I tried a little bit, and found that the point is we don't have a way to tell the Err() in [1] and the Err() in [2], where the former could be an Err() caused by decomposing a singular matrix for matched TransformLists, and the latter is an Err() caused by handling mismatched TransformLists. The ability to tell the difference could help us make the right decision here [3] about whether we should return Err() and let it fall back to the caller and do the discrete animation, or let it fall through to the code path [4] and do the Interpolate for mismatched TransformLists. This is exactly the bug Boris mentioned in comment 5.

One solution could be change the function signature in Animate trait [5], so we can let the return type of Err() be an enum or something similar, and tell the difference of the error reasons. But I'm not sure if this is an overkill. I guess, having the ability to report more specific errors for animation code path could be an extra benefit if we decide to do this way.

I'll keep trying if I can fix this without changing the function signature of animate(). Any feedback or suggestions are welcome.



[1] https://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/servo/components/style/properties/helpers/animated_properties.mako.rs#1193-1195
[2] https://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/servo/components/style/properties/helpers/animated_properties.mako.rs#1261
[3] https://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/servo/components/style/properties/helpers/animated_properties.mako.rs#2262
[4] https://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/servo/components/style/properties/helpers/animated_properties.mako.rs#2268-2274
[5] https://searchfox.org/mozilla-central/rev/44c693914255638d74bcf1ec3b0bcd448dfab2fd/servo/components/style/values/animated/mod.rs#46
Can't we return TransformOperation::InterpolateMatrix or AccumulateMatrix from TransformOperation::animate() respectively?  I think it's the fundamental problem because both of InterpolateMatrix and AccumulateMatrix are not an error at the moment.
Summary: stylo: Need a way to decompose singular matrix → stylo: animate singular matrix as discrete
(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> Can't we return TransformOperation::InterpolateMatrix or AccumulateMatrix
> from TransformOperation::animate() respectively?  I think it's the
> fundamental problem because both of InterpolateMatrix and AccumulateMatrix
> are not an error at the moment.

Good idea!!! Putting the fallback mechanism for animating singular matrices in TransformOperation::animate() makes sense. In this way, we could just fallback to InterpolateMatrix/AccumulateMatrix for animating singular matrices, and still let the Err() in TransformOperation::animate() [1] means "fallback to the mismatched TransformLists". Thanks for the suggestion.


[1] https://searchfox.org/mozilla-central/rev/00fa5dacedb925022f53d025121f1a919508e7ce/servo/components/style/properties/helpers/animated_properties.mako.rs#1255
Attachment #8906381 - Flags: review?(boris.chiou)
The patch is slightly different from what I meant.  What I meant in comment 15 is returning InterpolateMatrix in the case where we have different TransformOperation.

A couple of other notes;
1) we should write web plartform tests for this case.
2) we need to fix the fallback case for servo.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> The patch is slightly different from what I meant.  What I meant in comment
> 15 is returning InterpolateMatrix in the case where we have different
> TransformOperation.

Oh, I see.

> A couple of other notes;
> 1) we should write web plartform tests for this case.

IIUC, the current approach doesn't make any behavior change for Stylo. Could you enlighten me a bit more about what kind of test do you expect to see? 

> 2) we need to fix the fallback case for servo.

Will do.


Due to the feedback here, clear the review request for now.
Attachment #8906381 - Flags: review?(boris.chiou)
Currently we have no test cases for transform for discrete types [1].  It would be nice to have a couple of test cases there.

[1] https://hg.mozilla.org/mozilla-central/file/bda524beac24/testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js#l1393
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #20)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #19)
> > The patch is slightly different from what I meant.  What I meant in comment
> > 15 is returning InterpolateMatrix in the case where we have different
> > TransformOperation.
> 
> Oh, I see.
> 

Second thought, according to comment 11, I think we should just treat the singular matrices animation the same as mismatched transform lists. So, no need to worry how to tell the difference between the Err() reported from the mismatched TransformOperations and the Err() reported from the matched but singular TransformOperation::Matrix pair, they mean the same, which is "fallback to use the InterpolateMatrix". And then, the Servo_MatrixTransform_Operate() would handle the fallback discrete animation properly.

I'll update the patch, add tests, then request review again.
Summary: stylo: animate singular matrix as discrete → stylo: animate transform with singular matrix as discrete
Comment on attachment 8906381 [details]
Bug 1394284 - stylo: do not handle the fallback discrete animation inside the Animate trait.

https://reviewboard.mozilla.org/r/178092/#review183668

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1184
(Diff revision 3)
> -                Ok(TransformOperation::Matrix(
> -                    this.animate(other, procedure)?,
> -                ))
> +                match this.animate(other, procedure) {
> +                    Ok(result) => Ok(TransformOperation::Matrix(result)),
> +                    _ => Err(()),

I think using ```?``` has the same meaning?
```this.animate(other, procedure)?``` will return Err(()) if we cannot unwrap it.

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1414
(Diff revision 3)
> -                _ => {
> -                    let (this_weight, other_weight) = procedure.weights();
> -                    let result = if this_weight > other_weight { *self } else { *other };
> +                // For non-invertible matrices, instead of doing a discrete matrix animation,
> +                // we report Err here, and let the caller do the fallback procedure.
> +                _ => Err(())

Actually, there are two different cases which return Err(()) during matrix decomposition [1]. I'm not sure how to define the second case in mathimatical term, but it might be better to mention that. e.g. "For non-invertible matrices and other matrix decomposition errors"? It depends on you.

[1] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/servo/components/style/properties/helpers/animated_properties.mako.rs#1836-1838,1854-1857

::: servo/components/style/properties/helpers/animated_properties.mako.rs:1436
(Diff revision 3)
> -            _ => {
> -                let (this_weight, other_weight) = procedure.weights();
> -                let result = if this_weight > other_weight { *self } else { *other };
> +            // For non-invertible matrices, instead of doing a discrete matrix animation,
> +            // we report Err here, and let the caller do the fallback procedure.
> +            _ => Err(())

ditto.
Attachment #8906381 - Flags: review?(boris.chiou) → review+
Comment on attachment 8906908 [details]
Bug 1394284 - add wpt test for fallback discrete type of transform animation.

https://reviewboard.mozilla.org/r/178638/#review183678

::: testing/web-platform/tests/web-animations/animation-model/animation-types/property-list.js:1398
(Diff revision 1)
> +        options: [ [ 'matrix(1, 1, 0, 0, 0, 100)',
> +                     'matrix(-1, 0, 0, -1, 200, 0)' ] ] }

Could we have an other test cases like this:
```
'translate(100px) matrix(1, 1, 0, 0, 100, 0) rotate(45deg)',
'translate(50px) matrix(-1, 0, 0, -1, 200, 0) rotate(30deg)'
```

I would like to check this case.

For mismatched transform lists including a non-invertible matrix, it would be better.
e.g.
```
'translate(100px) matrix(1, 1, 0, 0, 100, 0) skew(45deg)',
'translate(50px) matrix(-1, 0, 0, -1, 200, 0) scale(2.0)'
```
Comment on attachment 8906381 [details]
Bug 1394284 - stylo: do not handle the fallback discrete animation inside the Animate trait.

https://reviewboard.mozilla.org/r/178092/#review183684

BTW, the current implementation of Animate of TransformList uses one pass to go through the transform lists. If there is any interpolation error in a matched transform function pair, it fall-back to "InterpolateMatrix" (which uses more memory and calculation), and leave the handle of discrete animation to ```Servo_MatrixTransform_Operate()```. In other words, we never use the handle of discrete animation in ```Servo_AnimationCompose``` [1]. I don't like this way, actually, because we shouldn't treat the non-invertible matrix case as InterpolateMatrix. This patch is ok, but I think we should file another bug for this problem. Thanks.

[1] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/servo/ports/geckolib/glue.rs#553-559
Comment on attachment 8906908 [details]
Bug 1394284 - add wpt test for fallback discrete type of transform animation.

https://reviewboard.mozilla.org/r/178638/#review183678

> Could we have an other test cases like this:
> ```
> 'translate(100px) matrix(1, 1, 0, 0, 100, 0) rotate(45deg)',
> 'translate(50px) matrix(-1, 0, 0, -1, 200, 0) rotate(30deg)'
> ```
> 
> I would like to check this case.
> 
> For mismatched transform lists including a non-invertible matrix, it would be better.
> e.g.
> ```
> 'translate(100px) matrix(1, 1, 0, 0, 100, 0) skew(45deg)',
> 'translate(50px) matrix(-1, 0, 0, -1, 200, 0) scale(2.0)'
> ```

Both tests look great!! I'll add them in the next version. Thanks.
Blocks: 1399049
Comment on attachment 8906381 [details]
Bug 1394284 - stylo: do not handle the fallback discrete animation inside the Animate trait.

https://reviewboard.mozilla.org/r/178092/#review183668

> I think using ```?``` has the same meaning?
> ```this.animate(other, procedure)?``` will return Err(()) if we cannot unwrap it.

Yes, you're right. I guess I forgot to undo this which is for the previous design approach.
I'll undo this in the next version.

> Actually, there are two different cases which return Err(()) during matrix decomposition [1]. I'm not sure how to define the second case in mathimatical term, but it might be better to mention that. e.g. "For non-invertible matrices and other matrix decomposition errors"? It depends on you.
> 
> [1] http://searchfox.org/mozilla-central/rev/51eae084534f522a502e4b808484cde8591cb502/servo/components/style/properties/helpers/animated_properties.mako.rs#1836-1838,1854-1857

Ok, I'll update the comment to make it more precise and correct.
Maybe something like "Matrices can be undecomposable due to couple reasons, e.g., non-invertible matrices. In this case, we should report Err here, and let the caller do the fallback procedure."
Comment on attachment 8906381 [details]
Bug 1394284 - stylo: do not handle the fallback discrete animation inside the Animate trait.

https://reviewboard.mozilla.org/r/178092/#review183684

Filed Bug 1399049 for this.
try looks good w/o the test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c025fa1623650a2abdb934ecd5e102c5c8f5d3d
So, looks like the current implementation didn't regress anything.

try w/ the wpt test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5657c9bf17843b23730cd352b6c06ae9a945c686&selectedJob=130275816
I guess I should handle the accumulation cases as well to fix these failures in accumulation-per-property.html and addition-per-property.html

Moreover, I should handle Gecko (as comment 0 said) as well to pass the tests on the stylo-disabled.
latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8a14bc110f95d17d4e309d321cf332bb9a5d3594

With the updated servo patch, and adding more tests, try looks good on stylo.

However, after I apply the fix for Gecko, I can't pass one of the newly added test. Not sure if Gecko does something special for matched transform lists... keep digging.
Comment on attachment 8906908 [details]
Bug 1394284 - add wpt test for fallback discrete type of transform animation.

https://reviewboard.mozilla.org/r/178638/#review185300

It seems you are trying to fix this on Gecko side, so I clear this review request for now.
Attachment #8906908 - Flags: review?(boris.chiou)
This is sooooo wierd. As to the test for matched transform function pair below:
```
From: transform: translate(50px)  matrix(-1, 0, 0, -1, 200, 0);
To: transform: translate(100px) matrix( 1, 1, 0,  0, 0, 100);
```

In Gecko, it seems that we somehow interpolate translate() first, and then multiply the result with the interpolate result of matrix(), which lead us to non-discrete behavior. Therefore, the tests for "non-invertible matrices in matched transform lists" would pass at 0ms, but fail at 499ms.
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #37)
> This is sooooo wierd. As to the test for matched transform function pair
> below:
> ```
> From: transform: translate(50px)  matrix(-1, 0, 0, -1, 200, 0);
> To: transform: translate(100px) matrix( 1, 1, 0,  0, 0, 100);
> ```
> 
> In Gecko, it seems that we somehow interpolate translate() first, and then
> multiply the result with the interpolate result of matrix(), which lead us
> to non-discrete behavior. Therefore, the tests for "non-invertible matrices
> in matched transform lists" would pass at 0ms, but fail at 499ms.

I'm going to annotate this test for Gecko, and request for review then. We could investigate the Gecko failure in a followup I think.

Looks like someone forgot to update manifest.json for wpt tests, so there are some unrelated diffs in the test part. Sorry about the noise...
Attachment #8907541 - Flags: review?(boris.chiou)
Attachment #8906908 - Flags: review?(boris.chiou)
Attachment #8906908 - Flags: review?(boris.chiou)
Blocks: 1400167
(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #38)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #37)
> > This is sooooo wierd. As to the test for matched transform function pair
> > below:
> > ```
> > From: transform: translate(50px)  matrix(-1, 0, 0, -1, 200, 0);
> > To: transform: translate(100px) matrix( 1, 1, 0,  0, 0, 100);
> > ```
> > 
> > In Gecko, it seems that we somehow interpolate translate() first, and then
> > multiply the result with the interpolate result of matrix(), which lead us
> > to non-discrete behavior. Therefore, the tests for "non-invertible matrices
> > in matched transform lists" would pass at 0ms, but fail at 499ms.
> 
> I'm going to annotate this test for Gecko, and request for review then. We
> could investigate the Gecko failure in a followup I think.

Filed Bug 1400167.

> Looks like someone forgot to update manifest.json for wpt tests, so there
> are some unrelated diffs in the test part. Sorry about the noise...
Comment on attachment 8906908 [details]
Bug 1394284 - add wpt test for fallback discrete type of transform animation.

https://reviewboard.mozilla.org/r/178638/#review185852
Attachment #8906908 - Flags: review?(boris.chiou) → review+
Attachment #8906908 - Flags: review?(hikezoe)
Hi hiro, the patch which adds tests to wpt is ok to me, but I'd like to see your comment on this patch because I'm not very familiar with addition and accumulation.
Attachment #8907541 - Flags: review?(hikezoe)
Comment on attachment 8907541 [details]
Bug 1394284 - add fallback discrete procedure for transform animation.

https://reviewboard.mozilla.org/r/179246/#review185860

Looks ok to me. I add hiro because I'd like to see what hiro thinks about the accumulation part.
Attachment #8907541 - Flags: review?(boris.chiou) → review+
Comment on attachment 8906908 [details]
Bug 1394284 - add wpt test for fallback discrete type of transform animation.

https://reviewboard.mozilla.org/r/178638/#review186218

Thanks for writing test cases! These test cases look pretty good. I think accumulation also behaves as discrete if there is any non-invertible matricies.  Also, we should definitely add the same test cases for addition to make it clear that addition never behaves as discrete.
Comment on attachment 8907541 [details]
Bug 1394284 - add fallback discrete procedure for transform animation.

https://reviewboard.mozilla.org/r/179246/#review186220

::: layout/style/nsStyleTransformMatrix.cpp:497
(Diff revision 2)
>      Operator::operateForScale(scale1, scale2, aProgress);
>    if (scale != Point3D(1.0, 1.0, 1.0)) {
>      result.PreScale(scale.x, scale.y, scale.z);
>    }
>  
> +  // XXX: Do we need a fallback procedure for accumulating non-invertible matrices?

Oh, I now realized this comment. Yes, we need the fallback for accumulation too.
Comment on attachment 8906908 [details]
Bug 1394284 - add wpt test for fallback discrete type of transform animation.

https://reviewboard.mozilla.org/r/178638/#review186222

::: testing/web-platform/meta/web-animations/animation-model/animation-types/interpolation-per-property.html.ini:18
(Diff revision 3)
> +  [transform: non-invertible matrices in matched transform lists]
> +    expected:
> +      if stylo: PASS
> +      FAIL

Please file a bug for gecko and make it block behavior changes bug (bug 1365771).
(In reply to Hiroyuki Ikezoe (:hiro) from comment #47)
> Comment on attachment 8906908 [details]
> Bug 1394284 - add wpt test for fallback discrete type of transform animation.
> 
> https://reviewboard.mozilla.org/r/178638/#review186218
> 
> Thanks for writing test cases! These test cases look pretty good. I think
> accumulation also behaves as discrete if there is any non-invertible
> matricies.  Also, we should definitely add the same test cases for addition
> to make it clear that addition never behaves as discrete.

Ok, IIUC, for non-invertible matrices, accumulation should behaves as discrete, just like interpolation for non-invertible matrices does.

I'll add the same test cases for addition to ensure that addition never behaves as discrete as well.

Probably we should file spec issues for these fallback behavior? Though, they are somehow included in the non-normative sections in https://www.w3.org/TR/web-animations/#procedures-for-animating-properties

(In reply to Hiroyuki Ikezoe (:hiro) from comment #49)
> Comment on attachment 8906908 [details]
> Bug 1394284 - add wpt test for fallback discrete type of transform animation.
> 
> https://reviewboard.mozilla.org/r/178638/#review186222
> 
> :::
> testing/web-platform/meta/web-animations/animation-model/animation-types/
> interpolation-per-property.html.ini:18
> (Diff revision 3)
> > +  [transform: non-invertible matrices in matched transform lists]
> > +    expected:
> > +      if stylo: PASS
> > +      FAIL
> 
> Please file a bug for gecko and make it block behavior changes bug (bug
> 1365771).

I've already filed Bug 1400167 for this, and I'll add the dependency accordingly, thank you.
Comment on attachment 8906908 [details]
Bug 1394284 - add wpt test for fallback discrete type of transform animation.

https://reviewboard.mozilla.org/r/178638/#review187038

Thanks for the test cases!  The test case for accumulation does not look right. Please refer to testAdditionOrAccumulation for discreteType.
Attachment #8906908 - Flags: review?(hikezoe)
Comment on attachment 8907541 [details]
Bug 1394284 - add fallback discrete procedure for transform animation.

https://reviewboard.mozilla.org/r/179246/#review187042

::: commit-message-b2a0c:1
(Diff revision 3)
> +Bug 1394284 - add fallback discrete interpolation procedure for transform animation.

Nit: We can drop 'interpolation' since we handle both of interpolation and accumulation cases.

::: layout/style/nsStyleTransformMatrix.cpp:415
(Diff revision 3)
>  
> +  // Check if both matrices are decomposable.
> +  bool isDecomposable;
>    Matrix matrix2d1, matrix2d2;
>    if (aMatrix1.Is2D(&matrix2d1) && aMatrix2.Is2D(&matrix2d2)) {
> -    Decompose2DMatrix(matrix2d1, scale1, shear1, rotate1, translate1);
> +    isDecomposable =

Nit: 'wasDicomposed' I guess?
Attachment #8907541 - Flags: review?(hikezoe) → review+
Comment on attachment 8906908 [details]
Bug 1394284 - add wpt test for fallback discrete type of transform animation.

https://reviewboard.mozilla.org/r/178638/#review187262

Looks better now.
In test, generally, test data that is far away from test codes is hard to read/understand, so I don't think creating testDiscreteAccumulation is a good idea.  I prefer to write each test with each test data that is written inside the test respectively. Also we should name each tests properly, one is the test case that 'from' is the singular, the other is that 'to' is the singular.
r=me with these changes.
Attachment #8906908 - Flags: review?(hikezoe) → review+
Comment on attachment 8906908 [details]
Bug 1394284 - add wpt test for fallback discrete type of transform animation.

https://reviewboard.mozilla.org/r/178638/#review187262

Ok, sounds reasonable. I'll rewrite these tests with proper naming and description. Thank you for the review.
Attachment #8906381 - Attachment is obsolete: true
Pushed by jichen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9044841344c9
add fallback discrete procedure for transform animation. r=boris,hiro
https://hg.mozilla.org/integration/autoland/rev/2d942fb27e39
add wpt test for fallback discrete type of transform animation. r=boris,hiro
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: