From 650a7566b3c592225b13c1788179ebe24156dcc1 Mon Sep 17 00:00:00 2001 From: ColdPaleLight Date: Tue, 1 Mar 2022 11:13:03 +0800 Subject: [PATCH 1/2] Animator stopped notifying delegate to render when pipeline is not empty --- shell/common/animator.cc | 15 +++++- shell/common/animator_unittests.cc | 70 ++++++++++++++++++++++++++-- shell/common/pipeline.h | 28 +++++++---- shell/common/pipeline_unittests.cc | 47 ++++++++++++------- shell/common/rasterizer.cc | 4 +- shell/common/rasterizer_unittests.cc | 45 +++++++++++------- 6 files changed, 157 insertions(+), 52 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index 48bcd6bfc4f69..11bb11cff3594 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -175,9 +175,20 @@ void Animator::Render(std::unique_ptr layer_tree) { frame_timings_recorder_->RecordBuildEnd(fml::TimePoint::Now()); // Commit the pending continuation. - bool result = producer_continuation_.Complete(std::move(layer_tree)); - if (!result) { + PipelineProduceResult result = + producer_continuation_.Complete(std::move(layer_tree)); + + if (!result.success) { FML_DLOG(INFO) << "No pending continuation to commit"; + return; + } + + if (!result.is_first_item) { + // It has been successfully pushed to the pipeline but not as the first + // item. Eventually the 'Rasterizer' will consume it, so we don't need to + // notify the delegate. + FML_DLOG(INFO) << "Not the first item in the pipeline, ignore it"; + return; } delegate_.OnAnimatorDraw(layer_tree_pipeline_, diff --git a/shell/common/animator_unittests.cc b/shell/common/animator_unittests.cc index 57fad157af38d..3e7fcb682416c 100644 --- a/shell/common/animator_unittests.cc +++ b/shell/common/animator_unittests.cc @@ -12,7 +12,9 @@ #include "flutter/shell/common/shell_test.h" #include "flutter/shell/common/shell_test_platform_view.h" +#include "flutter/testing/post_task_sync.h" #include "flutter/testing/testing.h" +#include "gmock/gmock.h" #include "gtest/gtest.h" // CREATE_NATIVE_ENTRY is leaky by design @@ -23,16 +25,17 @@ namespace testing { class FakeAnimatorDelegate : public Animator::Delegate { public: - void OnAnimatorBeginFrame(fml::TimePoint frame_target_time, - uint64_t frame_number) override {} + MOCK_METHOD2(OnAnimatorBeginFrame, + void(fml::TimePoint frame_target_time, uint64_t frame_number)); void OnAnimatorNotifyIdle(fml::TimePoint deadline) override { notify_idle_called_ = true; } - void OnAnimatorDraw( - std::shared_ptr> pipeline, - std::unique_ptr frame_timings_recorder) override {} + MOCK_METHOD2( + OnAnimatorDraw, + void(std::shared_ptr> pipeline, + std::unique_ptr frame_timings_recorder)); void OnAnimatorDrawLastLayerTree( std::unique_ptr frame_timings_recorder) override {} @@ -184,6 +187,63 @@ TEST_F(ShellTest, AnimatorDoesNotNotifyIdleBeforeRender) { latch.Wait(); } +TEST_F(ShellTest, AnimatorDoesNotNotifyDelegateIfPipelineIsNotEmpty) { + FakeAnimatorDelegate delegate; + TaskRunners task_runners = { + "test", + CreateNewThread(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + }; + + auto clock = std::make_shared(); + std::shared_ptr animator; + + auto flush_vsync_task = [&] { + fml::AutoResetWaitableEvent ui_latch; + task_runners.GetUITaskRunner()->PostTask([&] { ui_latch.Signal(); }); + do { + clock->SimulateVSync(); + } while (ui_latch.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(1))); + }; + + // Create the animator on the UI task runner. + PostTaskSync(task_runners.GetUITaskRunner(), [&] { + auto vsync_waiter = static_cast>( + std::make_unique(task_runners, clock)); + animator = std::make_unique(delegate, task_runners, + std::move(vsync_waiter)); + }); + + fml::AutoResetWaitableEvent begin_frame_latch; + EXPECT_CALL(delegate, OnAnimatorBeginFrame) + .WillRepeatedly( + [&](fml::TimePoint frame_target_time, uint64_t frame_number) { + begin_frame_latch.Signal(); + }); + + // It will only be called once even though we call the method Animator::Render + // twice. because it will only be called when the pipeline is empty. + EXPECT_CALL(delegate, OnAnimatorDraw).Times(1); + + for (int i = 0; i < 2; i++) { + task_runners.GetUITaskRunner()->PostTask([&] { + animator->RequestFrame(); + task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task); + }); + begin_frame_latch.Wait(); + + PostTaskSync(task_runners.GetUITaskRunner(), [&] { + auto layer_tree = + std::make_unique(SkISize::Make(600, 800), 1.0); + animator->Render(std::move(layer_tree)); + }); + } + + PostTaskSync(task_runners.GetUITaskRunner(), [&] { animator.reset(); }); +} + } // namespace testing } // namespace flutter diff --git a/shell/common/pipeline.h b/shell/common/pipeline.h index 1bc43d0cf5071..fbebfc895b8eb 100644 --- a/shell/common/pipeline.h +++ b/shell/common/pipeline.h @@ -16,6 +16,14 @@ namespace flutter { +struct PipelineProduceResult { + // Whether the item was successfully pushed into the pipeline. + bool success = false; + // Whether it is the first item of the pipeline. Only valid when 'success' is + // 'true'. + bool is_first_item = false; +}; + enum class PipelineConsumeResult { NoneAvailable, Done, @@ -60,8 +68,8 @@ class Pipeline { } } - [[nodiscard]] bool Complete(ResourcePtr resource) { - bool result = false; + [[nodiscard]] PipelineProduceResult Complete(ResourcePtr resource) { + PipelineProduceResult result; if (continuation_) { result = continuation_(std::move(resource), trace_id_); continuation_ = nullptr; @@ -75,7 +83,8 @@ class Pipeline { private: friend class Pipeline; - using Continuation = std::function; + using Continuation = + std::function; Continuation continuation_; size_t trace_id_; @@ -179,32 +188,35 @@ class Pipeline { std::mutex queue_mutex_; std::deque> queue_; - bool ProducerCommit(ResourcePtr resource, size_t trace_id) { + PipelineProduceResult ProducerCommit(ResourcePtr resource, size_t trace_id) { + bool is_first_item = false; { std::scoped_lock lock(queue_mutex_); + is_first_item = queue_.empty(); queue_.emplace_back(std::move(resource), trace_id); } // Ensure the queue mutex is not held as that would be a pessimization. available_.Signal(); - return true; + return {/*success=*/true, /*is_first_item=*/is_first_item}; } - bool ProducerCommitIfEmpty(ResourcePtr resource, size_t trace_id) { + PipelineProduceResult ProducerCommitIfEmpty(ResourcePtr resource, + size_t trace_id) { { std::scoped_lock lock(queue_mutex_); if (!queue_.empty()) { // Bail if the queue is not empty, opens up spaces to produce other // frames. empty_.Signal(); - return false; + return {/*success=*/false, /*is_first_item=*/false}; } queue_.emplace_back(std::move(resource), trace_id); } // Ensure the queue mutex is not held as that would be a pessimization. available_.Signal(); - return true; + return {/*success=*/true, /*is_first_item=*/true}; } FML_DISALLOW_COPY_AND_ASSIGN(Pipeline); diff --git a/shell/common/pipeline_unittests.cc b/shell/common/pipeline_unittests.cc index 159022ee53f12..11b8ed334133a 100644 --- a/shell/common/pipeline_unittests.cc +++ b/shell/common/pipeline_unittests.cc @@ -24,8 +24,10 @@ TEST(PipelineTest, ConsumeOneVal) { Continuation continuation = pipeline->Produce(); const int test_val = 1; - bool result = continuation.Complete(std::make_unique(test_val)); - ASSERT_EQ(result, true); + PipelineProduceResult result = + continuation.Complete(std::make_unique(test_val)); + ASSERT_EQ(result.success, true); + ASSERT_EQ(result.is_first_item, true); PipelineConsumeResult consume_result = pipeline->Consume( [&test_val](std::unique_ptr v) { ASSERT_EQ(*v, test_val); }); @@ -39,21 +41,23 @@ TEST(PipelineTest, ContinuationCanOnlyBeUsedOnce) { Continuation continuation = pipeline->Produce(); const int test_val = 1; - bool result = continuation.Complete(std::make_unique(test_val)); - ASSERT_EQ(result, true); + PipelineProduceResult result = + continuation.Complete(std::make_unique(test_val)); + ASSERT_EQ(result.success, true); + ASSERT_EQ(result.is_first_item, true); PipelineConsumeResult consume_result_1 = pipeline->Consume( [&test_val](std::unique_ptr v) { ASSERT_EQ(*v, test_val); }); result = continuation.Complete(std::make_unique(test_val)); - ASSERT_EQ(result, false); + ASSERT_EQ(result.success, false); ASSERT_EQ(consume_result_1, PipelineConsumeResult::Done); PipelineConsumeResult consume_result_2 = pipeline->Consume([](std::unique_ptr v) { FAIL(); }); result = continuation.Complete(std::make_unique(test_val)); - ASSERT_EQ(result, false); + ASSERT_EQ(result.success, false); ASSERT_EQ(consume_result_2, PipelineConsumeResult::NoneAvailable); } @@ -65,10 +69,12 @@ TEST(PipelineTest, PushingMoreThanDepthCompletesFirstSubmission) { Continuation continuation_2 = pipeline->Produce(); const int test_val_1 = 1, test_val_2 = 2; - bool result = continuation_1.Complete(std::make_unique(test_val_1)); - ASSERT_EQ(result, true); + PipelineProduceResult result = + continuation_1.Complete(std::make_unique(test_val_1)); + ASSERT_EQ(result.success, true); + ASSERT_EQ(result.is_first_item, true); result = continuation_2.Complete(std::make_unique(test_val_2)); - ASSERT_EQ(result, false); + ASSERT_EQ(result.success, false); PipelineConsumeResult consume_result_1 = pipeline->Consume( [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); @@ -84,10 +90,13 @@ TEST(PipelineTest, PushingMultiProcessesInOrder) { Continuation continuation_2 = pipeline->Produce(); const int test_val_1 = 1, test_val_2 = 2; - bool result = continuation_1.Complete(std::make_unique(test_val_1)); - ASSERT_EQ(result, true); + PipelineProduceResult result = + continuation_1.Complete(std::make_unique(test_val_1)); + ASSERT_EQ(result.success, true); + ASSERT_EQ(result.is_first_item, true); result = continuation_2.Complete(std::make_unique(test_val_2)); - ASSERT_EQ(result, true); + ASSERT_EQ(result.success, true); + ASSERT_EQ(result.is_first_item, false); PipelineConsumeResult consume_result_1 = pipeline->Consume( [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); @@ -106,10 +115,12 @@ TEST(PipelineTest, ProduceIfEmptyDoesNotConsumeWhenQueueIsNotEmpty) { Continuation continuation_2 = pipeline->ProduceIfEmpty(); const int test_val_1 = 1, test_val_2 = 2; - bool result = continuation_1.Complete(std::make_unique(test_val_1)); - ASSERT_EQ(result, true); + PipelineProduceResult result = + continuation_1.Complete(std::make_unique(test_val_1)); + ASSERT_EQ(result.success, true); + ASSERT_EQ(result.is_first_item, true); result = continuation_2.Complete(std::make_unique(test_val_2)); - ASSERT_EQ(result, false); + ASSERT_EQ(result.success, false); PipelineConsumeResult consume_result_1 = pipeline->Consume( [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); @@ -123,8 +134,10 @@ TEST(PipelineTest, ProduceIfEmptySuccessfulIfQueueIsEmpty) { Continuation continuation_1 = pipeline->ProduceIfEmpty(); const int test_val_1 = 1; - bool result = continuation_1.Complete(std::make_unique(test_val_1)); - ASSERT_EQ(result, true); + PipelineProduceResult result = + continuation_1.Complete(std::make_unique(test_val_1)); + ASSERT_EQ(result.success, true); + ASSERT_EQ(result.is_first_item, true); PipelineConsumeResult consume_result_1 = pipeline->Consume( [&test_val_1](std::unique_ptr v) { ASSERT_EQ(*v, test_val_1); }); diff --git a/shell/common/rasterizer.cc b/shell/common/rasterizer.cc index 904db93833453..eb21f4d49d0b3 100644 --- a/shell/common/rasterizer.cc +++ b/shell/common/rasterizer.cc @@ -198,9 +198,9 @@ RasterStatus Rasterizer::Draw( bool should_resubmit_frame = ShouldResubmitFrame(raster_status); if (should_resubmit_frame) { auto front_continuation = pipeline->ProduceIfEmpty(); - bool result = + PipelineProduceResult result = front_continuation.Complete(std::move(resubmitted_layer_tree_)); - if (result) { + if (result.success) { consume_result = PipelineConsumeResult::MoreAvailable; } } else if (raster_status == RasterStatus::kEnqueuePipeline) { diff --git a/shell/common/rasterizer_unittests.cc b/shell/common/rasterizer_unittests.cc index 26c16adcfc453..06418cf968549 100644 --- a/shell/common/rasterizer_unittests.cc +++ b/shell/common/rasterizer_unittests.cc @@ -170,8 +170,9 @@ TEST(RasterizerTest, auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); auto no_discard = [](LayerTree&) { return false; }; rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); latch.Signal(); @@ -229,8 +230,9 @@ TEST( auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); auto no_discard = [](LayerTree&) { return false; }; rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); latch.Signal(); @@ -294,8 +296,9 @@ TEST( auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); auto no_discard = [](LayerTree&) { return false; }; rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); } @@ -361,8 +364,9 @@ TEST(RasterizerTest, auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); auto no_discard = [](LayerTree&) { return false; }; // The Draw() will respectively call BeginFrame(), SubmitFrame() and @@ -405,8 +409,9 @@ TEST(RasterizerTest, externalViewEmbedderDoesntEndFrameWhenNoSurfaceIsSet) { auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); auto no_discard = [](LayerTree&) { return false; }; rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); latch.Signal(); @@ -498,8 +503,9 @@ TEST(RasterizerTest, auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); auto no_discard = [](LayerTree&) { return false; }; rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); latch.Signal(); @@ -549,8 +555,9 @@ TEST( auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); auto no_discard = [](LayerTree&) { return false; }; RasterStatus status = rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); @@ -601,8 +608,9 @@ TEST( auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); auto no_discard = [](LayerTree&) { return false; }; RasterStatus status = rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); @@ -652,8 +660,9 @@ TEST( auto pipeline = std::make_shared>(/*depth=*/10); auto layer_tree = std::make_unique(/*frame_size=*/SkISize(), /*device_pixel_ratio=*/2.0f); - bool result = pipeline->Produce().Complete(std::move(layer_tree)); - EXPECT_TRUE(result); + PipelineProduceResult result = + pipeline->Produce().Complete(std::move(layer_tree)); + EXPECT_TRUE(result.success); auto no_discard = [](LayerTree&) { return false; }; RasterStatus status = rasterizer->Draw(CreateFinishedBuildRecorder(), pipeline, no_discard); From 058bafc7f0ac0cedc32874342e3d9715dd568160 Mon Sep 17 00:00:00 2001 From: ColdPaleLight Date: Tue, 1 Mar 2022 15:22:55 +0800 Subject: [PATCH 2/2] Fix the unittests and tweak the code --- shell/common/animator.cc | 3 ++- shell/common/pipeline.h | 6 +++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index 11bb11cff3594..86496d1fbbff9 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -179,15 +179,16 @@ void Animator::Render(std::unique_ptr layer_tree) { producer_continuation_.Complete(std::move(layer_tree)); if (!result.success) { + frame_timings_recorder_.reset(); FML_DLOG(INFO) << "No pending continuation to commit"; return; } if (!result.is_first_item) { + frame_timings_recorder_.reset(); // It has been successfully pushed to the pipeline but not as the first // item. Eventually the 'Rasterizer' will consume it, so we don't need to // notify the delegate. - FML_DLOG(INFO) << "Not the first item in the pipeline, ignore it"; return; } diff --git a/shell/common/pipeline.h b/shell/common/pipeline.h index fbebfc895b8eb..676f7b7925900 100644 --- a/shell/common/pipeline.h +++ b/shell/common/pipeline.h @@ -198,7 +198,7 @@ class Pipeline { // Ensure the queue mutex is not held as that would be a pessimization. available_.Signal(); - return {/*success=*/true, /*is_first_item=*/is_first_item}; + return {.success = true, .is_first_item = is_first_item}; } PipelineProduceResult ProducerCommitIfEmpty(ResourcePtr resource, @@ -209,14 +209,14 @@ class Pipeline { // Bail if the queue is not empty, opens up spaces to produce other // frames. empty_.Signal(); - return {/*success=*/false, /*is_first_item=*/false}; + return {.success = false, .is_first_item = false}; } queue_.emplace_back(std::move(resource), trace_id); } // Ensure the queue mutex is not held as that would be a pessimization. available_.Signal(); - return {/*success=*/true, /*is_first_item=*/true}; + return {.success = true, .is_first_item = true}; } FML_DISALLOW_COPY_AND_ASSIGN(Pipeline);