Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
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
flar review.
  • Loading branch information
jonahwilliams committed Aug 29, 2024
commit 0cae7a853ed154a4edbb170016bd8ab0c1dde359
5 changes: 4 additions & 1 deletion impeller/display_list/dl_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -722,7 +722,10 @@ void DlDispatcherBase::saveLayer(const SkRect& bounds,
auto promise = options.content_is_clipped()
? ContentBoundsPromise::kMayClipContents
: ContentBoundsPromise::kContainsContents;
std::optional<Rect> impeller_bounds = skia_conversions::ToRect(bounds);
std::optional<Rect> impeller_bounds;
if (!options.content_is_unbounded() || options.bounds_from_caller()) {
impeller_bounds = skia_conversions::ToRect(bounds);
}
GetCanvas().SaveLayer(paint, impeller_bounds, ToImageFilter(backdrop),
promise, total_content_depth,
options.can_distribute_opacity());
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 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

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

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

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

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor

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

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

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

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

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?

Expand Down
32 changes: 19 additions & 13 deletions impeller/entity/save_layer_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,37 +32,43 @@ std::optional<Rect> ComputeSaveLayerCoverage(
coverage = Rect::MakeMaximum();
}

// If flood_output_coverage is true, then the restore is applied with a
// destructive blend mode that requires flooding to the coverage limit.
// Technically we could only allocated a render target as big as the input
// coverage and then use a decal sampling mode to perform the flood. Returning
// the coverage limit is a correct but non optimal means of ensuring correct
// rendering.
if (flood_output_coverage) {
return coverage_limit;
}

// The content coverage must be scaled by any image filters present on the
// saveLayer paint. For example, if a saveLayer has a coverage limit of
// 100x100, but it has a Matrix image filter that scales by one half, the
// actual coverage limit is 200x200.
if (image_filter) {
// Transform the input coverage into the global coordinate space before
// computing the bounds limit intersection. This is the "worst case"
// coverage value before we intersect with the content coverage below.
std::optional<Rect> source_coverage_limit =
image_filter->GetSourceCoverage(effect_transform, coverage_limit);
if (!source_coverage_limit.has_value()) {
// No intersection with parent coverage limit.
return std::nullopt;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to wrap my head around this. Why aren't we handing in content_coverage? That might be the same as coverage_limit if there was flooding, but it could be less.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean exactly. A case where this would fire is, you an IF that translates a SL entirely offscreen, or somethng like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was a case of clicking on the wrong line. I was referring to the fact that you pass the coverage_limit in to the GetSourceCoverage call and I was pointing out that the content size might be much smaller.

But, in typing that out I suddenly realized that the point there is not to compute the actual result, but to compute the "worst case scenario for input pixels" against which you clip the indicated coverage. So, I think this is fine, but I didn't get the explanation on my first reading.

The intersection with the source coverage happens right after this comment, so this code is only making the current input coverage smaller - but based on the reverse filter size of the output limit - which means passing in coverage_limit is the right data to compute on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved the comment around a bit so it should be clearer.

// The image filter may change the coverage limit required to flood
// the parent layer. Returning the source coverage limit so that we
// can guarantee the render target is larger enough.
//
// See note below on flood_output_coverage.
if (flood_output_coverage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be "|| coverage.isMaximum()" like below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I think the intersection might be safe, but no reason to make it unecessarily.

return source_coverage_limit;
}

// Transform the input coverage into the global coordinate space before
// computing the bounds limit intersection.
return coverage.TransformBounds(effect_transform)
.Intersection(source_coverage_limit.value());
}

// If the input coverage is maximum, just return the coverage limit that
// is already in the global coordinate space.
if (coverage.IsMaximum()) {
//
// If flood_output_coverage is true, then the restore is applied with a
// destructive blend mode that requires flooding to the coverage limit.
// Technically we could only allocated a render target as big as the input
// coverage and then use a decal sampling mode to perform the flood. Returning
// the coverage limit is a correct but non optimal means of ensuring correct
// rendering.
if (flood_output_coverage | coverage.IsMaximum()) {
return coverage_limit;
}

Expand Down