Closed Bug 1361663 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: boris, Assigned: boris)

References

Details

Attachments

(2 files)

Bug 1361663 - Part 1: Use double instead of float for the progress of interpolation.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1361663 - Part 2: Add tolerance because we use different ways for interpolation on the main/compositor thread.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
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).
Priority: -- → P2
Blocks: stylo-style-mochitest
Assignee: nobody → boris.chiou
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
One possible way is: fix Bug 1340005 first, so we can also use ServoAnimationValue on the compositor thread.
Summary: stylo: The interpolated result of ServoAnimationValue is not accurate enough → stylo: The interpolated result of ServoAnimationValue is not equal to that of StyleAnimationValue
Depends on: 1340005
(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.)
(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.
(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.
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().
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 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?
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.
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 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 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!
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
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/f0cb9894b02e
https://hg.mozilla.org/mozilla-central/rev/4191098db6cb
Status: ASSIGNED → RESOLVED
Closed: 7 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: