Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 38 commits into from
Aug 31, 2024

Conversation

jonahwilliams
Copy link
Member

@jonahwilliams jonahwilliams commented Aug 18, 2024

Separate out the math for computing a saveLayer's allocated texture size based on input bounds, paint, bdf, coverage limit, et cetera.

@jonahwilliams jonahwilliams marked this pull request as ready for review August 19, 2024 15:53
const Paint& paint,
const Matrix& effect_transform,
const Rect& coverage_limit,
bool user_provided_bounds = false);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this is dead code. Did you forget to push the entity_pass.cc changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I didn't change entity pass itself, just staged this separately. I could do that too though, if you'd like

Copy link
Member

Choose a reason for hiding this comment

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

Please do. It would be helpful to see user_provided_bounds being used in context.

@jonahwilliams
Copy link
Member Author

Lets see if it blends!

@bdero
Copy link
Member

bdero commented Aug 19, 2024

image
That's the spirit 😄

@jonahwilliams
Copy link
Member Author

Looks like two issues from the goldens:

  • advanced blends need to be treated like BDFs for devices without framebuffer fetch
  • Image matrix filters, yup.

@flutter-dashboard
Copy link

Golden file changes have been found for this pull request. Click here to view and triage (e.g. because this is an intentional change).

If you are still iterating on this change and are not ready to resolve the images on the Flutter Gold dashboard, consider marking this PR as a draft pull request above. You will still be able to view image results on the dashboard, commenting will be silenced, and the check will not try to resolve itself until marked ready for review.

Changes reported for pull request #54604 at sha 55a6a50

// 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
Member 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
Member 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
Member 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
Member 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
Member 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
Member 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?

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54604 at sha 1c55cf5

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54604 at sha a6f5f21

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

I'm leaving this as a "comment" review because it is just clarifying information which seems to conflict with the expectations here. It may require changes or maybe some rethinking about how to perform this process, but I think we need to be on the same page first.

GetCanvas().SaveLayer(paint, impeller_bounds, ToImageFilter(backdrop),
promise, total_content_depth,
options.can_distribute_opacity(),
options.bounds_from_caller());
options.bounds_from_caller() && !backdrop);
Copy link
Contributor

Choose a reason for hiding this comment

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

The "bounds from caller" hint isn't useful here AFAICT. If it matters then the bounds are being used wrong. I use it internally in the Builder to know if the recorded op has bounds from the caller or "dummy" bounds. I then combine the 2 in the restore call. At that point "bounds from caller" has essentially completed its entire "raison d'être" and is obsolete. I leave it in there because it isn't really useful to delete it - the receiver's shouldn't have any use for it.

In all cases the bounds are the bounds of the content of the saveLayer. How they were determined may be interesting, but not really relevant to the fact that they are accurately the bounds of what the code between the saveLayer and its corresponding restore draw, potentially clipped by the user supplied bounds (in which case the "clipped" flag will be set in the options.

When determining how much to allocate, you would determine:

  • If there is a BDF, then allocate enough to map to the current clip bounds.
  • If the BDF somehow were to indicate that it isn't going to draw into those entire bounds, you can take that into account, but then you would also need to factor in the contents.
  • If you don't query the BDF about its output intentions, or if the BDF does indicate that it will draw to the entire target area, then you are done and the clip bounds are the answer.
  • If there is no BDF, then the RT needs to cover the indicated bounds from the saveLayer call transformed by the CTM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Another way to look at this, the saveLayer will cause 2 kinds of rendering:

  • If there is a BDF, then the BDF will filter everything underneath the saveLayer and come up with some output pixels. That will produce output relative as per the BDF and the CTM of the saveLayer and the input of the BDF is the pixels from the parent RT which are in device pixels. That's one source of rendering.
  • Whether there is a BDF, the indicated saveLayer bounds will represent how much you need to hold the content that was between saveLayer and Restore. These bounds are relative to the CTM.
  • Combine those 2, and clip to the current clip bounds and that represents the pixels you want to capture for the eventual blit to the parent RT.

The saveLayer bounds may be irrelevant if the BDF says "I plan to render to the entire surface" then what does it matter what the saveLayer content does? But, if we ask the BDF "what's your output if you transform these pixels expressed in the device coordinate system by the BDF acting in the CTM?" then the 2 bounds may need to be combined.

@jonahwilliams jonahwilliams requested a review from flar August 20, 2024 14:53
@jonahwilliams
Copy link
Member Author

Yeah, fire away! I can wait on this one. @flar is reviewing further and I'd like to wait for his review too

@jonahwilliams
Copy link
Member Author

@flar and I iterated on this a bit offline and I'm going to make some adjustments based on those discussions, and see how it works against goldens. Good news is that I think we've reached saveLayer enlightenment - or at least as close as we need to be for now.

Also noticed: flutter/flutter#154035

@jonahwilliams
Copy link
Member Author

@flar this is updated based on our discussion last week. PTAL when you get the chance.

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

Too many questions to keep going on this pass. Hopefully their answers drive our mutual understanding towards a consensus.

Rect coverage = content_coverage;
// There are two conditions that currently trigger flood_input_coverage, the
// first is the presence of a backdrop filter and the second is the presence
// of a color filter that effects transparent black.
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 are talking about a CF or BDF on this SL itself?

Note that also a CF or a BDF on a child SL within the content of this SL can also flood. The options have a "content_is_unbounded" flag for that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added some clarifications, and the third case which has already been handled where the SL contains unbounded content.

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this override/overrule the previous test? Could they be swapped?

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 still not 100% sure on these 2 tests, though, but if they are correct then they can probably be swapped.)

Copy link
Member Author

Choose a reason for hiding this comment

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

They weren't quite right, I swapped the order.

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
Member 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
Member 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.

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

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
Member 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
Member 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
Member 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
Member 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
Member 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
Member 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
Member 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?

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

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

But, this return value will be used to control the allocation of an RT for the content of this SL. If there is a transforming IF then the coverage_limit might not be enough for that case.

Consider an IF that scales down by 50%. If you want to collect pixels so that they will be filtered to produce coverage_limit, then you need to collect 2x pixels in both directions, so you need coverage_limit*2 for the RT, which will then be processed by the IF to create pixels that will fill the coverage_limit.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh right, I was assuming that the IF could only make this smaller. But it could make it bigger too. If this case is true what I actually want is to return the source_coverage_limit instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I mean only add part of it back

Copy link
Member Author

Choose a reason for hiding this comment

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

woops wrong message

@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #54604 at sha b3ab833

coverage_limit, //
filter_contents, //
/*flood_output_coverage=*/
Entity::IsBlendModeDestructive(paint.blend_mode), //
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 examine the paint.color_filter?

Copy link
Member Author

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
Member Author

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)

bool flood_output_coverage,
bool flood_input_coverage) {
Rect coverage = content_coverage;
// There are three conditions that currently trigger flood_input_coverage, the
Copy link
Contributor

Choose a reason for hiding this comment

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

But currently only one condition sets this flag, which is !!BDF

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted this here

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

Just one question left I think

// 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
Member 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.

Copy link
Contributor

@flar flar left a comment

Choose a reason for hiding this comment

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

LGTM

@jonahwilliams jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 31, 2024
@auto-submit auto-submit bot merged commit 7cfef4b into flutter:main Aug 31, 2024
32 checks passed
@jonahwilliams jonahwilliams deleted the render_target_sizing branch August 31, 2024 00:50
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 3, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Sep 3, 2024
…154569)

flutter/engine@a3504c2...5373431

2024-09-03 skia-flutter-autoroll@skia.org Roll Skia from ab2317b94853 to 2d5a75027691 (3 revisions) (flutter/engine#54940)
2024-09-03 skia-flutter-autoroll@skia.org Roll Skia from 041fd378d332 to ab2317b94853 (3 revisions) (flutter/engine#54937)
2024-09-03 49699333+dependabot[bot]@users.noreply.github.com Bump actions/setup-python from 5.1.1 to 5.2.0 (flutter/engine#54933)
2024-09-03 skia-flutter-autoroll@skia.org Roll Skia from 49ea0f383706 to 041fd378d332 (1 revision) (flutter/engine#54932)
2024-09-03 robert.ancell@canonical.com Make FlApplication class (flutter/engine#54637)
2024-09-03 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from ksNJVM5ZoBB74rba_... to BaO2fyTu4jhvdTtdE... (flutter/engine#54931)
2024-09-02 skia-flutter-autoroll@skia.org Roll Skia from 3d0c9bf48176 to 49ea0f383706 (1 revision) (flutter/engine#54928)
2024-09-02 skia-flutter-autoroll@skia.org Roll Skia from 03bdb5c60304 to 3d0c9bf48176 (1 revision) (flutter/engine#54925)
2024-09-02 skia-flutter-autoroll@skia.org Roll Skia from c873eb5f38d5 to 03bdb5c60304 (1 revision) (flutter/engine#54924)
2024-09-02 skia-flutter-autoroll@skia.org Roll Skia from 514feab300b4 to c873eb5f38d5 (1 revision) (flutter/engine#54923)
2024-09-02 skia-flutter-autoroll@skia.org Roll Skia from 15641c0df7e8 to 514feab300b4 (1 revision) (flutter/engine#54922)
2024-09-01 skia-flutter-autoroll@skia.org Roll Skia from 80f2cd706443 to 15641c0df7e8 (1 revision) (flutter/engine#54919)
2024-09-01 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from uDdFN_cZC6F9BVTzz... to ksNJVM5ZoBB74rba_... (flutter/engine#54918)
2024-09-01 skia-flutter-autoroll@skia.org Roll Skia from 5477dbb533bb to 80f2cd706443 (1 revision) (flutter/engine#54916)
2024-09-01 skia-flutter-autoroll@skia.org Roll Skia from 95ef9caae482 to 5477dbb533bb (1 revision) (flutter/engine#54915)
2024-08-31 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from JKPo9G1NaAdstrimW... to uDdFN_cZC6F9BVTzz... (flutter/engine#54914)
2024-08-31 jonahwilliams@google.com [Impeller] separate algorithm for computing render target size. (flutter/engine#54604)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from JKPo9G1NaAds to BaO2fyTu4jhv

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC codefu@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller will affect goldens
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants