-
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 13 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 |
|---|---|---|
|
|
@@ -457,18 +457,21 @@ void Shell::OnPlatformViewCreated(std::unique_ptr<Surface> 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 | ||
|
|
@@ -800,10 +803,15 @@ void Shell::OnAnimatorDraw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline) { | |
| FML_DCHECK(is_setup_); | ||
|
|
||
| task_runners_.GetGPUTaskRunner()->PostTask( | ||
| [rasterizer = rasterizer_->GetWeakPtr(), | ||
| [this, rasterizer = rasterizer_->GetWeakPtr(), | ||
|
gaaclarke marked this conversation as resolved.
Outdated
|
||
| 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(); | ||
| } | ||
| } | ||
| }); | ||
| } | ||
|
|
@@ -1240,4 +1248,15 @@ Rasterizer::Screenshot Shell::Screenshot( | |
| return screenshot; | ||
| } | ||
|
|
||
| bool Shell::WaitForFirstFrame(fml::TimeDelta timeout) { | ||
|
gaaclarke marked this conversation as resolved.
Outdated
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 believe this will deadlock on iOS when there is a platform view composited inline. In that configuration, the GPU task runners and the platform task runners are the same. The DCHECKs will also trip in that case.
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. Done. |
||
| FML_DCHECK(is_setup_); | ||
| FML_DCHECK(!task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread()); | ||
|
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. Can we ensure that this only gets called on the platform task runner?
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. That is a bit fragile and not a technical requirement, it may interfere with non-standard threading models. I've switched up the checks as part of your your other feedback. |
||
| FML_DCHECK(!task_runners_.GetGPUTaskRunner()->RunsTasksOnCurrentThread()); | ||
| std::unique_lock<std::mutex> 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(); }); | ||
| 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 |
|---|---|---|
|
|
@@ -243,6 +243,12 @@ 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 | ||
|
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. Can you stick to the doxygen commenting scheme followed in the rest of this file. I think we may need a formatting reference for such comments as well. I just picked one that looked good to me but we should gather/document consensus on a format.
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. Done, that style didn't exist when I originally wrote this. |
||
| /// create a deadlock. | ||
| ///\returns true when there has been a timeout. | ||
| bool WaitForFirstFrame(fml::TimeDelta timeout); | ||
|
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. In this case, the return type is hard to interpret (I initially expected true meant it waited). I suggest a enum return value.
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
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've added a Status class as discuss offline. |
||
|
|
||
| private: | ||
| using ServiceProtocolHandler = | ||
| std::function<bool(const ServiceProtocol::Handler::ServiceProtocolMap&, | ||
|
|
@@ -271,6 +277,9 @@ class Shell final : public PlatformView::Delegate, | |
| uint64_t next_pointer_flow_id_ = 0; | ||
|
|
||
| bool first_frame_rasterized_ = false; | ||
| std::atomic<bool> waiting_for_first_frame_ = true; | ||
|
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. Nit:
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. std::atomic<bool> is used later in the file. |
||
| 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. | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.