Bug 1399049 - avoid using InterpolateMatrix as a fallback for matched transform function pair. draft
authorJeremy Chen <jeremychen@mozilla.com>
Wed, 18 Oct 2017 19:02:10 +0800
changeset 683220 a81ce48c45d89d19c1fb32809b68ca62de5e2c94
parent 683139 097044f71d4a4057dcc7bf76030f8b4cc379b56c
child 736563 04ee10513ce586c09c25eda7fc1cf858512cf45a
push id85288
push userbmo:jeremychen@mozilla.com
push dateThu, 19 Oct 2017 10:29:47 +0000
bugs1399049
milestone58.0a1
Bug 1399049 - avoid using InterpolateMatrix as a fallback for matched transform function pair. In the current implementation, if there is any interpolation error in a matched transform function pair, we fall-back to use InterpolateMatrix unconditionally. However, the error could be caused by: 1. mismatched transform function pair 2. matched transform function pair within at least one undecomposable matrix. Using InterpolateMatrix for case 1 makes sense, however, using InterpolateMatrix for case 2 does not. According to the spec, we should just report error for case 2, and let the caller do the fallback procedure. Using InterpolateMatrix for case 2 will go through more unnecessary code path, and produce more memory usage and calculation cost, which should be avoidable. In this patch, we add an extra pass to check if a transform function pair have matched operations in advance. With this information, we can easily tell whether the interpolation error in a equal-length transform function pair is caused by case 1 or case 2. So, we can avoid the unnecessary cost. MozReview-Commit-ID: E2Pl7nMCt7Z
servo/components/style/properties/helpers/animated_properties.mako.rs
--- a/servo/components/style/properties/helpers/animated_properties.mako.rs
+++ b/servo/components/style/properties/helpers/animated_properties.mako.rs
@@ -1154,16 +1154,37 @@ impl Animate for TransformOperation {
                     fd_matrix.animate(&td_matrix, procedure)?,
                 ))
             },
             _ => Err(()),
         }
     }
 }
 
+fn is_matched_operation(first: &TransformOperation, second: &TransformOperation) -> bool {
+    match (first, second) {
+        (&TransformOperation::Matrix(..),
+         &TransformOperation::Matrix(..)) |
+        (&TransformOperation::MatrixWithPercents(..),
+         &TransformOperation::MatrixWithPercents(..)) |
+        (&TransformOperation::Skew(..),
+         &TransformOperation::Skew(..)) |
+        (&TransformOperation::Translate(..),
+         &TransformOperation::Translate(..)) |
+        (&TransformOperation::Scale(..),
+         &TransformOperation::Scale(..)) |
+        (&TransformOperation::Rotate(..),
+         &TransformOperation::Rotate(..)) |
+        (&TransformOperation::Perspective(..),
+         &TransformOperation::Perspective(..)) => true,
+        // InterpolateMatrix and AccumulateMatrix are for mismatched transform.
+        _ => false
+    }
+}
+
 /// <https://www.w3.org/TR/css-transforms-1/#Rotate3dDefined>
 fn rotate_to_matrix(x: f32, y: f32, z: f32, a: Angle) -> ComputedMatrix {
     let half_rad = a.radians() / 2.0;
     let sc = (half_rad).sin() * (half_rad).cos();
     let sq = (half_rad).sin().powi(2);
 
     ComputedMatrix {
         m11: 1.0 - 2.0 * (y * y + z * z) * sq,
@@ -2135,33 +2156,47 @@ impl Animate for TransformList {
             Cow::Owned(other.to_animated_zero()?)
         };
         let other = if other.0.is_some() {
             Cow::Borrowed(other)
         } else {
             Cow::Owned(self.to_animated_zero()?)
         };
 
+        // For matched transform lists.
         {
             let this = (*this).0.as_ref().map_or(&[][..], |l| l);
             let other = (*other).0.as_ref().map_or(&[][..], |l| l);
             if this.len() == other.len() {
-                let result = this.iter().zip(other).map(|(this, other)| {
-                    this.animate(other, procedure)
-                }).collect::<Result<Vec<_>, _>>();
-                if let Ok(list) = result {
-                    return Ok(TransformList(if list.is_empty() {
-                        None
-                    } else {
-                        Some(list)
-                    }));
+                let is_matched_transforms = this.iter().zip(other).all(|(this, other)| {
+                    is_matched_operation(this, other)
+                });
+
+                if is_matched_transforms {
+                    let result = this.iter().zip(other).map(|(this, other)| {
+                        this.animate(other, procedure)
+                    }).collect::<Result<Vec<_>, _>>();
+                    if let Ok(list) = result {
+                        return Ok(TransformList(if list.is_empty() {
+                            None
+                        } else {
+                            Some(list)
+                        }));
+                    }
+
+                    // Can't animate for a pair of matched transform lists?
+                    // This means we have at least one undecomposable matrix,
+                    // so we should report Err here, and let the caller do
+                    // the fallback procedure.
+                    return Err(());
                 }
             }
         }
 
+        // For mismatched transform lists.
         match procedure {
             Procedure::Add => Err(()),
             Procedure::Interpolate { progress } => {
                 Ok(TransformList(Some(vec![TransformOperation::InterpolateMatrix {
                     from_list: this.into_owned(),
                     to_list: other.into_owned(),
                     progress: Percentage(progress as f32),
                 }])))