[Impeller] add basic culling checks during text frame dispatcher. #55168

jonahwilliams · 2024-09-13T03:33:13Z

Dl dispatching still relies on cull rects computed during that dispatch process. Make sure that the text frame dispatcher doesn't populate text frames that are way offscreen.

This culling is more conservative than the rendering dispatcher. We'd need to do some refactoring so the logic isn't repeated multiple times.

flar

Just a quick look while I'm OOO, but I had a couple of thoughts to consider...

flar · 2024-09-13T20:58:11Z

impeller/display_list/dl_dispatcher.h

 const ContentContext& renderer_;
 Matrix matrix_;
 std::vector<Matrix> stack_;
+ std::vector<Rect> cull_rect_state_;


I don't think you need a stack. At least not yet. SaveLayer bounds culling already happened so using a stack so that you can incorporate that into the culling of DL dispatching isn't likely to benefit much.

DL already culled - by failing to record - anything that was distinctly outside of a clip set in the DL stream, including the bounds of SaveLayers. So trying to intersect the cull_rect as you read through the current one isn't likely to improve the culling much compared to just culling against the (inverse transformed) frame bounds. If you remove the code in SaveLayer does it change the results much for your test case? If not then you don't really need a stack here until we do a full clip-based culling and even that would be icing on the cake rather than a critical step in the process.

We do still need to account for image filters, which aren't captured in the transform state. The locally transformed device bounds may be too small in some cases.

Are you sure? I don't think regular display list culling takes those into account. It just forwards the inverse-mapped clip bounds to the new Dispatch call.

Keep in mind that the bounds recorded in the RTree include any expansion due the filters applied to them, even including every filter on every saveLayer that they are inside of. That kind of inverts the responsibility there, but I'd have to look at it more to be sure this is right. If it is wrong then maybe we're doing the rendering side of culling wrong as well...

I'm sure the culling takes that into account, but this is more of a problem with the cull rect computation right? (assuming no other canvas ops) Suppose I have a NxM device rect and a saveLayer with a 2x scaling image filter on it. If I need to dispatch to a sub display list inside of that savelayer, It wouldn't be correct to use the NxM cull rect, I'd need a 2Nx2M cull rect.

For other transforms I can invert the transform matrix but we don't track the image filter transforms easily yet.

Within a display list that has already been taken care of by the way that the RTree rects are accumulated, but I think you are correct that it is as if there is an additional "transform" that goes on when you hit a saveLayer that has a filter. So, just as you have to take the original cull rect and un-transform it by the CTM, you also have to un-transform it by the actions of the SaveLayers for similar reasons.

So, yes, it is still an issue that this code punts on the filter un-transform of the savelayers as you mentioned in the code.

flar · 2024-09-13T20:59:15Z

shell/common/snapshot_controller_impeller.cc

+ context->GetContentContext(), //
+ impeller::Matrix(), //
+ impeller::Rect::MakeSize(render_target_size) //
+ );


This pattern of "first dispatch to a TFD and then to a DlDispatcher" happens in a number of places. It feels like a convenience method might be nice.

In #55019 I refactored this dispatching into a reusable static method.

flar · 2024-09-23T20:24:15Z

impeller/display_list/dl_dispatcher.cc

 }

 void TextFrameDispatcher::saveLayer(const DlRect& bounds,
 const flutter::SaveLayerOptions options,
 const flutter::DlImageFilter* backdrop) {
 save();
+


bounds here are in local coordinates, I think you need to transform them.

Also, your cull rect state doesn't look balanced. You always pop it in restore, but you push_back here. If save() doesn't push_back then restore will unbalance it. If save() does do a stack dup, then this code will over balance it...

My understanding was that the cull rect always need to be in local (to the display list) coordinate space. So I need to transform the cull rect into local space and then intersect with the bounds.

flar · 2024-09-23T20:55:23Z

impeller/display_list/dl_dispatcher.cc

+ if (paint_.image_filter) {
+ cull_rect_state_.push_back(bounds);
+ } else {
+ auto new_cull_rect = GetCurrentLocalCullingBounds().Intersection(bounds);


Reverse this - transform bounds - use raw device cull bounds

but bounds is in the local coordinate space, so I need the device cull rect transformed into local coordinates?

You need them both in the same coordinate space. The state rect is in device coordinates. You thus need to compute an answer in device coordinates. But, you transform the state rect into local coordinates to do the math and then push its result without transforming it back. Even if you transformed it back, it is less efficient then transforming the bounds to device coords and doing the math there - naturally ending up with an answer already in the desired answer's coordinate space.

flar · 2024-09-23T21:20:23Z

impeller/display_list/dl_dispatcher.cc

 }

 void TextFrameDispatcher::saveLayer(const DlRect& bounds,
 const flutter::SaveLayerOptions options,
 const flutter::DlImageFilter* backdrop) {
 save();
+
+ // This dispatcher does not track enough state to accurately compute
+ // cull rects with image filters.


For this, and other reasons, I think saveLayer should be a NOP.

First, DL has already done culling against things that appeared within the DL stream when it did the recording. So, the bounds you see here don't necessarily add any new information. The DL dispatch method is already culling things against the cull bounds that were provided in the initial dispatch call. DlBuilder already culled things against the bounds of save layers and clips when it did the recording. So, how is this particular enhancement really adding any value - other than some rare interactions where something might intersect the bounds here and also intersect the cull rect, but somehow not intersect the intersection of the two? If we were testing full geometries then strange cases like that might fall through the holes and we might be able to catch them with some more diligence here, but we are just testing bounds of the geometries, making it less likely to catch a corner case.

You could more easily do this same kind of calculation in the clip*() methods which don't have the fuzzy issues of saveLayer. Why choose saveLayer to enforce its possibly-fuzzily-potentially clipping when we aren't instrumenting the actual clip calls?

Really the important thing to track is how the filter manipulates the transform, but we aren't doing that. If anything, we should be setting the cull rect state to MaxRect here (like we have done in the non-experimental AIKS canvas) if there is a filter present, but that is not what the code here is doing...

I'd rather see this method just do:

if (filter) { cull_rect_state.back() = Maximum; }

Changed this to set to max cull rect if an image filter is encountered.

g

…ngine into basic_culling_text_frame

jonahwilliams · 2024-09-24T19:35:07Z

Okay, updated to use global coordinate space and to check for kMaxCullRect when dispatching.

flar

LGTM - suggestions...

flar · 2024-09-24T20:36:03Z

impeller/display_list/dl_dispatcher.cc

 stack_.emplace_back(matrix_);
+ if (push_cull_rect) {
+ cull_rect_state_.push_back(cull_rect_state_.back());


An easier way to do this would be to have Save() always push it, and then SL replaces the value on ToS (.back() = foo) when it updates it?

Done. Testing now

flar · 2024-09-24T20:38:00Z

impeller/display_list/dl_dispatcher.cc

+ has_image_filter_ = false;
+
+ auto local_cull_bounds = GetCurrentLocalCullingBounds();
+ if (!local_cull_bounds.IsMaximum()) {


if IsEmpty then skip it...?

…tcher. (flutter/engine#55168)

…155648) flutter/engine@559f2ff...746ce61 2024-09-25 [email protected] Roll Dart SDK from 7af8c4882e07 to eda9ae15367e (2 revisions) (flutter/engine#55417) 2024-09-25 [email protected] [Flutter GPU] Add CullMode. (flutter/engine#55409) 2024-09-25 [email protected] Roll Skia from 2e92f0b443f4 to 3541cdf2fae6 (1 revision) (flutter/engine#55412) 2024-09-24 [email protected] [Impeller] fix Impeller on windows. (flutter/engine#55323) 2024-09-24 [email protected] [Impeller] add basic culling checks during text frame dispatcher. (flutter/engine#55168) 2024-09-24 [email protected] Move each dart GN rule to a standalone file. (flutter/engine#55404) 2024-09-24 [email protected] Roll Skia from 118914b760cb to 2e92f0b443f4 (1 revision) (flutter/engine#55406) 2024-09-24 [email protected] [Impeller] Pack impeller:Path into 2 vecs instead of 3. (flutter/engine#55028) 2024-09-24 [email protected] Disallow time traveling frame times (flutter/engine#55310) 2024-09-24 [email protected] [Impeller] Delete command pools held by the CommandPoolRecyclerVK after decoding an image on the IO thread (flutter/engine#55398) 2024-09-24 [email protected] Roll Skia from cf28f9dd411d to 118914b760cb (1 revision) (flutter/engine#55405) 2024-09-24 [email protected] [Flutter GPU] Add pipeline stencil config. (flutter/engine#55272) 2024-09-24 [email protected] Roll Skia from 6e5ff9253147 to cf28f9dd411d (1 revision) (flutter/engine#55399) 2024-09-24 [email protected] [Impeller] delete expensive trace event. (flutter/engine#55400) 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 [email protected],[email protected],[email protected] 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

…lutter#155648) flutter/engine@559f2ff...746ce61 2024-09-25 [email protected] Roll Dart SDK from 7af8c4882e07 to eda9ae15367e (2 revisions) (flutter/engine#55417) 2024-09-25 [email protected] [Flutter GPU] Add CullMode. (flutter/engine#55409) 2024-09-25 [email protected] Roll Skia from 2e92f0b443f4 to 3541cdf2fae6 (1 revision) (flutter/engine#55412) 2024-09-24 [email protected] [Impeller] fix Impeller on windows. (flutter/engine#55323) 2024-09-24 [email protected] [Impeller] add basic culling checks during text frame dispatcher. (flutter/engine#55168) 2024-09-24 [email protected] Move each dart GN rule to a standalone file. (flutter/engine#55404) 2024-09-24 [email protected] Roll Skia from 118914b760cb to 2e92f0b443f4 (1 revision) (flutter/engine#55406) 2024-09-24 [email protected] [Impeller] Pack impeller:Path into 2 vecs instead of 3. (flutter/engine#55028) 2024-09-24 [email protected] Disallow time traveling frame times (flutter/engine#55310) 2024-09-24 [email protected] [Impeller] Delete command pools held by the CommandPoolRecyclerVK after decoding an image on the IO thread (flutter/engine#55398) 2024-09-24 [email protected] Roll Skia from cf28f9dd411d to 118914b760cb (1 revision) (flutter/engine#55405) 2024-09-24 [email protected] [Flutter GPU] Add pipeline stencil config. (flutter/engine#55272) 2024-09-24 [email protected] Roll Skia from 6e5ff9253147 to cf28f9dd411d (1 revision) (flutter/engine#55399) 2024-09-24 [email protected] [Impeller] delete expensive trace event. (flutter/engine#55400) 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 [email protected],[email protected],[email protected] 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

++

8edf328

github-actions bot added the e: impeller label Sep 13, 2024

jonahwilliams added 3 commits September 12, 2024 21:39

update dl playground.

2a1e880

++

a166676

++

3c3cbc7

jonahwilliams requested a review from flar September 13, 2024 16:22

jonahwilliams marked this pull request as ready for review September 13, 2024 16:22

flar reviewed Sep 13, 2024

View reviewed changes

Merge branch 'main' into basic_culling_text_frame

b6be14c

jonahwilliams requested a review from flar September 17, 2024 01:02

Merge branch 'main' into basic_culling_text_frame

82b8b06

flar requested changes Sep 23, 2024

View reviewed changes

jonahwilliams added 5 commits September 23, 2024 16:28

fix unbalanced save.'

d1e0b8a

g

Merge branch 'basic_culling_text_frame' of github.com:jonahwilliams/e…

2906092

…ngine into basic_culling_text_frame

++

90b85aa

keep in global coordinate space.

2397f96

++

a0b04d0

flar approved these changes Sep 24, 2024

View reviewed changes

jim review.

e620776

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 24, 2024

auto-submit bot merged commit 11d36ff into flutter:main Sep 24, 2024
30 checks passed

engine-flutter-autoroll mentioned this pull request Sep 25, 2024

Roll Flutter Engine from 559f2ff31c74 to 746ce6124844 (14 revisions) flutter/flutter#155648

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 25, 2024

11d36ff05 [Impeller] add basic culling checks during text frame dispa…

dc2bcdd

…tcher. (flutter/engine#55168)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Impeller] add basic culling checks during text frame dispatcher. #55168

[Impeller] add basic culling checks during text frame dispatcher. #55168

jonahwilliams commented Sep 13, 2024 •

edited

Loading

flar left a comment

flar Sep 13, 2024

jonahwilliams Sep 13, 2024

flar Sep 13, 2024

jonahwilliams Sep 13, 2024

flar Sep 14, 2024

flar Sep 13, 2024

jonahwilliams Sep 13, 2024

flar Sep 23, 2024

flar Sep 23, 2024

jonahwilliams Sep 23, 2024

flar Sep 23, 2024

jonahwilliams Sep 24, 2024

flar Sep 24, 2024

flar Sep 23, 2024

jonahwilliams Sep 24, 2024

jonahwilliams commented Sep 24, 2024

flar left a comment

flar Sep 24, 2024

jonahwilliams Sep 24, 2024

flar Sep 24, 2024

jonahwilliams Sep 24, 2024

[Impeller] add basic culling checks during text frame dispatcher. #55168

[Impeller] add basic culling checks during text frame dispatcher. #55168

Conversation

jonahwilliams commented Sep 13, 2024 • edited Loading

flar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Sep 24, 2024

flar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahwilliams commented Sep 13, 2024 •

edited

Loading