I tries to classify the types of failures: 1. from none to rotate: * interpolation of transitions: none to rotatey(720deg) -got "matrix(1, 0, 0, 1, 0, 0)", expected "matrix3d(-1, 0, 0, 0, 0, 1, 0, 0, 0, 0, -1, 0, 0, 0, 0, 1)" * interpolation of transitions: none to rotatex(720deg) - got "matrix(1, 0, 0, 1, 0, 0)", expected "matrix3d(1, 0, 0, 0, 0, -1, 0, 0, 0, 0, -1, 0, 0, 0, 0, 1)" 2. limited values? (i.e. 1.74846e-7 ~= 0) * interpolation of transitions: translate(calc(0.75 * 3em + 1.5 * 10%), calc(0.5 * 5em + 0.5 * 8%)) to rotate(90deg) translateY(20%) rotate(90deg) translateY(calc(10% + 0.5em)) rotate(180deg) - got "matrix(1, 1.74846e-7, -1.74846e-7, 1, 65, 35.25)", expected "matrix(1, 0, 0, 1, 65, 35.25)" 3. matrix: skew * interpolation of transitions: matrix(1, 0, 3, 1, 0, 0) to none: matrix(1, 8.51797e-8, 1.86538, 1.27722, 0, 0) should approximately equal matrix(1, 0, 2.25, 1, 0, 0) * interpolation of transitions: skewX(0) to skewX(-45deg) translate(0): matrix(1, 1.62043e-7, -0.195083, 1.02275, 0, 0) should approximately equal matrix(1, 0, -0.25, 1, 0, 0) 4. matrix decompositions * interpolation of transitions: skewY(60deg) to skewY(-60deg) translateX(0): matrix(0.649519, 0.875, 0.0625, 1.19078, 0, 0) should approximately equal matrix(1.73205, 1, 0.125, 0.649519, 0, 0) * interpolation of transitions: matrix(2, 0, 0, 2, 10, 20) scale(2) to none - got "matrix(3.0625, 5.35464e-7, -5.35464e-7, 3.0625, 7.5, 15)", expected "matrix(3.0625, 0, 0, 3.0625, 7.5, 15)" * interpolation of transitions: matrix(-1, 0, 0, 1, 0, 0) to matrix(1, 0, 0, 1, 0, 0) - got "matrix(-0.5, -8.74228e-8, -1.74846e-7, 1, 0, 0)", expected "matrix(-0.5, 0, 0, 1, 0, 0)" * interpolation of transitions: none to matrix(1, 0, 1.5, 1, 0, 0): matrix(1, 1.55381e-7, 0.249759, 1.06703, 0, 0) should approximately equal matrix(1, 0, 0.375, 1, 0, 0) * interpolation of transitions: skewX(-60deg) rotate(90deg) translate(0) to skewX(60deg) rotate(90deg): matrix(-0.875, 0.649519, -1.19079, 0.0625001, 0, 0) should approximately equal matrix(-1, 1.73205, -0.649519, 0.125, 0, 0) * ... and many others.

(In reply to Boris Chiou [:boris] from comment #0) > * interpolation of transitions: matrix(2, 0, 0, 2, 10, 20) scale(2) to > none - got "matrix(3.0625, 5.35464e-7, -5.35464e-7, 3.0625, 7.5, 15)", > expected "matrix(3.0625, 0, 0, 3.0625, 7.5, 15)" > * interpolation of transitions: matrix(-1, 0, 0, 1, 0, 0) to matrix(1, 0, > 0, 1, 0, 0) - got "matrix(-0.5, -8.74228e-8, -1.74846e-7, 1, 0, 0)", > expected "matrix(-0.5, 0, 0, 1, 0, 0)" I think these two cases are also precision issue. I don't remember how to happen the precision issue (Boris, do you remember?), should we use the same tolerance value we used for test_animations_omta.html?

(In reply to Hiroyuki Ikezoe (:hiro) from comment #1) > (In reply to Boris Chiou [:boris] from comment #0) > > > * interpolation of transitions: matrix(2, 0, 0, 2, 10, 20) scale(2) to > > none - got "matrix(3.0625, 5.35464e-7, -5.35464e-7, 3.0625, 7.5, 15)", > > expected "matrix(3.0625, 0, 0, 3.0625, 7.5, 15)" > > * interpolation of transitions: matrix(-1, 0, 0, 1, 0, 0) to matrix(1, 0, > > 0, 1, 0, 0) - got "matrix(-0.5, -8.74228e-8, -1.74846e-7, 1, 0, 0)", > > expected "matrix(-0.5, 0, 0, 1, 0, 0)" > > I think these two cases are also precision issue. I don't remember how to > happen the precision issue (Boris, do you remember?), should we use the same > tolerance value we used for test_animations_omta.html? For these cases, we are trying to interpolate on 2d matrix, and it seems there are some differences (different algorithms?) while decomposing/interpolating/recomposing 2D matrix between Gecko and Servo, so I think that's why we have the precision issue. I'm not sure which one is better (because Gecko may be worse.) And yes, we can add some tolerance for them.

Yeah, transform decomposition/interpolation/recomposition is really annoying since the spec is not perfect either! For example https://lists.w3.org/Archives/Public/www-style/2013May/0131.html

(In reply to Boris Chiou [:boris] from comment #0) > I tries to classify the types of failures: > > 1. from none to rotate: > * interpolation of transitions: none to rotatey(720deg) -got "matrix(1, 0, > 0, 1, 0, 0)", expected "matrix3d(-1, 0, 0, 0, 0, 1, 0, 0, 0, 0, -1, 0, 0, 0, > 0, 1)" > * interpolation of transitions: none to rotatex(720deg) - got "matrix(1, > 0, 0, 1, 0, 0)", expected "matrix3d(1, 0, 0, 0, 0, -1, 0, 0, 0, 0, -1, 0, 0, > 0, 0, 1)" It seems that the rotation on stylo reverses direction. I thought I fixed the reverse direction rotation in some case.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #4) > (In reply to Boris Chiou [:boris] from comment #0) > > I tries to classify the types of failures: > > > > 1. from none to rotate: > > * interpolation of transitions: none to rotatey(720deg) -got "matrix(1, 0, > > 0, 1, 0, 0)", expected "matrix3d(-1, 0, 0, 0, 0, 1, 0, 0, 0, 0, -1, 0, 0, 0, > > 0, 1)" > > * interpolation of transitions: none to rotatex(720deg) - got "matrix(1, > > 0, 0, 1, 0, 0)", expected "matrix3d(1, 0, 0, 0, 0, -1, 0, 0, 0, 0, -1, 0, 0, > > 0, 0, 1)" > > It seems that the rotation on stylo reverses direction. I thought I fixed > the reverse direction rotation in some case. Oh. these two cases are related to rotate3d, I will double-check it.

Will check this after pushing my ComputeSquaredDistance patches. If anyone is working on this or interested on this. Please feel free to take this.

> 1. from none to rotate: > * interpolation of transitions: none to rotatey(720deg) -got "matrix(1, 0, > 0, 1, 0, 0)", expected "matrix3d(-1, 0, 0, 0, 0, 1, 0, 0, 0, 0, -1, 0, 0, 0, > 0, 1)" > * interpolation of transitions: none to rotatex(720deg) - got "matrix(1, > 0, 0, 1, 0, 0)", expected "matrix3d(1, 0, 0, 0, 0, -1, 0, 0, 0, 0, -1, 0, 0, > 0, 0, 1)" It seems rotatex and rotatey is broken (on Servo side, main thread) because we always return an identity matrix. However, the visual result is correct because we have OMTA.

Move the rotatex/rotatey failures into Bug 1389429.

(In reply to Boris Chiou [:boris] from comment #0) > 3. matrix: skew > * interpolation of transitions: matrix(1, 0, 3, 1, 0, 0) to none: > matrix(1, 8.51797e-8, 1.86538, 1.27722, 0, 0) should approximately equal > matrix(1, 0, 2.25, 1, 0, 0) wow. About this case. It seems Webkit (Safari) and Servo backend have a similar visual result. However, Gecko and Blink (Google Chrome) have the similar result. Hmm.... Actually, the visual result of Gecko and Blink is much better to me. Looks like we have tweak the computation for 2d matrix decomposition/re-composition...

Visual result is weird on Safari and Stylo

Gecko and Stylo look the same to me on Nightly.

(In reply to Brian Birtles (:birtles) from comment #11) > Gecko and Stylo look the same to me on Nightly. Yes, because OMTA. OMTA uses Gecko's matrix decomposition/interpolation/re-composition. If you use "Servo" or Safari, the result is different. It seems Servo and Safari match the implementation of spec, but Gecko doesn't. (I guess Google Chrome doesn't, either.)

Oh Servo Servo! Yes, that's a little odd I guess.

Actually, I prefer the result of Gecko and Google Chrome, and I don't want to revise Gecko's code and many test cases, so I'd like to write a different version of 2d matrix decomposition/interpolation/recomposition for "Stylo".

(In reply to Boris Chiou [:boris] from comment #9) > (In reply to Boris Chiou [:boris] from comment #0) > > > 3. matrix: skew > > * interpolation of transitions: matrix(1, 0, 3, 1, 0, 0) to none: > > matrix(1, 8.51797e-8, 1.86538, 1.27722, 0, 0) should approximately equal > > matrix(1, 0, 2.25, 1, 0, 0) > > wow. About this case. It seems Webkit (Safari) and Servo backend have a > similar visual result. However, Gecko and Blink (Google Chrome) have the > similar result. Hmm.... Actually, the visual result of Gecko and Blink is > much better to me. > > Looks like we have tweak the computation for 2d matrix > decomposition/re-composition... Just wrote a different 2d matrix decomposition according to the algorithm from Gecko, and looks like we can fix this test.

(In reply to Boris Chiou [:boris] from comment #15) > Just wrote a different 2d matrix decomposition according to the algorithm > from Gecko, and looks like we can fix this test. WoW, this can fix most failures, Now we only have these two failures (only happened on compositor thread): 39573 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | compositor transform transition from 'skewY(45deg) rotate(90deg) translate(0)' to 'skewY(-45deg) rotate(90deg)' at 2/3rds duration matches computed style - got matrix(-4.37114e-8, 1, -1, 0.333333, 0, 0), expected matrix(1, 1, -1, 0.333333, 0, 0) 39574 INFO TEST-UNEXPECTED-FAIL | layout/style/test/test_transitions_per_property.html | compositor transform transition from 'skewY(45deg) rotate(90deg) translate(0)' to 'skewY(-45deg) rotate(90deg)' at 2/3rds duration matches computed style: OMTA style and computed style should be equal - OMTA matrix(-4.37114e-8, 1, -1, 0.333333, 0, 0), computed matrix(0, 1, -1, 0.333333, 0, 0)

Nice work Boris!

(In reply to Boris Chiou [:boris] from comment #16) > 39574 INFO TEST-UNEXPECTED-FAIL | > layout/style/test/test_transitions_per_property.html | compositor transform > transition from 'skewY(45deg) rotate(90deg) translate(0)' to 'skewY(-45deg) > rotate(90deg)' at 2/3rds duration matches computed style: OMTA style and > computed style should be equal - OMTA matrix(-4.37114e-8, 1, -1, 0.333333, > 0, 0), computed matrix(0, 1, -1, 0.333333, 0, 0) For this case (i.e. from 'skewY(45deg) rotate(90deg) translate(0)' to 'skewY(-45deg) rotate(90deg)' at 2/3rds duration): 1. The interpolated matrix: a) Gecko: matrix(-0.000000, 1.000000, 0.000000, 0.000000, -1.000000, 0.333333, 0.000000, 0.000000, 0.000000, 0.000000, 1.000000, 0.000000, 0.000000, 0.000000, 0.000000, 1.000000) b) Stylo: matrix( 0.000000, 1.000000, 0.000000, 0.000000, -1.000000, 0.333333, 0.000000, 0.000000, 0.000000, 0.000000, 1.000000, 0.000000, 0.000000, 0.000000, 0.000000, 1.000000) We can see the difference is "matrix.m11", which is "-0.0" in Gecko, and "0.0" in Stylo. 2. The serialization result a) Gecko: matrix(-4.37114e-8, 1, -1, 0.333333, 0, 0) b) Stylo: matrix(0, 1, -1, 0.333333, 0, 0) This seems a bug? in nsAutoString::AppendFloat(). a) AppendFloat(-0.0) => "-4.37114e-8" b) AppendFloat(0.0) => "0" So that the root-cause of this failure, I think

I have to rebase part 1 later.

Comment on attachment 8898724 [details] Bug 1387990 - Part 1: Rewrite decomposition of 2d matrix. https://reviewboard.mozilla.org/r/170130/#review175260 ::: commit-message-279cb:4 (Diff revision 1) > +Bug 1387990 - Part 1: Rewrite decomposition of 2d matrix. > + > +The implementation of 2d matrix decomposition in Servo matches that in > +spec, but it's result is really different from that in Gecko, so the s/it's/its/

Comment on attachment 8898725 [details] Bug 1387990 - Remove skipped transform tests and fix animation utils. https://reviewboard.mozilla.org/r/170132/#review175658 ::: commit-message-21739:6 (Diff revision 1) > +Bug 1387990 - Part 2: Remove skipped transform tests and fix animation utils. > + > +Sometimes, we may get a 2d transform matrix like this: > + matrix(0, 1, -1, 0.33, 0, 0) > +The reason is we might have "rotate(90deg)" in the test case, so the 1st or > +the 4th element could be 0.0, and we shouldn't assume them are 1.0 while assume they are ::: layout/style/test/animation_utils.js:608 (Diff revision 1) > + var m11 = (obj.a === 0.0) ? 0 : obj.a || obj.sx || obj.m11 || 1; > + var m22 = (obj.d === 0.0) ? 0 : obj.d || obj.sy || obj.m22 || 1; > return [ > [ > - obj.a || obj.sx || obj.m11 || 1, > + m11, Is the real bug here just that we are testing for truthiness when instead we should be testing for defined-ness? If so then perhaps we should write a `firstDefined` varargs helper that we can use like: return [ [ firstDefined(obj.a, obj,dx, obj.m11) || 1, obj.b || obj.m12 || 0, (i.e. for the rows where the fall-back value is 0 it probably doesn't matter) Probably something like: function firstDefined(...args) { return args.find(arg => typeof !== 'undefined'); }

Comment on attachment 8898725 [details] Bug 1387990 - Remove skipped transform tests and fix animation utils. https://reviewboard.mozilla.org/r/170132/#review175658 > Is the real bug here just that we are testing for truthiness when instead we should be testing for defined-ness? > > If so then perhaps we should write a `firstDefined` varargs helper that we can use like: > > return [ > [ > firstDefined(obj.a, obj,dx, obj.m11) || 1, > obj.b || obj.m12 || 0, > > (i.e. for the rows where the fall-back value is 0 it probably doesn't matter) > > Probably something like: > > function firstDefined(...args) { > return args.find(arg => typeof !== 'undefined'); > } Cool. I will try this. Yes. I think we should be testing for defined-ness. Only assign "1" if we don't define this value; otherwise, use the value we define. Thanks for the suggestion.

Great, thanks. I wonder if calling the method 'defined' would be enough.

Comment on attachment 8898725 [details] Bug 1387990 - Remove skipped transform tests and fix animation utils. https://reviewboard.mozilla.org/r/170132/#review175750 ::: layout/style/test/animation_utils.js:624 (Diff revision 2) > obj.m23 || 0, > obj.m24 || 0 > ], [ > obj.m31 || 0, > obj.m32 || 0, > obj.sz || obj.m33 || 1, We should use defined() here too right? (Since the default value is 1) ::: layout/style/test/animation_utils.js:630 (Diff revision 2) > obj.m34 || 0 > ], [ > obj.e || obj.tx || obj.m41 || 0, > obj.f || obj.ty || obj.m42 || 0, > obj.tz || obj.m43 || 0, > obj.m44 || 1 And here

Thanks Boris! I'll look at the other two patches tomorrow.

Comment on attachment 8898725 [details] Bug 1387990 - Remove skipped transform tests and fix animation utils. https://reviewboard.mozilla.org/r/170132/#review175750 > We should use defined() here too right? > > (Since the default value is 1) Oh yes. It might be zero for 3d rotate. I will update this. > And here In most cases, m44 shouldn't be zero. However, applying this function on m44 might be better for consistency.

Comment on attachment 8898724 [details] Bug 1387990 - Part 1: Rewrite decomposition of 2d matrix. https://reviewboard.mozilla.org/r/170130/#review176116 Do we have a bug to use Stylo's animation value routines on the compositor? Presumably at some point we want to drop the Gecko implementation? ::: servo/components/style/properties/helpers/animated_properties.mako.rs:1671 (Diff revision 2) > + _ => { > + if self_portion > other_portion { > + Ok(*self) > + } else { > + Ok(*other) > + } > + } Shouldn't we just return Err(()) here and rely on the caller to use discrete animation if needed?

Comment on attachment 8899351 [details] Bug 1387990 - Part 2: Use rewroten decomposition of 2d matrix to compute distance. https://reviewboard.mozilla.org/r/170564/#review176120 ::: commit-message-cd708:1 (Diff revision 1) > +Bug 1387990 - Part 2: Use rewroten decomposition of 2d matrix to compute distance. rewritten

(In reply to Brian Birtles (:birtles) from comment #32) > Do we have a bug to use Stylo's animation value routines on the compositor? > Presumably at some point we want to drop the Gecko implementation? Yes, Bug 1340005 is also for that, I think. It will remove the usage of StyleAnimationValue on the compositor.

Comment on attachment 8898724 [details] Bug 1387990 - Part 1: Rewrite decomposition of 2d matrix. https://reviewboard.mozilla.org/r/170130/#review176116 > Shouldn't we just return Err(()) here and rely on the caller to use discrete animation if needed? It's better. However, unfortunately, in the current implementation, the caller is add_weighted_transform_lists(), which doesn't propagate the Err(()) to Servo_AnimationCompose(). nox's patches fix this bug, so I will update here to return Err(()) after re-basing this. Thanks.

(In reply to Boris Chiou [:boris] from comment #35) > Comment on attachment 8898724 [details] > Bug 1387990 - Part 1: Rewrite decomposition of 2d matrix. > > https://reviewboard.mozilla.org/r/170130/#review176116 > > > Shouldn't we just return Err(()) here and rely on the caller to use discrete animation if needed? > > It's better. However, unfortunately, in the current implementation, the > caller is add_weighted_transform_lists(), which doesn't propagate the > Err(()) to Servo_AnimationCompose(). nox's patches fix this bug, so I will > update here to return Err(()) after re-basing this. Thanks. It seems nox's patches don't propagate the Err(()) to Servo_AnimationCompose(), either. I'd like to keep the original implementation, and file another bug to make sure we propagate Err(()) to Servo_AnimationCompose(), for each TransformOperation.

Pushed by bchiou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/cde574b6be98 Remove skipped transform tests and fix animation utils. r=birtles

(In reply to Boris Chiou [:boris] (pto, 8/25~8/29) from comment #38) > (In reply to Boris Chiou [:boris] from comment #35) > > Comment on attachment 8898724 [details] > > Bug 1387990 - Part 1: Rewrite decomposition of 2d matrix. > > > > https://reviewboard.mozilla.org/r/170130/#review176116 > > > > > Shouldn't we just return Err(()) here and rely on the caller to use discrete animation if needed? > > > > It's better. However, unfortunately, in the current implementation, the > > caller is add_weighted_transform_lists(), which doesn't propagate the > > Err(()) to Servo_AnimationCompose(). nox's patches fix this bug, so I will > > update here to return Err(()) after re-basing this. Thanks. > > It seems nox's patches don't propagate the Err(()) to > Servo_AnimationCompose(), either. I'd like to keep the original > implementation, and file another bug to make sure we propagate Err(()) to > Servo_AnimationCompose(), for each TransformOperation. Filed bug 1394284.