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

[Impeller] separate algorithm for computing render target size. #54604

Merged
auto-submit merged 38 commits into from
Aug 31, 2024
Merged

[Impeller] separate algorithm for computing render target size. #54604

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
9d59af1
[Impeller] separate algorithm for computing render target size.
Aug 18, 2024
a935594
++
Aug 18, 2024
7aa3a48
Update save_layer_utils_unittests.cc
Aug 18, 2024
54cea5a
nolint.
Aug 19, 2024
e6e56d8
oh boy here we go.
Aug 19, 2024
98a5d68
cleanups.
Aug 19, 2024
81ccaa0
remove print.
Aug 19, 2024
55a6a50
Update save_layer_utils_unittests.cc
Aug 19, 2024
3591395
dont overwrite save layer bounds.
Aug 19, 2024
a8f0e2c
Merge branch 'render_target_sizing' of github.com:jonahwilliams/engin…
Aug 19, 2024
1c55cf5
++
Aug 20, 2024
a6f5f21
Update dl_dispatcher.cc
Aug 20, 2024
a646c1b
++
Aug 20, 2024
8daf291
Merge branch 'main' of github.com:flutter/engine into render_target_s…
Aug 20, 2024
199a846
++
Aug 20, 2024
b2eb6b9
++
Aug 20, 2024
d347783
++
Aug 20, 2024
24442e0
flood clip.
Aug 21, 2024
78b7f60
add flood clip.
Aug 21, 2024
2a4ac2d
++
Aug 21, 2024
3eff88b
++
Aug 21, 2024
04b5be5
Update entity_pass.cc
Aug 21, 2024
086abb8
Update entity_pass.h
Aug 21, 2024
4a5f30b
++
Aug 21, 2024
fa3afcc
Merge branch 'render_target_sizing' of github.com:jonahwilliams/engin…
Aug 21, 2024
4e7a707
partial review feedback.
Aug 23, 2024
e0105d7
more adjustments.
Aug 23, 2024
e8f34ac
oops
Aug 23, 2024
8a3d1ef
Merge branch 'main' into render_target_sizing
Aug 27, 2024
24ef59a
Merge branch 'main' into render_target_sizing
Aug 28, 2024
0cae7a8
flar review.
Aug 29, 2024
ae57e62
Merge branch 'main' of github.com:flutter/engine into render_target_s…
Aug 29, 2024
bc2276e
no bds from caller.
Aug 30, 2024
b3ab833
update comment.
Aug 30, 2024
334e5e9
++
Aug 30, 2024
82a6621
++
Aug 30, 2024
5011b3c
++
Aug 30, 2024
008b6f9
++
Aug 30, 2024
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
1 change: 1 addition & 0 deletions ci/licenses_golden/excluded_files
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,7 @@
../../../flutter/impeller/entity/entity_unittests.cc
../../../flutter/impeller/entity/geometry/geometry_unittests.cc
../../../flutter/impeller/entity/render_target_cache_unittests.cc
../../../flutter/impeller/entity/save_layer_utils_unittests.cc
../../../flutter/impeller/fixtures
../../../flutter/impeller/geometry/README.md
../../../flutter/impeller/geometry/geometry_unittests.cc
Expand Down
4 changes: 4 additions & 0 deletions ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42870,6 +42870,8 @@ ORIGIN: ../../../flutter/impeller/entity/inline_pass_context.cc + ../../../flutt
ORIGIN: ../../../flutter/impeller/entity/inline_pass_context.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/render_target_cache.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/render_target_cache.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/save_layer_utils.cc + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/save_layer_utils.h + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/shaders/blending/advanced_blend.frag + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/shaders/blending/advanced_blend.vert + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/impeller/entity/shaders/blending/blend_select.glsl + ../../../flutter/LICENSE
Expand Down Expand Up @@ -45749,6 +45751,8 @@ FILE: ../../../flutter/impeller/entity/inline_pass_context.cc
FILE: ../../../flutter/impeller/entity/inline_pass_context.h
FILE: ../../../flutter/impeller/entity/render_target_cache.cc
FILE: ../../../flutter/impeller/entity/render_target_cache.h
FILE: ../../../flutter/impeller/entity/save_layer_utils.cc
FILE: ../../../flutter/impeller/entity/save_layer_utils.h
FILE: ../../../flutter/impeller/entity/shaders/blending/advanced_blend.frag
FILE: ../../../flutter/impeller/entity/shaders/blending/advanced_blend.vert
FILE: ../../../flutter/impeller/entity/shaders/blending/blend_select.glsl
Expand Down
8 changes: 7 additions & 1 deletion display_list/dl_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -620,7 +620,13 @@ void DisplayListBuilder::RestoreLayer() {
content_bounds.intersect(layer_op->rect);
}
}
layer_op->rect = content_bounds;
// If the caller specified save layer bounds, preserve those
// instead of overwriting with content bounds. The caller
// bounds may clip the contents or otherwise limit the bounds
// of backdrop entities.
if (!layer_op->options.bounds_from_caller()) {
Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 19, 2024

Choose a reason for hiding this comment

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

@flar I think this is a bug in that we can have a saveLayer with bounds but end up blowing away the coverage value. In the case I saw, we did a with bounds and a backdrop filter, the layer itself had no contents. The bounds from caller were overwritten to empty, however the save layer state of bounds from caller was still reported as true, which makes it look like the caller specified empty bounds.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 20, 2024

Choose a reason for hiding this comment

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

In this case I can work around it by ignoring bounds from caller when there is a bdf, as I don't believe its possible to specify saveLayer bounds on backdrop filters from flutter.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 20, 2024

Choose a reason for hiding this comment

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

oh, or maybe its the line above actually

Copy link
Contributor

@flar flar Aug 20, 2024

Choose a reason for hiding this comment

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

Um, the bounds of the content of the saveLayer bounds are empty and that is correct. It doesn't matter if the bounds came from the caller or not. Empty saveLayer bounds do not mean that the saveLayer is a NOP, they just mean that there is nothing to draw into the bounds.

But, the backdrop filter is a separate aspect of the operation.

Everything is working as intended, but you are interpreting the bounds as if it contains the BDF, but it does not. It is providing you with information about the contents of the layer and you must combine that with other information to figure out the RT size.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 20, 2024

Choose a reason for hiding this comment

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

In this case I am specifying non empty bounds of a particular size on a savayer with no content. The bounds are overwritten to empty here

Copy link
Contributor

@flar flar Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

You are specifying non-empty bounds? Are you saying that you have:

builder.SaveLayer(100x100 rect, paint, backdrop);
// No render calls here
builder.Restore();

if that is the case then you claimed that there was content that was 100x100 in size, but you gave no content, so I corrected it to 0x0. You also specified a backdrop filter which also (completely 100% independently of the saveLayer argument) contribute to the resulting output.

Everything there is working as intended.

If you want to clip the BDF, you have to put a clip around the saveLayer, as in:

builder.Save();
builder.ClipRect(100x100 rect);
builder.SaveLayer(<doesn't really matter>, paint, bdf);
// No render calls here
builder.Restore();
builder.Restore();

(The outer save/restore are only if you are installing that clip just for the SaveLayer.)

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 20, 2024

Choose a reason for hiding this comment

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

So bounds from caller really isn't what I wanted then. Bdf aside, the original reason I incorporated it was to try and deal with unbounded contents. But instead of bounds from caller I should be using the content clipped setting then

Thanks for the pointers!

Copy link
Contributor

@flar flar Aug 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure what you'd use the content clipped setting for, though. It means that the natural bounds of the content extends outside of the bounds provided in the saveLayer, such as:

builder.SaveLayer({10, 10, 20, 20}, null, null);
builder.DrawRect({0, 0, 50, 50}, DlPaint());
builder.Restore();

In that case the layer was suggested to capture a 10x10 set of pixels, though the rendering commands draw to a larger area. The legacy behavior of this is that only the 10x10 set of pixels is captured (in scale-translate cases).

It's primary intent is for the case where group opacity "might" be used and in that case it is a contra-indication. If that SaveLayer had alpha in the paint then you can't just completely elide the SaveLayer like you might do if you are applying opacity inheritance optimizations, because that would draw too much. It has to look like you actually allocated the layer, drew the content, and then faded it - which means some form of clipping would be involved. One solution might be to replace the SaveLayer with a Save and a clip, but so far the Skia implementation has simply avoided the opacity optimization instead. It is possible to notice the difference, but it depends on other factors like transform, etc. We talked about this before and the response was that we didn't want to do the clip in that case because it was expensive - except that the alternative to doing a clip is to use an alternate layer. Which would be faster? A clip or the layer?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 20, 2024

Choose a reason for hiding this comment

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

A clip will definitely be faster.

I'm not sure what you'd use the content clipped setting for

basically for destructive blends that flood the clip. But actually it sounds like display list bounds will already take that into account and I can take them as is?

layer_op->rect = content_bounds;
}
layer_op->max_blend_mode = current_layer().max_blend_mode;

if (current_layer().contains_backdrop_filter) {
Expand Down
9 changes: 5 additions & 4 deletions impeller/aiks/aiks_blend_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,13 @@ TEST_P(AiksTest, ColorWheel) {

draw_color_wheel(canvas);
auto color_wheel_picture = canvas.EndRecordingAsPicture();
auto snapshot = color_wheel_picture.Snapshot(renderer);
if (!snapshot.has_value() || !snapshot->texture) {
auto image = color_wheel_picture.ToImage(
renderer, ISize{GetWindowSize().width, GetWindowSize().height});
if (!image || !image->GetTexture()) {
return std::nullopt;
}
color_wheel_image = std::make_shared<Image>(snapshot->texture);
color_wheel_transform = snapshot->transform;
color_wheel_image = std::make_shared<Image>(image->GetTexture());
color_wheel_transform = Matrix();
}

Canvas canvas;
Expand Down
2 changes: 1 addition & 1 deletion impeller/aiks/canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,7 +837,7 @@ void Canvas::SaveLayer(const Paint& paint,

auto& new_layer_pass = GetCurrentPass();
if (bounds) {
new_layer_pass.SetBoundsLimit(bounds, bounds_promise);
new_layer_pass.SetBoundsLimit(bounds, bounds_from_caller);
}

// When applying a save layer, absorb any pending distributed opacity.
Expand Down
1 change: 1 addition & 0 deletions impeller/aiks/canvas_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "flutter/testing/testing.h"
#include "impeller/aiks/canvas.h"
#include "impeller/aiks/image_filter.h"
#include "impeller/geometry/color.h"
#include "impeller/geometry/path_builder.h"

// TODO(zanderso): https://github.com/flutter/flutter/issues/127701
Expand Down
53 changes: 24 additions & 29 deletions impeller/aiks/experimental_canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,12 @@
#include "impeller/base/validation.h"
#include "impeller/core/allocator.h"
#include "impeller/core/formats.h"
#include "impeller/entity/contents/filters/filter_contents.h"
#include "impeller/entity/contents/framebuffer_blend_contents.h"
#include "impeller/entity/contents/text_contents.h"
#include "impeller/entity/entity.h"
#include "impeller/entity/entity_pass_clip_stack.h"
#include "impeller/entity/save_layer_utils.h"
#include "impeller/geometry/color.h"
#include "impeller/renderer/render_target.h"

Expand Down Expand Up @@ -310,11 +312,6 @@ void ExperimentalCanvas::SaveLayer(
bool bounds_from_caller) {
TRACE_EVENT0("flutter", "Canvas::saveLayer");

if (bounds.has_value() && bounds->IsEmpty()) {
Save(total_content_depth);
return;
}

if (!clip_coverage_stack_.HasCoverage()) {
// The current clip is empty. This means the pass texture won't be
// visible, so skip it.
Expand All @@ -338,13 +335,6 @@ void ExperimentalCanvas::SaveLayer(
->GetSize()))
.Intersection(current_clip_coverage);

if (!maybe_coverage_limit.has_value()) {
Save(total_content_depth);
return;
}
maybe_coverage_limit = maybe_coverage_limit->Intersection(
Rect::MakeSize(render_target_.GetRenderTargetSize()));

if (!maybe_coverage_limit.has_value() || maybe_coverage_limit->IsEmpty()) {
Save(total_content_depth);
return;
Expand All @@ -359,12 +349,32 @@ void ExperimentalCanvas::SaveLayer(
return;
}

std::shared_ptr<FilterContents> filter_contents;
if (paint.image_filter) {
filter_contents = paint.image_filter->GetFilterContents();
}

std::optional<Rect> maybe_subpass_coverage = ComputeSaveLayerCoverage(
bounds.value_or(Rect::MakeMaximum()),
transform_stack_.back().transform, //
coverage_limit, //
filter_contents, //
/*destructive_blend=*/Entity::IsBlendModeDestructive(paint.blend_mode),
/*has_backdrop_filter=*/!!backdrop_filter,
/*bounds_from_caller=*/bounds_from_caller);

if (!maybe_subpass_coverage.has_value() ||
maybe_subpass_coverage->IsEmpty()) {
Save(total_content_depth);
return;
}
auto subpass_coverage = maybe_subpass_coverage.value();

// Backdrop filter state, ignored if there is no BDF.
std::shared_ptr<FilterContents> backdrop_filter_contents;
Point local_position = {0, 0};
if (backdrop_filter) {
local_position =
current_clip_coverage.GetOrigin() - GetGlobalPassPosition();
local_position = subpass_coverage.GetOrigin() - GetGlobalPassPosition();
EntityPass::BackdropFilterProc backdrop_filter_proc =
[backdrop_filter = backdrop_filter->Clone()](
const FilterInput::Ref& input, const Matrix& effect_transform,
Expand Down Expand Up @@ -397,21 +407,6 @@ void ExperimentalCanvas::SaveLayer(
paint_copy.color.alpha *= transform_stack_.back().distributed_opacity;
transform_stack_.back().distributed_opacity = 1.0;

// Backdrop Filter must expand bounds to at least the clip stack, otherwise
// the coverage of the parent render pass.
Rect subpass_coverage;
if (backdrop_filter_contents ||
Entity::IsBlendModeDestructive(paint.blend_mode) || !bounds.has_value()) {
subpass_coverage = coverage_limit;
// TODO(jonahwilliams): if we have tight bounds we should be able to reduce
// this size here. if (bounds.has_value() && bounds_from_caller) {
// subpass_coverage =
// coverage_limit.Intersection(bounds.value()).value_or(bounds.value());
// }
} else {
subpass_coverage = bounds->TransformBounds(GetCurrentTransform());
}

render_passes_.push_back(LazyRenderingConfig(
renderer_, //
CreateRenderTarget(renderer_, //
Expand Down
16 changes: 0 additions & 16 deletions impeller/aiks/picture.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,10 @@
#include <optional>

#include "impeller/base/validation.h"
#include "impeller/entity/entity.h"
#include "impeller/renderer/render_target.h"
#include "impeller/renderer/snapshot.h"

namespace impeller {

std::optional<Snapshot> Picture::Snapshot(AiksContext& context) {
auto coverage = pass->GetElementsCoverage(std::nullopt);
if (!coverage.has_value() || coverage->IsEmpty()) {
return std::nullopt;
}

const auto translate = Matrix::MakeTranslation(-coverage->GetOrigin());
auto texture =
RenderToTexture(context, ISize(coverage->GetSize()), translate);
return impeller::Snapshot{
.texture = std::move(texture),
.transform = Matrix::MakeTranslation(coverage->GetOrigin())};
}

std::shared_ptr<Image> Picture::ToImage(AiksContext& context,
ISize size) const {
if (size.IsEmpty()) {
Expand Down
2 changes: 0 additions & 2 deletions impeller/aiks/picture.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ namespace impeller {
struct Picture {
std::unique_ptr<EntityPass> pass;

std::optional<Snapshot> Snapshot(AiksContext& context);

std::shared_ptr<Image> ToImage(AiksContext& context, ISize size) const;

private:
Expand Down
14 changes: 1 addition & 13 deletions impeller/display_list/dl_dispatcher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,19 +622,7 @@ void DlDispatcherBase::saveLayer(const SkRect& bounds,
auto promise = options.content_is_clipped()
? ContentBoundsPromise::kMayClipContents
: ContentBoundsPromise::kContainsContents;
std::optional<Rect> impeller_bounds;
if (!options.content_is_unbounded() || options.bounds_from_caller()) {
impeller_bounds = skia_conversions::ToRect(bounds);
}

// Empty bounds on a save layer that contains a BDF or destructive blend
// should be treated as unbounded. All other empty bounds can be skipped.
if (impeller_bounds.has_value() && impeller_bounds->IsEmpty() &&
(backdrop != nullptr ||
Entity::IsBlendModeDestructive(paint.blend_mode))) {
impeller_bounds = std::nullopt;
}

std::optional<Rect> impeller_bounds = skia_conversions::ToRect(bounds);
GetCanvas().SaveLayer(paint, impeller_bounds, ToImageFilter(backdrop),
promise, total_content_depth,
options.can_distribute_opacity(),
Expand Down
3 changes: 3 additions & 0 deletions impeller/entity/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ impeller_component("entity") {
"inline_pass_context.h",
"render_target_cache.cc",
"render_target_cache.h",
"save_layer_utils.cc",
"save_layer_utils.h",
]

public_deps = [
Expand Down Expand Up @@ -257,6 +259,7 @@ impeller_component("entity_unittests") {
"entity_unittests.cc",
"geometry/geometry_unittests.cc",
"render_target_cache_unittests.cc",
"save_layer_utils_unittests.cc",
]

deps = [
Expand Down
Loading