-
Notifications
You must be signed in to change notification settings - Fork 6k
Synchronize main thread and gpu thread for first render frame #9506
Changes from 4 commits
5aa9df6
28f23c8
d93a5bc
6b55244
d246253
700f062
bc9cd95
ac7266b
040c146
48db249
8d63518
8a676f7
2915fe9
016c96d
d040af8
41406b9
6068985
1a7c9e1
82d19a7
9df3a65
b9d4c14
b6d35b5
4e25624
2b88ff8
f0b3a40
fdfa45c
aa12852
ad944f3
d55bf99
f7e2b8c
9c3b023
e713db8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -922,6 +922,14 @@ void Shell::OnFrameRasterized(const FrameTiming& timing) { | |
| FML_DCHECK(is_setup_); | ||
| FML_DCHECK(task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread()); | ||
|
|
||
| if (waiting_for_frame_.load()) { | ||
| { | ||
| std::lock_guard<std::mutex> 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) { | ||
|
|
@@ -1229,4 +1237,15 @@ Rasterizer::Screenshot Shell::Screenshot( | |
| return screenshot; | ||
| } | ||
|
|
||
| bool Shell::WaitForFrameRender(fml::TimeDelta timeout) { | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| std::unique_lock<std::mutex> lock(waiting_for_frame_mutex_); | ||
| task_runners_.GetUITaskRunner()->AwaitTask( | ||
| [this] { engine_->GetAnimator()->ForceVSync(); }); | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| waiting_for_frame_.store(true); | ||
| bool success = waiting_for_frame_condition_.wait_for( | ||
| lock, std::chrono::milliseconds(timeout.ToMilliseconds()), | ||
| [this] { return !waiting_for_frame_.load(); }); | ||
| return !success; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks to the tests added, I finally realized that this returns This is a little counter-intuitive as most
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to agree, but I was being consistent with the rest of our API: https://github.com/flutter/engine/blob/7f4f52f95294544a2970e514d163b9791efa9147/fml/synchronization/waitable_event.h#L58
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good finding. I think we probably should change that return type to enum too to reduce confusions.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, i've added the Status class as discussed. |
||
| } | ||
|
|
||
| } // namespace flutter | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) { | ||
|
gaaclarke marked this conversation as resolved.
|
||
| [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); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we introducing a new race condition by putting these timeouts here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tvolkert The current state of affairs is we issue the rendering of a frame asynchronously and we don't pay attention to when it is finished and plow right along on the main thread. That is what is causing the first frame to be black. We should be issuing the async render and actually paying attention to when it is done before the main thread continues on. That's what this is doing. The reason for the timeout is not because I'm afraid of deadlock, it's because our renderer is inefficient to the point where I'm saying "If rendering the first frame takes over 100ms it's better to show a black frame than to make the main thread wait any longer" We have outstanding work to do to make the first frame render faster. Right now we've gated all our initialization logic on knowing the dimensions of the surface we are rendering to, which is arbitrary and inefficient. In addition to this synchronization we should decouple that initialization that needs dimensions vs initialization that doesn't need dimensions so we can make the first frame even faster. Right now first frame is ~2.5x slower that the second frame. Even when we optimize that, we will still need to synchronize. We can't just fire off an event and not care about when it is finished.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm curious about the difference between:
To me, they seem equivalent. In both cases, the user won't be able to see any meaningful pixels or interact with the app at 100ms.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @liyuqian Both aren't desirable. Having the timeout is safer since in iOS if you occupy the main thread for too long the operating system will kill your app. Also, in the case where there is a bug that makes the attempt to request a frame ignored we won't get deadlocked. |
||
| #else | ||
| fml::TimeDelta::FromMilliseconds(200); | ||
| #endif | ||
| if (shell.WaitForFrameRender(waitTime)) { | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| 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 { | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.