diff --git a/AUTHORS b/AUTHORS index 4e6d33213223e8..5941416b0e21cc 100644 --- a/AUTHORS +++ b/AUTHORS @@ -1011,6 +1011,7 @@ Sam McDonald Samuel Attard Sanggi Hong Sanghee Lee +Sangheon Kim Sanghyun Park Sanghyup Lee Sangjoon Je diff --git a/cc/layers/picture_layer_impl.cc b/cc/layers/picture_layer_impl.cc index b8d830dca4930d..d0928bbcc7d333 100644 --- a/cc/layers/picture_layer_impl.cc +++ b/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; } diff --git a/cc/scheduler/scheduler.cc b/cc/scheduler/scheduler.cc index 3be3099800c0e4..51f076ead733b1 100644 --- a/cc/scheduler/scheduler.cc +++ b/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(); diff --git a/cc/scheduler/scheduler.h b/cc/scheduler/scheduler.h index 179b0e4202ca8b..198c3f58e7c33d 100644 --- a/cc/scheduler/scheduler.h +++ b/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); diff --git a/cc/scheduler/scheduler_state_machine.cc b/cc/scheduler/scheduler_state_machine.cc index 0e49dca023e51a..be47e820cbb2d2 100644 --- a/cc/scheduler/scheduler_state_machine.cc +++ b/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; } diff --git a/cc/scheduler/scheduler_state_machine.h b/cc/scheduler/scheduler_state_machine.h index 929d58550de115..0913b750018ab8 100644 --- a/cc/scheduler/scheduler_state_machine.h +++ b/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(); diff --git a/cc/test/fake_layer_tree_host_impl_client.cc b/cc/test/fake_layer_tree_host_impl_client.cc index c38f4a220a78cb..867ac860f79fbd 100644 --- a/cc/test/fake_layer_tree_host_impl_client.cc +++ b/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; } diff --git a/cc/test/fake_layer_tree_host_impl_client.h b/cc/test/fake_layer_tree_host_impl_client.h index fd4b052e105644..ccbcc715453b50 100644 --- a/cc/test/fake_layer_tree_host_impl_client.h +++ b/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 {} diff --git a/cc/test/layer_tree_test.cc b/cc/test/layer_tree_test.cc index 5b3483bba863a0..d64ddf578ead11 100644 --- a/cc/test/layer_tree_test.cc +++ b/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; } } diff --git a/cc/trees/layer_tree_host_impl.cc b/cc/trees/layer_tree_host_impl.cc index f997e3296e6976..281fca2cc23c9a 100644 --- a/cc/trees/layer_tree_host_impl.cc +++ b/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::AsWeakPtr() { return weak_factory_.GetWeakPtr(); } diff --git a/cc/trees/layer_tree_host_impl.h b/cc/trees/layer_tree_host_impl.h index 9e3ba3d12a6b66..a8df2698705eab 100644 --- a/cc/trees/layer_tree_host_impl.h +++ b/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, diff --git a/cc/trees/layer_tree_host_impl_unittest.cc b/cc/trees/layer_tree_host_impl_unittest.cc index f7bca00a04a82f..94b28bed8e07d0 100644 --- a/cc/trees/layer_tree_host_impl_unittest.cc +++ b/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 { diff --git a/cc/trees/layer_tree_host_unittest.cc b/cc/trees/layer_tree_host_unittest.cc index 4c38f34b3b8959..0f8da9326d7933 100644 --- a/cc/trees/layer_tree_host_unittest.cc +++ b/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 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( + 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( + 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 layer_on_main_; +}; +MULTI_THREAD_TEST_F(LayerTreeHostTestDelayRecreateTiling); + } // namespace } // namespace cc diff --git a/cc/trees/layer_tree_host_unittest_proxy.cc b/cc/trees/layer_tree_host_unittest_proxy.cc index 92c066cc2e7fb6..364df3eb9330dc 100644 --- a/cc/trees/layer_tree_host_unittest_proxy.cc +++ b/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. diff --git a/cc/trees/layer_tree_impl.cc b/cc/trees/layer_tree_impl.cc index 03cf2b4ccede28..4409b1108571bf 100644 --- a/cc/trees/layer_tree_impl.cc +++ b/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 diff --git a/cc/trees/layer_tree_impl.h b/cc/trees/layer_tree_impl.h index fd335404e219ac..31063c4d3e7c6d 100644 --- a/cc/trees/layer_tree_impl.h +++ b/cc/trees/layer_tree_impl.h @@ -165,6 +165,7 @@ class CC_EXPORT LayerTreeImpl { const scoped_refptr& display_list); TargetColorParams GetTargetColorParams( gfx::ContentColorUsage content_color_usage) const; + bool IsReadyToActivate() const; // Tree specific methods exposed to layer-impl tree. // --------------------------------------------------------------------------- diff --git a/cc/trees/proxy_impl.cc b/cc/trees/proxy_impl.cc index d7a12d0a16f2b0..9bce37ac5ed6d7 100644 --- a/cc/trees/proxy_impl.cc +++ b/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()); diff --git a/cc/trees/proxy_impl.h b/cc/trees/proxy_impl.h index 19cd52164e5d7b..2d545faef6ed43 100644 --- a/cc/trees/proxy_impl.h +++ b/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(). diff --git a/cc/trees/single_thread_proxy.cc b/cc/trees/single_thread_proxy.cc index c23b716f2da3e5..427392f27ebfae 100644 --- a/cc/trees/single_thread_proxy.cc +++ b/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()); diff --git a/cc/trees/single_thread_proxy.h b/cc/trees/single_thread_proxy.h index 76ed39ca2844bf..148c0d0cbdfb2b 100644 --- a/cc/trees/single_thread_proxy.h +++ b/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;