From 037041286e60096711b239eb9ed37e7e8f39ce8a Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 17 Aug 2023 09:26:41 -0700 Subject: [PATCH 01/10] Move discard to shell --- shell/common/rasterizer.cc | 10 ++--- shell/common/rasterizer.h | 10 ++--- shell/common/rasterizer_unittests.cc | 64 ++++++++++++++-------------- shell/common/shell.cc | 18 ++++---- shell/common/shell.h | 3 ++ 5 files changed, 52 insertions(+), 53 deletions(-) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index d7d1338c1a533..ce0f59e40b3f6 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -186,8 +186,7 @@ void Rasterizer::DrawLastLayerTree( } RasterStatus Rasterizer::Draw( - const std::shared_ptr& pipeline, - LayerTreeDiscardCallback discard_callback) { + const std::shared_ptr& pipeline) { TRACE_EVENT0("flutter", "GPURasterizer::Draw"); if (raster_thread_merger_ && !raster_thread_merger_->IsOnRasterizingThread()) { @@ -205,7 +204,7 @@ RasterStatus Rasterizer::Draw( std::unique_ptr frame_timings_recorder = std::move(item->frame_timings_recorder); float device_pixel_ratio = item->device_pixel_ratio; - if (discard_callback(*layer_tree.get())) { + if (delegate_.ShouldDiscardLayerTree(*layer_tree.get())) { raster_status = RasterStatus::kDiscarded; } else { raster_status = DoDraw(std::move(frame_timings_recorder), @@ -248,10 +247,9 @@ RasterStatus Rasterizer::Draw( case PipelineConsumeResult::MoreAvailable: { delegate_.GetTaskRunners().GetRasterTaskRunner()->PostTask( fml::MakeCopyable( - [weak_this = weak_factory_.GetWeakPtr(), pipeline, - discard_callback = std::move(discard_callback)]() mutable { + [weak_this = weak_factory_.GetWeakPtr(), pipeline]() mutable { if (weak_this) { - weak_this->Draw(pipeline, std::move(discard_callback)); + weak_this->Draw(pipeline); } })); break; diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index b17242342c084..9ca652a088acc 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -118,6 +118,8 @@ class Rasterizer final : public SnapshotDelegate, const = 0; virtual const Settings& GetSettings() const = 0; + + virtual bool ShouldDiscardLayerTree(flutter::LayerTree& layer_tree) = 0; }; //---------------------------------------------------------------------------- @@ -240,8 +242,6 @@ class Rasterizer final : public SnapshotDelegate, std::shared_ptr GetTextureRegistry() override; - using LayerTreeDiscardCallback = std::function; - //---------------------------------------------------------------------------- /// @brief Takes the next item from the layer tree pipeline and executes /// the raster thread frame workload for that pipeline item to @@ -270,11 +270,8 @@ class Rasterizer final : public SnapshotDelegate, /// /// @param[in] pipeline The layer tree pipeline to take the next layer tree /// to render from. - /// @param[in] discard_callback if specified and returns true, the layer tree - /// is discarded instead of being rendered /// - RasterStatus Draw(const std::shared_ptr& pipeline, - LayerTreeDiscardCallback discard_callback = NoDiscard); + RasterStatus Draw(const std::shared_ptr& pipeline); //---------------------------------------------------------------------------- /// @brief The type of the screenshot to obtain of the previously @@ -569,7 +566,6 @@ class Rasterizer final : public SnapshotDelegate, void FireNextFrameCallbackIfPresent(); - static bool NoDiscard(const flutter::LayerTree& layer_tree) { return false; } static bool ShouldResubmitFrame(const RasterStatus& raster_status); Delegate& delegate_; diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index 5307dcacccfdf..0b9c57f4c1ad4 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -43,6 +43,7 @@ class MockDelegate : public Rasterizer::Delegate { std::shared_ptr()); MOCK_METHOD0(CreateSnapshotSurface, std::unique_ptr()); MOCK_CONST_METHOD0(GetSettings, const Settings&()); + MOCK_METHOD1(ShouldDiscardLayerTree, bool(flutter::LayerTree&)); }; class MockSurface : public Surface { @@ -129,7 +130,8 @@ TEST(RasterizerTest, drawEmptyPipeline) { fml::AutoResetWaitableEvent latch; thread_host.raster_thread->GetTaskRunner()->PostTask([&] { auto pipeline = std::make_shared(/*depth=*/10); - rasterizer->Draw(pipeline, nullptr); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); latch.Signal(); }); latch.Wait(); @@ -199,8 +201,8 @@ TEST(RasterizerTest, PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); latch.Signal(); }); latch.Wait(); @@ -265,8 +267,8 @@ TEST( PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); latch.Signal(); }); latch.Wait(); @@ -336,8 +338,8 @@ TEST( PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); } TEST(RasterizerTest, @@ -410,11 +412,11 @@ TEST(RasterizerTest, PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; // The Draw() will respectively call BeginFrame(), SubmitFrame() and // EndFrame() one time. - rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); // The DrawLastLayerTree() will respectively call BeginFrame(), SubmitFrame() // and EndFrame() one more time, totally 2 times. @@ -460,8 +462,8 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNoSurfaceIsSet) { PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); latch.Signal(); }); latch.Wait(); @@ -517,8 +519,8 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNotUsedThisFrame) { pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); // Always discard the layer tree. - auto discard_callback = [](LayerTree&) { return true; }; - RasterStatus status = rasterizer->Draw(pipeline, discard_callback); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(true)); + RasterStatus status = rasterizer->Draw(pipeline); EXPECT_EQ(status, RasterStatus::kDiscarded); latch.Signal(); }); @@ -561,8 +563,8 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenPipelineIsEmpty) { fml::AutoResetWaitableEvent latch; thread_host.raster_thread->GetTaskRunner()->PostTask([&] { auto pipeline = std::make_shared(/*depth=*/10); - auto no_discard = [](LayerTree&) { return false; }; - RasterStatus status = rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + RasterStatus status = rasterizer->Draw(pipeline); EXPECT_EQ(status, RasterStatus::kFailed); latch.Signal(); }); @@ -619,8 +621,8 @@ TEST(RasterizerTest, PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); latch.Signal(); }); latch.Wait(); @@ -677,8 +679,8 @@ TEST( PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; - RasterStatus status = rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + RasterStatus status = rasterizer->Draw(pipeline); EXPECT_EQ(status, RasterStatus::kSuccess); latch.Signal(); }); @@ -735,8 +737,8 @@ TEST( PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; - RasterStatus status = rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + RasterStatus status = rasterizer->Draw(pipeline); EXPECT_EQ(status, RasterStatus::kSuccess); latch.Signal(); }); @@ -792,8 +794,8 @@ TEST( PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; - RasterStatus status = rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + RasterStatus status = rasterizer->Draw(pipeline); EXPECT_EQ(status, RasterStatus::kDiscarded); latch.Signal(); }); @@ -848,8 +850,8 @@ TEST( PipelineProduceResult result = pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); - auto no_discard = [](LayerTree&) { return false; }; - RasterStatus status = rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + RasterStatus status = rasterizer->Draw(pipeline); EXPECT_EQ(status, RasterStatus::kFailed); latch.Signal(); }); @@ -929,10 +931,10 @@ TEST(RasterizerTest, EXPECT_TRUE(result.success); EXPECT_EQ(result.is_first_item, i == 0); } - auto no_discard = [](LayerTree&) { return false; }; // Although we only call 'Rasterizer::Draw' once, it will be called twice // finally because there are two items in the pipeline. - rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); }); count_down_latch.Wait(); thread_host.raster_thread->GetTaskRunner()->PostTask([&] { @@ -1100,10 +1102,10 @@ TEST(RasterizerTest, presentationTimeSetWhenVsyncTargetInFuture) { EXPECT_TRUE(result.success); EXPECT_EQ(result.is_first_item, i == 0); } - auto no_discard = [](LayerTree&) { return false; }; // Although we only call 'Rasterizer::Draw' once, it will be called twice // finally because there are two items in the pipeline. - rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); }); submit_latch.Wait(); @@ -1181,8 +1183,8 @@ TEST(RasterizerTest, presentationTimeNotSetWhenVsyncTargetInPast) { pipeline->Produce().Complete(std::move(layer_tree_item)); EXPECT_TRUE(result.success); EXPECT_EQ(result.is_first_item, true); - auto no_discard = [](LayerTree&) { return false; }; - rasterizer->Draw(pipeline, no_discard); + ON_CALL(delegate, ShouldDiscardLayerTree).WillByDefault(Return(false)); + rasterizer->Draw(pipeline); }); submit_latch.Wait(); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 136c09b9c39d0..27374353d70b1 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1215,22 +1215,15 @@ void Shell::OnAnimatorUpdateLatestFrameTargetTime( void Shell::OnAnimatorDraw(std::shared_ptr pipeline) { FML_DCHECK(is_set_up_); - auto discard_callback = [this](flutter::LayerTree& tree) { - std::scoped_lock lock(resize_mutex_); - return !expected_frame_size_.isEmpty() && - tree.frame_size() != expected_frame_size_; - }; - task_runners_.GetRasterTaskRunner()->PostTask(fml::MakeCopyable( [&waiting_for_first_frame = waiting_for_first_frame_, &waiting_for_first_frame_condition = waiting_for_first_frame_condition_, rasterizer = rasterizer_->GetWeakPtr(), - weak_pipeline = std::weak_ptr(pipeline), - discard_callback = std::move(discard_callback)]() mutable { + weak_pipeline = std::weak_ptr(pipeline)]() mutable { if (rasterizer) { std::shared_ptr pipeline = weak_pipeline.lock(); if (pipeline) { - rasterizer->Draw(pipeline, std::move(discard_callback)); + rasterizer->Draw(pipeline); } if (waiting_for_first_frame.load()) { @@ -1580,6 +1573,13 @@ fml::TimePoint Shell::GetLatestFrameTargetTime() const { return latest_frame_target_time_.value(); } +// |Rasterizer::Delegate| +bool Shell::ShouldDiscardLayerTree(flutter::LayerTree& tree) { + std::scoped_lock lock(resize_mutex_); + return !expected_frame_size_.isEmpty() && + tree.frame_size() != expected_frame_size_; +} + // |ServiceProtocol::Handler| fml::RefPtr Shell::GetServiceProtocolHandlerTaskRunner( std::string_view method) const { diff --git a/shell/common/shell.h b/shell/common/shell.h index 23689332233c9..16940b1b4d987 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -658,6 +658,9 @@ class Shell final : public PlatformView::Delegate, // |Rasterizer::Delegate| fml::TimePoint GetLatestFrameTargetTime() const override; + // |Rasterizer::Delegate| + bool ShouldDiscardLayerTree(flutter::LayerTree& layer_tree) override; + // |ServiceProtocol::Handler| fml::RefPtr GetServiceProtocolHandlerTaskRunner( std::string_view method) const override; From a22a197845be05d4dc4ec20636f5ee433615a665 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 17 Aug 2023 10:58:11 -0700 Subject: [PATCH 02/10] WIP: Add logs --- shell/platform/embedder/tests/embedder_metal_unittests.mm | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/shell/platform/embedder/tests/embedder_metal_unittests.mm b/shell/platform/embedder/tests/embedder_metal_unittests.mm index 78f7b82ed2fbf..8d7185c3e8fb6 100644 --- a/shell/platform/embedder/tests/embedder_metal_unittests.mm +++ b/shell/platform/embedder/tests/embedder_metal_unittests.mm @@ -218,8 +218,10 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr auto rendered_scene = context.GetNextSceneImage(); + printf("main 1\n");fflush(stdout); auto engine = builder.LaunchEngine(); ASSERT_TRUE(engine.is_valid()); + printf("main 2\n");fflush(stdout); // Send a window metrics events so frames may be scheduled. FlutterWindowMetricsEvent event = {}; @@ -228,8 +230,10 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr event.height = 600; event.pixel_ratio = 1.0; ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); + printf("main 3\n");fflush(stdout); ASSERT_TRUE(ImageMatchesFixture("scene_without_custom_compositor.png", rendered_scene)); + printf("main 4\n");fflush(stdout); } TEST_F(EmbedderTest, TextureDestructionCallbackCalledWithoutCustomCompositorMetal) { From 782fd3087cc402bfa3404db89d9a820be395d620 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 17 Aug 2023 11:30:54 -0700 Subject: [PATCH 03/10] WIP: more logs, and no assert --- .../embedder/tests/embedder_metal_unittests.mm | 14 +++++++------- .../embedder/tests/embedder_test_context.cc | 1 + .../embedder/tests/embedder_unittests_util.cc | 6 ++++++ 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/shell/platform/embedder/tests/embedder_metal_unittests.mm b/shell/platform/embedder/tests/embedder_metal_unittests.mm index 8d7185c3e8fb6..2abc83527a147 100644 --- a/shell/platform/embedder/tests/embedder_metal_unittests.mm +++ b/shell/platform/embedder/tests/embedder_metal_unittests.mm @@ -108,9 +108,9 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr builder.SetMetalRendererConfig(texture_size); auto engine = builder.LaunchEngine(); - ASSERT_TRUE(engine.is_valid()); + EXPECT_TRUE(engine.is_valid()); - ASSERT_EQ(FlutterEngineRegisterExternalTexture(engine.get(), texture_id), kSuccess); + EXPECT_EQ(FlutterEngineRegisterExternalTexture(engine.get(), texture_id), kSuccess); auto rendered_scene = context.GetNextSceneImage(); @@ -120,9 +120,9 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr event.width = texture_size.width(); event.height = texture_size.height(); event.pixel_ratio = 1.0; - ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); + EXPECT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); - ASSERT_TRUE(ImageMatchesFixture("external_texture_metal.png", rendered_scene)); + EXPECT_TRUE(ImageMatchesFixture("external_texture_metal.png", rendered_scene)); } TEST_F(EmbedderTest, MetalCompositorMustBeAbleToRenderPlatformViews) { @@ -220,7 +220,7 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr printf("main 1\n");fflush(stdout); auto engine = builder.LaunchEngine(); - ASSERT_TRUE(engine.is_valid()); + EXPECT_TRUE(engine.is_valid()); printf("main 2\n");fflush(stdout); // Send a window metrics events so frames may be scheduled. @@ -229,10 +229,10 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr event.width = 800; event.height = 600; event.pixel_ratio = 1.0; - ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); + EXPECT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); printf("main 3\n");fflush(stdout); - ASSERT_TRUE(ImageMatchesFixture("scene_without_custom_compositor.png", rendered_scene)); + EXPECT_TRUE(ImageMatchesFixture("scene_without_custom_compositor.png", rendered_scene)); printf("main 4\n");fflush(stdout); } diff --git a/shell/platform/embedder/tests/embedder_test_context.cc b/shell/platform/embedder/tests/embedder_test_context.cc index 99b2fe0f5f8af..2a6edf220ce9b 100644 --- a/shell/platform/embedder/tests/embedder_test_context.cc +++ b/shell/platform/embedder/tests/embedder_test_context.cc @@ -257,6 +257,7 @@ std::future> EmbedderTestContext::GetNextSceneImage() { auto future = promise.get_future(); SetNextSceneCallback( fml::MakeCopyable([promise = std::move(promise)](auto image) mutable { + printf("Assign GetNextSceneImage\n");fflush(stdout); promise.set_value(image); })); return future; diff --git a/shell/platform/embedder/tests/embedder_unittests_util.cc b/shell/platform/embedder/tests/embedder_unittests_util.cc index a29bf166a3ae5..ae7a87c64de3b 100644 --- a/shell/platform/embedder/tests/embedder_unittests_util.cc +++ b/shell/platform/embedder/tests/embedder_unittests_util.cc @@ -151,6 +151,7 @@ bool WriteImageToDisk(const fml::UniqueFD& directory, bool ImageMatchesFixture(const std::string& fixture_file_name, const sk_sp& scene_image) { + printf("ImageMatchesFixture sync\n");fflush(stdout); fml::FileMapping fixture_image_mapping(OpenFixture(fixture_file_name)); FML_CHECK(fixture_image_mapping.GetSize() != 0u) @@ -162,6 +163,7 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, SkImages::DeferredFromEncodedData(std::move(encoded_image)) ->makeRasterImage(); + printf("ImageMatchesFixture 1\n");fflush(stdout); FML_CHECK(fixture_image) << "Could not create image from fixture: " << fixture_file_name; @@ -175,13 +177,16 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, << "Could not create image subset for fixture comparison: " << scene_image_subset; + printf("ImageMatchesFixture 2\n");fflush(stdout); const auto images_are_same = RasterImagesAreSame(scene_image_subset, fixture_image); + printf("ImageMatchesFixture 3\n");fflush(stdout); // If the images are not the same, this predicate is going to indicate test // failure. Dump both the actual image and the expectation to disk to the // test author can figure out what went wrong. if (!images_are_same) { + printf("ImageMatchesFixture diff\n");fflush(stdout); const auto fixtures_path = GetFixturesPath(); const auto actual_file_name = "actual_" + fixture_file_name; @@ -204,6 +209,7 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, << fml::paths::JoinPaths({fixtures_path, actual_file_name}) << std::endl; } + printf("ImageMatchesFixture 4\n");fflush(stdout); return images_are_same; } From 54c31c0cfc498ad6790a4f46ea98dbfa4ba09b35 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 17 Aug 2023 12:06:28 -0700 Subject: [PATCH 04/10] WIP: More logs --- shell/platform/embedder/tests/embedder_unittests_util.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/shell/platform/embedder/tests/embedder_unittests_util.cc b/shell/platform/embedder/tests/embedder_unittests_util.cc index ae7a87c64de3b..3dec9026e764e 100644 --- a/shell/platform/embedder/tests/embedder_unittests_util.cc +++ b/shell/platform/embedder/tests/embedder_unittests_util.cc @@ -153,6 +153,7 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, const sk_sp& scene_image) { printf("ImageMatchesFixture sync\n");fflush(stdout); fml::FileMapping fixture_image_mapping(OpenFixture(fixture_file_name)); + printf("ImageMatchesFixture sync 1\n");fflush(stdout); FML_CHECK(fixture_image_mapping.GetSize() != 0u) << "Could not find fixture: " << fixture_file_name; From 0172d763e709da02dc7e1dc548efc33f3d1d0c4f Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Thu, 17 Aug 2023 16:40:18 -0700 Subject: [PATCH 05/10] Compilable with multiview --- shell/common/rasterizer.cc | 2 +- shell/common/rasterizer.h | 3 ++- shell/common/rasterizer_unittests.cc | 2 +- shell/common/shell.cc | 7 ++++--- shell/common/shell.h | 3 ++- .../tests/embedder_metal_unittests.mm | 12 +++++++---- .../embedder/tests/embedder_test_context.cc | 3 ++- .../embedder/tests/embedder_unittests_util.cc | 21 ++++++++++++------- 8 files changed, 34 insertions(+), 19 deletions(-) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 2566abdbff701..5923c3cd0981e 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -216,7 +216,7 @@ RasterStatus Rasterizer::Draw( std::unique_ptr frame_timings_recorder = std::move(item->frame_timings_recorder); float device_pixel_ratio = item->device_pixel_ratio; - if (delegate_.ShouldDiscardLayerTree(*layer_tree.get())) { + if (delegate_.ShouldDiscardLayerTree(view_id, *layer_tree.get())) { raster_status = RasterStatus::kDiscarded; } else { raster_status = DoDraw(std::move(frame_timings_recorder), diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index c67ab08af7d54..eaa5125e96a30 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -119,7 +119,8 @@ class Rasterizer final : public SnapshotDelegate, virtual const Settings& GetSettings() const = 0; - virtual bool ShouldDiscardLayerTree(flutter::LayerTree& layer_tree) = 0; + virtual bool ShouldDiscardLayerTree(int64_t view_id, + flutter::LayerTree& tree) = 0; }; //---------------------------------------------------------------------------- diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index e245e68c0861a..71f360f57a4bf 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -43,7 +43,7 @@ class MockDelegate : public Rasterizer::Delegate { MOCK_CONST_METHOD0(GetIsGpuDisabledSyncSwitch, std::shared_ptr()); MOCK_CONST_METHOD0(GetSettings, const Settings&()); - MOCK_METHOD1(ShouldDiscardLayerTree, bool(flutter::LayerTree&)); + MOCK_METHOD2(ShouldDiscardLayerTree, bool(int64_t, flutter::LayerTree&)); }; class MockSurface : public Surface { diff --git a/shell/common/shell.cc b/shell/common/shell.cc index a51b0325381f5..49ab48d139896 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1577,10 +1577,11 @@ fml::TimePoint Shell::GetLatestFrameTargetTime() const { } // |Rasterizer::Delegate| -bool Shell::ShouldDiscardLayerTree(flutter::LayerTree& tree) { +bool Shell::ShouldDiscardLayerTree(int64_t view_id, flutter::LayerTree& tree) { std::scoped_lock lock(resize_mutex_); - return !expected_frame_size_.isEmpty() && - tree.frame_size() != expected_frame_size_; + auto expected_frame_size = ExpectedFrameSize(view_id); + return !expected_frame_size.isEmpty() && + tree.frame_size() != expected_frame_size; } // |ServiceProtocol::Handler| diff --git a/shell/common/shell.h b/shell/common/shell.h index 50e9cbb751bb6..095dac28c1ced 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -690,7 +690,8 @@ class Shell final : public PlatformView::Delegate, fml::TimePoint GetLatestFrameTargetTime() const override; // |Rasterizer::Delegate| - bool ShouldDiscardLayerTree(flutter::LayerTree& layer_tree) override; + bool ShouldDiscardLayerTree(int64_t view_id, + flutter::LayerTree& tree) override; // |ServiceProtocol::Handler| fml::RefPtr GetServiceProtocolHandlerTaskRunner( diff --git a/shell/platform/embedder/tests/embedder_metal_unittests.mm b/shell/platform/embedder/tests/embedder_metal_unittests.mm index 2abc83527a147..36a626e10f4db 100644 --- a/shell/platform/embedder/tests/embedder_metal_unittests.mm +++ b/shell/platform/embedder/tests/embedder_metal_unittests.mm @@ -218,10 +218,12 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr auto rendered_scene = context.GetNextSceneImage(); - printf("main 1\n");fflush(stdout); + printf("main 1\n"); + fflush(stdout); auto engine = builder.LaunchEngine(); EXPECT_TRUE(engine.is_valid()); - printf("main 2\n");fflush(stdout); + printf("main 2\n"); + fflush(stdout); // Send a window metrics events so frames may be scheduled. FlutterWindowMetricsEvent event = {}; @@ -230,10 +232,12 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr event.height = 600; event.pixel_ratio = 1.0; EXPECT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); - printf("main 3\n");fflush(stdout); + printf("main 3\n"); + fflush(stdout); EXPECT_TRUE(ImageMatchesFixture("scene_without_custom_compositor.png", rendered_scene)); - printf("main 4\n");fflush(stdout); + printf("main 4\n"); + fflush(stdout); } TEST_F(EmbedderTest, TextureDestructionCallbackCalledWithoutCustomCompositorMetal) { diff --git a/shell/platform/embedder/tests/embedder_test_context.cc b/shell/platform/embedder/tests/embedder_test_context.cc index 2a6edf220ce9b..0424c8bef6948 100644 --- a/shell/platform/embedder/tests/embedder_test_context.cc +++ b/shell/platform/embedder/tests/embedder_test_context.cc @@ -257,7 +257,8 @@ std::future> EmbedderTestContext::GetNextSceneImage() { auto future = promise.get_future(); SetNextSceneCallback( fml::MakeCopyable([promise = std::move(promise)](auto image) mutable { - printf("Assign GetNextSceneImage\n");fflush(stdout); + printf("Assign GetNextSceneImage\n"); + fflush(stdout); promise.set_value(image); })); return future; diff --git a/shell/platform/embedder/tests/embedder_unittests_util.cc b/shell/platform/embedder/tests/embedder_unittests_util.cc index 3dec9026e764e..f0adaee3b9902 100644 --- a/shell/platform/embedder/tests/embedder_unittests_util.cc +++ b/shell/platform/embedder/tests/embedder_unittests_util.cc @@ -151,9 +151,11 @@ bool WriteImageToDisk(const fml::UniqueFD& directory, bool ImageMatchesFixture(const std::string& fixture_file_name, const sk_sp& scene_image) { - printf("ImageMatchesFixture sync\n");fflush(stdout); + printf("ImageMatchesFixture sync\n"); + fflush(stdout); fml::FileMapping fixture_image_mapping(OpenFixture(fixture_file_name)); - printf("ImageMatchesFixture sync 1\n");fflush(stdout); + printf("ImageMatchesFixture sync 1\n"); + fflush(stdout); FML_CHECK(fixture_image_mapping.GetSize() != 0u) << "Could not find fixture: " << fixture_file_name; @@ -164,7 +166,8 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, SkImages::DeferredFromEncodedData(std::move(encoded_image)) ->makeRasterImage(); - printf("ImageMatchesFixture 1\n");fflush(stdout); + printf("ImageMatchesFixture 1\n"); + fflush(stdout); FML_CHECK(fixture_image) << "Could not create image from fixture: " << fixture_file_name; @@ -178,16 +181,19 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, << "Could not create image subset for fixture comparison: " << scene_image_subset; - printf("ImageMatchesFixture 2\n");fflush(stdout); + printf("ImageMatchesFixture 2\n"); + fflush(stdout); const auto images_are_same = RasterImagesAreSame(scene_image_subset, fixture_image); - printf("ImageMatchesFixture 3\n");fflush(stdout); + printf("ImageMatchesFixture 3\n"); + fflush(stdout); // If the images are not the same, this predicate is going to indicate test // failure. Dump both the actual image and the expectation to disk to the // test author can figure out what went wrong. if (!images_are_same) { - printf("ImageMatchesFixture diff\n");fflush(stdout); + printf("ImageMatchesFixture diff\n"); + fflush(stdout); const auto fixtures_path = GetFixturesPath(); const auto actual_file_name = "actual_" + fixture_file_name; @@ -210,7 +216,8 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, << fml::paths::JoinPaths({fixtures_path, actual_file_name}) << std::endl; } - printf("ImageMatchesFixture 4\n");fflush(stdout); + printf("ImageMatchesFixture 4\n"); + fflush(stdout); return images_are_same; } From 96dc99467c6105389eaa300bbffa8474770690cf Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Fri, 18 Aug 2023 23:43:45 -0700 Subject: [PATCH 06/10] use weak this --- shell/common/rasterizer.cc | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 5923c3cd0981e..48cc000d6885c 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -208,7 +208,8 @@ RasterStatus Rasterizer::Draw( RasterStatus raster_status = RasterStatus::kFailed; LayerTreePipeline::Consumer consumer = - [&](std::unique_ptr item) { + [&raster_status, weak_this = weak_factory_.GetWeakPtr(), + &delegate = delegate_](std::unique_ptr item) { // TODO(dkwingsmt): Use a proper view ID when Rasterizer supports // multi-view. int64_t view_id = kFlutterImplicitViewId; @@ -216,11 +217,13 @@ RasterStatus Rasterizer::Draw( std::unique_ptr frame_timings_recorder = std::move(item->frame_timings_recorder); float device_pixel_ratio = item->device_pixel_ratio; - if (delegate_.ShouldDiscardLayerTree(view_id, *layer_tree.get())) { + if (weak_this && + delegate.ShouldDiscardLayerTree(view_id, *layer_tree.get())) { raster_status = RasterStatus::kDiscarded; } else { - raster_status = DoDraw(std::move(frame_timings_recorder), - std::move(layer_tree), device_pixel_ratio); + raster_status = + weak_this->DoDraw(std::move(frame_timings_recorder), + std::move(layer_tree), device_pixel_ratio); } }; @@ -258,12 +261,11 @@ RasterStatus Rasterizer::Draw( switch (consume_result) { case PipelineConsumeResult::MoreAvailable: { delegate_.GetTaskRunners().GetRasterTaskRunner()->PostTask( - fml::MakeCopyable( - [weak_this = weak_factory_.GetWeakPtr(), pipeline]() mutable { - if (weak_this) { - weak_this->Draw(pipeline); - } - })); + [weak_this = weak_factory_.GetWeakPtr(), pipeline]() { + if (weak_this) { + weak_this->Draw(pipeline); + } + }); break; } default: From a9bf24e19e6fa8bf53c9c72db420fa6d7cc7af49 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Mon, 21 Aug 2023 11:06:22 -0700 Subject: [PATCH 07/10] =?UTF-8?q?Fix=20weak=5Fthis=E2=80=99s=20controlling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- shell/common/rasterizer.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 48cc000d6885c..50c004ab4e8dd 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -217,7 +217,7 @@ RasterStatus Rasterizer::Draw( std::unique_ptr frame_timings_recorder = std::move(item->frame_timings_recorder); float device_pixel_ratio = item->device_pixel_ratio; - if (weak_this && + if (!weak_this || delegate.ShouldDiscardLayerTree(view_id, *layer_tree.get())) { raster_status = RasterStatus::kDiscarded; } else { From aa0ec102c7b6ee1a57d9e305910e1cd019e22164 Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Tue, 22 Aug 2023 16:33:21 -0700 Subject: [PATCH 08/10] Remove prints --- .../tests/embedder_metal_unittests.mm | 22 ++++++------------- .../embedder/tests/embedder_test_context.cc | 2 -- .../embedder/tests/embedder_unittests_util.cc | 14 ------------ 3 files changed, 7 insertions(+), 31 deletions(-) diff --git a/shell/platform/embedder/tests/embedder_metal_unittests.mm b/shell/platform/embedder/tests/embedder_metal_unittests.mm index 36a626e10f4db..78f7b82ed2fbf 100644 --- a/shell/platform/embedder/tests/embedder_metal_unittests.mm +++ b/shell/platform/embedder/tests/embedder_metal_unittests.mm @@ -108,9 +108,9 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr builder.SetMetalRendererConfig(texture_size); auto engine = builder.LaunchEngine(); - EXPECT_TRUE(engine.is_valid()); + ASSERT_TRUE(engine.is_valid()); - EXPECT_EQ(FlutterEngineRegisterExternalTexture(engine.get(), texture_id), kSuccess); + ASSERT_EQ(FlutterEngineRegisterExternalTexture(engine.get(), texture_id), kSuccess); auto rendered_scene = context.GetNextSceneImage(); @@ -120,9 +120,9 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr event.width = texture_size.width(); event.height = texture_size.height(); event.pixel_ratio = 1.0; - EXPECT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); + ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); - EXPECT_TRUE(ImageMatchesFixture("external_texture_metal.png", rendered_scene)); + ASSERT_TRUE(ImageMatchesFixture("external_texture_metal.png", rendered_scene)); } TEST_F(EmbedderTest, MetalCompositorMustBeAbleToRenderPlatformViews) { @@ -218,12 +218,8 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr auto rendered_scene = context.GetNextSceneImage(); - printf("main 1\n"); - fflush(stdout); auto engine = builder.LaunchEngine(); - EXPECT_TRUE(engine.is_valid()); - printf("main 2\n"); - fflush(stdout); + ASSERT_TRUE(engine.is_valid()); // Send a window metrics events so frames may be scheduled. FlutterWindowMetricsEvent event = {}; @@ -231,13 +227,9 @@ GrBackendTexture backend_texture(texture_size.width(), texture_size.height(), Gr event.width = 800; event.height = 600; event.pixel_ratio = 1.0; - EXPECT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); - printf("main 3\n"); - fflush(stdout); + ASSERT_EQ(FlutterEngineSendWindowMetricsEvent(engine.get(), &event), kSuccess); - EXPECT_TRUE(ImageMatchesFixture("scene_without_custom_compositor.png", rendered_scene)); - printf("main 4\n"); - fflush(stdout); + ASSERT_TRUE(ImageMatchesFixture("scene_without_custom_compositor.png", rendered_scene)); } TEST_F(EmbedderTest, TextureDestructionCallbackCalledWithoutCustomCompositorMetal) { diff --git a/shell/platform/embedder/tests/embedder_test_context.cc b/shell/platform/embedder/tests/embedder_test_context.cc index 0424c8bef6948..99b2fe0f5f8af 100644 --- a/shell/platform/embedder/tests/embedder_test_context.cc +++ b/shell/platform/embedder/tests/embedder_test_context.cc @@ -257,8 +257,6 @@ std::future> EmbedderTestContext::GetNextSceneImage() { auto future = promise.get_future(); SetNextSceneCallback( fml::MakeCopyable([promise = std::move(promise)](auto image) mutable { - printf("Assign GetNextSceneImage\n"); - fflush(stdout); promise.set_value(image); })); return future; diff --git a/shell/platform/embedder/tests/embedder_unittests_util.cc b/shell/platform/embedder/tests/embedder_unittests_util.cc index f0adaee3b9902..a29bf166a3ae5 100644 --- a/shell/platform/embedder/tests/embedder_unittests_util.cc +++ b/shell/platform/embedder/tests/embedder_unittests_util.cc @@ -151,11 +151,7 @@ bool WriteImageToDisk(const fml::UniqueFD& directory, bool ImageMatchesFixture(const std::string& fixture_file_name, const sk_sp& scene_image) { - printf("ImageMatchesFixture sync\n"); - fflush(stdout); fml::FileMapping fixture_image_mapping(OpenFixture(fixture_file_name)); - printf("ImageMatchesFixture sync 1\n"); - fflush(stdout); FML_CHECK(fixture_image_mapping.GetSize() != 0u) << "Could not find fixture: " << fixture_file_name; @@ -166,8 +162,6 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, SkImages::DeferredFromEncodedData(std::move(encoded_image)) ->makeRasterImage(); - printf("ImageMatchesFixture 1\n"); - fflush(stdout); FML_CHECK(fixture_image) << "Could not create image from fixture: " << fixture_file_name; @@ -181,19 +175,13 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, << "Could not create image subset for fixture comparison: " << scene_image_subset; - printf("ImageMatchesFixture 2\n"); - fflush(stdout); const auto images_are_same = RasterImagesAreSame(scene_image_subset, fixture_image); - printf("ImageMatchesFixture 3\n"); - fflush(stdout); // If the images are not the same, this predicate is going to indicate test // failure. Dump both the actual image and the expectation to disk to the // test author can figure out what went wrong. if (!images_are_same) { - printf("ImageMatchesFixture diff\n"); - fflush(stdout); const auto fixtures_path = GetFixturesPath(); const auto actual_file_name = "actual_" + fixture_file_name; @@ -216,8 +204,6 @@ bool ImageMatchesFixture(const std::string& fixture_file_name, << fml::paths::JoinPaths({fixtures_path, actual_file_name}) << std::endl; } - printf("ImageMatchesFixture 4\n"); - fflush(stdout); return images_are_same; } From 32ea38115fe57b28f7677c854cfb3a33f2b9d0eb Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Tue, 22 Aug 2023 17:04:19 -0700 Subject: [PATCH 09/10] const ref --- shell/common/rasterizer.h | 2 +- shell/common/rasterizer_unittests.cc | 3 ++- shell/common/shell.cc | 3 ++- shell/common/shell.h | 2 +- 4 files changed, 6 insertions(+), 4 deletions(-) diff --git a/shell/common/rasterizer.h b/shell/common/rasterizer.h index e0f95fbac9fb0..9abe2a7b02f69 100644 --- a/shell/common/rasterizer.h +++ b/shell/common/rasterizer.h @@ -121,7 +121,7 @@ class Rasterizer final : public SnapshotDelegate, virtual const Settings& GetSettings() const = 0; virtual bool ShouldDiscardLayerTree(int64_t view_id, - flutter::LayerTree& tree) = 0; + const flutter::LayerTree& tree) = 0; }; //---------------------------------------------------------------------------- diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index 71f360f57a4bf..5a527e49a2e4e 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -43,7 +43,8 @@ class MockDelegate : public Rasterizer::Delegate { MOCK_CONST_METHOD0(GetIsGpuDisabledSyncSwitch, std::shared_ptr()); MOCK_CONST_METHOD0(GetSettings, const Settings&()); - MOCK_METHOD2(ShouldDiscardLayerTree, bool(int64_t, flutter::LayerTree&)); + MOCK_METHOD2(ShouldDiscardLayerTree, + bool(int64_t, const flutter::LayerTree&)); }; class MockSurface : public Surface { diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 5a63422d038e3..128607232d7e9 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1584,7 +1584,8 @@ fml::TimePoint Shell::GetLatestFrameTargetTime() const { } // |Rasterizer::Delegate| -bool Shell::ShouldDiscardLayerTree(int64_t view_id, flutter::LayerTree& tree) { +bool Shell::ShouldDiscardLayerTree(int64_t view_id, + const flutter::LayerTree& tree) { std::scoped_lock lock(resize_mutex_); auto expected_frame_size = ExpectedFrameSize(view_id); return !expected_frame_size.isEmpty() && diff --git a/shell/common/shell.h b/shell/common/shell.h index 3f243e0d672b5..e808caec0ef9e 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -695,7 +695,7 @@ class Shell final : public PlatformView::Delegate, // |Rasterizer::Delegate| bool ShouldDiscardLayerTree(int64_t view_id, - flutter::LayerTree& tree) override; + const flutter::LayerTree& tree) override; // |ServiceProtocol::Handler| fml::RefPtr GetServiceProtocolHandlerTaskRunner( From c4918960e8c12cdd7a3e91342a6b697eaf4a57ec Mon Sep 17 00:00:00 2001 From: Tong Mu Date: Tue, 22 Aug 2023 18:51:12 -0700 Subject: [PATCH 10/10] Remove weak_ptr? --- shell/common/rasterizer.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 50c004ab4e8dd..7464a5cc51fa0 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -208,7 +208,7 @@ RasterStatus Rasterizer::Draw( RasterStatus raster_status = RasterStatus::kFailed; LayerTreePipeline::Consumer consumer = - [&raster_status, weak_this = weak_factory_.GetWeakPtr(), + [&raster_status, this, &delegate = delegate_](std::unique_ptr item) { // TODO(dkwingsmt): Use a proper view ID when Rasterizer supports // multi-view. @@ -217,13 +217,11 @@ RasterStatus Rasterizer::Draw( std::unique_ptr frame_timings_recorder = std::move(item->frame_timings_recorder); float device_pixel_ratio = item->device_pixel_ratio; - if (!weak_this || - delegate.ShouldDiscardLayerTree(view_id, *layer_tree.get())) { + if (delegate.ShouldDiscardLayerTree(view_id, *layer_tree.get())) { raster_status = RasterStatus::kDiscarded; } else { - raster_status = - weak_this->DoDraw(std::move(frame_timings_recorder), - std::move(layer_tree), device_pixel_ratio); + raster_status = DoDraw(std::move(frame_timings_recorder), + std::move(layer_tree), device_pixel_ratio); } };