Closed Bug 1157455 Opened 6 years ago Closed 6 years ago

off-main-thread animations introduce floating-point inaccuracy for rotations in units other than radians

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

Details

Attachments

(3 files)

This is a followup to bug 1156456.

In bug 1156456 comment #19 I wrote:
> 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.

We should fix this so that we don't get this inaccuracy.  Probably the easiest way to do that is just to send the correct units to the compositor thread rather than sending values in radians to the compositor thread.

Once we do this we should readd the original test from bug 1156456 that used degrees -- and also keep the radians test.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
I also needed a patch to StyleAnimationValue.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa03167050d7
(Note that that try push had the Windows reftest-no-accel fuzzy-if commented out in the tip changeset to see if this helped, which it didn't.  But it does otherwise fix the problems with degrees.)
In bug 1156456 I landed tests with the transform in radians to work
around float rounding issues with having the transform in degrees.  This
bug will fix the problems with degrees, so I'm duplicating the tests
here to have variants with both degrees and radians.
Attachment #8596393 - Flags: review?(bbirtles)
This avoids accumulating floating point error from conversion, so that
when we switch to doubles at the start of
nsCSSValue::GetAngleValueInRadians we're using the original unit, rather
than a different one that will round differently.
Attachment #8596394 - Flags: review?(bbirtles)
This avoids accumulating floating point error from conversion, so that
when we switch to doubles at the start of
nsCSSValue::GetAngleValueInRadians we're using the original unit, rather
than a different one that will round differently.
Attachment #8596395 - Flags: review?(bbirtles)
Comment on attachment 8596393 [details] [diff] [review]
patch 1 - Add tests for OMTA transforms in degrees in addition to those in radians

>diff --git a/layout/reftests/transform-3d/animate-cube-ref.html b/layout/reftests/transform-3d/animate-cube-radians-ref.html
>copy from layout/reftests/transform-3d/animate-cube-ref.html
>copy to layout/reftests/transform-3d/animate-cube-radians-ref.html
>--- a/layout/reftests/transform-3d/animate-cube-ref.html
>+++ b/layout/reftests/transform-3d/animate-cube-radians-ref.html
>@@ -17,19 +17,16 @@ div, div::before, div::after {
> }
> 
> div {
>   position: absolute;
>   top: 0; right: 0; bottom: 0; left: 0;
>   margin: auto;
>   transform-origin: 50% 50% 100px;
>   background: #006;
>-  /* FIXME: use radians rather than degrees for now to avoid the rounding
>-     issue described in
>-     https://bugzilla.mozilla.org/show_bug.cgi?id=1156456#c19 */
>   /* transform: rotateY(-120deg) rotateX(60deg); */
>   transform: rotateY(-2.0944rad) rotateX(1.0472rad);
>   transform-style: preserve-3d;
> }

Shouldn't this also remove the commented-out degrees line?

  /* transform: rotateY(-120deg) rotateX(60deg); */

Likewise for all the *-radians test files.

r=me with that
Attachment #8596393 - Flags: review?(bbirtles) → review+
Comment on attachment 8596394 [details] [diff] [review]
patch 2 - Send angles (in CSS transform functions) to the compositor thread with their units rather than sending all such angles in radians

>diff --git a/layout/base/nsDisplayList.cpp b/layout/base/nsDisplayList.cpp
>--- a/layout/base/nsDisplayList.cpp
>+++ b/layout/base/nsDisplayList.cpp
...
>+static inline CSSAngle
>+MakeCSSAngle(const nsCSSValue& aValue)
>+{
>+  return CSSAngle(aValue.GetFloatValue(), aValue.GetUnit());
>+}

I think we want to call aValue.GetAngleValue() here? That way we assert that the unit we're getting back is actually an angular unit.

r=me with that.
Attachment #8596394 - Flags: review?(bbirtles) → review+
Attachment #8596395 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #7)
> Shouldn't this also remove the commented-out degrees line?
> 
>   /* transform: rotateY(-120deg) rotateX(60deg); */
> 
> Likewise for all the *-radians test files.

I wanted to leave that line because it shows where the numbers in radians come from.


(In reply to Brian Birtles (:birtles) from comment #8)
> I think we want to call aValue.GetAngleValue() here? That way we assert that
> the unit we're getting back is actually an angular unit.

Indeed.  I'd forgotten that GetAngleValue existed.
(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #9)
> (In reply to Brian Birtles (:birtles) from comment #7)
> > Shouldn't this also remove the commented-out degrees line?
> > 
> >   /* transform: rotateY(-120deg) rotateX(60deg); */
> > 
> > Likewise for all the *-radians test files.
> 
> I wanted to leave that line because it shows where the numbers in radians
> come from.

I changed it to:
  /* approximately rotateY(-120deg) rotateX(60deg) */
so it looks more like an explanatory comment than commented-out code.
You need to log in before you can comment on or make changes to this bug.