Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
5aa9df6
Got rid of the black frame by synchronizing the main thread with the
gaaclarke Jun 18, 2019
28f23c8
Fixed return result for WaitForFrameRender and added docstring.
gaaclarke Jun 26, 2019
d93a5bc
Fixed formatting.
gaaclarke Jun 26, 2019
6b55244
Made the bool parameter atomic since it was getting written to from
gaaclarke Jun 26, 2019
d246253
Removed AwaitTask since it was controversial and not absolutely
gaaclarke Jul 1, 2019
700f062
Added asserts to avoid deadlock.
gaaclarke Jul 1, 2019
bc9cd95
Updated documenation for WaitForFrameRender.
gaaclarke Jul 1, 2019
ac7266b
Made the CL much simplier by just waiting for the first frame instead
gaaclarke Jul 1, 2019
040c146
Update docstring.
gaaclarke Jul 1, 2019
48db249
Merge branch 'master' into black-frame2
gaaclarke Jul 2, 2019
8d63518
Updated timeout message.
gaaclarke Jul 2, 2019
8a676f7
Switched preprocessor macro for determining timeout for first frame.
gaaclarke Jul 2, 2019
2915fe9
missing semicolon, doh
gaaclarke Jul 2, 2019
016c96d
Removed captures of 'this'.
gaaclarke Jul 2, 2019
d040af8
Added unit test for wait for first frame.
gaaclarke Jul 2, 2019
41406b9
tweaked unit test.
gaaclarke Jul 2, 2019
6068985
Fixed typo
gaaclarke Jul 2, 2019
1a7c9e1
Updated doxygen format to match.
gaaclarke Jul 9, 2019
82d19a7
updated doxygen format again
gaaclarke Jul 9, 2019
9df3a65
1) Added Status class and made WaitForFirstFrame use it
gaaclarke Jul 9, 2019
b9d4c14
Added timeout to unit test.
gaaclarke Jul 9, 2019
b6d35b5
Added class docstring
gaaclarke Jul 9, 2019
4e25624
grammar typo
gaaclarke Jul 9, 2019
2b88ff8
Fixed typos
gaaclarke Jul 9, 2019
f0b3a40
Added status to the license golden.
gaaclarke Jul 9, 2019
fdfa45c
Updated the docstring to update the Status returns.
gaaclarke Jul 9, 2019
aa12852
Fixed comment formatting.
gaaclarke Jul 9, 2019
ad944f3
Moved to std::string_view.
gaaclarke Jul 9, 2019
d55bf99
Removed raw_code from Status.
gaaclarke Jul 9, 2019
f7e2b8c
Removed thread_host from test.
gaaclarke Jul 10, 2019
9c3b023
Removed enumeration values for StatusCodes.
gaaclarke Jul 10, 2019
e713db8
Merge branch 'master' of github.com:flutter/engine into black-frame2
gaaclarke Jul 10, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 37 additions & 13 deletions shell/common/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -457,18 +457,22 @@ 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([& 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);

// 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
Expand Down Expand Up @@ -564,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
Expand Down Expand Up @@ -800,10 +804,17 @@ void Shell::OnAnimatorDraw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline) {
FML_DCHECK(is_setup_);

task_runners_.GetGPUTaskRunner()->PostTask(
[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();
}
}
});
}
Expand Down Expand Up @@ -1240,4 +1251,17 @@ Rasterizer::Screenshot Shell::Screenshot(
return screenshot;
}

bool Shell::WaitForFirstFrame(fml::TimeDelta timeout) {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

FML_DCHECK(is_setup_);
FML_DCHECK(!task_runners_.GetUITaskRunner()->RunsTasksOnCurrentThread());
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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()),
[& waiting_for_first_frame = waiting_for_first_frame_] {
return !waiting_for_first_frame.load();
});
return !success;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks to the tests added, I finally realized that this returns !success instead of success.

This is a little counter-intuitive as most wait_for_something returns success. Maybe create an enum FirstFrameWaitingResult {timedout, successful}; and make that as the return type. That will make it much clearer.

Copy link
Member Author

Choose a reason for hiding this comment

The 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:

bool WaitWithTimeout(TimeDelta timeout);

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i've added the Status class as discussed.

}

} // namespace flutter
9 changes: 9 additions & 0 deletions shell/common/shell.h
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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);
Copy link
Member

Choose a reason for hiding this comment

The 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.

enum class WaitResult {
 kWaited, // or, kSuccess or whatever
 kTimeout
};

Copy link
Member Author

Choose a reason for hiding this comment

The 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:

bool WaitWithTimeout(TimeDelta timeout);

Copy link
Member Author

Choose a reason for hiding this comment

The 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&,
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: std::atomic_bool

Copy link
Member Author

@gaaclarke gaaclarke Jul 9, 2019

Choose a reason for hiding this comment

The 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.
Expand Down
53 changes: 53 additions & 0 deletions shell/common/shell_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -518,5 +518,58 @@ TEST_F(ShellTest, ReportTimingsIsCalledImmediatelyAfterTheFirstFrame) {
ASSERT_EQ(timestamps.size(), FrameTiming::kCount);
}

TEST_F(ShellTest, WaitForFirstFrame) {
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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());
bool result =
shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000));
ASSERT_FALSE(result);
}

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));
bool result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(10));
ASSERT_TRUE(result);
}

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());
bool result =
shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1000));
ASSERT_FALSE(result);
for (int i = 0; i < 100; ++i) {
result = shell->WaitForFirstFrame(fml::TimeDelta::FromMilliseconds(1));
ASSERT_FALSE(result);
}
}

} // namespace testing
} // namespace flutter
Original file line number Diff line number Diff line change
Expand Up @@ -406,8 +406,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];
Expand Down Expand Up @@ -698,8 +699,22 @@ - (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 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 "
<< "unoptimized builds. If this is a release build, you should load a less "
<< "complex frame to avoid the timeout.";
}
}
}

- (void)viewSafeAreaInsetsDidChange {
Expand Down