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

Status: RESOLVED FIXED

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

Description

[Boris Chiou [:boris]]

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

Comment 1

[Boris Chiou [:boris]]

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

Comment 2

[Boris Chiou [:boris]]

One possible way is: fix Bug 1340005 first, so we can also use ServoAnimationValue on the compositor thread.

Comment 3

[Boris Chiou [:boris]]

(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

Comment 4

[Brian Birtles (:birtles)]

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

Comment 5

[Boris Chiou [:boris]]

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

Comment 6

[Boris Chiou [:boris]]

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

Comment 7

[Boris Chiou [:boris]]

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

Comment 8

[Boris Chiou [:boris]]

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 9

[Boris Chiou [:boris]]

Attachment: Bug 1361663 - Part 1: Use double instead of float for the progress of interpolation.

We get the progress as |double|, and then pass it to a function by a |float|
type, and then finally cast it back to |double| for the interpolation.
We should avoid casting it back and forth, so change the function argument to
use |double| type for the progress.

Review commit: https://reviewboard.mozilla.org/r/142228/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142228/

Comment 10

[Boris Chiou [:boris]]

Attachment: Bug 1361663 - Part 2: Add tolerance because we use different ways for interpolation on the main/compositor thread.

On the main thread, we use Servo backend, and the type of computed::Length is
|app_units::Au| (i.e. i32).
However, on the compositor thread, we use Gecko backend, and the type of
the length in nsCSSValue is |float|, so there might be some difference.
Use "0.5 / 60" because we round the interpolated result to the nearest
integer, and convert it into an app unit value, so the tolerance could be
this value.

Review commit: https://reviewboard.mozilla.org/r/142230/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/142230/

Comment 11

[Boris Chiou [:boris]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142230/diff/1-2/

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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 13

[Boris Chiou [:boris]]

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 14

[Boris Chiou [:boris]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/142230/diff/2-3/

Comment 15

[Boris Chiou [:boris]]

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

Comment 16

[Boris Chiou [:boris]]

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

[Brian Birtles (:birtles)]

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

Comment 18

[Brian Birtles (:birtles)]

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

Comment 19

[Brian Birtles (:birtles)]

Thanks for investigating this!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0cb9894b02e
https://hg.mozilla.org/mozilla-central/rev/4191098db6cb