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.

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.

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.

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

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.

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

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

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.

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.

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

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.

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

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

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

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.

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.

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

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.

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

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

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

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

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