Only consider animatable properties when skipping commits

With CL [1], if a compositor animation was running, any changes
to transforms, filters, or effects were ignored during the animation,
including properties other than the ones being animated. For example,
if a transform animation was running, any changes to
backface-visibility would also be ignored, even though that change
is not handled by the compositor. With this CL, only changes to the
specific properties that can be animated on the compositor (e.g.
the transform matrix) will be ignored, while other changes will
still cause a commit.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/1459592


Bug: 933141
Change-Id: If6be65239c20dd36e721e5cf4e84db992e7369c6
Reviewed-on: https://chromium-review.googlesource.com/c/1484072
Commit-Queue: Mason Freed <masonfreed@chromium.org>
Auto-Submit: Mason Freed <masonfreed@chromium.org>
Reviewed-by: Philip Rogers <pdr@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636079}

third_party/blink/renderer/core/animation/compositor_animations.h

@@ -158,6 +158,8 @@ class CORE_EXPORT CompositorAnimations {
                            CannotStartElementOnCompositorEffectSVG);
   FRIEND_TEST_ALL_PREFIXES(AnimationCompositorAnimationsTest,
                            CancelIncompatibleCompositorAnimations);
+  FRIEND_TEST_ALL_PREFIXES(AnimationCompositorAnimationsTest,
+                           NonAnimatedTransformPropertyChangeGetsUpdated);
 };
 
 }  // namespace blink

third_party/blink/renderer/core/animation/compositor_animations_test.cc

@@ -37,6 +37,7 @@
 #include "base/memory/ptr_util.h"
 #include "base/memory/scoped_refptr.h"
 #include "cc/animation/animation_host.h"
+#include "cc/layers/picture_layer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/blink/public/web/web_settings.h"
 #include "third_party/blink/renderer/core/animation/animatable/animatable_double.h"
@@ -53,6 +54,7 @@
 #include "third_party/blink/renderer/core/frame/frame_test_helpers.h"
 #include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"
 #include "third_party/blink/renderer/core/layout/layout_object.h"
+#include "third_party/blink/renderer/core/paint/compositing/composited_layer_mapping.h"
 #include "third_party/blink/renderer/core/paint/object_paint_properties.h"
 #include "third_party/blink/renderer/core/paint/paint_layer.h"
 #include "third_party/blink/renderer/core/style/computed_style.h"
@@ -1906,6 +1908,51 @@ TEST_F(AnimationCompositorAnimationsTest, CanStartElementOnCompositorEffect) {
   EXPECT_EQ(host->CompositedAnimationsCount(), 1u);
 }
 
+TEST_F(AnimationCompositorAnimationsTest,
+       NonAnimatedTransformPropertyChangeGetsUpdated) {
+  LoadTestData("transform-animation-update.html");
+  Document* document = GetFrame()->GetDocument();
+  Element* target = document->getElementById("target");
+  const ObjectPaintProperties* properties =
+      target->GetLayoutObject()->FirstFragment().PaintProperties();
+  ASSERT_NE(nullptr, properties);
+  const auto* transform = properties->Transform();
+  ASSERT_NE(nullptr, transform);
+  // Make sure composited animation is running on #target.
+  if (RuntimeEnabledFeatures::CompositeAfterPaintEnabled())
+    EXPECT_TRUE(transform->HasDirectCompositingReasons());
+  CompositorAnimations::FailureCode code =
+      CompositorAnimations::CheckCanStartElementOnCompositor(*target);
+  EXPECT_EQ(code, CompositorAnimations::FailureCode::None());
+  EXPECT_EQ(document->Timeline().PendingAnimationsCount(), 1u);
+  cc::AnimationHost* host = document->View()->GetCompositorAnimationHost();
+  EXPECT_EQ(host->MainThreadAnimationsCount(), 0u);
+  EXPECT_EQ(host->CompositedAnimationsCount(), 1u);
+  // Make sure the backface-visibility is correctly set, both in blink and on
+  // the cc::Layer.
+  EXPECT_FALSE(transform->Matrix().IsIdentity());  // Rotated
+  EXPECT_EQ(transform->GetBackfaceVisibility(),
+            TransformPaintPropertyNode::BackfaceVisibility::kVisible);
+  const CompositedLayerMapping* composited_layer_mapping =
+      ToLayoutBoxModelObject(target->GetLayoutObject())
+          ->Layer()
+          ->GetCompositedLayerMapping();
+  ASSERT_NE(nullptr, composited_layer_mapping);
+  const cc::PictureLayer* layer =
+      composited_layer_mapping->MainGraphicsLayer()->CcLayer();
+  ASSERT_NE(nullptr, layer);
+  EXPECT_TRUE(layer->double_sided());
+  // Change the backface visibility, while the compositor animation is
+  // happening.
+  target->setAttribute(html_names::kClassAttr, "backface-hidden");
+  ForceFullCompositingUpdate();
+  // Make sure the setting made it to both blink and all the way to CC.
+  EXPECT_EQ(transform->GetBackfaceVisibility(),
+            TransformPaintPropertyNode::BackfaceVisibility::kHidden);
+  EXPECT_FALSE(layer->double_sided())
+      << "Change to hidden did not get propagated to CC";
+}
+
 // Regression test for https://crbug.com/781305. When we have a transform
 // animation on a SVG element, the effect can be started on compositor but the
 // element itself cannot.

third_party/blink/renderer/core/animation/test_data/transform-animation-update.html

+<!DOCTYPE html>
+<style>
+@keyframes rotate {
+  from {transform: rotateY(180deg);}
+  to {transform: rotateY(0deg);}
+}
+#target {
+  width: 100px;
+  height: 100px;
+  animation: rotate 9999s linear 0s infinite;
+}
+.backface-visible {
+  backface-visibility: visible;
+}
+.backface-hidden {
+  backface-visibility: hidden;
+}
+</style>
+<div id="target" class="backface-visible"></div>

third_party/blink/renderer/core/paint/paint_property_tree_builder.cc

@@ -709,9 +709,20 @@ void FragmentPaintPropertyTreeBuilder::UpdateTransform() {
             CompositorElementIdNamespace::kPrimaryTransform);
       }
 
+      // If nothing but the transform matrix changed, and animations are
+      // running, avoid setting property_changed.
+      bool other_properties_changed = true;
+      if (properties_->Transform()) {
+        other_properties_changed =
+            properties_->Transform()->HaveNonAnimatingPropertiesChanged(
+                *context_.current.transform, state);
+      }
+      bool running_animation =
+          style.IsRunningTransformAnimationOnCompositor() &&
+          !other_properties_changed;
       OnUpdate(properties_->UpdateTransform(*context_.current.transform,
                                             std::move(state)),
-               style.IsRunningTransformAnimationOnCompositor());
+               running_animation);
     } else {
       OnClear(properties_->ClearTransform());
     }
@@ -958,12 +969,22 @@ void FragmentPaintPropertyTreeBuilder::UpdateEffect() {
               object_.UniqueId(), CompositorElementIdNamespace::kPrimary);
         }
       }
-      bool running_effect_animation =
-          style.IsRunningOpacityAnimationOnCompositor() ||
-          style.IsRunningBackdropFilterAnimationOnCompositor();
+
+      // If nothing but the opacity and/or backdrop-filter changed, and
+      // animations are running, avoid setting property_changed.
+      bool other_properties_changed = true;
+      if (properties_->Effect()) {
+        other_properties_changed =
+            properties_->Effect()->HaveNonAnimatingPropertiesChanged(
+                *context_.current_effect, state);
+      }
+      bool running_animation =
+          (style.IsRunningOpacityAnimationOnCompositor() ||
+           style.IsRunningBackdropFilterAnimationOnCompositor()) &&
+          !other_properties_changed;
       OnUpdate(
           properties_->UpdateEffect(*context_.current_effect, std::move(state)),
-          running_effect_animation);
+          running_animation);
 
       if (mask_clip || has_spv1_composited_clip_path) {
         EffectPaintPropertyNode::State mask_state;
@@ -1108,9 +1129,19 @@ void FragmentPaintPropertyTreeBuilder::UpdateFilter() {
             object_.UniqueId(), CompositorElementIdNamespace::kEffectFilter);
       }
 
+      // If nothing but the filter changed, and animations are running, avoid
+      // setting property_changed.
+      bool other_properties_changed = true;
+      if (properties_->Filter()) {
+        other_properties_changed =
+            properties_->Filter()->HaveNonAnimatingPropertiesChanged(
+                *context_.current_effect, state);
+      }
+      bool running_animation = style.IsRunningFilterAnimationOnCompositor() &&
+                               !other_properties_changed;
       OnUpdate(
           properties_->UpdateFilter(*context_.current_effect, std::move(state)),
-          style.IsRunningFilterAnimationOnCompositor());
+          running_animation);
     } else {
       OnClear(properties_->ClearFilter());
     }

third_party/blink/renderer/platform/graphics/paint/effect_paint_property_node.h

@@ -54,15 +54,32 @@ class PLATFORM_EXPORT EffectPaintPropertyNode
     FloatPoint filters_origin;
 
     bool operator==(const State& o) const {
+      return EqualIgnoringAnimatingProperties(o, false);
+    }
+
+    bool EqualIgnoringAnimatingProperties(const State& o,
+                                          bool check_for_animations) const {
+      bool filters_equal =
+          (filter == o.filter) ||
+          (check_for_animations && (direct_compositing_reasons &
+                                    CompositingReason::kActiveFilterAnimation));
+      bool backdrops_equal =
+          (backdrop_filter == o.backdrop_filter) ||
+          (check_for_animations &&
+           (direct_compositing_reasons &
+            CompositingReason::kActiveBackdropFilterAnimation));
+      bool opacity_equal = (opacity == o.opacity) ||
+                           (check_for_animations &&
+                            (direct_compositing_reasons &
+                             CompositingReason::kActiveOpacityAnimation));
       return local_transform_space == o.local_transform_space &&
              output_clip == o.output_clip && color_filter == o.color_filter &&
-             filter == o.filter && opacity == o.opacity &&
-             backdrop_filter == o.backdrop_filter &&
              backdrop_filter_bounds == o.backdrop_filter_bounds &&
              blend_mode == o.blend_mode &&
              direct_compositing_reasons == o.direct_compositing_reasons &&
              compositor_element_id == o.compositor_element_id &&
-             filters_origin == o.filters_origin;
+             filters_origin == o.filters_origin && filters_equal &&
+             backdrops_equal && opacity_equal;
     }
   };
 
@@ -92,6 +109,12 @@ class PLATFORM_EXPORT EffectPaintPropertyNode
     return true;
   }
 
+  bool HaveNonAnimatingPropertiesChanged(const EffectPaintPropertyNode& parent,
+                                         State& state) const {
+    return !state.EqualIgnoringAnimatingProperties(state_, true) ||
+           HasParentChanged(&parent);
+  }
+
   // Checks if the accumulated effect from |this| to |relative_to_state
   // .Effect()| has changed in the space of |relative_to_state.Transform()|.
   // We check for changes of not only effect nodes, but also LocalTransformSpace

third_party/blink/renderer/platform/graphics/paint/paint_property_node.h

@@ -127,6 +127,9 @@ class PaintPropertyNode : public RefCounted<NodeType> {
     static_cast<NodeType*>(this)->SetChanged();
     return true;
   }
+  bool HasParentChanged(const NodeType* parent) const {
+    return parent != parent_;
+  }
 
   void SetChanged() {
     DCHECK(!IsRoot());

third_party/blink/renderer/platform/graphics/paint/transform_paint_property_node.h

@@ -61,8 +61,16 @@ class PLATFORM_EXPORT TransformPaintPropertyNode
     std::unique_ptr<CompositorStickyConstraint> sticky_constraint;
 
     bool operator==(const State& o) const {
-      return matrix == o.matrix && origin == o.origin &&
-             flattens_inherited_transform == o.flattens_inherited_transform &&
+      return EqualIgnoringAnimatingProperties(o, false);
+    }
+
+    bool EqualIgnoringAnimatingProperties(const State& o,
+                                          bool check_for_animations) const {
+      bool matrix_equal = ((matrix == o.matrix) && (origin == o.origin)) ||
+                          (check_for_animations &&
+                           (direct_compositing_reasons &
+                            CompositingReason::kActiveTransformAnimation));
+      return flattens_inherited_transform == o.flattens_inherited_transform &&
              backface_visibility == o.backface_visibility &&
              rendering_context_id == o.rendering_context_id &&
              direct_compositing_reasons == o.direct_compositing_reasons &&
@@ -72,7 +80,8 @@ class PLATFORM_EXPORT TransformPaintPropertyNode
                  o.affected_by_outer_viewport_bounds_delta &&
              ((!sticky_constraint && !o.sticky_constraint) ||
               (sticky_constraint && o.sticky_constraint &&
-               *sticky_constraint == *o.sticky_constraint));
+               *sticky_constraint == *o.sticky_constraint)) &&
+             matrix_equal;
     }
   };
 
@@ -105,6 +114,13 @@ class PLATFORM_EXPORT TransformPaintPropertyNode
     return true;
   }
 
+  bool HaveNonAnimatingPropertiesChanged(
+      const TransformPaintPropertyNode& parent,
+      State& state) const {
+    return !state.EqualIgnoringAnimatingProperties(state_, true) ||
+           HasParentChanged(&parent);
+  }
+
   // If |relative_to_node| is an ancestor of |this|, returns true if any node is
   // marked changed along the path from |this| to |relative_to_node| (not
   // included). Otherwise returns the combined changed status of the paths