[Scroll Unif.] Filter compositor transform nodes

Scroll Unification causes transform nodes to be created for all
scrollers, even unpainted ones. This can cause a violation of a DCHECK
in compositor_animations.cc which ensures that a given animation either
has all property nodes on the compositor or none at all.

This patch is to prototype solution given by
https://bugs.chromium.org/p/chromium/issues/detail?id=1385575#c18
in which we avoid creating property (transform) nodes for layers which
are not painted.

Bug: 1385575
Change-Id: I3eaf7806d1c3019685e58a82f8be97ffc4aab503
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4117703
Reviewed-by: Steve Kobes <skobes@chromium.org>
Commit-Queue: David Awogbemila <awogbemila@chromium.org>
Reviewed-by: Robert Flack <flackr@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1122033}

cc/input/input_handler.cc

@@ -1274,6 +1274,7 @@ InputHandler::ScrollStatus InputHandler::TryScroll(
     const ScrollTree& scroll_tree,
     ScrollNode* scroll_node) const {
   DCHECK(!base::FeatureList::IsEnabled(features::kScrollUnification));
+  DCHECK(scroll_node->transform_id != kInvalidPropertyNodeId);
 
   InputHandler::ScrollStatus scroll_status;
   scroll_status.main_thread_scrolling_reasons =
@@ -1616,6 +1617,10 @@ bool InputHandler::CalculateLocalScrollDeltaAndStartPoint(
     const gfx::Vector2dF& viewport_delta,
     gfx::Vector2dF* out_local_scroll_delta,
     gfx::PointF* out_local_start_point /*= nullptr*/) {
+  if (scroll_node.transform_id == kInvalidPropertyNodeId) {
+    return false;
+  }
+
   // Layers with non-invertible screen space transforms should not have passed
   // the scroll hit test in the first place.
   const gfx::Transform screen_space_transform =

cc/trees/layer_tree_host.cc

@@ -1058,11 +1058,10 @@ void LayerTreeHost::UpdateScrollOffsetFromImpl(
       // animations, but that is not needed for an impl-side scroll.
 
       // Update the offset in the transform node.
-      DCHECK(scroll_node->transform_id != kInvalidPropertyNodeId);
       TransformTree& transform_tree =
           property_trees()->transform_tree_mutable();
       auto* transform_node = transform_tree.Node(scroll_node->transform_id);
-      if (transform_node->scroll_offset != new_offset) {
+      if (transform_node && transform_node->scroll_offset != new_offset) {
         transform_node->scroll_offset = new_offset;
         transform_node->needs_local_transform_update = true;
         transform_node->transform_changed = true;

cc/trees/layer_tree_impl.cc

@@ -238,10 +238,13 @@ void LayerTreeImpl::DidUpdateScrollOffset(ElementId id) {
       !base::FeatureList::IsEnabled(features::kScrollUnification) ||
       scroll_tree.CanRealizeScrollsOnCompositor(*scroll_node);
 
-  DCHECK(scroll_node->transform_id != kInvalidPropertyNodeId);
-  TransformTree& transform_tree = property_trees()->transform_tree_mutable();
-  auto* transform_node = transform_tree.Node(scroll_node->transform_id);
-  if (should_realize_scroll_on_compositor) {
+  // A ScrollNode may have an invalid transform_id if its scroller is
+  // unpainted. Since an unpainted scroller would not be visible to the
+  // user, realizing this scroll is a nop.
+  if (should_realize_scroll_on_compositor &&
+      scroll_node->transform_id != kInvalidPropertyNodeId) {
+    TransformTree& transform_tree = property_trees()->transform_tree_mutable();
+    auto* transform_node = transform_tree.Node(scroll_node->transform_id);
     if (transform_node->scroll_offset !=
         scroll_tree.current_scroll_offset(id)) {
       transform_node->scroll_offset = scroll_tree.current_scroll_offset(id);

cc/trees/property_tree.cc

@@ -346,19 +346,10 @@ gfx::Vector2dF TransformTree::StickyPositionOffset(TransformNode* node) {
   const ScrollNode* scroll_node =
       property_trees()->scroll_tree().Node(sticky_data->scroll_ancestor);
   const TransformNode* transform_node = Node(scroll_node->transform_id);
-  const auto& scroll_offset = transform_node->scroll_offset;
-  // TODO(crbug.com/1206694): Understand why these values are not exactly equal
-  // and which one we should be using here.
-#if DCHECK_IS_ON()
-  {
-    const auto& scroll_offset_delta =
-        property_trees()->scroll_tree().current_scroll_offset(
-            scroll_node->element_id) -
-        scroll_offset;
-    DCHECK_LE(std::abs(scroll_offset_delta.x()), 0.5);
-    DCHECK_LE(std::abs(scroll_offset_delta.y()), 0.5);
-  }
-#endif
+  DCHECK(transform_node);
+  gfx::PointF scroll_offset =
+      property_trees()->scroll_tree().current_scroll_offset(
+          scroll_node->element_id);
   gfx::PointF scroll_position(scroll_offset.x(), scroll_offset.y());
   if (transform_node->scrolls) {
     // The scroll position does not include snapping which shifts the scroll
@@ -514,6 +505,9 @@ gfx::Vector2dF TransformTree::AnchorScrollOffset(TransformNode* node) {
       continue;
     }
     const TransformNode* transform_node = Node(scroll_node->transform_id);
+    // We don't ever expect that an anchor node or any of its scrolling
+    // containers should have an invalid transform_id.
+    DCHECK(scroll_node->transform_id != kInvalidPropertyNodeId);
     accumulated_scroll_offset +=
         transform_node->scroll_offset.OffsetFromOrigin();
   }
@@ -1614,7 +1608,7 @@ const gfx::PointF ScrollTree::GetPixelSnappedScrollOffset(
   // simply rounding of the scroll position and not using fractional scroll
   // deltas (see needs_scroll_update in PushScrollUpdatesFromMainThread).
 
-  if (transform_node->scrolls) {
+  if (transform_node && transform_node->scrolls) {
     // If necessary perform a update for this node to ensure snap amount is
     // accurate. This method is used by scroll timeline, so it is possible for
     // it to get called before transform tree has gone through a full update

third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor.cc

@@ -420,6 +420,15 @@ void PaintArtifactCompositor::LayerizeGroup(
   // O(p) due to copying the chunk list. Subtotal: O((qd + p)d) = O(qd^2 + pd)
   // Assuming p > d, the total complexity would be O(pqd + qd^2 + pd) = O(pqd)
   while (chunk_cursor != artifact->PaintChunks().end()) {
+    // Track painted ScrollTranslation nodes. with ScrollUnification enabled,
+    // PaintArtifactCompositor::Update uses this to know which
+    // ScrollTranslation nodes we need to create composited Transform nodes
+    // for.
+    if (chunk_cursor->hit_test_data &&
+        chunk_cursor->hit_test_data->scroll_translation) {
+      scroll_translation_nodes_.insert(
+          chunk_cursor->hit_test_data->scroll_translation.get());
+    }
     // Look at the effect node of the next chunk. There are 3 possible cases:
     // A. The next chunk belongs to the current group but no subgroup.
     // B. The next chunk does not belong to the current group.
@@ -680,6 +689,7 @@ void PaintArtifactCompositor::Update(
   wtf_size_t old_size = pending_layers_.size();
   OldPendingLayerMatcher old_pending_layer_matcher(std::move(pending_layers_));
   pending_layers_.reserve(old_size);
+  scroll_translation_nodes_.clear();
 
   // Make compositing decisions, storing the result in |pending_layers_|.
   CollectPendingLayers(std::move(artifact));
@@ -693,11 +703,6 @@ void PaintArtifactCompositor::Update(
   UpdateCompositorViewportProperties(viewport_properties, property_tree_manager,
                                      host);
 
-  // With ScrollUnification, we ensure a cc::ScrollNode for all
-  // |scroll_translation_nodes|.
-  if (unification_enabled)
-    property_tree_manager.EnsureCompositorScrollNodes(scroll_translation_nodes);
-
   for (auto& entry : synthesized_clip_cache_)
     entry.in_use = false;
 
@@ -737,7 +742,8 @@ void PaintArtifactCompositor::Update(
     const auto& scroll_translation =
         NearestScrollTranslationForLayer(pending_layer);
     int scroll_id =
-        property_tree_manager.EnsureCompositorScrollNode(scroll_translation);
+        property_tree_manager.EnsureCompositorScrollAndTransformNode(
+            scroll_translation);
 
     layer_list_builder.Add(&layer);
 
@@ -761,6 +767,35 @@ void PaintArtifactCompositor::Update(
     }
   }
 
+  if (unification_enabled) {
+    // We want to create a cc::TransformNode only if the scroller is painted.
+    // This avoids violating an assumption in CompositorAnimations that an
+    // element has property nodes for either all or none of its animating
+    // properties (see crbug.com/1385575).
+    // However, we want to create a cc::ScrollNode regardless of whether the
+    // scroller is painted. This ensures that scroll offset animations aren't
+    // affected by becoming unpainted."
+    Vector<const TransformPaintPropertyNode*> scroll_node_only;
+    for (auto* node : scroll_translation_nodes) {
+      if (scroll_translation_nodes_.Contains(node)) {
+        property_tree_manager.EnsureCompositorScrollAndTransformNode(*node);
+      } else {
+        // We can't ensure ScrollNode-only scroll translation nodes yet because
+        // we don't have a guarantee about the order of
+        // |scroll_translation_nodes|. If an unpainted child is encountered
+        // before its parent, EnsureCompositorScrollNode will create its parent
+        // node with invalid transform_id.
+        scroll_node_only.push_back(node);
+      }
+    }
+
+    // Ensure ScrollNode-only scroll translation nodes.
+    for (auto* node : scroll_node_only) {
+      property_tree_manager.EnsureCompositorScrollNode(*node->ScrollNode(),
+                                                       *node);
+    }
+  }
+
   root_layer_->layer_tree_host()->RegisterSelection(layer_selection);
 
   property_tree_manager.Finalize();
@@ -786,6 +821,7 @@ void PaintArtifactCompositor::Update(
   host->property_trees()->ResetCachedData();
   previous_update_for_testing_ = PreviousUpdateType::kFull;
   needs_update_ = false;
+  scroll_translation_nodes_.clear();
 
   UpdateDebugInfo();
 

third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor.h

@@ -22,6 +22,7 @@
 #include "third_party/blink/renderer/platform/graphics/paint/geometry_mapper.h"
 #include "third_party/blink/renderer/platform/graphics/paint/paint_chunk_subset.h"
 #include "third_party/blink/renderer/platform/graphics/paint/paint_controller.h"
+#include "third_party/blink/renderer/platform/graphics/paint/transform_paint_property_node.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
 #include "third_party/blink/renderer/platform/wtf/vector.h"
 
@@ -340,6 +341,10 @@ class PLATFORM_EXPORT PaintArtifactCompositor final
   class OldPendingLayerMatcher;
   PendingLayers pending_layers_;
 
+  // ScrollTranslationNodes of the PaintArtifact which are painted.
+  // This member variable is only used in PaintArtifactCompositor::Update.
+  HashSet<const TransformPaintPropertyNode*> scroll_translation_nodes_;
+
   friend class StubChromeClientForCAP;
   friend class PaintArtifactCompositorTest;
 };

third_party/blink/renderer/platform/graphics/compositing/paint_artifact_compositor_test.cc

@@ -20,6 +20,7 @@
 #include "cc/trees/effect_node.h"
 #include "cc/trees/layer_tree_host.h"
 #include "cc/trees/layer_tree_settings.h"
+#include "cc/trees/property_tree.h"
 #include "cc/trees/scroll_node.h"
 #include "cc/trees/transform_node.h"
 #include "cc/view_transition/view_transition_request.h"
@@ -4652,6 +4653,36 @@ TEST_P(PaintArtifactCompositorTest, AddNonCompositedScrollNodes) {
   WTF::Vector<const TransformPaintPropertyNode*> scroll_translation_nodes;
   scroll_translation_nodes.push_back(&scroll_state.Transform());
 
+  Update(TestPaintArtifact()
+             .Chunk(1)
+             .Properties(scroll_state.Transform(), c0(), e0())
+             .Build(),
+         ViewportProperties(), scroll_translation_nodes);
+
+  const auto& scroll_tree = GetPropertyTrees().scroll_tree();
+  auto* scroll_node = scroll_tree.FindNodeFromElementId(
+      scroll_state.Transform().ScrollNode()->GetCompositorElementId());
+  EXPECT_TRUE(scroll_node);
+  EXPECT_FALSE(scroll_node->is_composited);
+}
+
+TEST_P(PaintArtifactCompositorTest, AddUnpaintedNonCompositedScrollNodes) {
+  // This test requires scroll unification.
+  if (!base::FeatureList::IsEnabled(::features::kScrollUnification)) {
+    return;
+  }
+
+  const uint32_t main_thread_scrolling_reason =
+      cc::MainThreadScrollingReason::kNotOpaqueForTextAndLCDText;
+  ASSERT_TRUE(cc::MainThreadScrollingReason::HasNonCompositedScrollReasons(
+      main_thread_scrolling_reason));
+  auto scroll_state =
+      ScrollState1(PropertyTreeState::Root(), CompositingReason::kNone,
+                   main_thread_scrolling_reason);
+
+  WTF::Vector<const TransformPaintPropertyNode*> scroll_translation_nodes;
+  scroll_translation_nodes.push_back(&scroll_state.Transform());
+
   TestPaintArtifact artifact;
   Update(artifact.Build(), ViewportProperties(), scroll_translation_nodes);
 
@@ -4660,6 +4691,7 @@ TEST_P(PaintArtifactCompositorTest, AddNonCompositedScrollNodes) {
       scroll_state.Transform().ScrollNode()->GetCompositorElementId());
   EXPECT_TRUE(scroll_node);
   EXPECT_FALSE(scroll_node->is_composited);
+  EXPECT_EQ(scroll_node->transform_id, cc::kInvalidPropertyNodeId);
 }
 
 TEST_P(PaintArtifactCompositorTest, RepaintIndirectScrollHitTest) {