-
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 26 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 |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| // 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 { | ||
|
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. We aren't going to be stuffing these inside messages sent over the wire. This enum doesn't need to be typed.
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. The spec for Status specifically requires these are int's because of the contract of things like raw_code()
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. We don't need
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. Well I was thinking it would be helpful if someone wanted to define their own error code. Maybe we don't want that, I removed it. |
||
| 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 | ||
|
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. This does not apply to us and none of them require values.
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. There is no reason to go out of our way to be incompatible with absl. These values are just as good as anything else, no?
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'd rather not have FML become an agglomeration of things from absl. This was already how some stuff from Base and FXL got in this library with (in some cases) limited use. Let's build and use only the things we need.
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. K, done. |
||
| }; | ||
|
|
||
| /// 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 an asynchronous callback. | ||
| class Status final { | ||
| public: | ||
| /// Creates an 'ok' status. | ||
| Status(); | ||
|
|
||
| Status(fml::StatusCode code, fml::StringView message); | ||
|
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.
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, added deprecated note to StringView as well. |
||
|
|
||
| fml::StatusCode code() const; | ||
|
gaaclarke marked this conversation as resolved.
|
||
|
|
||
| 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_; | ||
|
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. Just the enum 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. Done.
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::StringView message_; | ||
| }; | ||
|
|
||
| inline Status::Status() | ||
|
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. All these can be inlined directly in the class.
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. Acknowledged. |
||
| : code_(static_cast<int>(fml::StatusCode::kOk)), message_() {} | ||
|
|
||
| inline Status::Status(fml::StatusCode code, fml::StringView message) | ||
| : code_(static_cast<int>(code)), message_(message) {} | ||
|
|
||
| inline fml::StatusCode Status::code() const { | ||
| return static_cast<fml::StatusCode>(code_); | ||
| } | ||
|
|
||
| inline int Status::raw_code() const { | ||
| return code_; | ||
| } | ||
|
|
||
| inline void Status::IgnoreError() const { | ||
| // noop | ||
| } | ||
|
|
||
| inline bool Status::ok() const { | ||
| return code_ == static_cast<int>(fml::StatusCode::kOk); | ||
| } | ||
|
|
||
| inline fml::StringView Status::message() const { | ||
| return message_; | ||
| } | ||
|
|
||
| } // namespace fml | ||
|
|
||
| #endif // FLUTTER_FML_SIZE_H_ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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" | ||
|
|
@@ -243,6 +244,15 @@ class Shell final : public PlatformView::Delegate, | |
| Rasterizer::Screenshot Screenshot(Rasterizer::ScreenshotType type, | ||
| bool base64_encode); | ||
|
|
||
| //---------------------------------------------------------------------------- | ||
| /// @brief Pauses the calling thread until the first frame is presented. | ||
| /// | ||
| /// @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); | ||
|
|
||
| private: | ||
| using ServiceProtocolHandler = | ||
| std::function<bool(const ServiceProtocol::Handler::ServiceProtocolMap&, | ||
|
|
@@ -271,6 +281,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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -518,5 +518,90 @@ TEST_F(ShellTest, ReportTimingsIsCalledImmediatelyAfterTheFirstFrame) { | |
| ASSERT_EQ(timestamps.size(), FrameTiming::kCount); | ||
| } | ||
|
|
||
| TEST_F(ShellTest, WaitForFirstFrame) { | ||
|
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. Please create a unit test for a shell in a single threaded configuration as well as one in which the platform and GPU task runners are the same. The former configuration is used by the test runners and the latter by iOS when there is a platform view composited inline.
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. |
||
| auto settings = CreateSettingsForFixture(); | ||
| std::unique_ptr<Shell> 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()); | ||
| fml::Status result = | ||
| shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); | ||
| ASSERT_TRUE(result.ok()); | ||
| } | ||
|
|
||
| TEST_F(ShellTest, WaitForFirstFrameTimeout) { | ||
| auto settings = CreateSettingsForFixture(); | ||
| std::unique_ptr<Shell> 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)); | ||
| fml::Status result = | ||
| shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(10)); | ||
| ASSERT_EQ(result.code(), fml::StatusCode::kDeadlineExceeded); | ||
| } | ||
|
|
||
| TEST_F(ShellTest, WaitForFirstFrameMultiple) { | ||
| auto settings = CreateSettingsForFixture(); | ||
| std::unique_ptr<Shell> 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()); | ||
| fml::Status result = | ||
| shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000)); | ||
| ASSERT_TRUE(result.ok()); | ||
| for (int i = 0; i < 100; ++i) { | ||
| result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1)); | ||
| ASSERT_TRUE(result.ok()); | ||
| } | ||
| } | ||
|
|
||
| // /// Makes sure that WaitForFirstFrame works if we rendered a frame with the | ||
|
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. Inconsistent single line comment prefixes.
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. |
||
| // /// single-thread setup. | ||
| TEST_F(ShellTest, WaitForFirstFrameInlined) { | ||
| Settings settings = CreateSettingsForFixture(); | ||
| ThreadHost thread_host("io.flutter.test." + GetCurrentTestName() + ".", | ||
|
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: You can get rid of the thread host and use GetThreadTaskRunner directly in this unit-test.
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 seems inconsistent with the other tests but done. |
||
| 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> 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(); | ||
| }); | ||
| ASSERT_FALSE(event.WaitWithTimeout(fml::TimeDelta::FromMilliseconds(1000))); | ||
| } | ||
|
|
||
| } // namespace testing | ||
| } // namespace flutter | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there should be any expectation that these should be in sync with grpc. We don't want folks converting the raw code from the status objects we use in the engine and send them over the wire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done