Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discard wrong size layer tree instead of rendering it #21179

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion flow/compositor_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ enum class RasterStatus {
// only used when thread configuration change occurs.
kEnqueuePipeline,
// Failed to rasterize the frame.
kFailed
kFailed,
// Layer tree was discarded due to LayerTreeDiscardCallback
kDiscarded
};

class CompositorContext {
Expand Down
9 changes: 8 additions & 1 deletion shell/common/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,13 @@ if (enable_unittests) {
]
}

config("shell_test_fixture_sources_config") {
defines = [
# Required for MSVC STL
"_ENABLE_ATOMIC_ALIGNMENT_FIX",
]
}

source_set("shell_test_fixture_sources") {
testonly = true

Expand Down Expand Up @@ -214,7 +221,7 @@ if (enable_unittests) {
"//third_party/skia",
]

public_configs = []
public_configs = [ ":shell_test_fixture_sources_config" ]

# SwiftShader only supports x86/x64_64
if (target_cpu == "x86" || target_cpu == "x64") {
Expand Down
9 changes: 7 additions & 2 deletions shell/common/rasterizer.cc
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ void Rasterizer::DrawLastLayerTree() {
DrawToSurface(*last_layer_tree_);
}

void Rasterizer::Draw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline) {
void Rasterizer::Draw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline,
LayerTreeDiscardCallback discardCallback) {
TRACE_EVENT0("flutter", "GPURasterizer::Draw");
if (raster_thread_merger_ &&
!raster_thread_merger_->IsOnRasterizingThread()) {
Expand All @@ -166,7 +167,11 @@ void Rasterizer::Draw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline) {
RasterStatus raster_status = RasterStatus::kFailed;
Pipeline<flutter::LayerTree>::Consumer consumer =
[&](std::unique_ptr<LayerTree> layer_tree) {
raster_status = DoDraw(std::move(layer_tree));
if (discardCallback(*layer_tree.get())) {
raster_status = RasterStatus::kDiscarded;
} else {
raster_status = DoDraw(std::move(layer_tree));
}
};

PipelineConsumeResult consume_result = pipeline->Consume(consumer);
Expand Down
9 changes: 8 additions & 1 deletion shell/common/rasterizer.h
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ class Rasterizer final : public SnapshotDelegate {
///
flutter::TextureRegistry* GetTextureRegistry();

using LayerTreeDiscardCallback = std::function<bool(flutter::LayerTree&)>;

//----------------------------------------------------------------------------
/// @brief Takes the next item from the layer tree pipeline and executes
/// the raster thread frame workload for that pipeline item to
Expand Down Expand Up @@ -232,8 +234,11 @@ class Rasterizer final : public SnapshotDelegate {
///
/// @param[in] pipeline The layer tree pipeline to take the next layer tree
/// to render from.
/// @param[in] discardCallback if specified and returns true, the layer tree
/// is discarded instead of being rendered
///
void Draw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline);
void Draw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline,
LayerTreeDiscardCallback discardCallback = NoDiscard);

//----------------------------------------------------------------------------
/// @brief The type of the screenshot to obtain of the previously
Expand Down Expand Up @@ -454,6 +459,8 @@ class Rasterizer final : public SnapshotDelegate {

void FireNextFrameCallbackIfPresent();

static bool NoDiscard(const flutter::LayerTree& layer_tree) { return false; }

FML_DISALLOW_COPY_AND_ASSIGN(Rasterizer);
};

Expand Down
18 changes: 15 additions & 3 deletions shell/common/shell.cc
Original file line number Diff line number Diff line change
Expand Up @@ -832,6 +832,12 @@ void Shell::OnPlatformViewSetViewportMetrics(const ViewportMetrics& metrics) {
engine->SetViewportMetrics(metrics);
}
});

{
std::scoped_lock<std::mutex> lock(resize_mutex_);
expected_frame_size_ =
SkISize::Make(metrics.physical_width, metrics.physical_height);
}
}

// |PlatformView::Delegate|
Expand Down Expand Up @@ -1021,13 +1027,19 @@ void Shell::OnAnimatorDraw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline,
}
}

auto discard_callback = [this](flutter::LayerTree& tree) {
std::scoped_lock<std::mutex> lock(resize_mutex_);
return !expected_frame_size_.isEmpty() &&
tree.frame_size() != expected_frame_size_;
};

task_runners_.GetRasterTaskRunner()->PostTask(
[&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)]() {
rasterizer = rasterizer_->GetWeakPtr(), pipeline = std::move(pipeline),
discard_callback = std::move(discard_callback)]() {
if (rasterizer) {
rasterizer->Draw(pipeline);
rasterizer->Draw(pipeline, std::move(discard_callback));

if (waiting_for_first_frame.load()) {
waiting_for_first_frame.store(false);
Expand Down
7 changes: 7 additions & 0 deletions shell/common/shell.h
Original file line number Diff line number Diff line change
Expand Up @@ -425,6 +425,13 @@ class Shell final : public PlatformView::Delegate,
// and read from the raster thread.
std::atomic<float> display_refresh_rate_ = 0.0f;

// protects expected_frame_size_ which is set on platform thread and read on
// raster thread
std::mutex resize_mutex_;

// used to discard wrong size layer tree produced during interactive resizing
SkISize expected_frame_size_ = SkISize::MakeEmpty();

// How many frames have been timed since last report.
size_t UnreportedFramesCount() const;

Expand Down
10 changes: 10 additions & 0 deletions shell/common/shell_test_external_view_embedder.cc
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ int ShellTestExternalViewEmbedder::GetSubmittedFrameCount() {
return submitted_frame_count_;
}

SkISize ShellTestExternalViewEmbedder::GetLastSubmittedFrameSize() {
return last_submitted_frame_size_;
}

// |ExternalViewEmbedder|
void ShellTestExternalViewEmbedder::CancelFrame() {}

Expand Down Expand Up @@ -61,6 +65,12 @@ void ShellTestExternalViewEmbedder::SubmitFrame(
GrDirectContext* context,
std::unique_ptr<SurfaceFrame> frame) {
frame->Submit();
if (frame && frame->SkiaSurface()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Windows toolchain doesn't seem to be liking this:

C:\b\s\w\ir\kitchen-checkout\depot_tools\win_toolchain\vs_files\418b3076791776573a815eb298c8aa590307af63\win_sdk\bin\..\..\VC\Tools\MSVC\14.16.27023\include\atomic(620,2): error: static_assert failed due to requirement 'alignof(SkISize) >= sizeof(unsigned long long)' "You've instantiated std::atomic<T> with sizeof(T) equal to 2/4/8 and alignof(T) < sizeof(T). Before VS 2015 Update 2, this would have misbehaved at runtime. VS 2015 Update 2 was fixed to handle this correctly, but the fix inherently changes layout and breaks binary compatibility. Please define _ENABLE_ATOMIC_ALIGNMENT_FIX to acknowledge that you understand this, and that everything you're linking has been compiled with VS 2015 Update 2 (or later)."
        static_assert(alignof(_Ty) >= sizeof(_My_int),
        ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
C:\b\s\w\ir\kitchen-checkout\depot_tools\win_toolchain\vs_files\418b3076791776573a815eb298c8aa590307af63\win_sdk\bin\..\..\VC\Tools\MSVC\14.16.27023\include\atomic(635,5): note: in instantiation of template class 'std::_Atomic_base<SkISize, 8>' requested here
                : _Atomic_base<_Ty, sizeof (_Ty)>
                  ^
../../flutter/shell/common/shell_test_external_view_embedder.h(86,24): note: in instantiation of template class 'std::atomic<SkISize>' requested here
  std::atomic<SkISize> last_submitted_frame_size_;
                       ^
1 error generated.

See https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket.appspot.com/8869006746431748192/+/steps/build_host_debug/0/stdout for full error.

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't going to be an issue in future, in the meanwhile perhaps we could define _ENABLE_ATOMIC_ALIGNMENT_FIX. There's nothing wrong with the code, it's just a windows STL quirk.

last_submitted_frame_size_ = SkISize::Make(frame->SkiaSurface()->width(),
frame->SkiaSurface()->height());
} else {
last_submitted_frame_size_ = SkISize::MakeEmpty();
}
submitted_frame_count_++;
}

Expand Down
4 changes: 4 additions & 0 deletions shell/common/shell_test_external_view_embedder.h
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder {
// the external view embedder.
int GetSubmittedFrameCount();

// Returns the size of last submitted frame surface
SkISize GetLastSubmittedFrameSize();

private:
// |ExternalViewEmbedder|
void CancelFrame() override;
Expand Down Expand Up @@ -80,6 +83,7 @@ class ShellTestExternalViewEmbedder final : public ExternalViewEmbedder {
bool support_thread_merging_;

std::atomic<int> submitted_frame_count_;
std::atomic<SkISize> last_submitted_frame_size_;

FML_DISALLOW_COPY_AND_ASSIGN(ShellTestExternalViewEmbedder);
};
Expand Down
55 changes: 55 additions & 0 deletions shell/common/shell_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2014,5 +2014,60 @@ TEST_F(ShellTest, OnServiceProtocolEstimateRasterCacheMemoryWorks) {
DestroyShell(std::move(shell));
}

TEST_F(ShellTest, DiscardLayerTreeOnResize) {
auto settings = CreateSettingsForFixture();

SkISize wrong_size = SkISize::Make(400, 100);
SkISize expected_size = SkISize::Make(400, 200);

fml::AutoResetWaitableEvent end_frame_latch;

auto end_frame_callback = [&](bool, fml::RefPtr<fml::RasterThreadMerger>) {
end_frame_latch.Signal();
};

std::shared_ptr<ShellTestExternalViewEmbedder> external_view_embedder =
std::make_shared<ShellTestExternalViewEmbedder>(
std::move(end_frame_callback), PostPrerollResult::kSuccess, true);

std::unique_ptr<Shell> shell = CreateShell(
settings, GetTaskRunnersForFixture(), false, external_view_embedder);

// Create the surface needed by rasterizer
PlatformViewNotifyCreated(shell.get());

fml::TaskRunner::RunNowOrPostTask(
shell->GetTaskRunners().GetPlatformTaskRunner(),
[&shell, &expected_size]() {
shell->GetPlatformView()->SetViewportMetrics(
{1.0, static_cast<double>(expected_size.width()),
static_cast<double>(expected_size.height())});
});

auto configuration = RunConfiguration::InferFromSettings(settings);
configuration.SetEntrypoint("emptyMain");

RunEngine(shell.get(), std::move(configuration));

fml::WeakPtr<RuntimeDelegate> runtime_delegate = shell->GetEngine();

PumpOneFrame(shell.get(), static_cast<double>(wrong_size.width()),
static_cast<double>(wrong_size.height()));

end_frame_latch.Wait();

ASSERT_EQ(0, external_view_embedder->GetSubmittedFrameCount());

iskakaushik marked this conversation as resolved.
Show resolved Hide resolved
PumpOneFrame(shell.get(), static_cast<double>(expected_size.width()),
static_cast<double>(expected_size.height()));

end_frame_latch.Wait();

ASSERT_EQ(1, external_view_embedder->GetSubmittedFrameCount());
ASSERT_EQ(expected_size, external_view_embedder->GetLastSubmittedFrameSize());

DestroyShell(std::move(shell));
}

} // namespace testing
} // namespace flutter