1156456 - z component of transform-origin incorrect when animated with off-main-thread animations with zoom or retina display

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[:kip (Kearwood Gilbert)]

In the latest (2015-04-20) nightly, the CSS 3D Transform sample at attached URL does not render correctly.  One face of the 3d spinning cube appears to be translated towards the center of the cube.

This appears to be a regression within the last few days.  Previously, the example rendered the cube correctly.

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: Expected Rendering of Cube

Comment 2

[:kip (Kearwood Gilbert)]

Attachment: Cube Rendering Failure

Comment 3

[:kip (Kearwood Gilbert)]

The screenshot was taken while the browser window was on the built-in retina display of a 15" MacBook Pro (MacBookPro11,3).  On the same machine, if the window is dragged to an external (non-retina) display, the problem still exists, but the offset is only a couple of pixels.

Comment 4

[Logan Rosen [:Logan]]

59:06.80 LOG: MainThread Bisector INFO Last good revision: ef53c6c25fb1
59:06.80 LOG: MainThread Bisector INFO First bad revision: eed5d2d610e2
59:06.80 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=ef53c6c25fb1&tochange=eed5d2d610e2

I found this window using a 15" Retina MacBook Pro, FWIW. Definitely caused by the fix to Bug 980770.

Comment 5

[Boris Zbarsky [:bzbarsky]]

[Tracking Requested - why for this release]: Behavior regression.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase from http://codepen.io/keithclark/full/dvigs/

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I guess I see brokenness, but somewhat different symptoms:  one face of the cube appears to get out of sync with the others, sometimes quite substantially, although most of the time only by a little bit.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Oh, wait, I see the original problem if I zoom in... which makes sense, since a retina display is like a zoom of 2x.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: minimal fix

This is the minimal fix.

However, I think it would be better to send these values across in device pixels, because it seems simpler.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Try run at:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=819d55662f00

I'm expected to have to add a small amount of fuzz to one or both of the tests (since locally I see 7 pixels off with max difference 16 on the non-zoom test, along one of the edges of the cube).  I'll use the results of the try run to do so, but I think the rest is ready for review.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1 - Reftests comparing a 3-D transformed cube generated by main-thread paths and off-main-thread animations paths

I confirmed that without the patch, the test marked as failing (the one
with zoom) in this patch (which will be unmarked in the next patch)
fails.

There is also a small amount of fuzz that will need to be annotated with
an additional patch on top of the patch stack.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2 - Transform the z component just like the x and y

I confirmed locally that this patch fixes the reftest (per reftest.list
change).

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3 - Send transform origin and perspective origin to layer in device pixels rather than CSS pixels

Comment 14

[Brian Birtles (:birtles)]

Comment on attachment 8595679 [details] [diff] [review]
patch 3 - Send transform origin and perspective origin to layer in device pixels rather than CSS pixels

>diff --git a/gfx/layers/composite/AsyncCompositionManager.cpp b/gfx/layers/composite/AsyncCompositionManager.cpp
>--- a/gfx/layers/composite/AsyncCompositionManager.cpp
>+++ b/gfx/layers/composite/AsyncCompositionManager.cpp
>@@ -401,28 +401,19 @@ SampleValue(float aPortion, Animation& a
...
>   TransformData& data = aAnimation.data().get_TransformData();
>   nsPoint origin = data.origin();
>-  // we expect all our transform data to arrive in css pixels, so here we must
>-  // adjust to dev pixels.
>-  double cssPerDev = double(nsDeviceContext::AppUnitsPerCSSPixel())
>-                     / double(data.appUnitsPerDevPixel());
>+  // we expect all our transform data to arrive in device pixels

I wonder if we still need this comment.

>diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp
>--- a/layout/base/nsDisplayList.cpp
>+++ b/layout/base/nsDisplayList.cpp
>@@ -525,18 +525,19 @@ nsDisplayListBuilder::AddAnimationsAndTr
...
>   AnimationData data;
>   if (aProperty == eCSSProperty_transform) {
>     nsRect bounds = nsDisplayTransform::GetFrameBoundsForTransform(aFrame);
>-    // all data passed directly to the compositor should be in css pixels
>-    float scale = nsDeviceContext::AppUnitsPerCSSPixel();
>+    // all data passed directly to the compositor should be in dev pixels
>+    int32_t d2a = aFrame->PresContext()->AppUnitsPerDevPixel();

Is d2a a convention? If not, something like devPixelsToAppUnits would be a little less cryptic to me, but it's up to you.

It's not entirely clear to me why passing device pixels is better (CSS pixels seems like a more appropriate value for an interface boundary) but I agree it simplifies this code.

Comment 15

[Brian Birtles (:birtles)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #11)
> Created attachment 8595677 [details] [diff] [review]
> patch 1 - Reftests comparing a 3-D transformed cube generated by main-thread
> paths and off-main-thread animations paths
> 
> I confirmed that without the patch, the test marked as failing (the one
> with zoom) in this patch (which will be unmarked in the next patch)
> fails.
> 
> There is also a small amount of fuzz that will need to be annotated with
> an additional patch on top of the patch stack.

Do we still need that fuzz?

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Brian Birtles (:birtles) from comment #15)
> Do we still need that fuzz?

Yes, for at least Linux 64 and Windows XP.  (But not for Linux 32, Mac, Win8, or Android.)  Still waiting on Win7.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That said, I think I've conclude that the fuzz is a bug, so I'm going to just try to debug and fix it.

(I was thinking it was because the layerization decisions were different, but they're not.  I've found there's a rounding difference in the aTransform._42 parameter to BasicCompositor::DrawQuad that seems suspicious, and I'm looking to see where that difference comes from.)

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

In nsDisplayTransform::GetResultingTransformMatrixInternal, there's a small difference following the ReadTransforms call (looks like the result of converting doubles to floats and back) that ends up being turned into a larger difference by the result.ChangeBasis(offsetBetweenOrigins); at the end of the function.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the reason for the subtle transform difference is that when we read the transform list via nsDisplayTransform::GetResultingTransformMatrix calling nsStyleTransformMatrix::ReadTransforms:

 * in the non-OMTA case, we're reading the specified transform, which is rotateY(-120deg)

 * in the OMTA case, we're reading the transform as converted to radians and passed around as a float

nsCSSValue::GetAngleValueInRadians produces different results in these cases since it converts the float to a double *before* doing the multiplication (so as to yield accurate results for round values in degrees).

I suspect the test could work around this by specifying the rotation as a round number in radians.



That doesn't, however, explain how this transform difference on the side that has OMT animations on it (the non-pseudo-element) causes differences at the edge of a *different* side.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #19)
> That doesn't, however, explain how this transform difference on the side
> that has OMT animations on it (the non-pseudo-element) causes differences at
> the edge of a *different* side.

Er, actually, it does, since the non-OMTA layers are children of the OMTA layer.

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Here's a new try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca9b01d97988
with:
 * the 10px of extra spacing workaround removed from the test
 * the angle on the div changed to be in radians, to work around comment 19

I filed bug 1157455 as a followup to fix the rounding problem.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

... and I'll still need to mark the test fuzzy for Win7 reftest-no-accel (only!), which I'm just going to do and not worry about it.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dc6b401584e9
https://hg.mozilla.org/integration/mozilla-inbound/rev/61bb4d672472
https://hg.mozilla.org/integration/mozilla-inbound/rev/e08e45fd8922

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b7f3a1a9dd3

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dc6b401584e9
https://hg.mozilla.org/mozilla-central/rev/61bb4d672472
https://hg.mozilla.org/mozilla-central/rev/e08e45fd8922
https://hg.mozilla.org/mozilla-central/rev/1b7f3a1a9dd3

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I plan to land one more followup, which is to land an opacity equivalent of the transform reftest landed above.  Try run for that reftest is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d70778c56ed

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c15263ebc72c

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c15263ebc72c

Comment 29

[jen strickland :jenstrickland]

Since this is fixed, we're removing tracking for 40.