Closed
Bug 1387990
Opened 8 years ago
Closed 7 years ago
stylo: Many interpolations of transform are failed in test_transitions_per_property.html
Categories
(Core :: CSS Parsing and Computation, defect, P2)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: boris, Assigned: boris)
References
Details
Attachments
(3 files, 2 obsolete files)
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.
Comment 1•8 years ago
|
||
(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?
Assignee | ||
Comment 2•8 years ago
|
||
(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.
Comment 3•8 years ago
|
||
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
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
(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.
Assignee | ||
Comment 6•8 years ago
|
||
Will check this after pushing my ComputeSquaredDistance patches. If anyone is working on this or interested on this. Please feel free to take this.
Assignee: nobody → boris.chiou
Assignee | ||
Updated•8 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 7•8 years ago
|
||
> 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.
Assignee | ||
Comment 8•8 years ago
|
||
Move the rotatex/rotatey failures into Bug 1389429.
Assignee | ||
Comment 9•8 years ago
|
||
(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...
Assignee | ||
Comment 10•8 years ago
|
||
Visual result is weird on Safari and Stylo
Comment 11•8 years ago
|
||
Gecko and Stylo look the same to me on Nightly.
Assignee | ||
Comment 12•8 years ago
|
||
(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.)
Comment 13•8 years ago
|
||
Oh Servo Servo! Yes, that's a little odd I guess.
Assignee | ||
Comment 14•8 years ago
|
||
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".
Updated•8 years ago
|
Priority: -- → P2
Assignee | ||
Comment 15•8 years ago
|
||
(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.
Assignee | ||
Comment 16•8 years ago
|
||
(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)
Comment 17•8 years ago
|
||
Nice work Boris!
Assignee | ||
Comment 18•7 years ago
|
||
(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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 21•7 years ago
|
||
I have to rebase part 1 later.
Assignee | ||
Comment 22•7 years ago
|
||
mozreview-review |
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 23•7 years ago
|
||
mozreview-review |
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');
}
Assignee | ||
Comment 24•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8898725 -
Flags: review?(bbirtles)
Comment 25•7 years ago
|
||
Great, thanks. I wonder if calling the method 'defined' would be enough.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
mozreview-review |
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
Attachment #8898725 -
Flags: review?(bbirtles) → review+
Comment 30•7 years ago
|
||
Thanks Boris! I'll look at the other two patches tomorrow.
Assignee | ||
Comment 31•7 years ago
|
||
mozreview-review-reply |
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 32•7 years ago
|
||
mozreview-review |
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?
Attachment #8898724 -
Flags: review?(bbirtles) → review+
Comment 33•7 years ago
|
||
mozreview-review |
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
Attachment #8899351 -
Flags: review?(bbirtles) → review+
Assignee | ||
Comment 34•7 years ago
|
||
(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.
Assignee | ||
Comment 35•7 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8898724 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8899351 -
Attachment is obsolete: true
Assignee | ||
Comment 37•7 years ago
|
||
Assignee | ||
Comment 38•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment 40•7 years ago
|
||
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cde574b6be98
Remove skipped transform tests and fix animation utils. r=birtles
Comment 41•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 42•7 years ago
|
||
(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.
You need to log in
before you can comment on or make changes to this bug.
Description
•