From 844e833974714f03bd8a3f2511f19d8e96464220 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 4 Oct 2021 10:18:37 -0700 Subject: [PATCH 1/8] 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/8] 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/8] 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 7011383bbc2d9d8df8a02b936de16a6e34fbe10f Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 4 Oct 2021 21:30:29 -0700 Subject: [PATCH 4/8] Do asset IO on bg thread --- shell/common/engine.cc | 19 +++++++++++-------- shell/common/engine.h | 5 +++++ 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 408567435613f..ecb3794ddaf39 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -118,7 +118,7 @@ std::unique_ptr Engine::Spawn( /*delegate=*/delegate, /*dispatcher_maker=*/dispatcher_maker, /*image_decoder_task_runner=*/ - runtime_controller_->GetDartVM()->GetConcurrentWorkerTaskRunner(), + concurrent_task_runner(), /*task_runners=*/task_runners_, /*settings=*/settings, /*animator=*/std::move(animator), @@ -548,14 +548,17 @@ void Engine::HandleAssetPlatformMessage( data.GetSize()); if (asset_manager_) { - std::unique_ptr asset_mapping = - asset_manager_->GetAsMapping(asset_name); - if (asset_mapping) { - response->Complete(std::move(asset_mapping)); - return; - } + concurrent_task_runner()->PostTask([asset_manager = asset_manager_, + asset_name = std::move(asset_name), + response = std::move(response)] { + std::unique_ptr asset_mapping = + asset_manager->GetAsMapping(asset_name); + if (asset_mapping) { + response->Complete(std::move(asset_mapping)); + } + }); + return; } - response->CompleteEmpty(); } diff --git a/shell/common/engine.h b/shell/common/engine.h index ca73c591432a6..273150443c0c2 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -979,6 +979,11 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { bool GetAssetAsBuffer(const std::string& name, std::vector* data); + std::shared_ptr concurrent_task_runner() const { + FML_DCHECK(runtime_controller_); + return runtime_controller_->GetDartVM()->GetConcurrentWorkerTaskRunner(); + } + friend class testing::ShellTest; FML_DISALLOW_COPY_AND_ASSIGN(Engine); From afebb1f4627782180de54bdac4f7e884d0bc84b5 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 5 Oct 2021 16:57:12 -0700 Subject: [PATCH 5/8] 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(); }); From 9219a875b0c382a0930c9a345b8f59f93e3d494b Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 6 Oct 2021 11:09:08 -0700 Subject: [PATCH 6/8] .. --- shell/common/engine.cc | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/shell/common/engine.cc b/shell/common/engine.cc index ecb3794ddaf39..241e92873f588 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -547,19 +547,19 @@ void Engine::HandleAssetPlatformMessage( std::string asset_name(reinterpret_cast(data.GetMapping()), data.GetSize()); - if (asset_manager_) { - concurrent_task_runner()->PostTask([asset_manager = asset_manager_, - asset_name = std::move(asset_name), - response = std::move(response)] { - std::unique_ptr asset_mapping = - asset_manager->GetAsMapping(asset_name); - if (asset_mapping) { - response->Complete(std::move(asset_mapping)); - } - }); + if (!asset_manager_) { + response->CompleteEmpty(); return; } - response->CompleteEmpty(); + concurrent_task_runner()->PostTask([asset_manager = asset_manager_, + asset_name = std::move(asset_name), + response = std::move(response)] { + std::unique_ptr asset_mapping = + asset_manager->GetAsMapping(asset_name); + if (asset_mapping) { + response->Complete(std::move(asset_mapping)); + } + }); } const std::string& Engine::GetLastEntrypoint() const { From 45d6c3fedfeb6f2edc9b880f3cb0592ee7d17de6 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 20 Oct 2021 14:34:10 -0700 Subject: [PATCH 7/8] start test --- fml/concurrent_message_loop.cc | 13 +++++ fml/concurrent_message_loop.h | 2 + shell/common/engine.cc | 4 ++ shell/common/engine.h | 5 ++ shell/common/engine_unittests.cc | 83 +++++++++++++++++++++++++++++++- 5 files changed, 105 insertions(+), 2 deletions(-) diff --git a/fml/concurrent_message_loop.cc b/fml/concurrent_message_loop.cc index b58431b7df3d2..822d1aec0402f 100644 --- a/fml/concurrent_message_loop.cc +++ b/fml/concurrent_message_loop.cc @@ -6,6 +6,7 @@ #include +#include "concurrent_message_loop.h" #include "flutter/fml/thread.h" #include "flutter/fml/trace_event.h" @@ -148,6 +149,18 @@ std::vector ConcurrentMessageLoop::GetThreadTasksLocked() { return pending_tasks; } +bool ConcurrentMessageLoop::RunsTasksOnCurrentThread() { + std::scoped_lock lock(tasks_mutex_); + for (const auto& worker_thread_id : worker_thread_ids_) { + FML_LOG(ERROR) << "wtid: " << worker_thread_id + << " == " << std::this_thread::get_id(); + if (worker_thread_id == std::this_thread::get_id()) { + return true; + } + } + return false; +} + ConcurrentTaskRunner::ConcurrentTaskRunner( std::weak_ptr weak_loop) : weak_loop_(std::move(weak_loop)) {} diff --git a/fml/concurrent_message_loop.h b/fml/concurrent_message_loop.h index ebd8de983508d..457365e754b1a 100644 --- a/fml/concurrent_message_loop.h +++ b/fml/concurrent_message_loop.h @@ -34,6 +34,8 @@ class ConcurrentMessageLoop void PostTaskToAllWorkers(fml::closure task); + bool RunsTasksOnCurrentThread(); + private: friend ConcurrentTaskRunner; diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 241e92873f588..806f7cada8c9f 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -496,6 +497,7 @@ void Engine::UpdateSemantics(SemanticsNodeUpdates update, void Engine::HandlePlatformMessage(std::unique_ptr message) { if (message->channel() == kAssetChannel) { + FML_LOG(ERROR) << "Asset message!"; HandleAssetPlatformMessage(std::move(message)); } else { delegate_.OnEngineHandlePlatformMessage(std::move(message)); @@ -539,6 +541,7 @@ void Engine::ScheduleSecondaryVsyncCallback(uintptr_t id, void Engine::HandleAssetPlatformMessage( std::unique_ptr message) { + FML_LOG(ERROR) << "Here.."; fml::RefPtr response = message->response(); if (!response) { return; @@ -554,6 +557,7 @@ void Engine::HandleAssetPlatformMessage( concurrent_task_runner()->PostTask([asset_manager = asset_manager_, asset_name = std::move(asset_name), response = std::move(response)] { + FML_DLOG(ERROR) << "Here: " << std::this_thread::get_id(); std::unique_ptr asset_mapping = asset_manager->GetAsMapping(asset_name); if (asset_mapping) { diff --git a/shell/common/engine.h b/shell/common/engine.h index 273150443c0c2..66ef219838206 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -36,6 +36,10 @@ namespace flutter { +namespace testing { +class EngineTest; +} + //------------------------------------------------------------------------------ /// The engine is a component owned by the shell that resides on the UI task /// runner and is responsible for managing the needs of the root isolate and its @@ -984,6 +988,7 @@ class Engine final : public RuntimeDelegate, PointerDataDispatcher::Delegate { return runtime_controller_->GetDartVM()->GetConcurrentWorkerTaskRunner(); } + friend class testing::EngineTest; friend class testing::ShellTest; FML_DISALLOW_COPY_AND_ASSIGN(Engine); diff --git a/shell/common/engine_unittests.cc b/shell/common/engine_unittests.cc index e841060c636ac..a5d56cb31a015 100644 --- a/shell/common/engine_unittests.cc +++ b/shell/common/engine_unittests.cc @@ -6,6 +6,7 @@ #include +#include "flutter/fml/synchronization/count_down_latch.h" #include "flutter/runtime/dart_vm_lifecycle.h" #include "flutter/shell/common/thread_host.h" #include "flutter/testing/fixture_test.h" @@ -15,11 +16,13 @@ #include "rapidjson/stringbuffer.h" #include "rapidjson/writer.h" +#include "flutter/fml/backtrace.h" + ///\note Deprecated MOCK_METHOD macros used until this issue is resolved: // https://github.com/google/googletest/issues/2490 namespace flutter { - +namespace testing { namespace { class MockDelegate : public Engine::Delegate { public: @@ -100,6 +103,37 @@ std::unique_ptr MakePlatformMessage( return message; } +class FakeAssetResolver : public AssetResolver { + public: + FakeAssetResolver(std::shared_ptr concurrent_loop) + : concurrent_loop_(std::move(concurrent_loop)) {} + + // |AssetResolver| + bool IsValid() const override { return true; } + + // |AssetResolver| + bool IsValidAfterAssetManagerChange() const override { return true; } + + // |AssetResolver| + AssetResolverType GetType() const { + return AssetResolverType::kApkAssetProvider; + } + + // |AssetResolver| + std::unique_ptr GetAsMapping( + const std::string& asset_name) const override { + FML_LOG(ERROR) << "asset: " << asset_name; + if (asset_name != "FontManifest.json") + EXPECT_TRUE(concurrent_loop_->RunsTasksOnCurrentThread()) + << fml::BacktraceHere(); + return nullptr; + } + + private: + std::shared_ptr concurrent_loop_; +}; +} // namespace + class EngineTest : public testing::FixtureTest { public: EngineTest() @@ -141,7 +175,6 @@ class EngineTest : public testing::FixtureTest { std::unique_ptr runtime_controller_; std::shared_ptr image_decoder_task_runner_; }; -} // namespace TEST_F(EngineTest, Create) { PostUITaskSync([this] { @@ -328,4 +361,50 @@ TEST_F(EngineTest, PassesLoadDartDeferredLibraryErrorToRuntime) { }); } +TEST_F(EngineTest, AssetIOIsHandledOnWorkerThread) { + PostUITaskSync([this] { + FML_LOG(ERROR) << "Test ID: " << std::this_thread::get_id(); + MockRuntimeDelegate client; + auto mock_runtime_controller = + std::make_unique(client, task_runners_); + auto vm_ref = DartVMRef::Create(settings_); + EXPECT_CALL(*mock_runtime_controller, GetDartVM()) + .WillRepeatedly(::testing::Return(vm_ref.get())); + EXPECT_CALL(*mock_runtime_controller, IsRootIsolateRunning()) + .WillRepeatedly(::testing::Return(true)); + EXPECT_CALL(*mock_runtime_controller, DispatchPlatformMessage(::testing::_)) + .WillRepeatedly(::testing::Return(true)); + auto engine = std::make_unique( + /*delegate=*/delegate_, + /*dispatcher_maker=*/dispatcher_maker_, + /*image_decoder_task_runner=*/image_decoder_task_runner_, + /*task_runners=*/task_runners_, + /*settings=*/settings_, + /*animator=*/std::move(animator_), + /*io_manager=*/io_manager_, + /*font_collection=*/std::make_shared(), + /*runtime_controller=*/std::move(mock_runtime_controller)); + + fml::RefPtr response = + fml::MakeRefCounted(); + std::string asset_name = "foo.png"; + auto message = std::make_unique( + "flutter/assets", + fml::MallocMapping::Copy(asset_name.c_str(), asset_name.length()), + response); + + auto asset_manager = std::make_shared(); + auto concurrent_loop = vm_ref->GetConcurrentMessageLoop(); + asset_manager->PushBack( + std::make_unique(concurrent_loop)); + engine->UpdateAssetManager(asset_manager); + engine->HandlePlatformMessage(std::move(message)); + + fml::CountDownLatch latch(concurrent_loop->GetWorkerCount()); + concurrent_loop->PostTaskToAllWorkers([&latch] { latch.CountDown(); }); + latch.Wait(); + }); +} + +} // namespace testing } // namespace flutter From 02122f696da8fd5b307703ba094e8dea9410cdf4 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 20 Oct 2021 22:49:08 -0700 Subject: [PATCH 8/8] fix test --- fml/concurrent_message_loop.cc | 2 -- shell/common/engine.cc | 4 ---- shell/common/engine_unittests.cc | 20 +++++++++++++++----- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/fml/concurrent_message_loop.cc b/fml/concurrent_message_loop.cc index 822d1aec0402f..2b6e6e6f760f5 100644 --- a/fml/concurrent_message_loop.cc +++ b/fml/concurrent_message_loop.cc @@ -152,8 +152,6 @@ std::vector ConcurrentMessageLoop::GetThreadTasksLocked() { bool ConcurrentMessageLoop::RunsTasksOnCurrentThread() { std::scoped_lock lock(tasks_mutex_); for (const auto& worker_thread_id : worker_thread_ids_) { - FML_LOG(ERROR) << "wtid: " << worker_thread_id - << " == " << std::this_thread::get_id(); if (worker_thread_id == std::this_thread::get_id()) { return true; } diff --git a/shell/common/engine.cc b/shell/common/engine.cc index 806f7cada8c9f..241e92873f588 100644 --- a/shell/common/engine.cc +++ b/shell/common/engine.cc @@ -7,7 +7,6 @@ #include #include #include -#include #include #include @@ -497,7 +496,6 @@ void Engine::UpdateSemantics(SemanticsNodeUpdates update, void Engine::HandlePlatformMessage(std::unique_ptr message) { if (message->channel() == kAssetChannel) { - FML_LOG(ERROR) << "Asset message!"; HandleAssetPlatformMessage(std::move(message)); } else { delegate_.OnEngineHandlePlatformMessage(std::move(message)); @@ -541,7 +539,6 @@ void Engine::ScheduleSecondaryVsyncCallback(uintptr_t id, void Engine::HandleAssetPlatformMessage( std::unique_ptr message) { - FML_LOG(ERROR) << "Here.."; fml::RefPtr response = message->response(); if (!response) { return; @@ -557,7 +554,6 @@ void Engine::HandleAssetPlatformMessage( concurrent_task_runner()->PostTask([asset_manager = asset_manager_, asset_name = std::move(asset_name), response = std::move(response)] { - FML_DLOG(ERROR) << "Here: " << std::this_thread::get_id(); std::unique_ptr asset_mapping = asset_manager->GetAsMapping(asset_name); if (asset_mapping) { diff --git a/shell/common/engine_unittests.cc b/shell/common/engine_unittests.cc index a5d56cb31a015..1aa34b8e56af0 100644 --- a/shell/common/engine_unittests.cc +++ b/shell/common/engine_unittests.cc @@ -122,13 +122,17 @@ class FakeAssetResolver : public AssetResolver { // |AssetResolver| std::unique_ptr GetAsMapping( const std::string& asset_name) const override { - FML_LOG(ERROR) << "asset: " << asset_name; + mapping_requests.push_back(asset_name); + // TODO(dnfield): FontManifest is loaded differently than most assets. + // Should we load it on a background thread? if (asset_name != "FontManifest.json") EXPECT_TRUE(concurrent_loop_->RunsTasksOnCurrentThread()) << fml::BacktraceHere(); return nullptr; } + mutable std::vector mapping_requests; + private: std::shared_ptr concurrent_loop_; }; @@ -157,6 +161,11 @@ class EngineTest : public testing::FixtureTest { latch.Wait(); } + void HandlePlatformMessage(Engine* engine, + std::unique_ptr message) { + engine->HandlePlatformMessage(std::move(message)); + } + protected: void SetUp() override { settings_ = CreateSettingsForFixture(); @@ -363,7 +372,6 @@ TEST_F(EngineTest, PassesLoadDartDeferredLibraryErrorToRuntime) { TEST_F(EngineTest, AssetIOIsHandledOnWorkerThread) { PostUITaskSync([this] { - FML_LOG(ERROR) << "Test ID: " << std::this_thread::get_id(); MockRuntimeDelegate client; auto mock_runtime_controller = std::make_unique(client, task_runners_); @@ -395,14 +403,16 @@ TEST_F(EngineTest, AssetIOIsHandledOnWorkerThread) { auto asset_manager = std::make_shared(); auto concurrent_loop = vm_ref->GetConcurrentMessageLoop(); - asset_manager->PushBack( - std::make_unique(concurrent_loop)); + auto resolver = std::make_unique(concurrent_loop); + auto raw_resolver = resolver.get(); + asset_manager->PushBack(std::move(resolver)); engine->UpdateAssetManager(asset_manager); - engine->HandlePlatformMessage(std::move(message)); + HandlePlatformMessage(engine.get(), std::move(message)); fml::CountDownLatch latch(concurrent_loop->GetWorkerCount()); concurrent_loop->PostTaskToAllWorkers([&latch] { latch.CountDown(); }); latch.Wait(); + EXPECT_EQ(raw_resolver->mapping_requests.back(), "foo.png"); }); }