Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

WIP: Discard wrong size layer tree instead of rendering it #21108

Closed
wants to merge 2 commits into from
Closed

WIP: Discard wrong size layer tree instead of rendering it #21108

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ddcb61
Add optional DiscardCallback parameter to Rasterizer::Draw
knopp Sep 11, 2020
0b5d499
Do not rasterzize wrong size layer tree after viewport metrics change
knopp Sep 11, 2020
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
9 changes: 7 additions & 2 deletions shell/common/rasterizer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,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,
DiscardCallback discardCallback) {
TRACE_EVENT0("flutter", "GPURasterizer::Draw");
if (raster_thread_merger_ &&
!raster_thread_merger_->IsOnRasterizingThread()) {
Expand All @@ -170,7 +171,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 && discardCallback(*layer_tree.get())) {
raster_status = RasterStatus::kSuccess;
} else {
raster_status = DoDraw(std::move(layer_tree));
}
};

PipelineConsumeResult consume_result = pipeline->Consume(consumer);
Expand Down
7 changes: 6 additions & 1 deletion shell/common/rasterizer.h
View file
Open in desktop
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 DiscardCallback = std::function<bool(flutter::LayerTree&)>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider calling this LayerTreeDiscardCallback.


//----------------------------------------------------------------------------
/// @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,
DiscardCallback discardCallback = nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than passing nullptr as the default here, what do you think of having a static callback that resolved to false each time?


//----------------------------------------------------------------------------
/// @brief The type of the screenshot to obtain of the previously
Expand Down
28 changes: 18 additions & 10 deletions shell/common/shell.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -822,20 +822,19 @@ void Shell::OnPlatformViewSetViewportMetrics(const ViewportMetrics& metrics) {

// This is the formula Android uses.
// https://android.googlesource.com/platform/frameworks/base/+/master/libs/hwui/renderthread/CacheManager.cpp#41
size_t max_bytes = metrics.physical_width * metrics.physical_height * 12 * 4;
task_runners_.GetRasterTaskRunner()->PostTask(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is setting the resource cache size moved to line 1036?

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's a good point. I think it's a remnant from the very first version where shell was actually synchronizing viewport event with rasterization. I don't think it's necessary anymore. I'll look into it.

[rasterizer = rasterizer_->GetWeakPtr(), max_bytes] {
if (rasterizer) {
rasterizer->SetResourceCacheMaxBytes(max_bytes, false);
}
});
resource_cache_max_bytes_update_ =
metrics.physical_width * metrics.physical_height * 12 * 4;

task_runners_.GetUITaskRunner()->PostTask(
[engine = engine_->GetWeakPtr(), metrics]() {
if (engine) {
engine->SetViewportMetrics(metrics);
}
});

std::scoped_lock<std::mutex> lock(resize_mutex_);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: consider creating a new scope for this

{
   lock;
   expeced_frame_size_ = ...;
}

When someone tries to add more functionality to this method, they will not hold the lock for longer than needed.

expected_frame_size_ =
SkISize::Make(metrics.physical_width, metrics.physical_height);
}

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

size_t max_bytes = resource_cache_max_bytes_update_;
resource_cache_max_bytes_update_ = 0;

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(), max_bytes,
pipeline = std::move(pipeline), this]() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think initializing the discard callback outside this lambda could make it easier to read and could avoid capturing this here.

Rasterizer::DiscardCallback discard_callback =
      [&resize_mutex = resize_mutex_,
       &expected_frame_size = expected_frame_size_](flutter::LayerTree& tree) {
        std::scoped_lock<std::mutex> lock(resize_mutex);
        return tree.frame_size() != expected_frame_size;
      };

And then passing &discard_callback = discard_callback rather than this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The capture should be then discard_callback = std::move(discard_callback). I'm not sure why capturing resize_mutex_ and expected_frame_size_ individually though instead of just capturing this in discard_callback.

if (rasterizer) {
rasterizer->Draw(pipeline);
if (max_bytes != 0) {
rasterizer->SetResourceCacheMaxBytes(max_bytes, false);
}
rasterizer->Draw(pipeline, [this](flutter::LayerTree& tree) {
std::scoped_lock<std::mutex> lock(resize_mutex_);
return tree.frame_size() != expected_frame_size_;
});

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

std::mutex resize_mutex_;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add comments explaining the newly added fields?

SkISize expected_frame_size_ = SkISize::MakeEmpty();
size_t resource_cache_max_bytes_update_ = 0;

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

Expand Down