This is an automated email from the git hooks/post-receive script.
pierov pushed a commit to branch geckoview-99.0.1-11.0-1 in repository tor-browser.
commit 136cd85c3504630133d8f9cad673647f2e42b33b Author: Emilio Cobos Álvarez emilio@crisal.io AuthorDate: Wed Jan 5 19:10:28 2022 +0000
Bug 1746084 - Avoid generating InterpolateMatrix operations if there are no size dependencies. r=hiro, a=dsmith
The issue here is that we end up with a transition between mismatched transform lists that ends up generating an InterpolateMatrix {} operation. So far so good, but we end up interpolating that a lot of times and generating an unboundedly-deep operation list.
This implementas an optimization that flattens them to a single matrix when possible (when there's no dependencies on the containing box).
This is similar to:
https://chromium.googlesource.com/chromium/src.git/+/2b89cc4df436e672ef9cf94...
We fix the to_pixel_length() behavior for LenghtPercentage to be correct (and update callers to preserve behavior).
Differential Revision: https://phabricator.services.mozilla.com/D134784 --- .../components/style/values/animated/transform.rs | 108 ++++++++++++--------- .../components/style/values/generics/transform.rs | 40 ++++---- 2 files changed, 83 insertions(+), 65 deletions(-)
diff --git a/servo/components/style/values/animated/transform.rs b/servo/components/style/values/animated/transform.rs index 542b4c3102313..8600281ce5b00 100644 --- a/servo/components/style/values/animated/transform.rs +++ b/servo/components/style/values/animated/transform.rs @@ -891,25 +891,8 @@ impl Animate for ComputedTransform { match (this_remainder, other_remainder) { // If there is a remainder from *both* lists we must have had mismatched functions. // => Add the remainders to a suitable ___Matrix function. - (Some(this_remainder), Some(other_remainder)) => match procedure { - Procedure::Add => { - debug_assert!(false, "Should have already dealt with add by the point"); - return Err(()); - }, - Procedure::Interpolate { progress } => { - result.push(TransformOperation::InterpolateMatrix { - from_list: Transform(this_remainder.to_vec().into()), - to_list: Transform(other_remainder.to_vec().into()), - progress: Percentage(progress as f32), - }); - }, - Procedure::Accumulate { count } => { - result.push(TransformOperation::AccumulateMatrix { - from_list: Transform(this_remainder.to_vec().into()), - to_list: Transform(other_remainder.to_vec().into()), - count: cmp::min(count, i32::max_value() as u64) as i32, - }); - }, + (Some(this_remainder), Some(other_remainder)) => { + result.push(TransformOperation::animate_mismatched_transforms(this_remainder, other_remainder, procedure)?); }, // If there is a remainder from just one list, then one list must be shorter but // completely match the type of the corresponding functions in the longer list. @@ -923,36 +906,19 @@ impl Animate for ComputedTransform { let identity = transform.to_animated_zero().unwrap();
match transform { - // We can't interpolate/accumulate ___Matrix types directly with a - // matrix. Instead we need to wrap it in another ___Matrix type. TransformOperation::AccumulateMatrix { .. } | TransformOperation::InterpolateMatrix { .. } => { - let transform_list = Transform(vec![transform.clone()].into()); - let identity_list = Transform(vec![identity].into()); - let (from_list, to_list) = if fill_right { - (transform_list, identity_list) + let (from, to) = if fill_right { + (transform, &identity) } else { - (identity_list, transform_list) + (&identity, transform) };
- match procedure { - Procedure::Add => Err(()), - Procedure::Interpolate { progress } => { - Ok(TransformOperation::InterpolateMatrix { - from_list, - to_list, - progress: Percentage(progress as f32), - }) - }, - Procedure::Accumulate { count } => { - Ok(TransformOperation::AccumulateMatrix { - from_list, - to_list, - count: cmp::min(count, i32::max_value() as u64) - as i32, - }) - }, - } + TransformOperation::animate_mismatched_transforms( + &[from.clone()], + &[to.clone()], + procedure, + ) }, _ => { let (lhs, rhs) = if fill_right { @@ -981,9 +947,13 @@ impl ComputeSquaredDistance for ComputedTransform {
// Roll back to matrix interpolation if there is any Err(()) in the // transform lists, such as mismatched transform functions. + // + // FIXME: Using a zero size here seems a bit sketchy but matches the + // previous behavior. if squared_dist.is_err() { - let matrix1: Matrix3D = self.to_transform_3d_matrix(None)?.0.into(); - let matrix2: Matrix3D = other.to_transform_3d_matrix(None)?.0.into(); + let rect = euclid::Rect::zero(); + let matrix1: Matrix3D = self.to_transform_3d_matrix(Some(&rect))?.0.into(); + let matrix2: Matrix3D = other.to_transform_3d_matrix(Some(&rect))?.0.into(); return matrix1.compute_squared_distance(&matrix2); }
@@ -1141,6 +1111,52 @@ impl Animate for ComputedTransformOperation { } }
+impl ComputedTransformOperation { + /// If there are no size dependencies, we try to animate in-place, to avoid + /// creating deeply nested Interpolate* operations. + fn try_animate_mismatched_transforms_in_place( + left: &[Self], + right: &[Self], + procedure: Procedure, + ) -> Result<Self, ()> { + let (left, _left_3d) = Transform::components_to_transform_3d_matrix(left, None)?; + let (right, _right_3d) = Transform::components_to_transform_3d_matrix(right, None)?; + ComputedTransformOperation::Matrix3D(left.into()).animate(&ComputedTransformOperation::Matrix3D(right.into()), procedure) + } + + fn animate_mismatched_transforms( + left: &[Self], + right: &[Self], + procedure: Procedure, + ) -> Result<Self, ()> { + if let Ok(op) = Self::try_animate_mismatched_transforms_in_place(left, right, procedure) { + return Ok(op); + } + let from_list = Transform(left.to_vec().into()); + let to_list = Transform(right.to_vec().into()); + Ok(match procedure { + Procedure::Add => { + debug_assert!(false, "Addition should've been handled earlier"); + return Err(()) + }, + Procedure::Interpolate { progress } => { + Self::InterpolateMatrix { + from_list, + to_list, + progress: Percentage(progress as f32), + } + } + Procedure::Accumulate { count } => { + Self::AccumulateMatrix { + from_list, + to_list, + count: cmp::min(count, i32::max_value() as u64) as i32, + } + } + }) + } +} + // This might not be the most useful definition of distance. It might be better, for example, // to trace the distance travelled by a point as its transform is interpolated between the two // lists. That, however, proves to be quite complicated so we take a simple approach for now. diff --git a/servo/components/style/values/generics/transform.rs b/servo/components/style/values/generics/transform.rs index 816dde92e4f94..1179001ccf884 100644 --- a/servo/components/style/values/generics/transform.rs +++ b/servo/components/style/values/generics/transform.rs @@ -404,15 +404,7 @@ impl ToAbsoluteLength for ComputedLength { impl ToAbsoluteLength for ComputedLengthPercentage { #[inline] fn to_pixel_length(&self, containing_len: Option<ComputedLength>) -> Result<CSSFloat, ()> { - match containing_len { - Some(relative_len) => Ok(self.resolve(relative_len).px()), - // If we don't have reference box, we cannot resolve the used value, - // so only retrieve the length part. This will be used for computing - // distance without any layout info. - // - // FIXME(emilio): This looks wrong. - None => Ok(self.resolve(Zero::zero()).px()), - } + Ok(self.maybe_percentage_relative_to(containing_len).ok_or(())?.px()) } }
@@ -572,12 +564,21 @@ impl<T> Transform<T> {
impl<T: ToMatrix> Transform<T> { /// Return the equivalent 3d matrix of this transform list. + /// /// We return a pair: the first one is the transform matrix, and the second one /// indicates if there is any 3d transform function in this transform list. #[cfg_attr(rustfmt, rustfmt_skip)] pub fn to_transform_3d_matrix( &self, reference_box: Option<&Rect<ComputedLength>> + ) -> Result<(Transform3D<CSSFloat>, bool), ()> { + Self::components_to_transform_3d_matrix(&self.0, reference_box) + } + + /// Converts a series of components to a 3d matrix. + pub fn components_to_transform_3d_matrix( + ops: &[T], + reference_box: Option<&Rect<ComputedLength>> ) -> Result<(Transform3D<CSSFloat>, bool), ()> { let cast_3d_transform = |m: Transform3D<f64>| -> Transform3D<CSSFloat> { use std::{f32, f64}; @@ -590,26 +591,27 @@ impl<T: ToMatrix> Transform<T> { ) };
- let (m, is_3d) = self.to_transform_3d_matrix_f64(reference_box)?; + let (m, is_3d) = Self::components_to_transform_3d_matrix_f64(ops, reference_box)?; Ok((cast_3d_transform(m), is_3d)) }
/// Same as Transform::to_transform_3d_matrix but a f64 version. - pub fn to_transform_3d_matrix_f64( - &self, + fn components_to_transform_3d_matrix_f64( + ops: &[T], reference_box: Option<&Rect<ComputedLength>>, ) -> Result<(Transform3D<f64>, bool), ()> { - // We intentionally use Transform3D<f64> during computation to avoid error propagation - // because using f32 to compute triangle functions (e.g. in rotation()) is not - // accurate enough. In Gecko, we also use "double" to compute the triangle functions. - // Therefore, let's use Transform3D<f64> during matrix computation and cast it into f32 - // in the end. + // We intentionally use Transform3D<f64> during computation to avoid + // error propagation because using f32 to compute triangle functions + // (e.g. in rotation()) is not accurate enough. In Gecko, we also use + // "double" to compute the triangle functions. Therefore, let's use + // Transform3D<f64> during matrix computation and cast it into f32 in + // the end. let mut transform = Transform3D::<f64>::identity(); let mut contain_3d = false;
- for operation in &*self.0 { + for operation in ops { let matrix = operation.to_3d_matrix(reference_box)?; - contain_3d |= operation.is_3d(); + contain_3d = contain_3d || operation.is_3d(); transform = matrix.then(&transform); }