From 5aa9df6156b93f74b4e1bd1ef472d727d412f35d Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 18 Jun 2019 16:45:35 -0700 Subject: [PATCH 01/30] Got rid of the black frame by synchronizing the main thread with the gpu thread to make sure a frame is rendered before presenting the view. --- fml/task_runner.cc | 10 +++++++++ fml/task_runner.h | 2 ++ shell/common/animator.cc | 7 ++++++ shell/common/animator.h | 2 ++ shell/common/engine.h | 2 ++ shell/common/shell.cc | 22 +++++++++++++++++++ shell/common/shell.h | 6 +++++ shell/common/vsync_waiter.h | 2 ++ shell/common/vsync_waiter_fallback.cc | 4 ++++ shell/common/vsync_waiter_fallback.h | 2 ++ .../framework/Source/FlutterViewController.mm | 18 +++++++++++++-- .../ios/framework/Source/vsync_waiter_ios.h | 2 ++ .../ios/framework/Source/vsync_waiter_ios.mm | 10 +++++++++ 13 files changed, 87 insertions(+), 2 deletions(-) diff --git a/fml/task_runner.cc b/fml/task_runner.cc index 2c4cfe4b638a2..bad8412e408b0 100644 --- a/fml/task_runner.cc +++ b/fml/task_runner.cc @@ -8,6 +8,7 @@ #include +#include "flutter/fml/synchronization/count_down_latch.h" #include "flutter/fml/logging.h" #include "flutter/fml/message_loop.h" #include "flutter/fml/message_loop_impl.h" @@ -23,6 +24,15 @@ void TaskRunner::PostTask(fml::closure task) { loop_->PostTask(std::move(task), fml::TimePoint::Now()); } +void TaskRunner::AwaitTask(fml::closure task) { + CountDownLatch latch(1); + PostTask([&]{ + task(); + latch.CountDown(); + }); + latch.Wait(); +} + void TaskRunner::PostTaskForTime(fml::closure task, fml::TimePoint target_time) { loop_->PostTask(std::move(task), target_time); diff --git a/fml/task_runner.h b/fml/task_runner.h index 72c4219029f34..01f52ba924850 100644 --- a/fml/task_runner.h +++ b/fml/task_runner.h @@ -21,6 +21,8 @@ class TaskRunner : public fml::RefCountedThreadSafe { virtual void PostTask(fml::closure task); + virtual void AwaitTask(fml::closure task); + virtual void PostTaskForTime(fml::closure task, fml::TimePoint target_time); virtual void PostDelayedTask(fml::closure task, fml::TimeDelta delay); diff --git a/shell/common/animator.cc b/shell/common/animator.cc index 3632476ff6a1b..f4216b98e69be 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -239,4 +239,11 @@ void Animator::AwaitVSync() { delegate_.OnAnimatorNotifyIdle(dart_frame_deadline_); } +void Animator::ForceVSync() { + regenerate_layer_tree_ = true; + frame_scheduled_ = true; + AwaitVSync(); + waiter_->ForceVSync(); +} + } // namespace flutter diff --git a/shell/common/animator.h b/shell/common/animator.h index 9f84468c01b78..d99a97ea3e7d1 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -62,6 +62,8 @@ class Animator final { // will be ended during the next |BeginFrame|. void EnqueueTraceFlowId(uint64_t trace_flow_id); + void ForceVSync(); + private: using LayerTreePipeline = Pipeline; diff --git a/shell/common/engine.h b/shell/common/engine.h index 293d78c1a7178..e1157017806ff 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -127,6 +127,8 @@ class Engine final : public RuntimeDelegate { // |RuntimeDelegate| FontCollection& GetFontCollection() override; + Animator* GetAnimator() { return animator_.get(); } + private: Engine::Delegate& delegate_; const Settings settings_; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index ba38f792e12d0..7a27307d144eb 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -922,6 +922,16 @@ void Shell::OnFrameRasterized(const FrameTiming& timing) { FML_DCHECK(is_setup_); FML_DCHECK(task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread()); + // waiting_for_frame is only modified on this thread so we should be able to + // read from it safely without guards. + if (waiting_for_frame_) { + { + std::lock_guard lock(waiting_for_frame_mutex_); + waiting_for_frame_ = false; + } + waiting_for_frame_condition_.notify_all(); + } + // The C++ callback defined in settings.h and set by Flutter runner. This is // independent of the timings report to the Dart side. if (settings_.frame_rasterized_callback) { @@ -1229,4 +1239,16 @@ Rasterizer::Screenshot Shell::Screenshot( return screenshot; } +bool Shell::WaitForFrameRender(fml::TimeDelta timeout) { + std::unique_lock lock(waiting_for_frame_mutex_); + task_runners_.GetUITaskRunner()->AwaitTask([this] { + engine_->GetAnimator()->ForceVSync(); + }); + waiting_for_frame_ = true; + bool success = waiting_for_frame_condition_.wait_for( + lock, std::chrono::milliseconds(timeout.ToMilliseconds()), + [this] { return !waiting_for_frame_; }); + return success; +} + } // namespace flutter diff --git a/shell/common/shell.h b/shell/common/shell.h index 97cbe5ef8a976..fe9c018c25060 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -90,6 +90,9 @@ class Shell final : public PlatformView::Delegate, Rasterizer::Screenshot Screenshot(Rasterizer::ScreenshotType type, bool base64_encode); + /// Returns true on timeout. + bool WaitForFrameRender(fml::TimeDelta timeout); + private: using ServiceProtocolHandler = std::function { // Return kUnknownRefreshRateFPS if the refresh rate is unknown. virtual float GetDisplayRefreshRate() const; + virtual void ForceVSync() {} + protected: // On some backends, the |FireCallback| needs to be made from a static C // method. diff --git a/shell/common/vsync_waiter_fallback.cc b/shell/common/vsync_waiter_fallback.cc index fc23c6180a4ff..4698b3a616261 100644 --- a/shell/common/vsync_waiter_fallback.cc +++ b/shell/common/vsync_waiter_fallback.cc @@ -36,4 +36,8 @@ void VsyncWaiterFallback::AwaitVSync() { FireCallback(next, next + kSingleFrameInterval); } +void VsyncWaiterFallback::ForceVSync() { + AwaitVSync(); +} + } // namespace flutter diff --git a/shell/common/vsync_waiter_fallback.h b/shell/common/vsync_waiter_fallback.h index 5226dda019504..8780f91a4ae15 100644 --- a/shell/common/vsync_waiter_fallback.h +++ b/shell/common/vsync_waiter_fallback.h @@ -19,6 +19,8 @@ class VsyncWaiterFallback final : public VsyncWaiter { ~VsyncWaiterFallback() override; + void ForceVSync() override; + private: fml::TimePoint phase_; diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index 89b6db71bc3bc..601f9bf1c6195 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -399,8 +399,9 @@ - (void)viewWillAppear:(BOOL)animated { // Only recreate surface on subsequent appearances when viewport metrics are known. // First time surface creation is done on viewDidLayoutSubviews. - if (_viewportMetrics.physical_width) + if (_viewportMetrics.physical_width) { [self surfaceUpdated:YES]; + } [[_engine.get() lifecycleChannel] sendMessage:@"AppLifecycleState.inactive"]; [super viewWillAppear:animated]; @@ -689,8 +690,21 @@ - (void)viewDidLayoutSubviews { // This must run after updateViewportMetrics so that the surface creation tasks are queued after // the viewport metrics update tasks. - if (firstViewBoundsUpdate) + if (firstViewBoundsUpdate) { [self surfaceUpdated:YES]; + + flutter::Shell& shell = [_engine.get() shell]; + fml::TimeDelta waitTime = +#if NDEBUG + fml::TimeDelta::FromMilliseconds(100); +#else + fml::TimeDelta::FromMilliseconds(200); +#endif + if (shell.WaitForFrameRender(waitTime)) { + FML_LOG(INFO) << "Timeout waiting for first frame. This is possible in debug builds but " + << "should be reported as an error on release builds."; + } + } } - (void)viewSafeAreaInsetsDidChange { diff --git a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h index 29097da8f7ad9..932108097730c 100644 --- a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h +++ b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h @@ -20,6 +20,8 @@ class VsyncWaiterIOS final : public VsyncWaiter { ~VsyncWaiterIOS() override; + void ForceVSync() override; + private: fml::scoped_nsobject client_; diff --git a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm index f16c656c075ef..f87402d96c743 100644 --- a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm +++ b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm @@ -23,6 +23,8 @@ - (void)await; - (void)invalidate; +- (void)forceVSync; + @end namespace flutter { @@ -45,6 +47,10 @@ - (void)invalidate; [client_.get() await]; } +void VsyncWaiterIOS::ForceVSync() { + [client_.get() forceVSync]; +} + } // namespace flutter @implementation VSyncClient { @@ -97,4 +103,8 @@ - (void)dealloc { [super dealloc]; } +- (void)forceVSync { + [self onDisplayLink:display_link_]; +} + @end From 28f23c8126453c47162ecf42dc25660c06e1324b Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 26 Jun 2019 15:19:58 -0700 Subject: [PATCH 02/30] Fixed return result for WaitForFrameRender and added docstring. --- shell/common/shell.cc | 2 +- shell/common/shell.h | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 7a27307d144eb..e6554b8605a6a 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1248,7 +1248,7 @@ bool Shell::WaitForFrameRender(fml::TimeDelta timeout) { bool success = waiting_for_frame_condition_.wait_for( lock, std::chrono::milliseconds(timeout.ToMilliseconds()), [this] { return !waiting_for_frame_; }); - return success; + return !success; } } // namespace flutter diff --git a/shell/common/shell.h b/shell/common/shell.h index fe9c018c25060..b70ff98d11839 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -90,7 +90,8 @@ class Shell final : public PlatformView::Delegate, Rasterizer::Screenshot Screenshot(Rasterizer::ScreenshotType type, bool base64_encode); - /// Returns true on timeout. + /// Schedules a frame to be rendered and waits for it to finish. + ///\returns true when there has been a timeout. bool WaitForFrameRender(fml::TimeDelta timeout); private: From d93a5bc3efb188a1e9ccc9ea573c58a4379b1cf4 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 26 Jun 2019 15:41:01 -0700 Subject: [PATCH 03/30] Fixed formatting. --- fml/task_runner.cc | 4 ++-- shell/common/shell.cc | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/fml/task_runner.cc b/fml/task_runner.cc index bad8412e408b0..672ded8930bc4 100644 --- a/fml/task_runner.cc +++ b/fml/task_runner.cc @@ -8,10 +8,10 @@ #include -#include "flutter/fml/synchronization/count_down_latch.h" #include "flutter/fml/logging.h" #include "flutter/fml/message_loop.h" #include "flutter/fml/message_loop_impl.h" +#include "flutter/fml/synchronization/count_down_latch.h" namespace fml { @@ -26,7 +26,7 @@ void TaskRunner::PostTask(fml::closure task) { void TaskRunner::AwaitTask(fml::closure task) { CountDownLatch latch(1); - PostTask([&]{ + PostTask([&] { task(); latch.CountDown(); }); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index e6554b8605a6a..0130060471704 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1241,9 +1241,8 @@ Rasterizer::Screenshot Shell::Screenshot( bool Shell::WaitForFrameRender(fml::TimeDelta timeout) { std::unique_lock lock(waiting_for_frame_mutex_); - task_runners_.GetUITaskRunner()->AwaitTask([this] { - engine_->GetAnimator()->ForceVSync(); - }); + task_runners_.GetUITaskRunner()->AwaitTask( + [this] { engine_->GetAnimator()->ForceVSync(); }); waiting_for_frame_ = true; bool success = waiting_for_frame_condition_.wait_for( lock, std::chrono::milliseconds(timeout.ToMilliseconds()), From 6b552448a74b1aef4c9a35536e84dada98c5b66a Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Wed, 26 Jun 2019 16:14:14 -0700 Subject: [PATCH 04/30] Made the bool parameter atomic since it was getting written to from multiple threads. --- shell/common/shell.cc | 10 ++++------ shell/common/shell.h | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 0130060471704..d8cf05d661ea6 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -922,12 +922,10 @@ void Shell::OnFrameRasterized(const FrameTiming& timing) { FML_DCHECK(is_setup_); FML_DCHECK(task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread()); - // waiting_for_frame is only modified on this thread so we should be able to - // read from it safely without guards. - if (waiting_for_frame_) { + if (waiting_for_frame_.load()) { { std::lock_guard lock(waiting_for_frame_mutex_); - waiting_for_frame_ = false; + waiting_for_frame_.store(false); } waiting_for_frame_condition_.notify_all(); } @@ -1243,10 +1241,10 @@ bool Shell::WaitForFrameRender(fml::TimeDelta timeout) { std::unique_lock lock(waiting_for_frame_mutex_); task_runners_.GetUITaskRunner()->AwaitTask( [this] { engine_->GetAnimator()->ForceVSync(); }); - waiting_for_frame_ = true; + waiting_for_frame_.store(true); bool success = waiting_for_frame_condition_.wait_for( lock, std::chrono::milliseconds(timeout.ToMilliseconds()), - [this] { return !waiting_for_frame_; }); + [this] { return !waiting_for_frame_.load(); }); return !success; } diff --git a/shell/common/shell.h b/shell/common/shell.h index b70ff98d11839..cfd0f4b64c3b0 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -122,7 +122,7 @@ class Shell final : public PlatformView::Delegate, uint64_t next_pointer_flow_id_ = 0; bool first_frame_rasterized_ = false; - bool waiting_for_frame_ = false; + std::atomic waiting_for_frame_ = false; std::mutex waiting_for_frame_mutex_; std::condition_variable waiting_for_frame_condition_; From d24625369d0273965ef963a27d6c3ce2e6abaa8f Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 1 Jul 2019 11:18:39 -0700 Subject: [PATCH 05/30] Removed AwaitTask since it was controversial and not absolutely necessary (we can shelve that conversation for another day). --- fml/task_runner.cc | 10 ---------- fml/task_runner.h | 2 -- shell/common/shell.cc | 4 ++-- 3 files changed, 2 insertions(+), 14 deletions(-) diff --git a/fml/task_runner.cc b/fml/task_runner.cc index 672ded8930bc4..2c4cfe4b638a2 100644 --- a/fml/task_runner.cc +++ b/fml/task_runner.cc @@ -11,7 +11,6 @@ #include "flutter/fml/logging.h" #include "flutter/fml/message_loop.h" #include "flutter/fml/message_loop_impl.h" -#include "flutter/fml/synchronization/count_down_latch.h" namespace fml { @@ -24,15 +23,6 @@ void TaskRunner::PostTask(fml::closure task) { loop_->PostTask(std::move(task), fml::TimePoint::Now()); } -void TaskRunner::AwaitTask(fml::closure task) { - CountDownLatch latch(1); - PostTask([&] { - task(); - latch.CountDown(); - }); - latch.Wait(); -} - void TaskRunner::PostTaskForTime(fml::closure task, fml::TimePoint target_time) { loop_->PostTask(std::move(task), target_time); diff --git a/fml/task_runner.h b/fml/task_runner.h index 01f52ba924850..72c4219029f34 100644 --- a/fml/task_runner.h +++ b/fml/task_runner.h @@ -21,8 +21,6 @@ class TaskRunner : public fml::RefCountedThreadSafe { virtual void PostTask(fml::closure task); - virtual void AwaitTask(fml::closure task); - virtual void PostTaskForTime(fml::closure task, fml::TimePoint target_time); virtual void PostDelayedTask(fml::closure task, fml::TimeDelta delay); diff --git a/shell/common/shell.cc b/shell/common/shell.cc index d8cf05d661ea6..0043fc7aaaa5d 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1238,9 +1238,9 @@ Rasterizer::Screenshot Shell::Screenshot( } bool Shell::WaitForFrameRender(fml::TimeDelta timeout) { - std::unique_lock lock(waiting_for_frame_mutex_); - task_runners_.GetUITaskRunner()->AwaitTask( + task_runners_.GetUITaskRunner()->PostTask( [this] { engine_->GetAnimator()->ForceVSync(); }); + std::unique_lock lock(waiting_for_frame_mutex_); waiting_for_frame_.store(true); bool success = waiting_for_frame_condition_.wait_for( lock, std::chrono::milliseconds(timeout.ToMilliseconds()), From 700f0622188c4417f07f7ebb135ed55e671bf191 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 1 Jul 2019 11:27:28 -0700 Subject: [PATCH 06/30] Added asserts to avoid deadlock. --- shell/common/shell.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 0043fc7aaaa5d..3907d1e3cc1e8 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1238,6 +1238,8 @@ Rasterizer::Screenshot Shell::Screenshot( } bool Shell::WaitForFrameRender(fml::TimeDelta timeout) { + FML_DCHECK(!task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); + FML_DCHECK(!task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread()); task_runners_.GetUITaskRunner()->PostTask( [this] { engine_->GetAnimator()->ForceVSync(); }); std::unique_lock lock(waiting_for_frame_mutex_); From bc9cd952009cf02af0a8068b65d703e973e802a9 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 1 Jul 2019 11:29:50 -0700 Subject: [PATCH 07/30] Updated documenation for WaitForFrameRender. --- shell/common/shell.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/shell/common/shell.h b/shell/common/shell.h index cfd0f4b64c3b0..439dae42bc71a 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -91,6 +91,8 @@ class Shell final : public PlatformView::Delegate, bool base64_encode); /// Schedules a frame to be rendered and waits for it to finish. + ///\details Don't call this from the GPU thread or the UI thread or you will + /// create a deadlock. ///\returns true when there has been a timeout. bool WaitForFrameRender(fml::TimeDelta timeout); From ac7266b7da6f5607167a630fd289f7e69e159c1c Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 1 Jul 2019 13:59:55 -0700 Subject: [PATCH 08/30] Made the CL much simplier by just waiting for the first frame instead of forcing a vsync. Forcing a vsync didn't help us because we didn't know the dimensions so init wasn't happening. --- shell/common/animator.cc | 7 --- shell/common/animator.h | 2 - shell/common/engine.h | 2 - shell/common/shell.cc | 52 +++++++++---------- shell/common/shell.h | 8 +-- shell/common/vsync_waiter.h | 2 - shell/common/vsync_waiter_fallback.cc | 4 -- shell/common/vsync_waiter_fallback.h | 2 - .../framework/Source/FlutterViewController.mm | 2 +- .../ios/framework/Source/vsync_waiter_ios.h | 2 - .../ios/framework/Source/vsync_waiter_ios.mm | 10 ---- 11 files changed, 30 insertions(+), 63 deletions(-) diff --git a/shell/common/animator.cc b/shell/common/animator.cc index f4216b98e69be..3632476ff6a1b 100644 --- a/shell/common/animator.cc +++ b/shell/common/animator.cc @@ -239,11 +239,4 @@ void Animator::AwaitVSync() { delegate_.OnAnimatorNotifyIdle(dart_frame_deadline_); } -void Animator::ForceVSync() { - regenerate_layer_tree_ = true; - frame_scheduled_ = true; - AwaitVSync(); - waiter_->ForceVSync(); -} - } // namespace flutter diff --git a/shell/common/animator.h b/shell/common/animator.h index d99a97ea3e7d1..9f84468c01b78 100644 --- a/shell/common/animator.h +++ b/shell/common/animator.h @@ -62,8 +62,6 @@ class Animator final { // will be ended during the next |BeginFrame|. void EnqueueTraceFlowId(uint64_t trace_flow_id); - void ForceVSync(); - private: using LayerTreePipeline = Pipeline; diff --git a/shell/common/engine.h b/shell/common/engine.h index e1157017806ff..293d78c1a7178 100644 --- a/shell/common/engine.h +++ b/shell/common/engine.h @@ -127,8 +127,6 @@ class Engine final : public RuntimeDelegate { // |RuntimeDelegate| FontCollection& GetFontCollection() override; - Animator* GetAnimator() { return animator_.get(); } - private: Engine::Delegate& delegate_; const Settings settings_; diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 3907d1e3cc1e8..4ef5c433bc546 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -457,18 +457,21 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { // This is a synchronous operation because certain platforms depend on // setup/suspension of all activities that may be interacting with the GPU in // a synchronous fashion. - fml::AutoResetWaitableEvent latch; - auto gpu_task = fml::MakeCopyable([rasterizer = rasterizer_->GetWeakPtr(), // - surface = std::move(surface), // - &latch]() mutable { - if (rasterizer) { - rasterizer->Setup(std::move(surface)); - } - // Step 3: All done. Signal the latch that the platform thread is waiting - // on. - latch.Signal(); - }); + auto gpu_task = + fml::MakeCopyable([this, rasterizer = rasterizer_->GetWeakPtr(), // + surface = std::move(surface), // + &latch]() mutable { + if (rasterizer) { + rasterizer->Setup(std::move(surface)); + } + + waiting_for_first_frame_.store(true); + + // Step 3: All done. Signal the latch that the platform thread is + // waiting on. + latch.Signal(); + }); // The normal flow executed by this method is that the platform thread is // starting the sequence and waiting on the latch. Later the UI thread posts @@ -790,10 +793,15 @@ void Shell::OnAnimatorDraw(fml::RefPtr> pipeline) { FML_DCHECK(is_setup_); task_runners_.GetGPUTaskRunner()->PostTask( - [rasterizer = rasterizer_->GetWeakPtr(), + [this, rasterizer = rasterizer_->GetWeakPtr(), pipeline = std::move(pipeline)]() { if (rasterizer) { rasterizer->Draw(pipeline); + + if (waiting_for_first_frame_.load()) { + waiting_for_first_frame_.store(false); + waiting_for_first_frame_condition_.notify_all(); + } } }); } @@ -922,14 +930,6 @@ void Shell::OnFrameRasterized(const FrameTiming& timing) { FML_DCHECK(is_setup_); FML_DCHECK(task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread()); - if (waiting_for_frame_.load()) { - { - std::lock_guard lock(waiting_for_frame_mutex_); - waiting_for_frame_.store(false); - } - waiting_for_frame_condition_.notify_all(); - } - // The C++ callback defined in settings.h and set by Flutter runner. This is // independent of the timings report to the Dart side. if (settings_.frame_rasterized_callback) { @@ -1237,16 +1237,14 @@ Rasterizer::Screenshot Shell::Screenshot( return screenshot; } -bool Shell::WaitForFrameRender(fml::TimeDelta timeout) { +bool Shell::WaitForFirstFrame(fml::TimeDelta timeout) { + FML_DCHECK(is_setup_); FML_DCHECK(!task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); FML_DCHECK(!task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread()); - task_runners_.GetUITaskRunner()->PostTask( - [this] { engine_->GetAnimator()->ForceVSync(); }); - std::unique_lock lock(waiting_for_frame_mutex_); - waiting_for_frame_.store(true); - bool success = waiting_for_frame_condition_.wait_for( + std::unique_lock lock(waiting_for_first_frame_mutex_); + bool success = waiting_for_first_frame_condition_.wait_for( lock, std::chrono::milliseconds(timeout.ToMilliseconds()), - [this] { return !waiting_for_frame_.load(); }); + [this] { return !waiting_for_first_frame_.load(); }); return !success; } diff --git a/shell/common/shell.h b/shell/common/shell.h index 439dae42bc71a..753c28aa54776 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -94,7 +94,7 @@ class Shell final : public PlatformView::Delegate, ///\details Don't call this from the GPU thread or the UI thread or you will /// create a deadlock. ///\returns true when there has been a timeout. - bool WaitForFrameRender(fml::TimeDelta timeout); + bool WaitForFirstFrame(fml::TimeDelta timeout); private: using ServiceProtocolHandler = @@ -124,9 +124,9 @@ class Shell final : public PlatformView::Delegate, uint64_t next_pointer_flow_id_ = 0; bool first_frame_rasterized_ = false; - std::atomic waiting_for_frame_ = false; - std::mutex waiting_for_frame_mutex_; - std::condition_variable waiting_for_frame_condition_; + std::atomic waiting_for_first_frame_ = true; + std::mutex waiting_for_first_frame_mutex_; + std::condition_variable waiting_for_first_frame_condition_; // Written in the UI thread and read from the GPU thread. Hence make it // atomic. diff --git a/shell/common/vsync_waiter.h b/shell/common/vsync_waiter.h index dff184667c47a..5e80dd4c09c6b 100644 --- a/shell/common/vsync_waiter.h +++ b/shell/common/vsync_waiter.h @@ -32,8 +32,6 @@ class VsyncWaiter : public std::enable_shared_from_this { // Return kUnknownRefreshRateFPS if the refresh rate is unknown. virtual float GetDisplayRefreshRate() const; - virtual void ForceVSync() {} - protected: // On some backends, the |FireCallback| needs to be made from a static C // method. diff --git a/shell/common/vsync_waiter_fallback.cc b/shell/common/vsync_waiter_fallback.cc index 4698b3a616261..fc23c6180a4ff 100644 --- a/shell/common/vsync_waiter_fallback.cc +++ b/shell/common/vsync_waiter_fallback.cc @@ -36,8 +36,4 @@ void VsyncWaiterFallback::AwaitVSync() { FireCallback(next, next + kSingleFrameInterval); } -void VsyncWaiterFallback::ForceVSync() { - AwaitVSync(); -} - } // namespace flutter diff --git a/shell/common/vsync_waiter_fallback.h b/shell/common/vsync_waiter_fallback.h index 8780f91a4ae15..5226dda019504 100644 --- a/shell/common/vsync_waiter_fallback.h +++ b/shell/common/vsync_waiter_fallback.h @@ -19,8 +19,6 @@ class VsyncWaiterFallback final : public VsyncWaiter { ~VsyncWaiterFallback() override; - void ForceVSync() override; - private: fml::TimePoint phase_; diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index 601f9bf1c6195..66e0b786b5cc9 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -700,7 +700,7 @@ - (void)viewDidLayoutSubviews { #else fml::TimeDelta::FromMilliseconds(200); #endif - if (shell.WaitForFrameRender(waitTime)) { + if (shell.WaitForFirstFrame(waitTime)) { FML_LOG(INFO) << "Timeout waiting for first frame. This is possible in debug builds but " << "should be reported as an error on release builds."; } diff --git a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h index 932108097730c..29097da8f7ad9 100644 --- a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h +++ b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.h @@ -20,8 +20,6 @@ class VsyncWaiterIOS final : public VsyncWaiter { ~VsyncWaiterIOS() override; - void ForceVSync() override; - private: fml::scoped_nsobject client_; diff --git a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm index f87402d96c743..f16c656c075ef 100644 --- a/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm +++ b/shell/platform/darwin/ios/framework/Source/vsync_waiter_ios.mm @@ -23,8 +23,6 @@ - (void)await; - (void)invalidate; -- (void)forceVSync; - @end namespace flutter { @@ -47,10 +45,6 @@ - (void)forceVSync; [client_.get() await]; } -void VsyncWaiterIOS::ForceVSync() { - [client_.get() forceVSync]; -} - } // namespace flutter @implementation VSyncClient { @@ -103,8 +97,4 @@ - (void)dealloc { [super dealloc]; } -- (void)forceVSync { - [self onDisplayLink:display_link_]; -} - @end From 040c1463bfa2509047028d70f1a30abde376c942 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Mon, 1 Jul 2019 14:18:33 -0700 Subject: [PATCH 09/30] Update docstring. --- shell/common/shell.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/common/shell.h b/shell/common/shell.h index 753c28aa54776..d394f1cad5998 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -90,7 +90,7 @@ class Shell final : public PlatformView::Delegate, Rasterizer::Screenshot Screenshot(Rasterizer::ScreenshotType type, bool base64_encode); - /// Schedules a frame to be rendered and waits for it to finish. + /// Pauses the calling thread until the first frame is presented. ///\details Don't call this from the GPU thread or the UI thread or you will /// create a deadlock. ///\returns true when there has been a timeout. From 8d6351826385eda4ca6e572cd59d3d1f22e10565 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 2 Jul 2019 11:31:37 -0700 Subject: [PATCH 10/30] Updated timeout message. --- .../darwin/ios/framework/Source/FlutterViewController.mm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index 6738d0d489e74..d3da9a36c13db 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -710,8 +710,9 @@ - (void)viewDidLayoutSubviews { fml::TimeDelta::FromMilliseconds(200); #endif if (shell.WaitForFirstFrame(waitTime)) { - FML_LOG(INFO) << "Timeout waiting for first frame. This is possible in debug builds but " - << "should be reported as an error on release builds."; + FML_LOG(INFO) << "Timeout waiting for the first frame to render. This may happen in " + << "unoptimized builds. If this is a release build, you should load a less " + << "complex frame to avoid the timeout." } } } From 8a676f703370195a277278f7f53532ca962e1fcd Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 2 Jul 2019 11:34:56 -0700 Subject: [PATCH 11/30] Switched preprocessor macro for determining timeout for first frame. --- .../darwin/ios/framework/Source/FlutterViewController.mm | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index d3da9a36c13db..21c3505fa6f16 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -704,10 +704,10 @@ - (void)viewDidLayoutSubviews { flutter::Shell& shell = [_engine.get() shell]; fml::TimeDelta waitTime = -#if NDEBUG - fml::TimeDelta::FromMilliseconds(100); -#else +#if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG fml::TimeDelta::FromMilliseconds(200); +#else + fml::TimeDelta::FromMilliseconds(100); #endif if (shell.WaitForFirstFrame(waitTime)) { FML_LOG(INFO) << "Timeout waiting for the first frame to render. This may happen in " From 2915fe93f9e47b3d76f39293ede3f6f32e175afa Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 2 Jul 2019 11:48:40 -0700 Subject: [PATCH 12/30] missing semicolon, doh --- .../darwin/ios/framework/Source/FlutterViewController.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index 21c3505fa6f16..cadad4095fc93 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -712,7 +712,7 @@ - (void)viewDidLayoutSubviews { if (shell.WaitForFirstFrame(waitTime)) { FML_LOG(INFO) << "Timeout waiting for the first frame to render. This may happen in " << "unoptimized builds. If this is a release build, you should load a less " - << "complex frame to avoid the timeout." + << "complex frame to avoid the timeout."; } } } From 016c96d27cd3ebb064c3700be362d04b0b3de50a Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 2 Jul 2019 16:05:02 -0700 Subject: [PATCH 13/30] Removed captures of 'this'. --- shell/common/shell.cc | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 13988df8823b8..e686f075ea134 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -459,14 +459,15 @@ void Shell::OnPlatformViewCreated(std::unique_ptr surface) { // a synchronous fashion. fml::AutoResetWaitableEvent latch; auto gpu_task = - fml::MakeCopyable([this, rasterizer = rasterizer_->GetWeakPtr(), // - surface = std::move(surface), // + fml::MakeCopyable([& waiting_for_first_frame = waiting_for_first_frame_, + rasterizer = rasterizer_->GetWeakPtr(), // + surface = std::move(surface), // &latch]() mutable { if (rasterizer) { rasterizer->Setup(std::move(surface)); } - waiting_for_first_frame_.store(true); + waiting_for_first_frame.store(true); // Step 3: All done. Signal the latch that the platform thread is // waiting on. @@ -567,7 +568,7 @@ void Shell::OnPlatformViewDestroyed() { }; // The normal flow executed by this method is that the platform thread is - // starting the sequence and waiting on the latch. Later the UI thread posts + // starting the sequence and w on the latch. Later the UI thread posts // gpu_task to the GPU thread triggers signaling the latch(on the IO thread). // If the GPU the and platform threads are the same this results in a deadlock // as the gpu_task will never be posted to the plaform/gpu thread that is @@ -803,14 +804,16 @@ void Shell::OnAnimatorDraw(fml::RefPtr> pipeline) { FML_DCHECK(is_setup_); task_runners_.GetGPUTaskRunner()->PostTask( - [this, rasterizer = rasterizer_->GetWeakPtr(), + [& waiting_for_first_frame = waiting_for_first_frame_, + &waiting_for_first_frame_condition = waiting_for_first_frame_condition_, + rasterizer = rasterizer_->GetWeakPtr(), pipeline = std::move(pipeline)]() { if (rasterizer) { rasterizer->Draw(pipeline); - if (waiting_for_first_frame_.load()) { - waiting_for_first_frame_.store(false); - waiting_for_first_frame_condition_.notify_all(); + if (waiting_for_first_frame.load()) { + waiting_for_first_frame.store(false); + waiting_for_first_frame_condition.notify_all(); } } }); @@ -1255,7 +1258,9 @@ bool Shell::WaitForFirstFrame(fml::TimeDelta timeout) { std::unique_lock lock(waiting_for_first_frame_mutex_); bool success = waiting_for_first_frame_condition_.wait_for( lock, std::chrono::milliseconds(timeout.ToMilliseconds()), - [this] { return !waiting_for_first_frame_.load(); }); + [& waiting_for_first_frame = waiting_for_first_frame_] { + return !waiting_for_first_frame.load(); + }); return !success; } From d040af8b7f64f3338de8d94b37ee4b529a311db6 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 2 Jul 2019 16:09:51 -0700 Subject: [PATCH 14/30] Added unit test for wait for first frame. --- shell/common/shell_unittests.cc | 52 +++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 0084850533c00..f7ee61ff4ffe8 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -518,5 +518,57 @@ TEST_F(ShellTest, ReportTimingsIsCalledImmediatelyAfterTheFirstFrame) { ASSERT_EQ(timestamps.size(), FrameTiming::kCount); } +TEST_F(ShellTest, WaitForFirstFrame) { + auto settings = CreateSettingsForFixture(); + std::unique_ptr shell = CreateShell(settings); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); + bool result = + shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); + ASSERT_FALSE(result); +} + +TEST_F(ShellTest, WaitForFirstFrameTimeout) { + auto settings = CreateSettingsForFixture(); + std::unique_ptr shell = CreateShell(settings); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + bool result = + shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(10)); + ASSERT_TRUE(result); +} + +TEST_F(ShellTest, WaitForFirstFrameMultiple) { + auto settings = CreateSettingsForFixture(); + std::unique_ptr shell = CreateShell(settings); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); + for (int i = 0; i < 100; ++i) { + bool result = + shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); + ASSERT_FALSE(result); + } +} + } // namespace testing } // namespace flutter From 41406b967844a65d1fa39c10a316705d5ea23981 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 2 Jul 2019 16:15:04 -0700 Subject: [PATCH 15/30] tweaked unit test. --- shell/common/shell_unittests.cc | 9 +++++---- .../darwin/ios/framework/Source/FlutterViewController.mm | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index f7ee61ff4ffe8..176cf44a0a73f 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -546,8 +546,7 @@ TEST_F(ShellTest, WaitForFirstFrameTimeout) { configuration.SetEntrypoint("emptyMain"); RunEngine(shell.get(), std::move(configuration)); - bool result = - shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(10)); + bool result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(10)); ASSERT_TRUE(result); } @@ -563,9 +562,11 @@ TEST_F(ShellTest, WaitForFirstFrameMultiple) { RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); + bool result = + shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); + ASSERT_FALSE(result); for (int i = 0; i < 100; ++i) { - bool result = - shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); + result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1)); ASSERT_FALSE(result); } } diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index cadad4095fc93..cebe226dd0e8d 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -707,7 +707,7 @@ - (void)viewDidLayoutSubviews { #if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG fml::TimeDelta::FromMilliseconds(200); #else - fml::TimeDelta::FromMilliseconds(100); + ; #endif if (shell.WaitForFirstFrame(waitTime)) { FML_LOG(INFO) << "Timeout waiting for the first frame to render. This may happen in " From 6068985b4503a55e81f4c19187f8bf6beedc6fc1 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 2 Jul 2019 16:47:47 -0700 Subject: [PATCH 16/30] Fixed typo --- .../darwin/ios/framework/Source/FlutterViewController.mm | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index cebe226dd0e8d..cadad4095fc93 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -707,7 +707,7 @@ - (void)viewDidLayoutSubviews { #if FLUTTER_RUNTIME_MODE == FLUTTER_RUNTIME_MODE_DEBUG fml::TimeDelta::FromMilliseconds(200); #else - ; + fml::TimeDelta::FromMilliseconds(100); #endif if (shell.WaitForFirstFrame(waitTime)) { FML_LOG(INFO) << "Timeout waiting for the first frame to render. This may happen in " From 1a7c9e17d383b4b379b78ac5bb56c71ae2cd8deb Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 10:07:20 -0700 Subject: [PATCH 17/30] Updated doxygen format to match. --- shell/common/shell.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/shell/common/shell.h b/shell/common/shell.h index e19c588b74cb6..3f92feeb895f9 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -243,10 +243,11 @@ class Shell final : public PlatformView::Delegate, Rasterizer::Screenshot Screenshot(Rasterizer::ScreenshotType type, bool base64_encode); - /// Pauses the calling thread until the first frame is presented. - ///\details Don't call this from the GPU thread or the UI thread or you will - /// create a deadlock. - ///\returns true when there has been a timeout. + //---------------------------------------------------------------------------- + /// @brief Pauses the calling thread until the first frame is presented. + /// @details Don't call this from the GPU thread or the UI thread or you will + /// create a deadlock. + /// @returns true when there has been a timeout. bool WaitForFirstFrame(fml::TimeDelta timeout); private: From 82d19a7467fa546b18dfb4a1ad015e66fe1c31e8 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 10:15:12 -0700 Subject: [PATCH 18/30] updated doxygen format again --- shell/common/shell.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/shell/common/shell.h b/shell/common/shell.h index 3f92feeb895f9..7abf2038af383 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -245,9 +245,12 @@ class Shell final : public PlatformView::Delegate, //---------------------------------------------------------------------------- /// @brief Pauses the calling thread until the first frame is presented. + /// /// @details Don't call this from the GPU thread or the UI thread or you will /// create a deadlock. - /// @returns true when there has been a timeout. + /// + /// @return 'true' when there has been a timeout. + /// bool WaitForFirstFrame(fml::TimeDelta timeout); private: From 9df3a6567ee562faf139441abe60855702fff3c3 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 12:54:40 -0700 Subject: [PATCH 19/30] 1) Added Status class and made WaitForFirstFrame use it 2) Added a unit test for inlined threading model. --- fml/status.h | 88 +++++++++++++++++++ shell/common/shell.cc | 16 +++- shell/common/shell.h | 3 +- shell/common/shell_unittests.cc | 46 ++++++++-- .../framework/Source/FlutterViewController.mm | 2 +- 5 files changed, 142 insertions(+), 13 deletions(-) create mode 100644 fml/status.h diff --git a/fml/status.h b/fml/status.h new file mode 100644 index 0000000000000..ba9b43c16fe3d --- /dev/null +++ b/fml/status.h @@ -0,0 +1,88 @@ +// Copyright 2013 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef FLUTTER_FML_STATUS_H_ +#define FLUTTER_FML_STATUS_H_ + +#include "flutter/fml/string_view.h" + +namespace fml { + +/// Google's canonical list of errors, should be in sync with: +/// https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto +enum class StatusCode : int { + kOk = 0, + kCancelled = 1, + kUnknown = 2, + kInvalidArgument = 3, + kDeadlineExceeded = 4, + kNotFound = 5, + kAlreadyExists = 6, + kPermissionDenied = 7, + kResourceExhausted = 8, + kFailedPrecondition = 9, + kAborted = 10, + kOutOfRange = 11, + kUnimplemented = 12, + kInternal = 13, + kUnavailable = 14, + kDataLoss = 15, + kUnauthenticated = 16, + kDoNotUseReservedForFutureExpansionUseDefaultInSwitchInstead_ = 20 +}; + +class Status final { + public: + /// Creates an 'ok' status. + Status(); + + Status(fml::StatusCode code, fml::StringView message); + + fml::StatusCode code() const; + + int raw_code() const; + + /// A noop that helps with static analysis tools if you decide to ignore an + /// error. + void IgnoreError() const; + + /// @return 'true' when the code is kOk. + bool ok() const; + + fml::StringView message() const; + + private: + int code_; + fml::StringView message_; +}; + +inline Status::Status() + : code_(static_cast(fml::StatusCode::kOk)), message_() {} + +inline Status::Status(fml::StatusCode code, fml::StringView message) + : code_(static_cast(code)), message_(message) {} + +inline fml::StatusCode Status::code() const { + return static_cast(code_); +} + +inline int Status::raw_code() const { + return code_; +} + +inline void Status::IgnoreError() const { + // noop +} + +inline bool Status::ok() const { + return code_ == static_cast(fml::StatusCode::kOk); +} + +inline fml::StringView Status::message() const { + return message_; +} + +} // namespace fml + +#endif // FLUTTER_FML_SIZE_H_ diff --git a/shell/common/shell.cc b/shell/common/shell.cc index e686f075ea134..7a06f5f18ad40 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -1251,17 +1251,25 @@ Rasterizer::Screenshot Shell::Screenshot( return screenshot; } -bool Shell::WaitForFirstFrame(fml::TimeDelta timeout) { +fml::Status Shell::WaitForFirstFrame(fml::TimeDelta timeout) { FML_DCHECK(is_setup_); - FML_DCHECK(!task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); - FML_DCHECK(!task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread()); + if (task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()) { + return fml::Status(fml::StatusCode::kFailedPrecondition, + "WaitForFirstFrame called from thread that can't wait " + "because it is responsible for generating the frame."); + } + std::unique_lock lock(waiting_for_first_frame_mutex_); bool success = waiting_for_first_frame_condition_.wait_for( lock, std::chrono::milliseconds(timeout.ToMilliseconds()), [& waiting_for_first_frame = waiting_for_first_frame_] { return !waiting_for_first_frame.load(); }); - return !success; + if (success) { + return fml::Status(); + } else { + return fml::Status(fml::StatusCode::kDeadlineExceeded, "timeout"); + } } } // namespace flutter diff --git a/shell/common/shell.h b/shell/common/shell.h index 7abf2038af383..493881bdb6e46 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -16,6 +16,7 @@ #include "flutter/fml/memory/ref_ptr.h" #include "flutter/fml/memory/thread_checker.h" #include "flutter/fml/memory/weak_ptr.h" +#include "flutter/fml/status.h" #include "flutter/fml/string_view.h" #include "flutter/fml/synchronization/thread_annotations.h" #include "flutter/fml/synchronization/waitable_event.h" @@ -251,7 +252,7 @@ class Shell final : public PlatformView::Delegate, /// /// @return 'true' when there has been a timeout. /// - bool WaitForFirstFrame(fml::TimeDelta timeout); + fml::Status WaitForFirstFrame(fml::TimeDelta timeout); private: using ServiceProtocolHandler = diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 176cf44a0a73f..9dea7f5e0f002 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -530,9 +530,9 @@ TEST_F(ShellTest, WaitForFirstFrame) { RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); - bool result = + fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); - ASSERT_FALSE(result); + ASSERT_TRUE(result.ok()); } TEST_F(ShellTest, WaitForFirstFrameTimeout) { @@ -546,8 +546,9 @@ TEST_F(ShellTest, WaitForFirstFrameTimeout) { configuration.SetEntrypoint("emptyMain"); RunEngine(shell.get(), std::move(configuration)); - bool result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(10)); - ASSERT_TRUE(result); + fml::Status result = + shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(10)); + ASSERT_EQ(result.code(), fml::StatusCode::kDeadlineExceeded); } TEST_F(ShellTest, WaitForFirstFrameMultiple) { @@ -562,14 +563,45 @@ TEST_F(ShellTest, WaitForFirstFrameMultiple) { RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); - bool result = + fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); - ASSERT_FALSE(result); + ASSERT_TRUE(result.ok()); for (int i = 0; i < 100; ++i) { result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1)); - ASSERT_FALSE(result); + ASSERT_TRUE(result.ok()); } } +// /// Makes sure that WaitForFirstFrame works if we rendered a frame with the +// /// single-thread setup. +TEST_F(ShellTest, WaitForFirstFrameInlined) { + Settings settings = CreateSettingsForFixture(); + ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", + ThreadHost::Type::Platform | ThreadHost::Type::GPU | + ThreadHost::Type::IO | ThreadHost::Type::UI); + auto task_runner = thread_host.platform_thread->GetTaskRunner(); + TaskRunners task_runners("test", task_runner, task_runner, task_runner, + task_runner); + std::unique_ptr shell = + CreateShell(std::move(settings), std::move(task_runners)); + + // Create the surface needed by rasterizer + PlatformViewNotifyCreated(shell.get()); + + auto configuration = RunConfiguration::InferFromSettings(settings); + configuration.SetEntrypoint("emptyMain"); + + RunEngine(shell.get(), std::move(configuration)); + PumpOneFrame(shell.get()); + fml::AutoResetWaitableEvent event; + thread_host.platform_thread->GetTaskRunner()->PostTask([&shell, &event] { + fml::Status result = + shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); + ASSERT_EQ(result.code(), fml::StatusCode::kFailedPrecondition); + event.Signal(); + }); + event.Wait(); +} + } // namespace testing } // namespace flutter diff --git a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm index cadad4095fc93..99c1920e4e43f 100644 --- a/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm +++ b/shell/platform/darwin/ios/framework/Source/FlutterViewController.mm @@ -709,7 +709,7 @@ - (void)viewDidLayoutSubviews { #else fml::TimeDelta::FromMilliseconds(100); #endif - if (shell.WaitForFirstFrame(waitTime)) { + if (shell.WaitForFirstFrame(waitTime).code() == fml::StatusCode::kDeadlineExceeded) { FML_LOG(INFO) << "Timeout waiting for the first frame to render. This may happen in " << "unoptimized builds. If this is a release build, you should load a less " << "complex frame to avoid the timeout."; From b9d4c14d9feeb3a18b00b9fad4e60d9ec0d7157b Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 13:26:32 -0700 Subject: [PATCH 20/30] Added timeout to unit test. --- shell/common/shell_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index 9dea7f5e0f002..e70a87fac9be5 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -600,7 +600,7 @@ TEST_F(ShellTest, WaitForFirstFrameInlined) { ASSERT_EQ(result.code(), fml::StatusCode::kFailedPrecondition); event.Signal(); }); - event.Wait(); + ASSERT_FALSE(event.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(1000))); } } // namespace testing From b6d35b5114dcd0e7452e2f3a26f89b1b579f1575 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 13:29:06 -0700 Subject: [PATCH 21/30] Added class docstring --- fml/status.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fml/status.h b/fml/status.h index ba9b43c16fe3d..9724d2b31578d 100644 --- a/fml/status.h +++ b/fml/status.h @@ -32,6 +32,9 @@ enum class StatusCode : int { kDoNotUseReservedForFutureExpansionUseDefaultInSwitchInstead_ = 20 }; +/// Class that represents the resolution of the execution of a procedure. This +/// is used similarly to how exceptions might be used, typically as the return +/// value to a synchronous procedure or an argument to a asynchronous callback. class Status final { public: /// Creates an 'ok' status. From 4e256249355f1b26600ad74907dc34300da26a6f Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 13:30:53 -0700 Subject: [PATCH 22/30] grammar typo --- fml/status.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fml/status.h b/fml/status.h index 9724d2b31578d..0286c74b06ecc 100644 --- a/fml/status.h +++ b/fml/status.h @@ -34,7 +34,7 @@ enum class StatusCode : int { /// Class that represents the resolution of the execution of a procedure. This /// is used similarly to how exceptions might be used, typically as the return -/// value to a synchronous procedure or an argument to a asynchronous callback. +/// value to a synchronous procedure or an argument to an asynchronous callback. class Status final { public: /// Creates an 'ok' status. From 2b88ff84fa8d592ccb1c96ebb6525fbf449c776d Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 13:34:39 -0700 Subject: [PATCH 23/30] Fixed typos --- shell/common/shell.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/shell/common/shell.cc b/shell/common/shell.cc index 7a06f5f18ad40..2ac7119af96a0 100644 --- a/shell/common/shell.cc +++ b/shell/common/shell.cc @@ -568,7 +568,7 @@ void Shell::OnPlatformViewDestroyed() { }; // The normal flow executed by this method is that the platform thread is - // starting the sequence and w on the latch. Later the UI thread posts + // starting the sequence and waiting on the latch. Later the UI thread posts // gpu_task to the GPU thread triggers signaling the latch(on the IO thread). // If the GPU the and platform threads are the same this results in a deadlock // as the gpu_task will never be posted to the plaform/gpu thread that is @@ -1253,7 +1253,8 @@ Rasterizer::Screenshot Shell::Screenshot( fml::Status Shell::WaitForFirstFrame(fml::TimeDelta timeout) { FML_DCHECK(is_setup_); - if (task_runners_.GetPlatformTaskRunner()->RunsTasksOnCurrentThread()) { + if (task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread() || + task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread()) { return fml::Status(fml::StatusCode::kFailedPrecondition, "WaitForFirstFrame called from thread that can't wait " "because it is responsible for generating the frame."); From f0b3a402e9271d65f9088ee2296c829e9bb7b563 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 13:45:50 -0700 Subject: [PATCH 24/30] Added status to the license golden. --- ci/licenses_golden/licenses_flutter | 1 + 1 file changed, 1 insertion(+) diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 1418af878d6d9..fa0091c1b8ae2 100644 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -187,6 +187,7 @@ FILE: ../../../flutter/fml/platform/win/native_library_win.cc FILE: ../../../flutter/fml/platform/win/paths_win.cc FILE: ../../../flutter/fml/platform/win/wstring_conversion.h FILE: ../../../flutter/fml/size.h +FILE: ../../../flutter/fml/status.h FILE: ../../../flutter/fml/string_view.cc FILE: ../../../flutter/fml/string_view.h FILE: ../../../flutter/fml/string_view_unittest.cc From fdfa45cf872c2df8eddee15d053a026eaf5716a2 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 13:50:07 -0700 Subject: [PATCH 25/30] Updated the docstring to update the Status returns. --- shell/common/shell.h | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/shell/common/shell.h b/shell/common/shell.h index 493881bdb6e46..c9b2214775135 100644 --- a/shell/common/shell.h +++ b/shell/common/shell.h @@ -247,10 +247,9 @@ class Shell final : public PlatformView::Delegate, //---------------------------------------------------------------------------- /// @brief Pauses the calling thread until the first frame is presented. /// - /// @details Don't call this from the GPU thread or the UI thread or you will - /// create a deadlock. - /// - /// @return 'true' when there has been a timeout. + /// @return 'kOk' when the first frame has been presented before the timeout + /// successfully, 'kFailedPrecondition' if called from the GPU or UI + /// thread, 'kDeadlineExceeded' if there is a timeout. /// fml::Status WaitForFirstFrame(fml::TimeDelta timeout); From aa128528d48a20f7e426060b3a16ac5d9d576ac9 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 15:14:53 -0700 Subject: [PATCH 26/30] Fixed comment formatting. --- shell/common/shell_unittests.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index e70a87fac9be5..ecd1fca8c8263 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -572,8 +572,8 @@ TEST_F(ShellTest, WaitForFirstFrameMultiple) { } } -// /// Makes sure that WaitForFirstFrame works if we rendered a frame with the -// /// single-thread setup. +/// Makes sure that WaitForFirstFrame works if we rendered a frame with the +/// single-thread setup. TEST_F(ShellTest, WaitForFirstFrameInlined) { Settings settings = CreateSettingsForFixture(); ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", From ad944f3c38642de03594bc622b007457cad4eea2 Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 16:46:19 -0700 Subject: [PATCH 27/30] Moved to std::string_view. --- fml/status.h | 12 ++++++------ fml/string_view.h | 3 ++- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/fml/status.h b/fml/status.h index 0286c74b06ecc..064fe8449ccd7 100644 --- a/fml/status.h +++ b/fml/status.h @@ -5,7 +5,7 @@ #ifndef FLUTTER_FML_STATUS_H_ #define FLUTTER_FML_STATUS_H_ -#include "flutter/fml/string_view.h" +#include namespace fml { @@ -40,7 +40,7 @@ class Status final { /// Creates an 'ok' status. Status(); - Status(fml::StatusCode code, fml::StringView message); + Status(fml::StatusCode code, std::string_view message); fml::StatusCode code() const; @@ -53,17 +53,17 @@ class Status final { /// @return 'true' when the code is kOk. bool ok() const; - fml::StringView message() const; + std::string_view message() const; private: int code_; - fml::StringView message_; + std::string_view message_; }; inline Status::Status() : code_(static_cast(fml::StatusCode::kOk)), message_() {} -inline Status::Status(fml::StatusCode code, fml::StringView message) +inline Status::Status(fml::StatusCode code, std::string_view message) : code_(static_cast(code)), message_(message) {} inline fml::StatusCode Status::code() const { @@ -82,7 +82,7 @@ inline bool Status::ok() const { return code_ == static_cast(fml::StatusCode::kOk); } -inline fml::StringView Status::message() const { +inline std::string_view Status::message() const { return message_; } diff --git a/fml/string_view.h b/fml/string_view.h index 226b84f21479a..855a2b861f8c1 100644 --- a/fml/string_view.h +++ b/fml/string_view.h @@ -21,7 +21,8 @@ namespace fml { -// A string-like object that points to a sized piece of memory. +/// A string-like object that points to a sized piece of memory. +/// @deprecated used std::string_view class StringView { public: // Types. From d55bf99c22104075af4c81f57a4427a0d410b2ca Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 16:52:05 -0700 Subject: [PATCH 28/30] Removed raw_code from Status. --- fml/status.h | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/fml/status.h b/fml/status.h index 064fe8449ccd7..3f584f24830c5 100644 --- a/fml/status.h +++ b/fml/status.h @@ -44,8 +44,6 @@ class Status final { fml::StatusCode code() const; - int raw_code() const; - /// A noop that helps with static analysis tools if you decide to ignore an /// error. void IgnoreError() const; @@ -56,21 +54,16 @@ class Status final { std::string_view message() const; private: - int code_; + fml::StatusCode code_; std::string_view message_; }; -inline Status::Status() - : code_(static_cast(fml::StatusCode::kOk)), message_() {} +inline Status::Status() : code_(fml::StatusCode::kOk), message_() {} inline Status::Status(fml::StatusCode code, std::string_view message) - : code_(static_cast(code)), message_(message) {} + : code_(code), message_(message) {} inline fml::StatusCode Status::code() const { - return static_cast(code_); -} - -inline int Status::raw_code() const { return code_; } @@ -79,7 +72,7 @@ inline void Status::IgnoreError() const { } inline bool Status::ok() const { - return code_ == static_cast(fml::StatusCode::kOk); + return code_ == fml::StatusCode::kOk; } inline std::string_view Status::message() const { From f7e2b8c4d5deed099093889fc4f655d9ca1abeaa Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 17:03:01 -0700 Subject: [PATCH 29/30] Removed thread_host from test. --- shell/common/shell_unittests.cc | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/shell/common/shell_unittests.cc b/shell/common/shell_unittests.cc index ecd1fca8c8263..45d2a658e2707 100644 --- a/shell/common/shell_unittests.cc +++ b/shell/common/shell_unittests.cc @@ -576,10 +576,7 @@ TEST_F(ShellTest, WaitForFirstFrameMultiple) { /// single-thread setup. TEST_F(ShellTest, WaitForFirstFrameInlined) { Settings settings = CreateSettingsForFixture(); - ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", - ThreadHost::Type::Platform | ThreadHost::Type::GPU | - ThreadHost::Type::IO | ThreadHost::Type::UI); - auto task_runner = thread_host.platform_thread->GetTaskRunner(); + auto task_runner = GetThreadTaskRunner(); TaskRunners task_runners("test", task_runner, task_runner, task_runner, task_runner); std::unique_ptr shell = @@ -594,7 +591,7 @@ TEST_F(ShellTest, WaitForFirstFrameInlined) { RunEngine(shell.get(), std::move(configuration)); PumpOneFrame(shell.get()); fml::AutoResetWaitableEvent event; - thread_host.platform_thread->GetTaskRunner()->PostTask([&shell, &event] { + task_runner->PostTask([&shell, &event] { fml::Status result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); ASSERT_EQ(result.code(), fml::StatusCode::kFailedPrecondition); From 9c3b023e38b9b10f17f25f2943c66d014b6f39ca Mon Sep 17 00:00:00 2001 From: Aaron Clarke Date: Tue, 9 Jul 2019 17:11:56 -0700 Subject: [PATCH 30/30] Removed enumeration values for StatusCodes. --- fml/status.h | 39 ++++++++++++++++++--------------------- 1 file changed, 18 insertions(+), 21 deletions(-) diff --git a/fml/status.h b/fml/status.h index 3f584f24830c5..8eed10823f144 100644 --- a/fml/status.h +++ b/fml/status.h @@ -9,27 +9,24 @@ namespace fml { -/// Google's canonical list of errors, should be in sync with: -/// https://github.com/googleapis/googleapis/blob/master/google/rpc/code.proto -enum class StatusCode : int { - kOk = 0, - kCancelled = 1, - kUnknown = 2, - kInvalidArgument = 3, - kDeadlineExceeded = 4, - kNotFound = 5, - kAlreadyExists = 6, - kPermissionDenied = 7, - kResourceExhausted = 8, - kFailedPrecondition = 9, - kAborted = 10, - kOutOfRange = 11, - kUnimplemented = 12, - kInternal = 13, - kUnavailable = 14, - kDataLoss = 15, - kUnauthenticated = 16, - kDoNotUseReservedForFutureExpansionUseDefaultInSwitchInstead_ = 20 +enum class StatusCode { + kOk, + kCancelled, + kUnknown, + kInvalidArgument, + kDeadlineExceeded, + kNotFound, + kAlreadyExists, + kPermissionDenied, + kResourceExhausted, + kFailedPrecondition, + kAborted, + kOutOfRange, + kUnimplemented, + kInternal, + kUnavailable, + kDataLoss, + kUnauthenticated }; /// Class that represents the resolution of the execution of a procedure. This