From 844e833974714f03bd8a3f2511f19d8e96464220 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 4 Oct 2021 10:18:37 -0700 Subject: [PATCH 1/4] Start animator paused --- shell/common/animator.cc | 6 ------ shell/common/animator.h | 12 ++++++------ shell/common/animator_unittests.cc | 26 ++++++++++++++++++++++++++ shell/common/engine.cc | 2 +- shell/common/shell_test.cc | 17 +++++++++++++++++ shell/common/shell_test.h | 2 ++ 6 files changed, 52 insertions(+), 13 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index 39cb93b88e9ea..9361752c6991b 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -27,7 +27,6 @@ Animator::Animator(Delegate& delegate, : delegate_(delegate), task_runners_(std::move(task_runners)), waiter_(std::move(waiter)), - dart_frame_deadline_(0), #if SHELL_ENABLE_METAL layer_tree_pipeline_(std::make_shared(2)), #else // SHELL_ENABLE_METAL @@ -41,11 +40,6 @@ Animator::Animator(Delegate& delegate, : 2)), #endif // SHELL_ENABLE_METAL pending_frame_semaphore_(1), - paused_(false), - regenerate_layer_tree_(false), - frame_scheduled_(false), - notify_idle_task_id_(0), - dimension_change_pending_(false), weak_factory_(this) { } diff --git a/shell/common/animator.h b/shell/common/animator.h index 8061e223a5b03..8f65eb3a111b8 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -106,15 +106,15 @@ class Animator final { std::unique_ptr frame_timings_recorder_; uint64_t frame_request_number_ = 1; - int64_t dart_frame_deadline_; + int64_t dart_frame_deadline_ = 0; std::shared_ptr layer_tree_pipeline_; fml::Semaphore pending_frame_semaphore_; LayerTreePipeline::ProducerContinuation producer_continuation_; - bool paused_; - bool regenerate_layer_tree_; - bool frame_scheduled_; - int notify_idle_task_id_; - bool dimension_change_pending_; + bool paused_ = true; + bool regenerate_layer_tree_ = false; + bool frame_scheduled_ = false; + int notify_idle_task_id_ = 0; + bool dimension_change_pending_ = false; SkISize last_layer_tree_size_ = {0, 0}; std::deque trace_flow_ids_; diff --git a/shell/common/animator_unittests.cc b/shell/common/animator_unittests.cc index ecb9e254a97f5..34c237ea93182 100644 --- a/shell/common/animator_unittests.cc +++ b/shell/common/animator_unittests.cc @@ -73,6 +73,9 @@ TEST_F(ShellTest, VSyncTargetTime) { fml::TaskRunner::RunNowOrPostTask(shell->GetTaskRunners().GetUITaskRunner(), [engine = shell->GetEngine()]() { if (engine) { + // Engine needs a surface for frames to + // be scheduled. + engine->OnOutputSurfaceCreated(); // this implies we can re-use the last // frame to trigger begin frame rather // than re-generating the layer tree. @@ -94,5 +97,28 @@ TEST_F(ShellTest, VSyncTargetTime) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } +TEST_F(ShellTest, AnimatorStartsPaused) { + // Create all te prerequisites for a shell. + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); + auto settings = CreateSettingsForFixture(); + TaskRunners task_runners = GetTaskRunnersForFixture(); + + auto shell = CreateShell(std::move(settings), task_runners); + ASSERT_TRUE(DartVMRef::IsInstanceRunning()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + ASSERT_TRUE(configuration.IsValid()); + + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + + ASSERT_FALSE(IsAnimatorRunning(shell.get())); + + // teardown. + DestroyShell(std::move(shell), std::move(task_runners)); + ASSERT_FALSE(DartVMRef::IsInstanceRunning()); +} + } // namespace testing } // namespace flutter diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 28321a9fd166c..8f0a4503e0a49 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -265,7 +265,6 @@ tonic::DartErrorHandleType Engine::GetUIIsolateLastError() { void Engine::OnOutputSurfaceCreated() { have_surface_ = true; - StartAnimatorIfPossible(); ScheduleFrame(); } @@ -465,6 +464,7 @@ std::string Engine::DefaultRouteName() { } void Engine::ScheduleFrame(bool regenerate_layer_tree) { + StartAnimatorIfPossible(); animator_->RequestFrame(regenerate_layer_tree); } diff --git a/shell/common/shell_test.cc b/shell/common/shell_test.cc index 3e55654a19c88..b5cc9c8e3ae52 100644 --- a/shell/common/shell_test.cc +++ b/shell/common/shell_test.cc @@ -381,5 +381,22 @@ void ShellTest::DestroyShell(std::unique_ptr shell, latch.Wait(); } +bool ShellTest::IsAnimatorRunning(Shell* shell) { + fml::AutoResetWaitableEvent latch; + bool running = false; + if (!shell) { + return running; + } + fml::TaskRunner::RunNowOrPostTask( + shell->GetTaskRunners().GetUITaskRunner(), [shell, &running, &latch]() { + if (shell && shell->engine_ && shell->engine_->animator_) { + running = !shell->engine_->animator_->paused_; + } + latch.Signal(); + }); + latch.Wait(); + return running; +} + } // namespace testing } // namespace flutter diff --git a/shell/common/shell_test.h b/shell/common/shell_test.h index b2f1052fa98e1..1524c150e377a 100644 --- a/shell/common/shell_test.h +++ b/shell/common/shell_test.h @@ -96,6 +96,8 @@ class ShellTest : public FixtureTest { const SkData& key, const SkData& value); + static bool IsAnimatorRunning(Shell* shell); + enum ServiceProtocolEnum { kGetSkSLs, kEstimateRasterCacheMemory, From 8c2e7a7543d80432743e0273ca1d29d86ac9c116 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 4 Oct 2021 10:59:08 -0700 Subject: [PATCH 2/4] use fml::TimePoint --- shell/common/animator.cc | 9 +++++---- shell/common/animator.h | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index 9361752c6991b..d0f91bcaae8f4 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -86,10 +86,10 @@ const char* Animator::FrameParity() { return (frame_number % 2) ? "even" : "odd"; } -static int64_t FxlToDartOrEarlier(fml::TimePoint time) { - int64_t dart_now = Dart_TimelineGetMicros(); +static fml::TimePoint FxlToDartOrEarlier(fml::TimePoint time) { + auto dart_now = fml::TimeDelta::FromMicroseconds(Dart_TimelineGetMicros()); fml::TimePoint fxl_now = fml::TimePoint::Now(); - return (time - fxl_now).ToMicroseconds() + dart_now; + return fml::TimePoint::FromEpochDelta(time - fxl_now + dart_now); } void Animator::BeginFrame( @@ -269,7 +269,8 @@ void Animator::AwaitVSync() { } }); - delegate_.OnAnimatorNotifyIdle(dart_frame_deadline_); + delegate_.OnAnimatorNotifyIdle( + dart_frame_deadline_.ToEpochDelta().ToMicroseconds()); } void Animator::ScheduleSecondaryVsyncCallback(uintptr_t id, diff --git a/shell/common/animator.h b/shell/common/animator.h index 8f65eb3a111b8..544c6c77ba370 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -106,7 +106,7 @@ class Animator final { std::unique_ptr frame_timings_recorder_; uint64_t frame_request_number_ = 1; - int64_t dart_frame_deadline_ = 0; + fml::TimePoint dart_frame_deadline_; std::shared_ptr layer_tree_pipeline_; fml::Semaphore pending_frame_semaphore_; LayerTreePipeline::ProducerContinuation producer_continuation_; From 489c152d1d53cc3b096f77058c85f2f1a280f0e7 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 4 Oct 2021 19:34:40 -0700 Subject: [PATCH 3/4] Do not notify idle before rendering a frame --- shell/common/animator.cc | 10 ++-- shell/common/animator.h | 1 + shell/common/animator_unittests.cc | 81 +++++++++++++++++++++++++++++- 3 files changed, 87 insertions(+), 5 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index d0f91bcaae8f4..32403f1c2e70a 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -147,7 +147,7 @@ void Animator::BeginFrame( delegate_.OnAnimatorBeginFrame(frame_target_time, frame_number); } - if (!frame_scheduled_) { + if (!frame_scheduled_ && has_rendered_) { // Under certain workloads (such as our parent view resizing us, which is // communicated to us by repeat viewport metrics events), we won't // actually have a frame scheduled yet, despite the fact that we *will* be @@ -177,6 +177,7 @@ void Animator::BeginFrame( } void Animator::Render(std::unique_ptr layer_tree) { + has_rendered_ = true; if (dimension_change_pending_ && layer_tree->frame_size() != last_layer_tree_size_) { dimension_change_pending_ = false; @@ -268,9 +269,10 @@ void Animator::AwaitVSync() { } } }); - - delegate_.OnAnimatorNotifyIdle( - dart_frame_deadline_.ToEpochDelta().ToMicroseconds()); + if (has_rendered_) { + delegate_.OnAnimatorNotifyIdle( + dart_frame_deadline_.ToEpochDelta().ToMicroseconds()); + } } void Animator::ScheduleSecondaryVsyncCallback(uintptr_t id, diff --git a/shell/common/animator.h b/shell/common/animator.h index 544c6c77ba370..85f940700dbc6 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -117,6 +117,7 @@ class Animator final { bool dimension_change_pending_ = false; SkISize last_layer_tree_size_ = {0, 0}; std::deque trace_flow_ids_; + bool has_rendered_ = false; fml::WeakPtrFactory weak_factory_; diff --git a/shell/common/animator_unittests.cc b/shell/common/animator_unittests.cc index 34c237ea93182..9310a3b72a01d 100644 --- a/shell/common/animator_unittests.cc +++ b/shell/common/animator_unittests.cc @@ -18,6 +18,25 @@ namespace flutter { namespace testing { +class FakeAnimatorDelegate : public Animator::Delegate { + public: + void OnAnimatorBeginFrame(fml::TimePoint frame_target_time, + uint64_t frame_number) override {} + + void OnAnimatorNotifyIdle(int64_t deadline) override { + notify_idle_called_ = true; + } + + void OnAnimatorDraw( + std::shared_ptr> pipeline, + std::unique_ptr frame_timings_recorder) override {} + + void OnAnimatorDrawLastLayerTree( + std::unique_ptr frame_timings_recorder) override {} + + bool notify_idle_called_ = false; +}; + TEST_F(ShellTest, VSyncTargetTime) { // Add native callbacks to listen for window.onBeginFrame int64_t target_time; @@ -103,7 +122,8 @@ TEST_F(ShellTest, AnimatorStartsPaused) { auto settings = CreateSettingsForFixture(); TaskRunners task_runners = GetTaskRunnersForFixture(); - auto shell = CreateShell(std::move(settings), task_runners); + auto shell = CreateShell(std::move(settings), task_runners, + /* simulate_vsync=*/true); ASSERT_TRUE(DartVMRef::IsInstanceRunning()); auto configuration = RunConfiguration::InferFromSettings(settings); @@ -120,5 +140,64 @@ TEST_F(ShellTest, AnimatorStartsPaused) { ASSERT_FALSE(DartVMRef::IsInstanceRunning()); } +TEST_F(ShellTest, AnimatorDoesNotNotifyIdleBeforeRender) { + FakeAnimatorDelegate delegate; + TaskRunners task_runners = GetTaskRunnersForFixture(); + auto vsync_waiter = static_cast>( + std::make_unique(task_runners)); + + fml::AutoResetWaitableEvent latch; + std::shared_ptr animator; + + // Create the animator on the UI task runner. + task_runners.GetUITaskRunner()->PostTask([&] { + animator = std::make_unique(delegate, task_runners, + std::move(vsync_waiter)); + latch.Signal(); + }); + latch.Wait(); + + // Validate it has not notified idle and start it. + task_runners.GetUITaskRunner()->PostTask([&] { + ASSERT_FALSE(delegate.notify_idle_called_); + animator->Start(); + latch.Signal(); + }); + latch.Wait(); + + // Validate it has not notified idle and request a frame. + task_runners.GetUITaskRunner()->PostTask([&] { + ASSERT_FALSE(delegate.notify_idle_called_); + animator->RequestFrame(); + latch.Signal(); + }); + latch.Wait(); + + // Validate it has not notified idle and try to render. + task_runners.GetUITaskRunner()->PostTask([&] { + ASSERT_FALSE(delegate.notify_idle_called_); + auto layer_tree = std::make_unique(SkISize::Make(600, 800), 1.0); + animator->Render(std::move(layer_tree)); + latch.Signal(); + }); + latch.Wait(); + + // Still hasn't notified idle because there has been no frame request. + task_runners.GetUITaskRunner()->PostTask([&] { + ASSERT_FALSE(delegate.notify_idle_called_); + animator->RequestFrame(); + latch.Signal(); + }); + latch.Wait(); + + // Now it should notify idle. Make sure it is destroyed on the UI thread. + task_runners.GetUITaskRunner()->PostTask([&] { + ASSERT_TRUE(delegate.notify_idle_called_); + animator.reset(); + latch.Signal(); + }); + latch.Wait(); +} + } // namespace testing } // namespace flutter From afebb1f4627782180de54bdac4f7e884d0bc84b5 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 5 Oct 2021 16:57:12 -0700 Subject: [PATCH 4/4] Clean up animator correctly and more closely control vsync --- shell/common/animator_unittests.cc | 68 ++++++++++++++++++++---------- 1 file changed, 46 insertions(+), 22 deletions(-) diff --git a/shell/common/animator_unittests.cc b/shell/common/animator_unittests.cc index 9310a3b72a01d..0b11d06c8929f 100644 --- a/shell/common/animator_unittests.cc +++ b/shell/common/animator_unittests.cc @@ -142,57 +142,81 @@ TEST_F(ShellTest, AnimatorStartsPaused) { TEST_F(ShellTest, AnimatorDoesNotNotifyIdleBeforeRender) { FakeAnimatorDelegate delegate; - TaskRunners task_runners = GetTaskRunnersForFixture(); - auto vsync_waiter = static_cast>( - std::make_unique(task_runners)); + TaskRunners task_runners = { + "test", + CreateNewThread(), // platform + CreateNewThread(), // raster + CreateNewThread(), // ui + CreateNewThread() // io + }; + auto clock = std::make_shared(); fml::AutoResetWaitableEvent latch; 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))); + latch.Signal(); + }; + // Create the animator on the UI task runner. task_runners.GetUITaskRunner()->PostTask([&] { + auto vsync_waiter = static_cast>( + std::make_unique(task_runners, clock)); animator = std::make_unique(delegate, task_runners, std::move(vsync_waiter)); latch.Signal(); }); latch.Wait(); - // Validate it has not notified idle and start it. + // Validate it has not notified idle and start it. This will request a frame. task_runners.GetUITaskRunner()->PostTask([&] { ASSERT_FALSE(delegate.notify_idle_called_); animator->Start(); - latch.Signal(); - }); - latch.Wait(); - - // Validate it has not notified idle and request a frame. - task_runners.GetUITaskRunner()->PostTask([&] { - ASSERT_FALSE(delegate.notify_idle_called_); - animator->RequestFrame(); - latch.Signal(); + // Immediately request a frame saying it can reuse the last layer tree to + // avoid more calls to BeginFrame by the animator. + animator->RequestFrame(false); + task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task); }); latch.Wait(); + ASSERT_FALSE(delegate.notify_idle_called_); // Validate it has not notified idle and try to render. - task_runners.GetUITaskRunner()->PostTask([&] { - ASSERT_FALSE(delegate.notify_idle_called_); - auto layer_tree = std::make_unique(SkISize::Make(600, 800), 1.0); - animator->Render(std::move(layer_tree)); - latch.Signal(); - }); + task_runners.GetUITaskRunner()->PostDelayedTask( + [&] { + ASSERT_FALSE(delegate.notify_idle_called_); + auto layer_tree = + std::make_unique(SkISize::Make(600, 800), 1.0); + animator->Render(std::move(layer_tree)); + task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task); + }, + // See kNotifyIdleTaskWaitTime in animator.cc. + fml::TimeDelta::FromMilliseconds(60)); latch.Wait(); // Still hasn't notified idle because there has been no frame request. task_runners.GetUITaskRunner()->PostTask([&] { ASSERT_FALSE(delegate.notify_idle_called_); - animator->RequestFrame(); - latch.Signal(); + // False to avoid getting cals to BeginFrame that will request more frames + // before we are ready. + animator->RequestFrame(false); + task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task); }); latch.Wait(); // Now it should notify idle. Make sure it is destroyed on the UI thread. + ASSERT_TRUE(delegate.notify_idle_called_); + + // Stop and do one more flush so we can safely clean up on the UI thread. + animator->Stop(); + task_runners.GetPlatformTaskRunner()->PostTask(flush_vsync_task); + latch.Wait(); + task_runners.GetUITaskRunner()->PostTask([&] { - ASSERT_TRUE(delegate.notify_idle_called_); animator.reset(); latch.Signal(); });