Open Bug 1499862 Opened 4 years ago Updated 4 months ago

Result of matrix interpolation is incorrect

Categories

(Core :: CSS Transitions and Animations, defect, P3)

defect

Tracking

()

ASSIGNED
Tracking Status
firefox64 --- wontfix
firefox105 --- affected

People

(Reporter: arai, Assigned: birtles)

References

(Blocks 1 open bug)

Details

(Keywords: parity-chrome, parity-safari)

Attachments

(6 files, 4 obsolete files)

Attached file testcase
Steps to reproduce:
  1. run Firefox Nightly 64.0a1 (2018-10-17) (64-bit) with clean profile on macOS
  2. open the attached testcase
  3. hover on "good"
  4. hover on "bad"

  the testcase is created from https://loreseeker.fenhl.net/card/emn/15b

Actual result:
  "good" and "bad" rotates in the different direction, between step 3 and step 4
  the only difference is the existence of "translateY(0px)" rule in "bad"

Expected result:
  both rotates in the same direction

  Chrome and Safari rotates in Firefox's "good" direction in both "good" and "bad"
Attached file another testcase
interestingly, if the rotation starts from 0deg instead of 180deg, Chrome and Safari also rotate them in the different direction.
I haven't yet checked the recent spec change, but will be fixed by bug 1472917?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> I haven't yet checked the recent spec change, but will be fixed by bug
> 1472917?

I've tried the binary from bug 1472917 comment #2, but it behaves in the same way as current Nightly used in the comment #0,
and the difference between chrome/safari still exists.
about the website in comment #0, I've suggested the author to apply the same set of transformation between hover vs non-hover, so the different behavior will be no more reproducible there.
That sounds an issue should be fixed by the smarter interpolation, I mean bug 1472917 or more spec changes.
Attached file Simplified test case
Attached file Matrix interpolation debugging (obsolete) —
I don't think this is related to smarter interpolation. Both before and after the new rules this should use matrix interpolation (and I don't think we're going to expand the rules to cover this case).

But even when using matrix interpolation I think Gecko is returning the wrong result. In attachment 9018130 [details] I implemented the steps for matrix interpolation outlined in the spec using JS and at 50% of the way through this animation we should get:

  matrix(-0.5, -0.866025, 0.866025, -0.5, 0, 0)

But instead we get:

  matrix(0.5, 0.866025, -0.866025, 0.5, 0, 0)

Chrome gets the expected result.

This might explain some of the results in bug 1322189 but I think a number of the tests there are bogus in light of the smarter interpolation rules.

I'll dig further into this after fixing bug 1472917.
Assignee: nobody → bbirtles
Status: NEW → ASSIGNED
Summary: The existence of translateY affects the direction of rotateY → Result of matrix interpolation is incorrect
Looks like we end up taking the Matrix3D code path here since decompose_2d_matrix always returns a decomposed 3D matrix.

That takes us to the Animate implementation for Matrix 3D which has the following ominous comment[1]:

    // Gecko doesn't exactly follow the spec here; we use a different procedure
    // to match it

That's as far as I've dug but at a guess, and this is only a guess, we fail to do the angle / scale fixup described here:

    https://drafts.csswg.org/css-transforms-1/#interpolation-of-decomposed-2d-matrix-values

because it seems like we then interpolate the components of the MatrixDecomposed3D independently.

I have no idea why we decided to match Gecko's non-conformant behavior though. Maybe Manish remembers.

[1] https://searchfox.org/mozilla-central/rev/eef79962ba73f7759fd74da658f6e5ceae0fc730/servo/components/style/properties/helpers/animated_properties.mako.rs#1503-1504
I guess I also need to check that that fixup aligns with the 3D version in CSS Transforms 2:

  https://drafts.csswg.org/css-transforms-2/#interpolation-of-decomposed-3d-matrix-values

(I'm not even sure if they're supposed to be compatible or if there's is supposed to be a behavior switch when going from 2D to 3D.)
Attached file Matrix interpolation debugging v2 (obsolete) —
Fixed a bug in the recompose method.
Attachment #9018130 - Attachment is obsolete: true
WebKit clearly has a separate code path for 2D:

  https://github.com/WebKit/webkit/blob/c8125d66e8dd7adc81e45f39f75a99c85b7876eb/Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp#L1587-L1596

We could possibly fix this by just having decompose_2d_matrix return a MatrixDecomposed2D and fall back to the 2D code path which appears to follow the spec. However, looking at Chromium it appears to use the 3D code path for everything so it's probably worth working out why it produces the correct result even for 2D content.

Taking the content from the test case:

  from:     rotate(180deg)
  to:       translate(0px) rotate(300deg)
  progress: 50%

The correct result per CSS Transforms 1 is:

  matrix(-0.5, -0.866025, 0.866025, -0.5, 0, 0)

Gecko currently gives:

  matrix(0.5, 0.866025, -0.866025, 0.5, 0, 0)

So let's work it through and compare the Servo code and Chromium code along
the way:

  from matrix: [-1,0,0,-1,0,0]
  to matrix: [0.5,-0.866025,0.866025,0.5,0,0]

Decompose (we'll skip the details because this part seems to be ok):

  In 2D, CSS Transforms 1 says we should have:

      translate: [0, 0]         translate: [0, 0]
      scale: [1, 1]             scale: [1, 1]
      angle: 180                angle: -60
      m11: 1                    m11: 1
      m12: 0                    m12: 0
      m21: 0                    m21: 0
      m22: 1                    m22: 1

  In Servo we get:

    from:

      MatrixDecomposed3D {
        translate: Translate3D(0.0, 0.0, 0.0),
        scale: Scale3D(1.0, 1.0, 1.0),
        skew: Skew(0.0, 0.0, 0.0),
        perspective: Perspective(0.0, 0.0, 0.0, 1.0),
        quaternion: Quaternion(0.0, 0.0, 0.999999999999999, -0.00000004371139000186241)
      }

    to:

      MatrixDecomposed3D {
        translate: Translate3D(0.0, 0.0, 0.0),
        scale: Scale3D(1.0, 1.0, 1.0),
        skew: Skew(0.0, 0.0, 0.0),
        perspective: Perspective(0.0, 0.0, 0.0, 1.0),
        quaternion: Quaternion(-0.0, -0.0, -0.5000000126183913, 0.8660253964992068)
      }

   i.e. as for interpolation, only the interpolation of the angle/quaternion is interesting.

The dot product we get is:

  -0.5000000504735647

which matches my calculation of:

  -0.49999991257807147

The interpolated result is then:

  Quaternion(0.0, 0.0, 0.5000000126183911, 0.8660253964992068)

which likewise matches my calculation of:

  [0,0,0.5000000437109624,0.8660253785479012]


That's as far as I've gotten so far. I still need to dig into the recompose part but a few discrepancies I've noticed include:

1. The rotation matrix built duration recomposition has the following order in the spec:

      rotationMatrix[0][0] = 1 - 2 * (y * y + z * z)
      rotationMatrix[0][1] = 2 * (x * y - z * w)
      rotationMatrix[0][2] = 2 * (x * z + y * w)
      rotationMatrix[1][0] = 2 * (x * y + z * w)
      rotationMatrix[1][1] = 1 - 2 * (x * x + z * z)
      rotationMatrix[1][2] = 2 * (y * z - x * w)
      rotationMatrix[2][0] = 2 * (x * z - y * w)
      rotationMatrix[2][1] = 2 * (y * z + x * w)
      rotationMatrix[2][2] = 1 - 2 * (x * x + y * y)

   but in Servo it's:

      rotation_matrix.m11 = 1.0 - 2.0 * (y * y + z * z) as f32;
      rotation_matrix.m12 = 2.0 * (x * y + z * w) as f32;
      rotation_matrix.m13 = 2.0 * (x * z - y * w) as f32;
      rotation_matrix.m21 = 2.0 * (x * y - z * w) as f32;
      rotation_matrix.m22 = 1.0 - 2.0 * (x * x + z * z) as f32;
      rotation_matrix.m23 = 2.0 * (y * z + x * w) as f32;
      rotation_matrix.m31 = 2.0 * (x * z + y * w) as f32;
      rotation_matrix.m32 = 2.0 * (y * z - x * w) as f32;
      rotation_matrix.m33 = 1.0 - 2.0 * (x * x + y * y) as f32;

   Notice the difference in sign on some operations. From what I can tell,
   though, both the spec and Servo use column-major indexing so this seems
   off.

   If I fix this, I get the expected result mathematically but not visually so
   I'm clearly misunderstanding something.

2. On the next line the spec has:

     matrix = multiply(matrix, rotationMatrix)

   but Servo has:

     matrix = multiply(rotation_matrix, matrix);

   which would appear to be doing pre-multiplication instead of
   post-multiplication.

3. When interpolation quaternions the spec has:

    if (product == 1.0)
      quaternionDst = quaternionA
      return

   But this fails to account for when product is -1 (which will cause divide
   by zero). It should probably use abs().

   Servo also fails to account for this.

4. The spec defines clamping between -1 and 1 as:

     // Clamp product to -1.0 <= product <= 1.0
     product = max(product, 1.0)
     product = min(product, -1.0)

   but this clearly has the max/min back to front.

I've checked out the chromium source so I can do a 3 way comparison next week between the spec, Servo, and chromium and see what's going on.
Attached file Matrix debugging v3 (obsolete) —
Added a partial 3D code path to my JS implementation of the spec.

I suspect it's still got a few bugs though.
Attachment #9018470 - Attachment is obsolete: true
That's enough for this week, but regarding point (1) above, I'm starting to suspect that Servo is right and the spec is wrong (perhaps it copied the text from gem which is row-major?).

I'm also wondering if the discrepancy with chromium comes from the decomposition to quaternion where chromium's logic may differ from the spec:

  https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/transforms/transformation_matrix.cc?l=702-730&rcl=9bcf97f9b8c37c0d4d14eb649da7a1f4dc85b5ff

At least superficially they appear somewhat different.
Attached file Matrix debugging v4 (obsolete) —
Attachment #9018525 - Attachment is obsolete: true
I wrote up a summary of my findings here:

  https://github.com/w3c/csswg-drafts/issues/3230

Basically, following the spec will _not_ produce the correct result (i.e. same result as CSS Transforms Level 1). I'm still not sure how to produce the same result as Chromium using our current approach. I tried reversing the unit vector and following the recompose steps found in the spec, but it didn't cause the animation to proceed in the right direction. I start to wonder if Chromium is actually doing something different on the compositor.
(In reply to Brian Birtles (:birtles) from comment #17)
> I wrote up a summary of my findings here:
> 
>   https://github.com/w3c/csswg-drafts/issues/3230

GitHub still appears to be down so here is the text of what I posted:


Over in [Mozilla bug 1499862](https://bugzilla.mozilla.org/show_bug.cgi?id=1499862) I'm trying to work out why Gecko rotates in a different direction to WebKit/Chromium when doing matrix interpolation for some content.

I've [re-implemented both the 2D and 3D code paths in JS](https://bug1499862.bmoattachments.org/attachment.cgi?id=9018947) and done a comparison between the spec, Chromium, WebKit, and Gecko.

The particular test case I'm debugging is when animating from `rotate(180deg)` and `translate(0px) rotate(300deg)` which should use matrix interpolation.

At 50% progress the results I get are:

* CSS Transforms 1 (2D code path): `matrix(-0.5, -0.866025, 0.866025, -0.5, 0, 0)`
* CSS Transforms 2 (3D code path): `matrix(0.5, -0.866025, 0.866025, 0.5, 0, 0)`
* Chromium / WebKit: `matrix(-0.5, -0.866025, 0.866025, -0.5, 0, 0)`
* Gecko: `matrix(0.5, 0.866025, -0.866025, 0.5, 0, 0)`

In WebKit there is a separate 2D code path, but for Chromium the same 3D code path appears to be used for all cases, at least when running on the main thread (but this code appears not to have changed since it was introduced to WebKit in 2009).

After debugging where Chromium, Gecko, and the spec differ, the first place they differ is the calculation of quaternions.

After calculating the initial value of the quaternion, the spec has the following steps:

```
if (row[2][1] > row[1][2])
    quaternion[0] = -quaternion[0]
if (row[0][2] > row[2][0])
    quaternion[1] = -quaternion[1]
if (row[1][0] > row[0][1])
    quaternion[2] = -quaternion[2]
```

The corresponding code in [WebKit](https://github.com/WebKit/webkit/blob/c8125d66e8dd7adc81e45f39f75a99c85b7876eb/Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp#L489-L525) and [Chromium](https://cs.chromium.org/chromium/src/third_party/blink/renderer/platform/transforms/transformation_matrix.cc?l=702-738&rcl=9ef9d7d626a0a44c4104e3dcb60251feaa1a5df6), however, does not have these steps causing the calculated quaternion to differ.

For the 'to' value in this case I get:

* CSS Transforms 2: `0, 0, -0.49999991257807147, 0.8660254542575067`
* Chromium: `0.000000, 0.000000, 0.500000, 0.866025`
* Gecko: `-0.0, -0.0, -0.5000000126183913, 0.8660253964992068`

So it would appear Gecko is doing the right thing here. The problem is doing the right thing seems to produce a different result to the 2D code path which is undesirable.

The subsequent difference between Gecko and the spec comes about because the recompose step where the spec has:

```
// Construct a composite rotation matrix from the quaternion values
// rotationMatrix is a identity 4x4 matrix initially
rotationMatrix[0][0] = 1 - 2 * (y * y + z * z)
rotationMatrix[0][1] = 2 * (x * y - z * w)
rotationMatrix[0][2] = 2 * (x * z + y * w)
rotationMatrix[1][0] = 2 * (x * y + z * w)
rotationMatrix[1][1] = 1 - 2 * (x * x + z * z)
rotationMatrix[1][2] = 2 * (y * z - x * w)
rotationMatrix[2][0] = 2 * (x * z - y * w)
rotationMatrix[2][1] = 2 * (y * z + x * w)
rotationMatrix[2][2] = 1 - 2 * (x * x + y * y)
``` 

This calculation appears to be based on the WebKit source but will produce the wrong result. The calculation should be:

```
rotationMatrix[0][0] = 1 - 2 * (y * y + z * z)
rotationMatrix[0][1] = 2 * (x * y + z * w)
rotationMatrix[0][2] = 2 * (x * z - y * w)
rotationMatrix[1][0] = 2 * (x * y - z * w)
rotationMatrix[1][1] = 1 - 2 * (x * x + z * z)
rotationMatrix[1][2] = 2 * (y * z - x * w)
rotationMatrix[2][0] = 2 * (x * z - y * w)
rotationMatrix[2][1] = 2 * (y * z + x * w)
rotationMatrix[2][2] = 1 - 2 * (x * x + y * y)
``` 

(Notice the difference in sign in the second, third, and fourth lines.)

However, interestingly this order appears to be correct if we **don't** do the quaternion fix up steps mentioned above.

I don't really understand the theory behind all this but the summary is:

* CSS Transforms Level 1 - Does not use quaternions but tries to produce a certain rotation effect
* CSS Transforms Level 2 - Produces completely the wrong results because it includes __both__ fixups to the quaternion and then recomposition steps that presumably assume such steps were not taken
* Gecko - Follows the spec but fixes the recomposition steps so that the output is sane, but differs from what CSS Transforms Level 1 would have us do
* Chromium - Deviates from the spec during decomposition but ends up producing the same result as CSS Transforms Level 1
* WebKit - Appears to follow separate 2D and 3D codepaths but otherwise is the same as Chromium

The simple solution would appear to be to drop the following steps:

```
if (row[2][1] > row[1][2])
    quaternion[0] = -quaternion[0]
if (row[0][2] > row[2][0])
    quaternion[1] = -quaternion[1]
if (row[1][0] > row[0][1])
    quaternion[2] = -quaternion[2]
```

from the spec, but I still need to work out what this is doing (since [Gecko takes a different approach altogether](https://searchfox.org/mozilla-central/rev/0ec9f5972ef3e4a479720630ad7285a03776cdfc/servo/components/style/properties/helpers/animated_properties.mako.rs#1750-1766) that produces the same result as these steps which makes me think that mathematically they are correct.)
Priority: -- → P3
Attached file Matrix debugging v5
Minor tweaks
Attachment #9018947 - Attachment is obsolete: true
I've sent mail to Kip, Pomax, and Boris to try and get help on this.
Attached patch WIP patchSplinter Review
I caught up with Kip today and we worked through the various approaches to this.

It seems the individual inversion of quaternion values in isolation is odd and doesn't match other implementations or the 2D version (which compare the combination of values).

So this patch moves the inversion from the calculation of individual quaternions to the interpolation step. I have idea if it works for the the full range of values but it seems to fix this particular case.

It needs further testing.

I also still need to work out why the sign of the calculations we use for the recompose step differs.

Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=84fbf819c366f50cdfd427a00d8fc3f9993cc8df
Tool to compare the currently specced behavior vs the changes I am considering to propose.
Oh, that file won't be served as a static file so it won't work :/

I guess I need to host it somewhere.
Looking at the results, I was concerned that the WIP patch I posted regressed the behavior for the following:

  @keyframes animation {
    from { transform: matrix(-1, 0, 0, 1, 0, 0); }
    to { transform: matrix(1, 0, 0, 1, 0, 0); }
  }

Currently Gecko will do a flip around the vertical axis but with the proposed changes it will match Chrome and do a 2D flip. Safari, since it uses a separate code path for 2D, will still do the horizontal flip.

Test case: https://codepen.io/birtles/pen/OrWMme

That would seem unfortunate to lose (although it's nice to match Chrome) but when I checked the vertical variant:

  @keyframes animation {
    from { transform: matrix(1, 0, 0, -1, 0, 0); }
    to { transform: matrix(1, 0, 0, 1, 0, 0); }
  }

I notice Gecko is doing something completely whacky: https://codepen.io/birtles/pen/WLRwpE

So I'm somewhat less concerned about "breaking" this. It would seem better to get the 3D algorithm to properly match Chrome for now -- since at least it seems more consistent than what we're currently doing.

If authors actually want the horizontal flip behavior, they should use rotateY() (e.g. https://www.queness.com/resources/html/css3dflip/) -- that should work in all browsers.

For what it's worth, I just had a look at the test cases in Nightly 105.0a1 (2022-08-12) and could still reproduce the issue.

Sebastian

Whiteboard: [parity-chrome][parity-webkit]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.