From b435e7af416508c9e7e75a8f6a9a11f5bc539cda Mon Sep 17 00:00:00 2001 From: danakj Date: Mon, 20 Apr 2015 15:41:41 -0700 Subject: [PATCH] cc: Don't claim ownership of resources in the pending tree. DelegatedRendererLayerImpl will claim ownership of resources in a frame, and this makes unused resources from the previous frame be returned to the child. However, this was being done on the pending tree, but the previous frame was still in the active tree and its resources would be returned early. Instead, delay telling the ResourceProvider about the frame's resources until the frame replaces the resources on the active tree. R=enne, piman@chromium.org BUG=475894 Review URL: https://codereview.chromium.org/1087843006 Cr-Commit-Position: refs/heads/master@{#325927} --- cc/layers/delegated_renderer_layer_impl.cc | 21 +++- cc/layers/delegated_renderer_layer_impl.h | 2 + .../layer_tree_host_unittest_delegated.cc | 98 ++++++++++++++++++- 3 files changed, 116 insertions(+), 5 deletions(-) diff --git a/cc/layers/delegated_renderer_layer_impl.cc b/cc/layers/delegated_renderer_layer_impl.cc index 98ac103db44eb..aa16beb56883d 100644 --- a/cc/layers/delegated_renderer_layer_impl.cc +++ b/cc/layers/delegated_renderer_layer_impl.cc @@ -80,6 +80,9 @@ void DelegatedRendererLayerImpl::PushPropertiesTo(LayerImpl* layer) { if (have_render_passes_to_push_) { // This passes ownership of the render passes to the active tree. delegated_layer->SetRenderPasses(&render_passes_in_draw_order_); + // Once resources are on the active tree, give them to the ResourceProvider + // and release unused resources from the old frame. + delegated_layer->TakeOwnershipOfResourcesIfOnActiveTree(resources_); DCHECK(render_passes_in_draw_order_.empty()); have_render_passes_to_push_ = false; } @@ -130,14 +133,17 @@ void DelegatedRendererLayerImpl::SetFrameData( } if (invalid_frame) { - // Declare we are still using the last frame's resources. + // Declare we are still using the last frame's resources. Drops ownership of + // any invalid resources, keeping only those in use by the active tree. resource_provider->DeclareUsedResourcesFromChild(child_id_, resources_); return; } - // Declare we are using the new frame's resources. + // Save the new frame's resources, but don't give them to the ResourceProvider + // until they are active, since the resources on the active tree will still be + // used and we don't want to return them early. resources_.swap(resources_in_frame); - resource_provider->DeclareUsedResourcesFromChild(child_id_, resources_); + TakeOwnershipOfResourcesIfOnActiveTree(resources_); inverse_device_scale_factor_ = 1.0f / frame_data->device_scale_factor; // Display size is already set so we can compute what the damage rect @@ -155,6 +161,15 @@ void DelegatedRendererLayerImpl::SetFrameData( have_render_passes_to_push_ = true; } +void DelegatedRendererLayerImpl::TakeOwnershipOfResourcesIfOnActiveTree( + const ResourceProvider::ResourceIdSet& resources) { + DCHECK(child_id_); + if (!layer_tree_impl()->IsActiveTree()) + return; + layer_tree_impl()->resource_provider()->DeclareUsedResourcesFromChild( + child_id_, resources); +} + void DelegatedRendererLayerImpl::SetRenderPasses( RenderPassList* render_passes_in_draw_order) { ClearRenderPasses(); diff --git a/cc/layers/delegated_renderer_layer_impl.h b/cc/layers/delegated_renderer_layer_impl.h index d59ccc2e82c8a..617d76f3b1e3a 100644 --- a/cc/layers/delegated_renderer_layer_impl.h +++ b/cc/layers/delegated_renderer_layer_impl.h @@ -67,6 +67,8 @@ class CC_EXPORT DelegatedRendererLayerImpl : public LayerImpl { void AppendRainbowDebugBorder(RenderPass* render_pass); + void TakeOwnershipOfResourcesIfOnActiveTree( + const ResourceProvider::ResourceIdSet& resources); void SetRenderPasses(RenderPassList* render_passes_in_draw_order); void ClearRenderPasses(); diff --git a/cc/trees/layer_tree_host_unittest_delegated.cc b/cc/trees/layer_tree_host_unittest_delegated.cc index 87f667cefe2fc..6d0fb0a886ade 100644 --- a/cc/trees/layer_tree_host_unittest_delegated.cc +++ b/cc/trees/layer_tree_host_unittest_delegated.cc @@ -243,6 +243,24 @@ class LayerTreeHostDelegatedTest : public LayerTreeTest { output_surface()->ReturnResource(resources_to_return[i], &ack); host_impl->ReclaimResources(&ack); } + + void ReturnAllResourcesFromParent(LayerTreeHostImpl* host_impl) { + DelegatedFrameData* delegated_frame_data = + output_surface()->last_sent_frame().delegated_frame_data.get(); + if (!delegated_frame_data) + return; + + const TransferableResourceArray& resources_held_by_parent = + output_surface()->resources_held_by_parent(); + + if (resources_held_by_parent.empty()) + return; + + CompositorFrameAck ack; + for (size_t i = 0; i < resources_held_by_parent.size(); ++i) + output_surface()->ReturnResource(resources_held_by_parent[i].id, &ack); + host_impl->ReclaimResources(&ack); + } }; class LayerTreeHostDelegatedTestCaseSingleDelegatedLayer @@ -1225,7 +1243,7 @@ class LayerTreeHostDelegatedTestBadFrame switch (host_impl->active_tree()->source_frame_number()) { case 1: { - // We have the first good frame with just 990 and 555 in it. + // We have the first good frame with just 999 and 555 in it. // layer. EXPECT_EQ(2u, map.size()); EXPECT_EQ(1u, map.count(999)); @@ -1293,7 +1311,7 @@ class LayerTreeHostDelegatedTestUnnamedResource public: void BeginTest() override { PostSetNeedsCommitToMainThread(); } - void DidCommit() override { + void DidCommitAndDrawFrame() override { scoped_ptr frame; ReturnedResourceArray resources; @@ -2165,5 +2183,81 @@ class LayerTreeHostDelegatedTestRemoveAndChangeResources SINGLE_AND_MULTI_THREAD_TEST_F( LayerTreeHostDelegatedTestRemoveAndChangeResources); +class LayerTreeHostDelegatedTestActiveFrameIsValid + : public LayerTreeHostDelegatedTestCaseSingleDelegatedLayer { + public: + LayerTreeHostDelegatedTestActiveFrameIsValid() + : drew_with_pending_tree_(false) {} + + void DidCommitAndDrawFrame() override { + scoped_ptr frame; + switch (layer_tree_host()->source_frame_number()) { + case 1: + // This frame goes to the active tree. + frame = CreateFrameData(gfx::Rect(0, 0, 1, 1), gfx::Rect(0, 0, 1, 1)); + AddTextureQuad(frame.get(), 999); + AddTransferableResource(frame.get(), 999); + SetFrameData(frame.Pass()); + break; + case 2: + // This frame stops in the pending tree while we redraw the active tree. + frame = CreateFrameData(gfx::Rect(0, 0, 1, 1), gfx::Rect(0, 0, 1, 1)); + AddTextureQuad(frame.get(), 555); + AddTransferableResource(frame.get(), 555); + SetFrameData(frame.Pass()); + break; + } + } + + void DrawLayersOnThread(LayerTreeHostImpl* host_impl) override { + if (host_impl->active_tree()->source_frame_number() < 1) + return; + + LayerImpl* root_impl = host_impl->active_tree()->root_layer(); + FakeDelegatedRendererLayerImpl* delegated_impl = + static_cast(root_impl->children()[0]); + const ResourceProvider::ResourceIdMap& map = + host_impl->resource_provider()->GetChildToParentMap( + delegated_impl->ChildId()); + + switch (host_impl->active_tree()->source_frame_number()) { + case 1: + if (!host_impl->pending_tree()) { + // Frame 2 is blocked from activating until another draw happens with + // Frame 1. This ensures we draw a different active frame from + // what's in the pending tree. + host_impl->BlockNotifyReadyToActivateForTesting(true); + host_impl->SetNeedsRedrawRect(gfx::Rect(1, 1)); + break; + } + + // The resources in the active tree should be valid. + EXPECT_EQ(1u, map.count(999)); + + host_impl->BlockNotifyReadyToActivateForTesting(false); + drew_with_pending_tree_ = true; + break; + case 2: + EXPECT_TRUE(drew_with_pending_tree_); + + // The resources in the active tree should be valid. + EXPECT_EQ(1u, map.count(555)); + EndTest(); + break; + } + } + + void SwapBuffersOnThread(LayerTreeHostImpl* host_impl, bool result) override { + // Return everything so that we can reliably delete resources that lose + // their references. This would happen if the tab was backgrounded or + // the parent decided to drop all resources for some reason. + ReturnAllResourcesFromParent(host_impl); + } + + bool drew_with_pending_tree_; +}; + +MULTI_THREAD_IMPL_TEST_F(LayerTreeHostDelegatedTestActiveFrameIsValid); + } // namespace } // namespace cc