From 317d012d7099fec32915da845f3c8a4c7c6042b4 Mon Sep 17 00:00:00 2001 From: ColdPaleLight Date: Wed, 16 Mar 2022 15:59:36 +0800 Subject: [PATCH 1/4] Make sure that 'BeginFrame' and 'EndFrame' of the 'ExternalViewEmbedder' are paired --- flow/embedded_views.h | 13 +++++++ shell/common/rasterizer.cc | 8 +++-- shell/common/rasterizer_unittests.cc | 53 ++++++++++++++++++++++++++++ 3 files changed, 72 insertions(+), 2 deletions(-) diff --git a/flow/embedded_views.h b/flow/embedded_views.h index f62e4e86fe443..1248577d6beb8 100644 --- a/flow/embedded_views.h +++ b/flow/embedded_views.h @@ -335,6 +335,19 @@ class ExternalViewEmbedder { // embedder. virtual void Teardown(); + // Change the flag about whether it is used in this frame, it will be set to + // true when 'BeginFrame' and false when 'EndFrame'. + void set_used_this_frame(bool used_this_frame) { + used_this_frame_ = used_this_frame; + } + + // Whether it is used in this frame, returns true between 'BeginFrame' and + // 'EndFrame', otherwise returns false. + bool used_this_frame() const { return used_this_frame_; } + + private: + bool used_this_frame_ = false; + FML_DISALLOW_COPY_AND_ASSIGN(ExternalViewEmbedder); }; // ExternalViewEmbedder diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 031775e2be5da..cf5efc41fd7fc 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -151,10 +151,11 @@ void Rasterizer::DrawLastLayerTree( DrawToSurface(*frame_timings_recorder, *last_layer_tree_); // EndFrame should perform cleanups for the external_view_embedder. - if (external_view_embedder_) { + if (external_view_embedder_ && external_view_embedder_->used_this_frame()) { bool should_resubmit_frame = ShouldResubmitFrame(raster_status); external_view_embedder_->EndFrame(should_resubmit_frame, raster_thread_merger_); + external_view_embedder_->set_used_this_frame(false); } } @@ -208,9 +209,11 @@ RasterStatus Rasterizer::Draw( } // EndFrame should perform cleanups for the external_view_embedder. - if (surface_ && external_view_embedder_) { + if (surface_ && external_view_embedder_ && + external_view_embedder_->used_this_frame()) { external_view_embedder_->EndFrame(should_resubmit_frame, raster_thread_merger_); + external_view_embedder_->set_used_this_frame(false); } // Consume as many pipeline items as possible. But yield the event loop @@ -524,6 +527,7 @@ RasterStatus Rasterizer::DrawToSurfaceUnsafe( external_view_embedder_->BeginFrame( layer_tree.frame_size(), surface_->GetContext(), layer_tree.device_pixel_ratio(), raster_thread_merger_); + external_view_embedder_->set_used_this_frame(true); embedder_root_canvas = external_view_embedder_->GetRootCanvas(); } diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index 06418cf968549..d5891d1127296 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -419,6 +419,59 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNoSurfaceIsSet) { latch.Wait(); } +TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNotUsedThisFrame) { + std::string test_name = + ::testing::UnitTest::GetInstance()->current_test_info()->name(); + ThreadHost thread_host("io.flutter.test." + test_name + ".", + ThreadHost::Type::Platform | ThreadHost::Type::RASTER | + ThreadHost::Type::IO | ThreadHost::Type::UI); + TaskRunners task_runners("test", thread_host.platform_thread->GetTaskRunner(), + thread_host.raster_thread->GetTaskRunner(), + thread_host.ui_thread->GetTaskRunner(), + thread_host.io_thread->GetTaskRunner()); + MockDelegate delegate; + EXPECT_CALL(delegate, GetTaskRunners()) + .WillRepeatedly(ReturnRef(task_runners)); + + auto rasterizer = std::make_unique(delegate); + auto surface = std::make_unique(); + EXPECT_CALL(*surface, MakeRenderContextCurrent()) + .WillOnce(Return(ByMove(std::make_unique(true)))); + + std::shared_ptr external_view_embedder = + std::make_shared(); + rasterizer->SetExternalViewEmbedder(external_view_embedder); + rasterizer->Setup(std::move(surface)); + + EXPECT_CALL(*external_view_embedder, + BeginFrame(/*frame_size=*/SkISize(), /*context=*/nullptr, + /*device_pixel_ratio=*/2.0, + /*raster_thread_merger=*/_)) + .Times(0); + EXPECT_CALL( + *external_view_embedder, + EndFrame(/*should_resubmit_frame=*/false, + /*raster_thread_merger=*/fml::RefPtr( + nullptr))) + .Times(0); + + fml::AutoResetWaitableEvent latch; + thread_host.raster_thread->GetTaskRunner()->PostTask([&] { + auto pipeline = std::make_shared>(/*depth=*/10); + auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), + /*device_pixel_ratio=*/2.0f); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); + auto no_discard = [](LayerTree&) { return true; }; + RasterStatus status = + rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); + EXPECT_EQ(status, RasterStatus::kDiscarded); + latch.Signal(); + }); + latch.Wait(); +} + TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenPipelineIsEmpty) { std::string test_name = ::testing::UnitTest::GetInstance()->current_test_info()->name(); From b42759899b0e3f12c0c557db65886d87e5fcbfcf Mon Sep 17 00:00:00 2001 From: ColdPaleLight Date: Wed, 16 Mar 2022 19:39:03 +0800 Subject: [PATCH 2/4] Add some comments --- shell/common/rasterizer_unittests.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index d5891d1127296..b626bd386bf46 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -463,9 +463,10 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNotUsedThisFrame) { PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return true; }; - RasterStatus status = - rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); + // Always discard the layer tree. + auto discard_callback = [](LayerTree&) { return true; }; + RasterStatus status = rasterizer->Draw(CreateFinishedBuildRecorder(), + pipeline, discard_callback); EXPECT_EQ(status, RasterStatus::kDiscarded); latch.Signal(); }); From 28fc23daa01950e851b52da38874013caf28dd44 Mon Sep 17 00:00:00 2001 From: ColdPaleLight Date: Thu, 17 Mar 2022 11:07:11 +0800 Subject: [PATCH 3/4] tweak some codes --- flow/embedded_views.h | 4 ++-- shell/common/rasterizer.cc | 11 ++++++----- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/flow/embedded_views.h b/flow/embedded_views.h index 1248577d6beb8..7a5a34f33069b 100644 --- a/flow/embedded_views.h +++ b/flow/embedded_views.h @@ -337,13 +337,13 @@ class ExternalViewEmbedder { // Change the flag about whether it is used in this frame, it will be set to // true when 'BeginFrame' and false when 'EndFrame'. - void set_used_this_frame(bool used_this_frame) { + void SetUsedThisFrame(bool used_this_frame) { used_this_frame_ = used_this_frame; } // Whether it is used in this frame, returns true between 'BeginFrame' and // 'EndFrame', otherwise returns false. - bool used_this_frame() const { return used_this_frame_; } + bool GetUsedThisFrame() const { return used_this_frame_; } private: bool used_this_frame_ = false; diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index cf5efc41fd7fc..96420fd5a8b60 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -151,11 +151,11 @@ void Rasterizer::DrawLastLayerTree( DrawToSurface(*frame_timings_recorder, *last_layer_tree_); // EndFrame should perform cleanups for the external_view_embedder. - if (external_view_embedder_ && external_view_embedder_->used_this_frame()) { + if (external_view_embedder_ && external_view_embedder_->GetUsedThisFrame()) { bool should_resubmit_frame = ShouldResubmitFrame(raster_status); external_view_embedder_->EndFrame(should_resubmit_frame, raster_thread_merger_); - external_view_embedder_->set_used_this_frame(false); + external_view_embedder_->SetUsedThisFrame(false); } } @@ -210,10 +210,10 @@ RasterStatus Rasterizer::Draw( // EndFrame should perform cleanups for the external_view_embedder. if (surface_ && external_view_embedder_ && - external_view_embedder_->used_this_frame()) { + external_view_embedder_->GetUsedThisFrame()) { external_view_embedder_->EndFrame(should_resubmit_frame, raster_thread_merger_); - external_view_embedder_->set_used_this_frame(false); + external_view_embedder_->SetUsedThisFrame(false); } // Consume as many pipeline items as possible. But yield the event loop @@ -524,10 +524,11 @@ RasterStatus Rasterizer::DrawToSurfaceUnsafe( SkCanvas* embedder_root_canvas = nullptr; if (external_view_embedder_) { + FML_DCHECK(!external_view_embedder_->GetUsedThisFrame()); + external_view_embedder_->SetUsedThisFrame(true); external_view_embedder_->BeginFrame( layer_tree.frame_size(), surface_->GetContext(), layer_tree.device_pixel_ratio(), raster_thread_merger_); - external_view_embedder_->set_used_this_frame(true); embedder_root_canvas = external_view_embedder_->GetRootCanvas(); } From 97c3f9755922308ef5b51c691beba3e8cdafd2cf Mon Sep 17 00:00:00 2001 From: ColdPaleLight Date: Thu, 17 Mar 2022 21:45:32 +0800 Subject: [PATCH 4/4] Add TODO with linked issue to disabled tests --- shell/common/shell_unittests.cc | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index bee55ab690190..14df5743fc0ad 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -2396,6 +2396,8 @@ TEST_F(ShellTest, OnServiceProtocolEstimateRasterCacheMemoryWorks) { // TODO(https://github.com/flutter/flutter/issues/100273): Disabled due to // flakiness. +// TODO(https://github.com/flutter/flutter/issues/100299): Fix it when +// re-enabling. TEST_F(ShellTest, DISABLED_DiscardLayerTreeOnResize) { auto settings = CreateSettingsForFixture(); @@ -2448,6 +2450,8 @@ TEST_F(ShellTest, DISABLED_DiscardLayerTreeOnResize) { // TODO(https://github.com/flutter/flutter/issues/100273): Disabled due to // flakiness. +// TODO(https://github.com/flutter/flutter/issues/100299): Fix it when +// re-enabling. TEST_F(ShellTest, DISABLED_DiscardResubmittedLayerTreeOnResize) { auto settings = CreateSettingsForFixture();