Delay recreating tiling when activate is already ready

Problem Steps
1. blink commit happen
2. Before activating tree, draw is executed first
   e.g., active_tree_needs_first_draw_ is true
         readytoactivate is delayed because raster take time
3. In DrawInternal, pending_tree()->UpdateDrawProperties() is called.
   At this time, if pending tree's raster translation is different
   from the existing, the existing tiling is not reused
   and new tiling is created.
4. Now scheduler swap the pending tree which tiling is recreated in step 4.
5. When drawing the swapped active tree, checkerboard is drawn
   to the screen before raster is completed.
    So to the user, the content appears to be flickering.

In Step3, recreating tiling should be delayed until the scheduled
activation finish.

R=khushalsagar@chromium.org, sunnyps@chromium.org, vmpstr@chromium.org

Bug: 1215543
Change-Id: I66aa363953b8de63f9003e65720833f805353eb2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3518712
Reviewed-by: Sunny Sachanandani <sunnyps@chromium.org>
Reviewed-by: Khushal Sagar <khushalsagar@chromium.org>
Commit-Queue: Laszlo Gombos <l.gombos@samsung.com>
Cr-Commit-Position: refs/heads/main@{#995984}

AUTHORS

@@ -1011,6 +1011,7 @@ Sam McDonald <sam@sammcd.com>
 Samuel Attard <samuel.r.attard@gmail.com>
 Sanggi Hong <sanggi.hong11@gmail.com>
 Sanghee Lee <sanghee.lee1992@gmail.com>
+Sangheon Kim <sangheon77.kim@samsung.com>
 Sanghyun Park <sh919.park@samsung.com>
 Sanghyup Lee <sh53.lee@samsung.com>
 Sangjoon Je <htamop@gmail.com>

cc/layers/picture_layer_impl.cc

@@ -1243,6 +1243,11 @@ bool PictureLayerImpl::CanRecreateHighResTilingForLCDTextAndRasterTransform(
   if (lcd_text_disallowed_reason_ == LCDTextDisallowedReason::kNoText &&
       high_res.raster_transform().scale() == raster_contents_scale_)
     return false;
+  // If ReadyToActivate() is already scheduled, recreating tiling should be
+  // delayed until the activation is executed. Otherwise the tiles in viewport
+  // will be deleted.
+  if (layer_tree_impl()->IsReadyToActivate())
+    return false;
   return true;
 }
 

cc/scheduler/scheduler.cc

@@ -107,6 +107,10 @@ void Scheduler::NotifyReadyToActivate() {
   ProcessScheduledActions();
 }
 
+bool Scheduler::IsReadyToActivate() {
+  return state_machine_.IsReadyToActivate();
+}
+
 void Scheduler::NotifyReadyToDraw() {
   // Future work might still needed for crbug.com/352894.
   state_machine_.NotifyReadyToDraw();

cc/scheduler/scheduler.h

@@ -132,6 +132,7 @@ class CC_EXPORT Scheduler : public viz::BeginFrameObserverBase {
   // pending_tree, call this method to notify that this pending tree is ready to
   // be activated, that is to be copied to the active tree.
   void NotifyReadyToActivate();
+  bool IsReadyToActivate();
   void NotifyReadyToDraw();
   void SetBeginFrameSource(viz::BeginFrameSource* source);
 

cc/scheduler/scheduler_state_machine.cc

@@ -1469,6 +1469,10 @@ bool SchedulerStateMachine::NotifyReadyToActivate() {
   return true;
 }
 
+bool SchedulerStateMachine::IsReadyToActivate() {
+  return pending_tree_is_ready_for_activation_;
+}
+
 void SchedulerStateMachine::NotifyReadyToDraw() {
   active_tree_is_ready_to_draw_ = true;
 }

cc/scheduler/scheduler_state_machine.h

@@ -287,6 +287,7 @@ class CC_EXPORT SchedulerStateMachine {
   // the notification received updated the state for the current pending tree,
   // if any.
   bool NotifyReadyToActivate();
+  bool IsReadyToActivate();
 
   // Indicates the active tree's visible tiles are ready to be drawn.
   void NotifyReadyToDraw();

cc/test/fake_layer_tree_host_impl_client.cc

@@ -20,6 +20,10 @@ void FakeLayerTreeHostImplClient::NotifyReadyToActivate() {
   ready_to_activate_ = true;
 }
 
+bool FakeLayerTreeHostImplClient::IsReadyToActivate() {
+  return ready_to_activate();
+}
+
 void FakeLayerTreeHostImplClient::NotifyReadyToDraw() {
   ready_to_draw_ = true;
 }

cc/test/fake_layer_tree_host_impl_client.h

@@ -21,6 +21,7 @@ class FakeLayerTreeHostImplClient : public LayerTreeHostImplClient {
   void DidReceiveCompositorFrameAckOnImplThread() override {}
   void OnCanDrawStateChanged(bool can_draw) override {}
   void NotifyReadyToActivate() override;
+  bool IsReadyToActivate() override;
   void NotifyReadyToDraw() override;
   void SetNeedsRedrawOnImplThread() override {}
   void SetNeedsOneBeginImplFrameOnImplThread() override {}

cc/test/layer_tree_test.cc

@@ -277,15 +277,18 @@ class LayerTreeHostImplForTesting : public LayerTreeHostImpl {
     test_hooks_->NotifyAllTileTasksCompleted(this);
   }
 
-  void BlockNotifyReadyToActivateForTesting(bool block) override {
+  void BlockNotifyReadyToActivateForTesting(bool block,
+                                            bool notify_if_blocked) override {
     CHECK(task_runner_provider()->ImplThreadTaskRunner())
         << "Not supported for single-threaded mode.";
     block_notify_ready_to_activate_for_testing_ = block;
     if (!block && notify_ready_to_activate_was_blocked_) {
-      task_runner_provider_->ImplThreadTaskRunner()->PostTask(
-          FROM_HERE,
-          base::BindOnce(&LayerTreeHostImplForTesting::NotifyReadyToActivate,
-                         base::Unretained(this)));
+      if (notify_if_blocked) {
+        task_runner_provider_->ImplThreadTaskRunner()->PostTask(
+            FROM_HERE,
+            base::BindOnce(&LayerTreeHostImplForTesting::NotifyReadyToActivate,
+                           base::Unretained(this)));
+      }
       notify_ready_to_activate_was_blocked_ = false;
     }
   }

cc/trees/layer_tree_host_impl.cc

@@ -1732,7 +1732,9 @@ void LayerTreeHostImpl::EvictTexturesForTesting() {
   UpdateTileManagerMemoryPolicy(ManagedMemoryPolicy(0));
 }
 
-void LayerTreeHostImpl::BlockNotifyReadyToActivateForTesting(bool block) {
+void LayerTreeHostImpl::BlockNotifyReadyToActivateForTesting(
+    bool block,
+    bool notify_if_blocked) {
   NOTREACHED();
 }
 
@@ -5134,6 +5136,10 @@ void LayerTreeHostImpl::RequestInvalidationForAnimatedImages() {
   client_->NeedsImplSideInvalidation(needs_first_draw_on_activation);
 }
 
+bool LayerTreeHostImpl::IsReadyToActivate() const {
+  return client_->IsReadyToActivate();
+}
+
 base::WeakPtr<LayerTreeHostImpl> LayerTreeHostImpl::AsWeakPtr() {
   return weak_factory_.GetWeakPtr();
 }

cc/trees/layer_tree_host_impl.h

@@ -119,6 +119,7 @@ class LayerTreeHostImplClient {
   virtual void DidReceiveCompositorFrameAckOnImplThread() = 0;
   virtual void OnCanDrawStateChanged(bool can_draw) = 0;
   virtual void NotifyReadyToActivate() = 0;
+  virtual bool IsReadyToActivate() = 0;
   virtual void NotifyReadyToDraw() = 0;
   // Please call these 2 functions through
   // LayerTreeHostImpl's SetNeedsRedraw() and SetNeedsOneBeginImplFrame().
@@ -481,8 +482,11 @@ class CC_EXPORT LayerTreeHostImpl : public TileManagerClient,
 
   // When blocking, this prevents client_->NotifyReadyToActivate() from being
   // called. When disabled, it calls client_->NotifyReadyToActivate()
-  // immediately if any notifications had been blocked while blocking.
-  virtual void BlockNotifyReadyToActivateForTesting(bool block);
+  // immediately if any notifications had been blocked while blocking and
+  // notify_if_blocked is true.
+  virtual void BlockNotifyReadyToActivateForTesting(
+      bool block,
+      bool notify_if_blocked = true);
 
   // Prevents notifying the |client_| when an impl side invalidation request is
   // made. When unblocked, the disabled request will immediately be called.
@@ -879,6 +883,8 @@ class CC_EXPORT LayerTreeHostImpl : public TileManagerClient,
     return throttle_decider_.ids();
   }
 
+  bool IsReadyToActivate() const;
+
  protected:
   LayerTreeHostImpl(
       const LayerTreeSettings& settings,

cc/trees/layer_tree_host_impl_unittest.cc

@@ -215,6 +215,11 @@ class LayerTreeHostImplTest : public testing::Test,
     did_notify_ready_to_activate_ = true;
     host_impl_->ActivateSyncTree();
   }
+  bool IsReadyToActivate() override {
+    // in NotifyReadyToActivate(), call ActivateSyncTree() directly
+    // so this is always false
+    return false;
+  }
   void NotifyReadyToDraw() override {}
   void SetNeedsRedrawOnImplThread() override { did_request_redraw_ = true; }
   void SetNeedsOneBeginImplFrameOnImplThread() override {

cc/trees/layer_tree_host_unittest.cc

@@ -10142,5 +10142,150 @@ class LayerTreeHostTestNoCommitDeadlock : public LayerTreeHostTest {
 };
 
 SINGLE_AND_MULTI_THREAD_TEST_F(LayerTreeHostTestNoCommitDeadlock);
+
+// In real site, problem happened like this
+// 1. commit
+// 2. tiling is delayed, so NotifyReadyToActivate is not triggered
+// 3. Draw is called and NotifyReadyToActivate is triggered
+//    during PrepareDraw() by TileManager's CheckForCompletedTask().
+// 4. pending_tree()::UpdateDrawProperties() is called after PrepareDraw(),
+//    and tiling is recreated if transform is changed
+// 5. Activation happen right after the Draw
+//    So tiling with empty tiles will be activated to active tree.
+class LayerTreeHostTestDelayRecreateTiling
+    : public LayerTreeHostTestWithHelper {
+ public:
+  LayerTreeHostTestDelayRecreateTiling() {}
+
+  void SetupTree() override {
+    client_.set_fill_with_nonsolid_color(true);
+    scoped_refptr<FakePictureLayer> root_layer =
+        FakePictureLayer::Create(&client_);
+    root_layer->SetBounds(gfx::Size(150, 150));
+    root_layer->SetIsDrawable(true);
+
+    layer_on_main_ =
+        CreateAndAddFakePictureLayer(gfx::Size(30, 30), root_layer.get());
+
+    // initial transform to force transform node
+    gfx::Transform transform;
+    transform.Scale(2.0f, 1.0f);
+    layer_on_main_->SetTransform(transform);
+
+    layer_tree_host()->SetRootLayer(root_layer);
+
+    LayerTreeHostTest::SetupTree();
+    client_.set_bounds(root_layer->bounds());
+
+    layer_id_ = layer_on_main_->id();
+  }
+
+  void WillCommit(const CommitState&) override {
+    TransformTree& transform_tree =
+        layer_tree_host()->property_trees()->transform_tree_mutable();
+    TransformNode* node =
+        transform_tree.Node(layer_on_main_->transform_tree_index());
+
+    gfx::Transform transform;
+    transform.Scale(2.0f, 1.0f);
+    transform.Translate(0.0f, 0.8f);
+
+    switch (layer_tree_host()->SourceFrameNumber()) {
+      case 1:
+        // in frame1, translation changed and animation start
+        transform_tree.OnTransformAnimated(layer_on_main_->element_id(),
+                                           transform);
+        node->has_potential_animation = true;
+        break;
+      case 3:
+        // animation finished
+        node->has_potential_animation = false;
+        transform_tree.set_needs_update(true);
+        break;
+    }
+  }
+
+  void BeginTest() override { PostSetNeedsCommitToMainThread(); }
+
+  void CommitCompleteOnThread(LayerTreeHostImpl* host_impl) override {
+    FakePictureLayerImpl* layer_impl = static_cast<FakePictureLayerImpl*>(
+        host_impl->pending_tree()->LayerById(layer_id_));
+
+    TransformTree& transform_tree =
+        host_impl->pending_tree()->property_trees()->transform_tree_mutable();
+    TransformNode* node =
+        transform_tree.Node(layer_impl->transform_tree_index());
+
+    if (host_impl->pending_tree()->source_frame_number() == 2) {
+      // delay Activation for this pending tree
+      host_impl->BlockNotifyReadyToActivateForTesting(true);
+
+      // to reproduce problem, conditions to recreate tiling should be changed
+      // after commitcomplete
+      // e.g., transform change, animation status change
+      // commitcomplete -> beginimpl -> draw (pending's updatedrawproperties)
+      // in beginimpl, scroll can be handled, so transform can be changed
+      // in draw, UpdateAnimationState can change animation status
+      node->has_potential_animation = false;
+      transform_tree.set_needs_update(true);
+      host_impl->pending_tree()->set_needs_update_draw_properties();
+
+      // to make sure Draw happen
+      host_impl->SetNeedsRedraw();
+    }
+  }
+
+  DrawResult PrepareToDrawOnThread(LayerTreeHostImpl* host_impl,
+                                   LayerTreeHostImpl::FrameData* frame_data,
+                                   DrawResult draw_result) override {
+    if (host_impl->pending_tree() &&
+        host_impl->pending_tree()->source_frame_number() == 2) {
+      host_impl->BlockNotifyReadyToActivateForTesting(false, false);
+      // BlockNotifyReadyToActivateForTesting(false) call NotifyReadyToActivate,
+      // but NotifyReadyToActivate should be called directly instead of PostTask
+      // because it should called inside PrepareToDraw()
+      host_impl->NotifyReadyToActivate();
+    }
+    return draw_result;
+  }
+
+  void DidActivateTreeOnThread(LayerTreeHostImpl* host_impl) override {
+    FakePictureLayerImpl* layer_impl = static_cast<FakePictureLayerImpl*>(
+        host_impl->active_tree()->LayerById(layer_id_));
+
+    gfx::AxisTransform2d tiling_transform =
+        layer_impl->HighResTiling()->raster_transform();
+
+    switch (host_impl->active_tree()->source_frame_number()) {
+      case 0:
+        PostSetNeedsCommitToMainThread();
+        break;
+      case 1:
+        PostSetNeedsCommitToMainThread();
+        break;
+      case 2:
+        // translation is changed in frame2, but recreating tiling should not
+        // happen because ReadyToActivate is true
+        ASSERT_EQ(tiling_transform.scale(), gfx::Vector2dF(2.0f, 1.0f));
+        ASSERT_EQ(tiling_transform.translation(), gfx::Vector2dF(0, 0));
+        PostSetNeedsCommitToMainThread();
+        break;
+      case 3:
+        // in next commit, the new tiling with new translation is generated
+        ASSERT_EQ(tiling_transform.scale(), gfx::Vector2dF(2.0f, 1.0f));
+        ASSERT_EQ(tiling_transform.translation(), gfx::Vector2dF(0, 0.8f));
+        EndTest();
+    }
+  }
+
+ protected:
+  FakeContentLayerClient client_;
+  // to access layer information in main and impl
+  int layer_id_;
+  // to access layer information in main's WillCommit()
+  scoped_refptr<FakePictureLayer> layer_on_main_;
+};
+MULTI_THREAD_TEST_F(LayerTreeHostTestDelayRecreateTiling);
+
 }  // namespace
 }  // namespace cc

cc/trees/layer_tree_host_unittest_proxy.cc

@@ -387,7 +387,7 @@ class LayerTreeHostProxyTestCommitWaitsForActivationMFBA
         // case above). We unblock activate to allow this main frame to commit.
         auto unblock = base::BindOnce(
             &LayerTreeHostImpl::BlockNotifyReadyToActivateForTesting,
-            base::Unretained(impl), false);
+            base::Unretained(impl), false, true);
         // Post the unblock instead of doing it immediately so that the main
         // frame is fully processed by the compositor thread, and it has a full
         // opportunity to wrongly unblock the main thread.

cc/trees/layer_tree_impl.cc

@@ -2895,4 +2895,8 @@ bool LayerTreeImpl::HasDocumentTransitionRequests() const {
   return !document_transition_requests_.empty();
 }
 
+bool LayerTreeImpl::IsReadyToActivate() const {
+  return host_impl_->IsReadyToActivate();
+}
+
 }  // namespace cc

cc/trees/layer_tree_impl.h

@@ -165,6 +165,7 @@ class CC_EXPORT LayerTreeImpl {
       const scoped_refptr<DisplayItemList>& display_list);
   TargetColorParams GetTargetColorParams(
       gfx::ContentColorUsage content_color_usage) const;
+  bool IsReadyToActivate() const;
 
   // Tree specific methods exposed to layer-impl tree.
   // ---------------------------------------------------------------------------

cc/trees/proxy_impl.cc

@@ -386,6 +386,11 @@ void ProxyImpl::NotifyReadyToActivate() {
   scheduler_->NotifyReadyToActivate();
 }
 
+bool ProxyImpl::IsReadyToActivate() {
+  DCHECK(IsImplThread());
+  return scheduler_->IsReadyToActivate();
+}
+
 void ProxyImpl::NotifyReadyToDraw() {
   TRACE_EVENT0("cc", "ProxyImpl::NotifyReadyToDraw");
   DCHECK(IsImplThread());

cc/trees/proxy_impl.h

@@ -97,6 +97,7 @@ class CC_EXPORT ProxyImpl : public LayerTreeHostImplClient,
   void DidReceiveCompositorFrameAckOnImplThread() override;
   void OnCanDrawStateChanged(bool can_draw) override;
   void NotifyReadyToActivate() override;
+  bool IsReadyToActivate() override;
   void NotifyReadyToDraw() override;
   // Please call these 2 functions through
   // LayerTreeHostImpl's SetNeedsRedraw() and SetNeedsOneBeginImplFrame().

cc/trees/single_thread_proxy.cc

@@ -444,6 +444,13 @@ void SingleThreadProxy::NotifyReadyToActivate() {
     scheduler_on_impl_thread_->NotifyReadyToActivate();
 }
 
+bool SingleThreadProxy::IsReadyToActivate() {
+  DCHECK(!task_runner_provider_->HasImplThread() ||
+         task_runner_provider_->IsImplThread());
+  return scheduler_on_impl_thread_ &&
+         scheduler_on_impl_thread_->IsReadyToActivate();
+}
+
 void SingleThreadProxy::NotifyReadyToDraw() {
   DCHECK(!task_runner_provider_->HasImplThread() ||
          task_runner_provider_->IsImplThread());

cc/trees/single_thread_proxy.h

@@ -112,6 +112,7 @@ class CC_EXPORT SingleThreadProxy : public Proxy,
   void DidReceiveCompositorFrameAckOnImplThread() override;
   void OnCanDrawStateChanged(bool can_draw) override;
   void NotifyReadyToActivate() override;
+  bool IsReadyToActivate() override;
   void NotifyReadyToDraw() override;
   void SetNeedsRedrawOnImplThread() override;
   void SetNeedsOneBeginImplFrameOnImplThread() override;