From 5d4462e0e51ee42fa884e7a84f7749eeca9be49c Mon Sep 17 00:00:00 2001 From: jbauman Date: Fri, 16 Oct 2015 11:34:45 -0700 Subject: Reland of Add mechanism to signal DelegatedFrameHost that a surface is about to draw. (patchset #1 id:1 of https://codereview.chromium.org/1407173002/ ) Reason for revert: Memory failure was fixed in r354541. Original issue's description: > Revert of Add mechanism to signal DelegatedFrameHost that a surface is about to draw. (patchset #7 id:120001 of https://codereview.chromium.org/1365793002/ ) > > Reason for revert: > Causing memory failures on SurfaceAggregatorValidSurfaceTest.CopyRequest in cc_unittests on > http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20MSan%20Tests/builds/10514 > and > http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20MSan%20Tests/builds/4653 > > [ RUN ] SurfaceAggregatorValidSurfaceTest.CopyRequest > ==31905==WARNING: MemorySanitizer: use-of-uninitialized-value > #0 0x7fccb3920f7e in cc::Surface::RequestCopyOfOutput(scoped_ptr\u003Ccc::CopyOutputRequest, base::DefaultDeleter\u003Ccc::CopyOutputRequest> >) cc/surfaces/surface.cc:106:15 > #1 0x7fccb3942ec9 in cc::SurfaceFactory::RequestCopyOfSurface(cc::SurfaceId, scoped_ptr\u003Ccc::CopyOutputRequest, base::DefaultDeleter\u003Ccc::CopyOutputRequest> >) cc/surfaces/surface_factory.cc:75:3 > #2 0x7fccb31ce931 in cc::(anonymous namespace)::SurfaceAggregatorValidSurfaceTest_CopyRequest_Test::TestBody() cc/surfaces/surface_aggregator_unittest.cc:285:3 > #3 0x7fccb33160d2 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2458:12 > #4 0x7fccb33160d2 in testing::Test::Run() testing/gtest/src/gtest.cc:2474:0 > #5 0x7fccb331972c in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:5 > #6 0x7fccb331b0a2 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:5 > #7 0x7fccb333948b in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4647:11 > #8 0x7fccb3338468 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12 > #9 0x7fccb3338468 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255:0 > #10 0x7fccb32867da in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:10 > #11 0x7fccb32867da in base::TestSuite::Run() base/test/test_suite.cc:230:0 > #12 0x7fccb327388e in Run base/callback.h:396:12 > #13 0x7fccb327388e in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:187:0 > #14 0x7fccb32730de in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:426:10 > #15 0x7fccb0ba0c15 in main cc/test/run_all_unittests.cc:12:10 > #16 0x7fccaa41076c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226:0 > #17 0x7fccb0b41ca8 in _start ??:0:0 > > Uninitialized value was created by a heap allocation > #0 0x7fccb0b9fd92 in operator new(unsigned long) ??:0:0 > #1 0x7fccb31ce897 in CreateEmptyRequest cc/output/copy_output_request.h:26:28 > #2 0x7fccb31ce897 in cc::(anonymous namespace)::SurfaceAggregatorValidSurfaceTest_CopyRequest_Test::TestBody() cc/surfaces/surface_aggregator_unittest.cc:283:0 > #3 0x7fccb33160d2 in HandleExceptionsInMethodIfSupported\u003Ctesting::Test, void> testing/gtest/src/gtest.cc:2458:12 > #4 0x7fccb33160d2 in testing::Test::Run() testing/gtest/src/gtest.cc:2474:0 > #5 0x7fccb331972c in testing::TestInfo::Run() testing/gtest/src/gtest.cc:2656:5 > #6 0x7fccb331b0a2 in testing::TestCase::Run() testing/gtest/src/gtest.cc:2774:5 > #7 0x7fccb333948b in testing::internal::UnitTestImpl::RunAllTests() testing/gtest/src/gtest.cc:4647:11 > #8 0x7fccb3338468 in HandleExceptionsInMethodIfSupported\u003Ctesting::internal::UnitTestImpl, bool> testing/gtest/src/gtest.cc:2458:12 > #9 0x7fccb3338468 in testing::UnitTest::Run() testing/gtest/src/gtest.cc:4255:0 > #10 0x7fccb32867da in RUN_ALL_TESTS testing/gtest/include/gtest/gtest.h:2237:10 > #11 0x7fccb32867da in base::TestSuite::Run() base/test/test_suite.cc:230:0 > #12 0x7fccb327388e in Run base/callback.h:396:12 > #13 0x7fccb327388e in base::(anonymous namespace)::LaunchUnitTestsInternal(base::Callback\u003Cint ()> const&, int, bool, base::Callback\u003Cvoid ()> const&) base/test/launcher/unit_test_launcher.cc:187:0 > #14 0x7fccb32730de in base::LaunchUnitTests(int, char**, base::Callback\u003Cint ()> const&) base/test/launcher/unit_test_launcher.cc:426:10 > #15 0x7fccb0ba0c15 in main cc/test/run_all_unittests.cc:12:10 > #16 0x7fccaa41076c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226:0 > > SUMMARY: MemorySanitizer: use-of-uninitialized-value (/tmp/runqXGplV/out/Release/cc_unittests+0x32e1f7e) > Exiting > > Original issue's description: > > Add mechanism to signal DelegatedFrameHost that a surface is about to draw. > > > > This allows the FrameSubscriber to request a copy of the output even when the renderer it's subscribing to hasn't changed its contents, but a child renderer (oopif or webview) has. > > > > BUG=529378 > > CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel > > > > Committed: https://crrev.com/5ed9ab93dd9f448aac722483b6adc6b7a14cdf4b > > Cr-Commit-Position: refs/heads/master@{#354390} > > TBR=miu@chromium.org,sadrul@chromium.org,jbauman@chromium.org > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=529378 > > Committed: https://crrev.com/1b8d59763b4266783794a950eee388ddc110ada6 > Cr-Commit-Position: refs/heads/master@{#354446} TBR=miu@chromium.org,sadrul@chromium.org,mpearson@chromium.org NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=529378 Review URL: https://codereview.chromium.org/1408173002 Cr-Commit-Position: refs/heads/master@{#354554} --- cc/surfaces/surface.cc | 19 +++++++--- cc/surfaces/surface_aggregator.cc | 10 ++++-- cc/surfaces/surface_aggregator_unittest.cc | 11 ++++++ cc/surfaces/surface_factory.cc | 6 ++++ cc/surfaces/surface_factory.h | 3 ++ cc/surfaces/surface_factory_client.h | 5 +++ cc/surfaces/surface_factory_unittest.cc | 58 ++++++++++++++++++++++++++++++ 7 files changed, 105 insertions(+), 7 deletions(-) (limited to 'cc/surfaces') diff --git a/cc/surfaces/surface.cc b/cc/surfaces/surface.cc index 16987cb..e3b0f5e 100644 --- a/cc/surfaces/surface.cc +++ b/cc/surfaces/surface.cc @@ -98,11 +98,22 @@ void Surface::QueueFrame(scoped_ptr frame, void Surface::RequestCopyOfOutput(scoped_ptr copy_request) { if (current_frame_ && - !current_frame_->delegated_frame_data->render_pass_list.empty()) - current_frame_->delegated_frame_data->render_pass_list.back() - ->copy_requests.push_back(copy_request.Pass()); - else + !current_frame_->delegated_frame_data->render_pass_list.empty()) { + ScopedPtrVector& copy_requests = + current_frame_->delegated_frame_data->render_pass_list.back() + ->copy_requests; + + if (void* source = copy_request->source()) { + // Remove existing CopyOutputRequests made on the Surface by the same + // source. + auto to_remove = copy_requests.remove_if([source]( + const CopyOutputRequest* x) { return x->source() == source; }); + copy_requests.erase(to_remove, copy_requests.end()); + } + copy_requests.push_back(copy_request.Pass()); + } else { copy_request->SendEmptyResult(); + } } void Surface::TakeCopyOutputRequests( diff --git a/cc/surfaces/surface_aggregator.cc b/cc/surfaces/surface_aggregator.cc index 69618ba..902f836 100644 --- a/cc/surfaces/surface_aggregator.cc +++ b/cc/surfaces/surface_aggregator.cc @@ -547,9 +547,6 @@ gfx::Rect SurfaceAggregator::PrewalkTree(SurfaceId surface_id) { if (provider_) provider_->DeclareUsedResourcesFromChild(child_id, referenced_resources); - for (const auto& render_pass : frame_data->render_pass_list) - has_copy_requests_ |= !render_pass->copy_requests.empty(); - gfx::Rect damage_rect; if (!frame_data->render_pass_list.empty()) { RenderPass* last_pass = frame_data->render_pass_list.back(); @@ -566,6 +563,13 @@ gfx::Rect SurfaceAggregator::PrewalkTree(SurfaceId surface_id) { damage_rect.Union( MathUtil::MapEnclosingClippedRect(surface_info.second, surface_damage)); } + + if (surface->factory()) + surface->factory()->WillDrawSurface(surface->surface_id(), damage_rect); + + for (const auto& render_pass : frame_data->render_pass_list) + has_copy_requests_ |= !render_pass->copy_requests.empty(); + referenced_surfaces_.erase(it); return damage_rect; } diff --git a/cc/surfaces/surface_aggregator_unittest.cc b/cc/surfaces/surface_aggregator_unittest.cc index bfec6e6..bee479c 100644 --- a/cc/surfaces/surface_aggregator_unittest.cc +++ b/cc/surfaces/surface_aggregator_unittest.cc @@ -43,6 +43,13 @@ gfx::Size SurfaceSize() { class EmptySurfaceFactoryClient : public SurfaceFactoryClient { public: void ReturnResources(const ReturnedResourceArray& resources) override {} + void WillDrawSurface(SurfaceId id, const gfx::Rect& damage_rect) override { + last_surface_id_ = id; + last_damage_rect_ = damage_rect; + } + + gfx::Rect last_damage_rect_; + SurfaceId last_surface_id_; }; class SurfaceAggregatorTest : public testing::Test { @@ -166,6 +173,10 @@ TEST_F(SurfaceAggregatorValidSurfaceTest, SimpleFrame) { SurfaceId ids[] = {root_surface_id_}; AggregateAndVerify(passes, arraysize(passes), ids, arraysize(ids)); + + // Check that WillDrawSurface was called. + EXPECT_EQ(gfx::Rect(SurfaceSize()), empty_client_.last_damage_rect_); + EXPECT_EQ(root_surface_id_, empty_client_.last_surface_id_); } TEST_F(SurfaceAggregatorValidSurfaceTest, OpacityCopied) { diff --git a/cc/surfaces/surface_factory.cc b/cc/surfaces/surface_factory.cc index 3e83c8c..72ebdc4 100644 --- a/cc/surfaces/surface_factory.cc +++ b/cc/surfaces/surface_factory.cc @@ -8,6 +8,7 @@ #include "cc/output/compositor_frame.h" #include "cc/output/copy_output_request.h" #include "cc/surfaces/surface.h" +#include "cc/surfaces/surface_factory_client.h" #include "cc/surfaces/surface_manager.h" #include "ui/gfx/geometry/size.h" @@ -75,6 +76,11 @@ void SurfaceFactory::RequestCopyOfSurface( manager_->SurfaceModified(surface_id); } +void SurfaceFactory::WillDrawSurface(SurfaceId id, + const gfx::Rect& damage_rect) { + client_->WillDrawSurface(id, damage_rect); +} + void SurfaceFactory::ReceiveFromChild( const TransferableResourceArray& resources) { holder_.ReceiveFromChild(resources); diff --git a/cc/surfaces/surface_factory.h b/cc/surfaces/surface_factory.h index 5af8d2e..762d5a2 100644 --- a/cc/surfaces/surface_factory.h +++ b/cc/surfaces/surface_factory.h @@ -11,6 +11,7 @@ #include "base/containers/scoped_ptr_hash_map.h" #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" +#include "base/observer_list.h" #include "cc/output/compositor_frame.h" #include "cc/surfaces/surface_id.h" #include "cc/surfaces/surface_resource_holder.h" @@ -55,6 +56,8 @@ class CC_SURFACES_EXPORT SurfaceFactory void RequestCopyOfSurface(SurfaceId surface_id, scoped_ptr copy_request); + void WillDrawSurface(SurfaceId id, const gfx::Rect& damage_rect); + SurfaceFactoryClient* client() { return client_; } void ReceiveFromChild(const TransferableResourceArray& resources); diff --git a/cc/surfaces/surface_factory_client.h b/cc/surfaces/surface_factory_client.h index 9866b27..c2a04af 100644 --- a/cc/surfaces/surface_factory_client.h +++ b/cc/surfaces/surface_factory_client.h @@ -6,7 +6,9 @@ #define CC_SURFACES_SURFACE_FACTORY_CLIENT_H_ #include "cc/resources/returned_resource.h" +#include "cc/surfaces/surface_id.h" #include "cc/surfaces/surfaces_export.h" +#include "ui/gfx/geometry/rect.h" namespace cc { @@ -15,6 +17,9 @@ class CC_SURFACES_EXPORT SurfaceFactoryClient { virtual ~SurfaceFactoryClient() {} virtual void ReturnResources(const ReturnedResourceArray& resources) = 0; + + virtual void WillDrawSurface(SurfaceId surface_id, + const gfx::Rect& damage_rect) {} }; } // namespace cc diff --git a/cc/surfaces/surface_factory_unittest.cc b/cc/surfaces/surface_factory_unittest.cc index c7ce964..9629a4e 100644 --- a/cc/surfaces/surface_factory_unittest.cc +++ b/cc/surfaces/surface_factory_unittest.cc @@ -4,6 +4,8 @@ #include "base/bind.h" #include "cc/output/compositor_frame.h" +#include "cc/output/copy_output_request.h" +#include "cc/output/copy_output_result.h" #include "cc/output/delegated_frame_data.h" #include "cc/resources/resource_provider.h" #include "cc/surfaces/surface.h" @@ -519,5 +521,61 @@ TEST_F(SurfaceFactoryTest, DestroyCycle) { surface_id_ = SurfaceId(); } +void CopyRequestTestCallback(bool* called, + scoped_ptr result) { + *called = true; +} + +TEST_F(SurfaceFactoryTest, DuplicateCopyRequest) { + { + scoped_ptr render_pass(RenderPass::Create()); + render_pass->referenced_surfaces.push_back(surface_id_); + scoped_ptr frame_data(new DelegatedFrameData); + frame_data->render_pass_list.push_back(render_pass.Pass()); + scoped_ptr frame(new CompositorFrame); + frame->delegated_frame_data = frame_data.Pass(); + factory_.SubmitCompositorFrame(surface_id_, frame.Pass(), + SurfaceFactory::DrawCallback()); + } + void* source1 = &source1; + void* source2 = &source2; + + bool called1 = false; + scoped_ptr request; + request = CopyOutputRequest::CreateRequest( + base::Bind(&CopyRequestTestCallback, &called1)); + request->set_source(source1); + + factory_.RequestCopyOfSurface(surface_id_, request.Pass()); + EXPECT_FALSE(called1); + + bool called2 = false; + request = CopyOutputRequest::CreateRequest( + base::Bind(&CopyRequestTestCallback, &called2)); + request->set_source(source2); + + factory_.RequestCopyOfSurface(surface_id_, request.Pass()); + // Callbacks have different sources so neither should be called. + EXPECT_FALSE(called1); + EXPECT_FALSE(called2); + + bool called3 = false; + request = CopyOutputRequest::CreateRequest( + base::Bind(&CopyRequestTestCallback, &called3)); + request->set_source(source1); + + factory_.RequestCopyOfSurface(surface_id_, request.Pass()); + // Two callbacks are from source1, so the first should be called. + EXPECT_TRUE(called1); + EXPECT_FALSE(called2); + EXPECT_FALSE(called3); + + factory_.Destroy(surface_id_); + surface_id_ = SurfaceId(); + EXPECT_TRUE(called1); + EXPECT_TRUE(called2); + EXPECT_TRUE(called3); +} + } // namespace } // namespace cc -- cgit v1.1