[animation-worklet] Take pixel snapping into account in ScrollTimeline

Update ScrollTimeline to use post-snapped scroll offsets instead of pre-snapped
one. This fixes jitter when scroll-linked worklet animation is trying to
position items against the scroll offset.

Background:
Scrolling layers are pixel snapped which means they are shifted so that
they get aligned to pixel boundaries. This means that the raw scroll offset
of a scrolling layer is not exactly what is display on the screen.

The snapping is done by adjusting the effective transform of the associated
TransformNode for this scrolling element.

Previously ScrollTimeline would use raw scroll offset to compute its time.
This patch introduces a new method on the ScrollTree that will use the info
from TransformNode to calculate an effective scroll offset that represents the
post-snapped position of the scrolling node.

Bug: 585458
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I30bb052d8cfef8a32ec1a613921d7594bef3c982
Reviewed-on: https://chromium-review.googlesource.com/1153583
Commit-Queue: Majid Valipour <majidvp@chromium.org>
Reviewed-by: Stephen McGruer <smcgruer@chromium.org>
Reviewed-by: Ali Juma <ajuma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583819}

cc/animation/DEPS

@@ -13,5 +13,6 @@ include_rules = [
   "+cc/trees/property_animation_state.h",
   "+cc/trees/property_tree.h",
   "+cc/trees/scroll_node.h",
+  "+cc/trees/transform_node.h",
   "+cc/trees/target_property.h",
 ]

cc/animation/animation_host_unittest.cc

@@ -13,6 +13,7 @@
 #include "cc/test/animation_timelines_test_common.h"
 #include "cc/test/mock_layer_tree_mutator.h"
 #include "cc/trees/scroll_node.h"
+#include "cc/trees/transform_node.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -258,14 +259,39 @@ bool Animation1TimeEquals20(MutatorInputState* input) {
 
 void CreateScrollingNodeForElement(ElementId element_id,
                                    PropertyTrees* property_trees) {
-  ScrollNode node;
-  node.scrollable = true;
+  // A scrolling node in cc has a corresponding transform node (See
+  // |ScrollNode::transform_id|). This setup here creates both nodes and links
+  // them as they would normally be. This more complete setup is necessary here
+  // because ScrollTimelin depends on both nodes for its calculations.
+  TransformNode transform_node;
+  transform_node.scrolls = true;
+  transform_node.source_node_id = TransformTree::kRootNodeId;
+  int transform_node_id =
+      property_trees->transform_tree.Insert(transform_node, 0);
+  property_trees->element_id_to_transform_node_index[element_id] =
+      transform_node_id;
+
+  ScrollNode scroll_node;
+  scroll_node.scrollable = true;
   // Setup scroll dimention to be 100x100.
-  node.bounds = gfx::Size(200, 200);
-  node.container_bounds = gfx::Size(100, 100);
+  scroll_node.bounds = gfx::Size(200, 200);
+  scroll_node.container_bounds = gfx::Size(100, 100);
+  scroll_node.element_id = element_id;
+  scroll_node.transform_id = transform_node_id;
 
-  int node_id = property_trees->scroll_tree.Insert(node, 0);
-  property_trees->element_id_to_scroll_node_index[element_id] = node_id;
+  int scroll_node_id = property_trees->scroll_tree.Insert(scroll_node, 0);
+  property_trees->element_id_to_scroll_node_index[element_id] = scroll_node_id;
+}
+
+void SetScrollOffset(PropertyTrees* property_trees,
+                     ElementId element_id,
+                     gfx::ScrollOffset offset) {
+  // Update both scroll and transform trees
+  property_trees->scroll_tree.SetScrollOffset(element_id, offset);
+  TransformNode* transform_node =
+      property_trees->transform_tree.FindNodeFromElementId(element_id);
+  transform_node->scroll_offset = offset;
+  transform_node->needs_local_transform_update = true;
 }
 
 TEST_F(AnimationHostTest, LayerTreeMutatorUpdateReflectsScrollAnimations) {
@@ -281,20 +307,18 @@ TEST_F(AnimationHostTest, LayerTreeMutatorUpdateReflectsScrollAnimations) {
 
   PropertyTrees property_trees;
   property_trees.is_main_thread = false;
+  property_trees.is_active = true;
   CreateScrollingNodeForElement(element_id, &property_trees);
 
-  ScrollTree& scroll_tree = property_trees.scroll_tree;
   // Set an initial scroll value.
-  scroll_tree.SetScrollOffsetDeltaForTesting(element_id,
-                                             gfx::Vector2dF(10, 10));
+  SetScrollOffset(&property_trees, element_id, gfx::ScrollOffset(10, 10));
 
   scoped_refptr<MockAnimation> mock_scroll_animation(
       new MockAnimation(animation_id1));
   EXPECT_CALL(*mock_scroll_animation, Tick(_))
       .WillOnce(InvokeWithoutArgs([&]() {
         // Scroll to 20% of the max value.
-        scroll_tree.SetScrollOffsetDeltaForTesting(element_id,
-                                                   gfx::Vector2dF(20, 20));
+        SetScrollOffset(&property_trees, element_id, gfx::ScrollOffset(20, 20));
       }));
 
   // Ensure scroll animation is ticking.
@@ -326,7 +350,8 @@ TEST_F(AnimationHostTest, LayerTreeMutatorUpdateReflectsScrollAnimations) {
   // Ticking host should cause scroll animation to scroll which should also be
   // reflected in the input of the layer tree mutator in the same animation
   // frame.
-  host_impl_->TickAnimations(base::TimeTicks(), scroll_tree, false);
+  host_impl_->TickAnimations(base::TimeTicks(), property_trees.scroll_tree,
+                             false);
 }
 
 }  // namespace

cc/animation/scroll_timeline.cc

@@ -40,15 +40,18 @@ double ScrollTimeline::CurrentTime(const ScrollTree& scroll_tree,
 
   // The scroller may not be in the ScrollTree if it is not currently scrollable
   // (e.g. has overflow: visible). By the spec, return an unresolved time value.
-  if (!scroll_tree.FindNodeFromElementId(scroller_id))
+  const ScrollNode* scroll_node =
+      scroll_tree.FindNodeFromElementId(scroller_id);
+  if (!scroll_node)
     return std::numeric_limits<double>::quiet_NaN();
 
-  gfx::ScrollOffset offset = scroll_tree.current_scroll_offset(scroller_id);
+  gfx::ScrollOffset offset =
+      scroll_tree.GetPixelSnappedScrollOffset(scroll_node->id);
   DCHECK_GE(offset.x(), 0);
   DCHECK_GE(offset.y(), 0);
 
-  gfx::ScrollOffset scroll_dimensions = scroll_tree.MaxScrollOffset(
-      scroll_tree.FindNodeFromElementId(scroller_id)->id);
+  gfx::ScrollOffset scroll_dimensions =
+      scroll_tree.MaxScrollOffset(scroll_node->id);
 
   double current_offset = (orientation_ == Vertical) ? offset.y() : offset.x();
   double max_offset = (orientation_ == Vertical) ? scroll_dimensions.y()

cc/animation/scroll_timeline_unittest.cc

@@ -5,11 +5,49 @@
 #include "cc/animation/scroll_timeline.h"
 #include "cc/trees/property_tree.h"
 #include "cc/trees/scroll_node.h"
+#include "cc/trees/transform_node.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/gfx/geometry/scroll_offset.h"
 
 namespace cc {
 
+void SetScrollOffset(PropertyTrees* property_trees,
+                     ElementId scroller_id,
+                     gfx::ScrollOffset offset) {
+  // Update both scroll and transform trees
+  property_trees->scroll_tree.SetScrollOffset(scroller_id, offset);
+  TransformNode* transform_node =
+      property_trees->transform_tree.FindNodeFromElementId(scroller_id);
+  transform_node->scroll_offset = offset;
+  transform_node->needs_local_transform_update = true;
+}
+
+void CreateScrollingElement(PropertyTrees* property_trees,
+                            ElementId scroller_id,
+                            gfx::Size content_size,
+                            gfx::Size container_size) {
+  // Create a corresponding TransformNode for the scrolling element.
+  TransformNode transform_node;
+  transform_node.scrolls = true;
+  transform_node.source_node_id = TransformTree::kRootNodeId;
+  int transform_node_id =
+      property_trees->transform_tree.Insert(transform_node, 0);
+  property_trees->element_id_to_transform_node_index[scroller_id] =
+      transform_node_id;
+
+  // Add the scrolling node for the scrolling and link it to the above transform
+  // node.
+  ScrollNode scroll_node;
+  scroll_node.scrollable = true;
+  scroll_node.bounds = content_size;
+  scroll_node.container_bounds = container_size;
+  scroll_node.element_id = scroller_id;
+  scroll_node.transform_id = transform_node_id;
+  int scroll_node_id = property_trees->scroll_tree.Insert(scroll_node, 0);
+
+  property_trees->element_id_to_scroll_node_index[scroller_id] = scroll_node_id;
+}
+
 class ScrollTimelineTest : public ::testing::Test {
  public:
   ScrollTimelineTest()
@@ -19,17 +57,14 @@ class ScrollTimelineTest : public ::testing::Test {
     property_trees_.is_main_thread = true;
     property_trees_.is_active = false;
 
-    // We add a single node that is scrolling a 550x1100 contents inside a
-    // 50x100 container.
-    ScrollNode node;
-    node.scrollable = true;
-    node.bounds = content_size_;
-    node.container_bounds = container_size_;
-
-    int node_id = property_trees_.scroll_tree.Insert(node, 0);
-    property_trees_.element_id_to_scroll_node_index[scroller_id_] = node_id;
+    // Create a single scroller that is scrolling a 500x500 contents inside a
+    // 100x100 container.
+    CreateScrollingElement(&property_trees_, scroller_id_, content_size_,
+                           container_size_);
   }
 
+  PropertyTrees& property_trees() { return property_trees_; }
+
   ScrollTree& scroll_tree() { return property_trees_.scroll_tree; }
   ElementId scroller_id() const { return scroller_id_; }
   gfx::Size container_size() const { return container_size_; }
@@ -54,12 +89,12 @@ TEST_F(ScrollTimelineTest, BasicCurrentTimeCalculations) {
                                      time_range);
 
   // Unscrolled, both timelines should read a current time of 0.
-  scroll_tree().SetScrollOffset(scroller_id(), gfx::ScrollOffset());
+  SetScrollOffset(&property_trees(), scroller_id(), gfx::ScrollOffset());
   EXPECT_FLOAT_EQ(0, vertical_timeline.CurrentTime(scroll_tree(), false));
   EXPECT_FLOAT_EQ(0, horizontal_timeline.CurrentTime(scroll_tree(), false));
 
   // Now do some scrolling and make sure that the ScrollTimelines update.
-  scroll_tree().SetScrollOffset(scroller_id(), gfx::ScrollOffset(75, 50));
+  SetScrollOffset(&property_trees(), scroller_id(), gfx::ScrollOffset(75, 50));
 
   // As noted above, we have mapped the time range such that current time should
   // just be the scroll offset.
@@ -73,7 +108,8 @@ TEST_F(ScrollTimelineTest, CurrentTimeIsAdjustedForTimeRange) {
   ScrollTimeline timeline(scroller_id(), ScrollTimeline::Vertical, 100);
 
   double halfwayY = (content_size().height() - container_size().height()) / 2.;
-  scroll_tree().SetScrollOffset(scroller_id(), gfx::ScrollOffset(0, halfwayY));
+  SetScrollOffset(&property_trees(), scroller_id(),
+                  gfx::ScrollOffset(0, halfwayY));
 
   EXPECT_FLOAT_EQ(50, timeline.CurrentTime(scroll_tree(), false));
 }
@@ -95,17 +131,11 @@ TEST_F(ScrollTimelineTest, ActiveTimeIsSetOnlyAfterPromotion) {
 
   // Initially only the pending tree has the scroll node.
   ElementId scroller_id(1);
-  ScrollNode node;
-  node.scrollable = true;
-  node.bounds = content_size();
-  node.container_bounds = container_size();
-
-  int node_id = pending_tree.scroll_tree.Insert(node, 0);
-  pending_tree.element_id_to_scroll_node_index[scroller_id] = node_id;
+  CreateScrollingElement(&pending_tree, scroller_id, content_size(),
+                         container_size());
 
   double halfwayY = (content_size().height() - container_size().height()) / 2.;
-  pending_tree.scroll_tree.SetScrollOffset(scroller_id,
-                                           gfx::ScrollOffset(0, halfwayY));
+  SetScrollOffset(&pending_tree, scroller_id, gfx::ScrollOffset(0, halfwayY));
 
   ScrollTimeline main_timeline(scroller_id, ScrollTimeline::Vertical, 100);
 
@@ -131,4 +161,19 @@ TEST_F(ScrollTimelineTest, ActiveTimeIsSetOnlyAfterPromotion) {
                   impl_timeline->CurrentTime(pending_tree.scroll_tree, false));
 }
 
+TEST_F(ScrollTimelineTest, CurrentTimeIsAdjustedForPixelSnapping) {
+  double time_range = content_size().height() - container_size().height();
+  ScrollTimeline timeline(scroller_id(), ScrollTimeline::Vertical, time_range);
+
+  SetScrollOffset(&property_trees(), scroller_id(), gfx::ScrollOffset(0, 50));
+
+  // For simplicity emulate snapping by directly setting snap_amount of
+  // transform node.
+  TransformNode* transform_node =
+      property_trees().transform_tree.FindNodeFromElementId(scroller_id());
+  transform_node->snap_amount = gfx::Vector2dF(0, 0.5);
+
+  EXPECT_FLOAT_EQ(49.5, timeline.CurrentTime(scroll_tree(), false));
+}
+
 }  // namespace cc

cc/trees/property_tree.cc

@@ -1434,6 +1434,33 @@ const gfx::ScrollOffset ScrollTree::current_scroll_offset(ElementId id) const {
              : gfx::ScrollOffset();
 }
 
+const gfx::ScrollOffset ScrollTree::GetPixelSnappedScrollOffset(
+    int scroll_node_id) const {
+  const ScrollNode* scroll_node = Node(scroll_node_id);
+  DCHECK(scroll_node);
+  gfx::ScrollOffset offset = current_scroll_offset(scroll_node->element_id);
+
+  const TransformNode* transform_node =
+      property_trees()->transform_tree.Node(scroll_node->transform_id);
+  DCHECK(offset == transform_node->scroll_offset)
+      << "Transform node scroll offset does not match the actual offset, this "
+         "means the snapped_amount calculation will be incorrect";
+
+  if (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
+    // cycle so this node snap amount may be stale.
+    if (transform_node->needs_local_transform_update)
+      property_trees()->transform_tree.UpdateTransforms(transform_node->id);
+
+    offset.set_x(offset.x() - transform_node->snap_amount.x());
+    offset.set_y(offset.y() - transform_node->snap_amount.y());
+  }
+
+  return offset;
+}
+
 gfx::ScrollOffset ScrollTree::PullDeltaForMainThread(
     SyncedScrollOffset* scroll_offset) {
   DCHECK(property_trees()->is_active);

cc/trees/property_tree.h

@@ -419,6 +419,18 @@ class CC_EXPORT ScrollTree final : public PropertyTree<ScrollNode> {
   // on the active tree.
   const gfx::ScrollOffset current_scroll_offset(ElementId id) const;
 
+  // Returns the scroll offset taking into account any adjustments that may be
+  // included due to pixel snapping.
+  //
+  // Note: Using this method may causes the associated transform node for this
+  // scroll node to update its transforms.
+  //
+  // TODO(crbug.com/585458): Updating single transform node only works for
+  // simple cases but we really should update the whole transform tree otherwise
+  // we are ignoring any parent transform node that needs updating and thus our
+  // snap amount can be incorrect.
+  const gfx::ScrollOffset GetPixelSnappedScrollOffset(int scroll_node_id) const;
+
   // Collects deltas for scroll changes on the impl thread that need to be
   // reported to the main thread during the main frame. As such, should only be
   // called on the impl thread side PropertyTrees.