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 1 commit
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
Prev Previous commit
Next Next commit
++
  • Loading branch information
jonahwilliams committed Aug 20, 2024
commit a646c1bbf0dc6048023ba80f011b39e625046690
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_from_caller);
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 @@ -77,8 +77,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
18 changes: 8 additions & 10 deletions impeller/aiks/experimental_canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,7 @@ 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 (!clip_coverage_stack_.HasCoverage()) {
Expand Down Expand Up @@ -354,14 +353,13 @@ void ExperimentalCanvas::SaveLayer(
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);
std::optional<Rect> maybe_subpass_coverage =
ComputeSaveLayerCoverage(bounds.value_or(Rect::MakeMaximum()),
transform_stack_.back().transform, //
coverage_limit, //
filter_contents, //
/*has_backdrop_filter=*/!!backdrop_filter //
);

if (!maybe_subpass_coverage.has_value() ||
maybe_subpass_coverage->IsEmpty()) {
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
3 changes: 1 addition & 2 deletions impeller/display_list/dl_dispatcher.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,8 +625,7 @@ void DlDispatcherBase::saveLayer(const SkRect& bounds,
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() && !backdrop);
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
8 changes: 2 additions & 6 deletions impeller/entity/entity_pass.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,10 +56,8 @@ void EntityPass::SetDelegate(std::shared_ptr<EntityPassDelegate> delegate) {
delegate_ = std::move(delegate);
}

void EntityPass::SetBoundsLimit(std::optional<Rect> bounds_limit,
bool bounds_from_caller) {
void EntityPass::SetBoundsLimit(std::optional<Rect> bounds_limit) {
bounds_limit_ = bounds_limit;
bounds_from_caller_ = bounds_from_caller;
}

std::optional<Rect> EntityPass::GetBoundsLimit() const {
Expand Down Expand Up @@ -488,9 +486,7 @@ EntityPass::EntityResult EntityPass::GetEntityForElement(
subpass->transform_, //
coverage_limit.value(), //
image_filter, //
/*destructive_blend=*/subpass->flood_clip_, //
/*has_backdrop_filter=*/!!subpass_backdrop_filter_contents, //
/*bounds_from_caller=*/subpass->bounds_from_caller_ //
/*has_backdrop_filter=*/!!subpass_backdrop_filter_contents //
);

if (!subpass_coverage.has_value()) {
Expand Down
4 changes: 1 addition & 3 deletions impeller/entity/entity_pass.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ class EntityPass {
/// specified in the saveLayer call and must be treated as a clip that
/// restricts the size of the saveLayer, even if that layer has a bdf
/// or flood.
void SetBoundsLimit(std::optional<Rect> content_bounds,
bool bounds_from_caller);
void SetBoundsLimit(std::optional<Rect> content_bounds);

/// @brief Get the bounds limit, which is provided by the user when creating
/// a SaveLayer.
Expand Down Expand Up @@ -293,7 +292,6 @@ class EntityPass {
BlendMode blend_mode_ = BlendMode::kSourceOver;
bool flood_clip_ = false;
std::optional<Rect> bounds_limit_;
bool bounds_from_caller_ = false;
int32_t required_mip_count_ = 1;

/// These values indicate whether something has been added to the EntityPass
Expand Down
9 changes: 3 additions & 6 deletions impeller/entity/save_layer_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,21 +11,18 @@ std::optional<Rect> ComputeSaveLayerCoverage(
const Matrix& effect_transform,
const Rect& coverage_limit,
const std::shared_ptr<FilterContents>& image_filter,
bool destructive_blend,
bool has_backdrop_filter,
bool bounds_from_caller) {
bool has_backdrop_filter) {
// If the saveLayer is unbounded, the coverage is the same as the
// coverage limit. By default, the coverage limit begins as the screen
// coverage. Either a lack of bounds, or the presence of a backdrop
// effecting paint property indicates that the saveLayer is unbounded.
// The o ne special case is when the input coverage was specified by the
// The one special case is when the input coverage was specified by the
// developer, in this case we still use the content coverage even if the
// saveLayer itself is unbounded.
bool save_layer_is_unbounded = has_backdrop_filter || destructive_blend;
// Otherwise, the save layer is bounded by either its contents or by
// a specified coverage limit. In these cases the coverage value is used
// and intersected with the coverage limit.
Rect input_coverage = (save_layer_is_unbounded && !bounds_from_caller)
Rect input_coverage = (has_backdrop_filter)
? Rect::MakeMaximum()
: content_coverage;

Expand Down
4 changes: 1 addition & 3 deletions impeller/entity/save_layer_utils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,7 @@ std::optional<Rect> ComputeSaveLayerCoverage(
const Matrix& effect_transform,
const Rect& coverage_limit,
const std::shared_ptr<FilterContents>& image_filter,
bool destructive_blend = false,
bool has_backdrop_filter = false,
bool bounds_from_caller = false);
bool has_backdrop_filter = false);

} // namespace impeller

Expand Down
71 changes: 12 additions & 59 deletions impeller/entity/save_layer_utils_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,27 +26,13 @@ TEST(SaveLayerUtilsTest, SimplePaintComputedCoverage) {
EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 10, 10));
}

TEST(SaveLayerUtilsTest, DestructivePaintComputedCoverage) {
// Destructive paint, computed coverage
auto coverage = ComputeSaveLayerCoverage(
/*content_coverage=*/Rect::MakeLTRB(0, 0, 10, 10), //
/*effect_transform=*/{}, //
/*coverage_limit=*/Rect::MakeLTRB(0, 0, 2400, 1800), //
/*image_filter=*/nullptr, //
/*destructive_blend=*/true //
);
ASSERT_TRUE(coverage.has_value());
EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 2400, 1800));
}

TEST(SaveLayerUtilsTest, BackdropFiterComputedCoverage) {
// Backdrop Filter, computed coverage
auto coverage = ComputeSaveLayerCoverage(
/*content_coverage=*/Rect::MakeLTRB(0, 0, 10, 10), //
/*effect_transform=*/{}, //
/*coverage_limit=*/Rect::MakeLTRB(0, 0, 2400, 1800), //
/*image_filter=*/nullptr, //
/*destructive_blend=*/false, //
/*has_backdrop_filter=*/true //
);

Expand Down Expand Up @@ -130,23 +116,22 @@ TEST(SaveLayerUtilsTest, DisjointCoverageTransformedByImageFilter) {
);

ASSERT_TRUE(coverage.has_value());
// Is this the right value? should it actually be (0, 0, 10, 10)?
EXPECT_EQ(coverage.value(), Rect::MakeLTRB(200, 200, 210, 210));
}

// TEST(SaveLayerUtilsTest, DisjointCoverageNotTransformedByCTM) {
// // Coverage disjoint from parent coverage. CTM does not impact lack of
// // intersection as it has already been "absorbed" by child coverage.
// Matrix ctm = Matrix::MakeTranslation({-200, -200, 0});
// auto coverage = ComputeSaveLayerCoverage(
// /*content_coverage=*/Rect::MakeLTRB(200, 200, 210, 210), //
// /*effect_transform=*/ctm, //
// /*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), //
// /*image_filter=*/nullptr //
// );
TEST(SaveLayerUtilsTest, DisjointCoveragTransformedByCTM) {
// Coverage disjoint from parent coverage.
Matrix ctm = Matrix::MakeTranslation({-200, -200, 0});
auto coverage = ComputeSaveLayerCoverage(
/*content_coverage=*/Rect::MakeLTRB(200, 200, 210, 210), //
/*effect_transform=*/ctm, //
/*coverage_limit=*/Rect::MakeLTRB(0, 0, 100, 100), //
/*image_filter=*/nullptr //
);

// ASSERT_FALSE(coverage.has_value());
// }
ASSERT_TRUE(coverage.has_value());
EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 10, 10));
}

TEST(SaveLayerUtilsTest, BasicEmptyCoverage) {
auto coverage = ComputeSaveLayerCoverage(
Expand Down Expand Up @@ -181,45 +166,13 @@ TEST(SaveLayerUtilsTest, BackdropFilterEmptyCoverage) {
/*effect_transform=*/{}, //
/*coverage_limit=*/Rect::MakeLTRB(0, 0, 2400, 1800), //
/*image_filter=*/nullptr, //
/*destructive_blend=*/false, //
/*has_backdrop_filter=*/true //
);

ASSERT_TRUE(coverage.has_value());
EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 2400, 1800));
}

TEST(SaveLayerUtilsTest, DestructivePaintUserSpecifiedBounds) {
// Bounds from caller overrides clip flooding.
auto coverage = ComputeSaveLayerCoverage(
/*content_coverage=*/Rect::MakeLTRB(0, 0, 10, 10), //
/*effect_transform=*/{}, //
/*coverage_limit=*/Rect::MakeLTRB(0, 0, 2400, 1800), //
/*image_filter=*/nullptr, //
/*destructive_blend=*/true, //
/*has_backdrop_filter=*/false, //
/*bounds_from_caller=*/true //
);

ASSERT_TRUE(coverage.has_value());
EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 10, 10));
}

TEST(SaveLayerUtilsTest, BackdropFilterUserSpecifiedBounds) {
// Bounds from caller overrides bdf flooding.
auto coverage = ComputeSaveLayerCoverage(
/*content_coverage=*/Rect::MakeLTRB(0, 0, 10, 10), //
/*effect_transform=*/{}, //
/*coverage_limit=*/Rect::MakeLTRB(0, 0, 2400, 1800), //
/*image_filter=*/nullptr, //
/*destructive_blend=*/false, //
/*has_backdrop_filter=*/true, //
/*bounds_from_caller=*/true //
);

ASSERT_TRUE(coverage.has_value());
EXPECT_EQ(coverage.value(), Rect::MakeLTRB(0, 0, 10, 10));
}

} // namespace testing
} // namespace impeller
Expand Down