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

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.

(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

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

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

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