stylo: The interpolated result of ServoAnimationValue is not equal to that of StyleAnimationValue

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: boris, Assigned: boris)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

3 months ago
According to Bug 1334036 Comment 56, I noticed that the interpolated result of ServoAnimationValue is different from that of StyleAnimationValue, and this can be easily reproduced after I enable OMTA (because we still use StyleAnimationValue on the compositor thread).
(Assignee)

Updated

3 months ago
Priority: -- → P2
(Assignee)

Updated

3 months ago
Blocks: 1321197
(Assignee)

Updated

2 months ago
Assignee: nobody → boris.chiou
(Assignee)

Comment 1

2 months ago
OK. This is a type conversion bug.

e.g. If we want to compute the interpolation for this:
 transform: from [translate3d(20px, 0px, 0px)]
            to   [translate3d(60px, 0px, 0px)]
            at position: p: 0.682540506401154
* Gecko:
  20 * p + 60 * (1 - p) = 47.3016xxxxx (type: float), and then assign this as float to an nsCSSValue.

Therefore, the result of the interpolation by StyleAniamtionValue::Interpolate() is 47.3016px.

* Servo:
  We use this type, LengthOrPercentage::Length(Au), as each component in Transform::Translate(x, y, z), and |Au| is an |i32|, so the interpolation is something like:

  Au((20 as f64 * p + 60 as f64 * (1-p)).round() as i32) [1]

We compute the interpolation, and then "round()" the result and convert to i32, (i.e. f64 -> i32), so we lost the precision. As a result, the final result is:

  1200 * p + 3600 * (1-p) = 2838.0972153627695
  2838.0972153627695.round() = 2838
  2838 / 60 = 40.3px

Therefore, the result of the interpolation by trait Animatable is 47.3px.

In conclusion, we have different results (i.e. 47.3016px != 47.3px).

[1] http://searchfox.org/mozilla-central/rev/79f625ac56d936ef5d17ebe13a123b25efee4241/servo/components/style/properties/helpers/animated_properties.mako.rs#728
(Assignee)

Comment 2

2 months ago
One possible way is: fix Bug 1340005 first, so we can also use ServoAnimationValue on the compositor thread.
(Assignee)

Updated

2 months ago
Summary: stylo: The interpolated result of ServoAnimationValue is not accurate enough → stylo: The interpolated result of ServoAnimationValue is not equal to that of StyleAnimationValue
(Assignee)

Updated

2 months ago
Depends on: 1340005
(Assignee)

Comment 3

2 months ago
(In reply to Boris Chiou [:boris] from comment #1)
>   200 * p + 3600 * (1-p) = 2838.0972153627695
>   2838.0972153627695.round() = 2838
>   2838 / 60 = 40.3px
47.3px
Another possibility is that just like we use an intermediate representation for RGBA colors, we might need to introduce an intermediate animation type for lengths that uses a 64-bit float. Maybe we don't need to do that yet, but if we are doing all our animation work in integer space, the errors will accumulate as the calculations become more complex and I suspect we'll end up getting bug reports about this (elements not lining up quite right etc.)
(Assignee)

Comment 5

2 months ago
(In reply to Brian Birtles (:birtles) from comment #4)
> Another possibility is that just like we use an intermediate representation
> for RGBA colors, we might need to introduce an intermediate animation type
> for lengths that uses a 64-bit float. Maybe we don't need to do that yet,
> but if we are doing all our animation work in integer space, the errors will
> accumulate as the calculations become more complex and I suspect we'll end
> up getting bug reports about this (elements not lining up quite right etc.)

OK. This sounds a possible solution. I notice that Bug 1340005 may need much work, and we still need to make sure the interpolated result of ServoAnimationValue should be accurate enough.
(Assignee)

Comment 6

2 months ago
(In reply to Boris Chiou [:boris] from comment #5)
> (In reply to Brian Birtles (:birtles) from comment #4)
> > Another possibility is that just like we use an intermediate representation
> > for RGBA colors, we might need to introduce an intermediate animation type
> > for lengths that uses a 64-bit float. Maybe we don't need to do that yet,
> > but if we are doing all our animation work in integer space, the errors will
> > accumulate as the calculations become more complex and I suspect we'll end
> > up getting bug reports about this (elements not lining up quite right etc.)
> 
> OK. This sounds a possible solution. I notice that Bug 1340005 may need much
> work, and we still need to make sure the interpolated result of
> ServoAnimationValue should be accurate enough.

OK. This solution may be a better one, so I am trying to figure out how to introduce an intermediate animation type for length.
(Assignee)

Comment 7

2 months ago
Looks like we use |CSSFloat| for |specified::LengthOrPercentage| and |specified::Length|, so what I need to do in this bug:

1. Introduce IntermediateLength and IntermediateLengthOrPercentage
2. Introduce IntermediateTransformComputedOperation:
e.g.
pub enum IntermediateTransformComputedOperation {
    MatrixWithPercents(ComputedMatrixWithPercents),
    Skew(computed::Angle, computed::Angle),
    Translate(IntermediateLengthOrPercentage,
              IntermediateLengthOrPercentage,
              IntermediateLength),
    Scale(CSSFloat, CSSFloat, CSSFloat),
    Rotate(CSSFloat, CSSFloat, CSSFloat, computed::Angle),
    Perspective(IntermediateLength),
}
3. Implement Animatable for IntermediateTransformComputedOperation, IntermediateLength, and IntermediateLengthOrPercentage.
4. Make sure the uncompute() function doesn't round the float values, so we can get accurate specified transform value.


However, if we compute the specified value into computed value during matching and cascading, it will be clamped again because we still use i32 for computed values. Therefore, we still cannot fix this bug.

Or maybe we should use CSSFloat for Translate() and Perspective().
(Assignee)

Comment 8

2 months ago
OK. It seems the root cause is that we use |i32| (i.e. app_units::Au) for the computed value of length on Servo, and use |float| for the computed value of length on Gecko, so the returned values of getComputedStyle() and getOMTAComputedStyle() are different now. Bug 1340005 is still the possible way to fix this. However, I think it needs to update many parts because we use nsCSSValueSharedList everywhere in AnimationHelper.cpp, AsyncCompositionManager.cpp, and nsDisplayList.cpp.

We shouldn't be blocked by this kind of test failures, so let's just tune tolerance value for now, according to Bug 1361938 Comment 3.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

2 months ago
mozreview-review
Comment on attachment 8870737 [details]
Bug 1361663 - Part 2: Add tolerance because we use different ways for interpolation on the main/compositor thread.

https://reviewboard.mozilla.org/r/142230/#review145900

::: layout/style/test/animation_utils.js:412
(Diff revision 2)
> +// enabling Servo style backend, and still use |StyleAnimationValue| on the
> +// compositor thread. |RawServoAnimationValue| rounds the interpolated results
> +// to a nearest |app_units::Au| (i.e. i32), so we might have a tiny difference
> +// between the results from getOMTAStyle() and getComputedStyle().
> +// Note: 1 AU ~= 60 CSS pixel unit.
> +const toleranceForServoBackend = 0.5 / 60.0;

Should we use this value only if the pref , layout.css.servo.enabled, is true?
(Assignee)

Comment 13

2 months ago
mozreview-review-reply
Comment on attachment 8870737 [details]
Bug 1361663 - Part 2: Add tolerance because we use different ways for interpolation on the main/compositor thread.

https://reviewboard.mozilla.org/r/142230/#review145900

> Should we use this value only if the pref , layout.css.servo.enabled, is true?

Yes! Thanks for this suggestion.
Comment hidden (mozreview-request)
Comment hidden (typo)
(Assignee)

Comment 16

2 months ago
After applying these patches, the mochitest expected number of test_animations_omta.html is reduced to 20 (from 86) based on my local build. Hope bug 1361938 could replace [*] with a fixed number.

Comment 17

2 months ago
mozreview-review
Comment on attachment 8870736 [details]
Bug 1361663 - Part 1: Use double instead of float for the progress of interpolation.

https://reviewboard.mozilla.org/r/142228/#review146250
Attachment #8870736 - Flags: review?(bbirtles) → review+

Comment 18

2 months ago
mozreview-review
Comment on attachment 8870737 [details]
Bug 1361663 - Part 2: Add tolerance because we use different ways for interpolation on the main/compositor thread.

https://reviewboard.mozilla.org/r/142230/#review146252
Attachment #8870737 - Flags: review?(bbirtles) → review+
Thanks for investigating this!

Comment 20

2 months ago
Pushed by bchiou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f0cb9894b02e
Part 1: Use double instead of float for the progress of interpolation. r=birtles
https://hg.mozilla.org/integration/autoland/rev/4191098db6cb
Part 2: Add tolerance because we use different ways for interpolation on the main/compositor thread. r=birtles
(Assignee)

Updated

2 months ago
Status: NEW → ASSIGNED

Comment 21

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f0cb9894b02e
https://hg.mozilla.org/mozilla-central/rev/4191098db6cb
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.