WIP: Discard wrong size layer tree instead of rendering it #21108
Conversation
If this callback returns true for given layer tree, the layer tree will be discarded and not rasterized This is useful after the resize event, when layer tree with certain size is expected, but there is a tree with previous size still in pipeline
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat. Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
There was a problem hiding this comment.
I would like to see some tests for this change.
One test that we should definitely have is:
- Send a metrics event to update the frame size.
- Assert that the discard callback gets called.
Tests in shell_unittests.cc should have some examples on how this can be achieved, see: SetResourceCacheSizeEarly, Screenshot as examples that relate to metrics events and exercising the rasterizer.
| @@ -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( | |||
There was a problem hiding this comment.
Why is setting the resource cache size moved to line 1036?
There was a problem hiding this comment.
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.
| @@ -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_; | |||
There was a problem hiding this comment.
Can you add comments explaining the newly added fields?
| rasterizer = rasterizer_->GetWeakPtr(), | ||
| pipeline = std::move(pipeline)]() { | ||
| rasterizer = rasterizer_->GetWeakPtr(), max_bytes, | ||
| pipeline = std::move(pipeline), this]() { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -204,6 +204,8 @@ class Rasterizer final : public SnapshotDelegate { | |||
| /// | |||
| flutter::TextureRegistry* GetTextureRegistry(); | |||
|
|
|||
| using DiscardCallback = std::function<bool(flutter::LayerTree&)>; | |||
There was a problem hiding this comment.
Consider calling this LayerTreeDiscardCallback.
| /// | ||
| void Draw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline); | ||
| void Draw(fml::RefPtr<Pipeline<flutter::LayerTree>> pipeline, | ||
| DiscardCallback discardCallback = nullptr); |
There was a problem hiding this comment.
Rather than passing nullptr as the default here, what do you think of having a static callback that resolved to false each time?
|
|
||
| task_runners_.GetUITaskRunner()->PostTask( | ||
| [engine = engine_->GetWeakPtr(), metrics]() { | ||
| if (engine) { | ||
| engine->SetViewportMetrics(metrics); | ||
| } | ||
| }); | ||
|
|
||
| std::scoped_lock<std::mutex> lock(resize_mutex_); |
There was a problem hiding this comment.
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.
|
Thank you for the feedback. I addressed the change requests in #21179. |
Description
This PR will ensure that after viewport change metrics event is received, layer tree with previous (wrong size) are discarded and not rasterized. This is required for smooth resizing on both Mac and Windows.
Important: This will potentially affect iOS and Android embedders. We need to make sure this doesn't cause any issues.
Related Issues
flutter/flutter#44136
Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes (
[x]). This will ensure a smooth and quick review process.Breaking Change
Did any tests fail when you ran them? Please read handling breaking changes.