1499862 - Result of matrix interpolation is incorrect

Status: ASSIGNED

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

Description

[Tooru Fujisawa [:arai]]

Attachment: 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"

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: another testcase

interestingly, if the rotation starts from 0deg instead of 180deg, Chrome and Safari also rotate them in the different direction.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

I haven't yet checked the recent spec change, but will be fixed by bug 1472917?

Comment 3

[Tooru Fujisawa [:arai]]

(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.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Then bug 1322189?

Comment 5

[Tooru Fujisawa [:arai]]

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.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

That sounds an issue should be fixed by the smarter interpolation, I mean bug 1472917 or more spec changes.

Comment 7

[Brian Birtles (:birtles)]

Attachment: Simplified test case

Comment 8

[Brian Birtles (:birtles)]

Attachment: Matrix interpolation debugging

Comment 9

[Brian Birtles (:birtles)]

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.

Comment 10

[Brian Birtles (:birtles)]

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

Comment 11

[Brian Birtles (:birtles)]

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.)

Comment 12

[Brian Birtles (:birtles)]

Attachment: Matrix interpolation debugging v2

Fixed a bug in the recompose method.

Comment 13

[Brian Birtles (:birtles)]

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.

Comment 14

[Brian Birtles (:birtles)]

Attachment: Matrix debugging v3

Added a partial 3D code path to my JS implementation of the spec.

I suspect it's still got a few bugs though.

Comment 15

[Brian Birtles (:birtles)]

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.

Comment 16

[Brian Birtles (:birtles)]

Attachment: Matrix debugging v4

Comment 17

[Brian Birtles (:birtles)]

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.

Comment 18

[Brian Birtles (:birtles)]

(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.)

Comment 19

[Brian Birtles (:birtles)]

Attachment: Matrix debugging v5

Minor tweaks

Comment 20

[Brian Birtles (:birtles)]

I've sent mail to Kip, Pomax, and Boris to try and get help on this.

Comment 21

[Brian Birtles (:birtles)]

Attachment: WIP patch

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

Comment 22

[Brian Birtles (:birtles)]

Attachment: Interpolation comparison v1

Tool to compare the currently specced behavior vs the changes I am considering to propose.

Comment 23

[Brian Birtles (:birtles)]

Oh, that file won't be served as a static file so it won't work :/

I guess I need to host it somewhere.

Comment 24

[Brian Birtles (:birtles)]

Here we go: https://coconut-stop.glitch.me/?from=rotate(180deg)&to=translate(0px)+rotate(300deg)&progress=0.5&from=matrix(-1%2C+0%2C+0%2C+-1%2C+0%2C+0)&to=matrix(1%2C+0%2C+0%2C+1%2C+0%2C+0)&progress=0.25&from=matrix(-1%2C+0%2C+0%2C+1%2C+0%2C+0)&to=matrix(1%2C+0%2C+0%2C+1%2C+0%2C+0)&progress=0.25&from=matrix(1%2C+0%2C+0%2C+-1%2C+0%2C+0)&to=matrix(1%2C+0%2C+0%2C+1%2C+0%2C+0)&progress=0.25&from=matrix(0%2C+1%2C+1%2C+0%2C+0%2C+0)&to=matrix(1%2C+0%2C+0%2C+1%2C+0%2C+0)&progress=0.25&from=matrix(0%2C+-1%2C+-1%2C+0%2C+0%2C+0)&to=matrix(1%2C+0%2C+0%2C+1%2C+0%2C+0)&progress=0.25&from=none&to=matrix(-1%2C+0%2C+2%2C+-1%2C+0%2C+0)&progress=0.25&from=none&to=matrix(1%2C+0%2C+1%2C+-1%2C+0%2C+0)&progress=0.25&from=none&to=matrix(-1%2C+0%2C+-1%2C+1%2C+0%2C+0)&progress=0.25&from=none&to=matrix(0%2C+1%2C+1%2C+-2%2C+0%2C+0)&progress=0.25&from=none&to=matrix(0%2C+-1%2C+-1%2C+0.5%2C+0%2C+0)&progress=0.25

Comment 25

[Brian Birtles (:birtles)]

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.

Comment 26

[Sebastian Zartner [:sebo]]

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