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 30 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 @@ -42871,6 +42871,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 @@ -45757,6 +45759,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
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) {
return std::nullopt;
}
color_wheel_image = snapshot->texture;
color_wheel_transform = snapshot->transform;
color_wheel_image = image;
color_wheel_transform = Matrix();
}

Canvas canvas;
Expand Down
5 changes: 2 additions & 3 deletions impeller/aiks/canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,8 +814,7 @@ void Canvas::SaveLayer(const Paint& paint,
const std::shared_ptr<ImageFilter>& backdrop_filter,
ContentBoundsPromise bounds_promise,
uint32_t total_content_depth,
bool can_distribute_opacity,
bool bounds_from_caller) {
bool can_distribute_opacity) {
if (can_distribute_opacity && !backdrop_filter &&
Paint::CanApplyOpacityPeephole(paint) &&
bounds_promise != ContentBoundsPromise::kMayClipContents) {
Expand All @@ -837,7 +836,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);
}

// When applying a save layer, absorb any pending distributed opacity.
Expand Down
3 changes: 1 addition & 2 deletions impeller/aiks/canvas.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,7 @@ class Canvas {
const std::shared_ptr<ImageFilter>& backdrop_filter = nullptr,
ContentBoundsPromise bounds_promise = ContentBoundsPromise::kUnknown,
uint32_t total_content_depth = kMaxDepth,
bool can_distribute_opacity = false,
bool bounds_from_caller = false);
bool can_distribute_opacity = false);

virtual bool Restore();

Expand Down
57 changes: 26 additions & 31 deletions impeller/aiks/experimental_canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,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 @@ -313,15 +315,9 @@ void ExperimentalCanvas::SaveLayer(
const std::shared_ptr<ImageFilter>& backdrop_filter,
ContentBoundsPromise bounds_promise,
uint32_t total_content_depth,
bool can_distribute_opacity,
bool bounds_from_caller) {
bool can_distribute_opacity) {
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 @@ -345,13 +341,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 @@ -366,12 +355,33 @@ 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, //
/*flood_output_coverage=*/
Entity::IsBlendModeDestructive(paint.blend_mode), //
Copy link
Contributor

@flar flar Aug 30, 2024

Choose a reason for hiding this comment

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

Should this also examine the paint.color_filter?

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 30, 2024

Choose a reason for hiding this comment

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

We don't know that bit currently, but we will eventually: flutter/flutter#154035

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 30, 2024

Choose a reason for hiding this comment

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

(once we delete impeller::ColorFilter)

/*flood_input_coverage=*/!!backdrop_filter //
);

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 @@ -408,21 +418,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
3 changes: 1 addition & 2 deletions impeller/aiks/experimental_canvas.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,7 @@ class ExperimentalCanvas : public Canvas {
const std::shared_ptr<ImageFilter>& backdrop_filter,
ContentBoundsPromise bounds_promise,
uint32_t total_content_depth,
bool can_distribute_opacity,
bool bounds_from_caller) override;
bool can_distribute_opacity) override;

bool Restore() override;

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<Texture> 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 @@ -16,8 +16,6 @@ namespace impeller {
struct Picture {
std::unique_ptr<EntityPass> pass;

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

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

private:
Expand Down
17 changes: 2 additions & 15 deletions impeller/display_list/dl_dispatcher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,23 +722,10 @@ 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(),
options.bounds_from_caller());
options.can_distribute_opacity());
Copy link
Contributor

@flar flar Aug 29, 2024

Choose a reason for hiding this comment

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

I think you also need options.content_is_unbounded().

  • The input will be unbounded if this SL has a BDF.
  • The input ?might? also be unbounded if this SL is applied with a flooding CF (I'm still wrapping my brain around that case).
  • But, the input will also be unbounded if any child content between SL and Restore is itself output-unbounded. You can't tell this case from the paint object or the presence of a BDF, you have to look at the flags in the options.

The supplied bounds are still provided because the flag overrides them and we did calculate them, so why not, but they don't promise that the content of this SL is bounded if that flag is present.

If there is unbounded child content, DL might substitute the clip at the moment that content is discovered - but only if that enclosing clip is inside the same DL/layer. If there has not yet been a restricting clip before we get to the unbounded content inside the SL, then the SL itself is marked unbounded via the options.

So, there are 3 conditions that lead to unbounded contents.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 29, 2024

Choose a reason for hiding this comment

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

The unbounded content should be handled in the line about where I replace the content bounds with std::nullopt:

https://github.com/flutter/engine/blob/main/impeller/display_list/dl_dispatcher.cc#L726-L728

this doesn't solve the problem of impeller/dl speaking different terms of unbounded. Longer term, we should probably just plumb this flag through.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 29, 2024

Choose a reason for hiding this comment

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

The input ?might? also be unbounded if this SL is applied with a flooding CF (I'm still wrapping my brain around that case).

We handle that by only applying clear color on srcOver, but layer on that is the flood clip check in entity_pass

Copy link
Contributor

@flar flar Aug 29, 2024

Choose a reason for hiding this comment

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

The unbounded line was deleted. You are pointing to what the code "used to do"...

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 29, 2024

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor

@flar flar Aug 30, 2024

Choose a reason for hiding this comment

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

I have to think more about that. The unbounded is a reason to ignore the bounds, but I'm not sure that user supplied bounds are a reason to use them even in the unbounded case.

Scratches head...

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 30, 2024

Choose a reason for hiding this comment

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

Oh right, we discussed this! That is why I deleted the line, but I need to add it back.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 30, 2024

Choose a reason for hiding this comment

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

OK, removed bounds from caller. We just check if its unbounded.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 30, 2024

Choose a reason for hiding this comment

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

agh, I think we hit this already. If a saveLayer contains unbounded content, but does not have a bdf or destructive blend, but the saveLayer was bounded - then bounds.is_clipped should be true and we don't null out the bds.

Copy link
Contributor Author

@jonahwilliams jonahwilliams Aug 30, 2024

Choose a reason for hiding this comment

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

Okay so this is a case where maybe there is actually a DL misunderstanding. On the test ImageFilteredSaveLayerWithUnboundedContents we expect the content to be clipped via the saveLayer bounds. This saveLayer has a specified bounds of (0, 0, 200, 200) but options.content_is_unbounded is true.

I guess, its technically correct that this saveLayer, which contains a drawPaint, is unbounded - but we decided to treat is bounded so maybe content_is_unbounded should return false?

}

// |flutter::DlOpReceiver|
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 @@ -206,6 +206,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 @@ -260,6 +262,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