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
dont overwrite save layer bounds.
  • Loading branch information
jonahwilliams committed Aug 19, 2024
commit 3591395e45846d2f653bfbda83141408e959788a
8 changes: 7 additions & 1 deletion display_list/dl_builder.cc
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

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

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

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

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

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

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

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

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

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
6 changes: 1 addition & 5 deletions impeller/display_list/dl_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -622,11 +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);
}

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
14 changes: 4 additions & 10 deletions impeller/entity/save_layer_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ std::optional<Rect> ComputeSaveLayerCoverage(
// 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.
Expand All @@ -40,22 +39,17 @@ std::optional<Rect> ComputeSaveLayerCoverage(
// into any computed bounds. That is, a canvas scaling transform of 0.5
// changes the coverage of the contained entities directly and doesnt need to
// be additionally incorporated into the coverage computation here.
std::optional<Rect> result_bounds;
if (image_filter) {
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.

result_bounds = input_coverage.Intersection(source_coverage_limit.value());

// Finally, transform the resulting bounds back into the global coordinate
// space.
if (result_bounds.has_value()) {
return result_bounds->TransformBounds(effect_transform);
}
return std::nullopt;
// Transform the input coverage into the global coordinate space before
// computing the bounds limit intersection.
return input_coverage.TransformBounds(effect_transform)
.Intersection(source_coverage_limit.value());
}

// If the input coverage is maximum, just return the coverage limit that
Expand Down