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.

Sign up for GitHub

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

Jump to bottom

[Impeller] Turn off Aiks bounds tracking for filtered SaveLayers. #49076

Merged
merged 1 commit into from
Dec 15, 2023
Merged

[Impeller] Turn off Aiks bounds tracking for filtered SaveLayers. #49076

merged 1 commit into from
Dec 15, 2023

Conversation

bdero
Copy link
Member

@bdero bdero commented Dec 15, 2023 •
edited
Loading

Resolves flutter/flutter#139294.

We use this bounds rect for querying the DisplayList rtree when culling. But when sub-DisplayLists are being dispatched, the DisplayList's rtree is local and doesn't incorporate parent layer filters. So sub-DisplayLists are getting culled as if no filters are being applied.

Technically our local bounds tracking in Aiks is actually correct here, but this is a quick fix to alleviate the problem. I don't know what the best longterm solution for this is.

@bdero bdero self-assigned this Dec 15, 2023
@bdero bdero mentioned this pull request Dec 15, 2023
Closed
2 tasks
@flar
Copy link
Contributor

flar commented Dec 15, 2023

Here is similar code in the DisplayListBuilder for reference:

engine/display_list/dl_builder.cc

Line 548 in 91f65ee

// We use |resetCullRect| here because we will be accumulating bounds of

engine/display_list/dl_builder.cc

Line 420 in 91f65ee

if (!accumulator()->restore(

flar
flar approved these changes Dec 15, 2023
View reviewed changes
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.

Safe, but we should look at some more optimum solutions at some point.

impeller/aiks/canvas.cc
// applied.
// See also: https://github.com/flutter/flutter/issues/139294
if (paint.image_filter) {
transform_stack_.back().cull_rect = 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.

Another technique could be to run the current cull_rect through the filter to get a reverse bounds, as in:

engine/impeller/entity/contents/filters/filter_contents.h

Line 137 in c48b08a

/// @brief Determines the coverage of source pixels that will be needed

Copy link
Contributor

Choose a reason for hiding this comment

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

Another optimization would be to see if the filter modifies location at all. The ColorFilterImageFilter would not need to clear the cull_rect, for instance.

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 don't think that's quite enough. In order to produce a correct cull rect, wouldn't we need to pass each new clip's bounds through the chain of all parent filters? Also, it would need to be forward bounds, not reverse, since the cull rect needs to expand for kernel filters.

Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is done for each filter on each restore:

https://github.com/flutter/engine/blob/832238a3b493bed828e9e6696c5b56b65a9abcda/display_list/dl_builder.cc#L418C1-L435C1

@bdero bdero merged commit ca329dd into flutter:main Dec 15, 2023
26 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Dec 15, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2023
@engine-flutter-autoroll
ca329dd84 [Impeller] Turn off Aiks bounds tracking for filtered SaveL…
868e94d 
…ayers. (flutter/engine#49076)
fluttermirroringbot pushed a commit to flutter/flutter that referenced this pull request Dec 16, 2023
@engine-flutter-autoroll
Roll Flutter Engine from 6e3a04874be5 to ca329dd84207 (3 revisions) (#…
388ba45 
…140255)

flutter/engine@6e3a048...ca329dd

2023-12-15 bdero@google.com [Impeller] Turn off Aiks bounds tracking for filtered SaveLayers. (flutter/engine#49076)
2023-12-15 skia-flutter-autoroll@skia.org Roll Skia from cf84bab13da3 to 191943f9c43e (1 revision) (flutter/engine#49105)
2023-12-15 skia-flutter-autoroll@skia.org Roll Dart SDK from e6528d1b7997 to d66f9c854db7 (1 revision) (flutter/engine#49104)

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 jsimmons@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
@bdero bdero mentioned this pull request Dec 18, 2023
Open
bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 18, 2023
@bdero
[Impeller] Turn off Aiks bounds tracking for filtered SaveLayers. (fl…
f8d8fec 
…utter#49076)

We use this bounds rect for querying the DisplayList rtree when culling.
But when sub-DisplayLists are being dispatched, the DisplayList's rtree
is local and doesn't incorporate parent layer filters. So
sub-DisplayLists are getting culled as if no filters are being applied.

Technically our local bounds tracking in Aiks is actually correct here,
but this is a quick fix to alleviate the problem. I don't know what the
best longterm solution for this is.

(cherry picked from commit ca329dd)
@bdero bdero mentioned this pull request Dec 18, 2023
Merged
bdero added a commit to bdero/flutter-engine that referenced this pull request Dec 18, 2023
@bdero
[Impeller] Turn off Aiks bounds tracking for filtered SaveLayers. (fl…
e1cd700 
…utter#49076)

We use this bounds rect for querying the DisplayList rtree when culling.
But when sub-DisplayLists are being dispatched, the DisplayList's rtree
is local and doesn't incorporate parent layer filters. So
sub-DisplayLists are getting culled as if no filters are being applied.

Technically our local bounds tracking in Aiks is actually correct here,
but this is a quick fix to alleviate the problem. I don't know what the
best longterm solution for this is.

(cherry picked from commit ca329dd)
auto-submit bot pushed a commit that referenced this pull request Dec 18, 2023
@bdero
[CP] [Impeller] Turn off Aiks bounds tracking for filtered SaveLayers. (
5a8e5c5 
#49198)

Cherry-pick for #49076.

For issue: flutter/flutter#139294
bryanoltman added a commit to shorebirdtech/engine that referenced this pull request Jan 2, 2024
@bryanoltman @zanderso
@bdero @XilaiZhang
Roll to Flutter 3.16.5 (#70)
001b600 
* [CP] Fix _availability_version_check for iOS 11 and 12 (flutter#48926)

CP of flutter@5587d26 into 3.16 for flutter/flutter#138711.

* [CP] [Impeller] Turn off Aiks bounds tracking for filtered SaveLayers. (flutter#49198)

Cherry-pick for flutter#49076.

For issue: flutter/flutter#139294

* [CP] Fix for AnimatedOpacity affects blended color overlay render (flutter#49190)

This PR cherry-picks the following two commits:

flutter@2dbc5d2
flutter@0b0fab8

to address flutter/flutter#139571

* [cherrypick-stable]Skip unexpected events in MultiPlatformViewBackgroundForegroundScenar… (flutter#49247)

�io (flutter#48456)

Fixes flutter/flutter#138193.

Was first attempted to fix in flutter#48096, but that was not reliable since it's all asynchronous. 

-------------------------------------------

cherry pick to fix mac unopt failures on stable branch: 
https://flutter-dashboard.appspot.com/#/build?repo=engine&branch=flutter-3.16-candidate.0

---------

Co-authored-by: Zachary Anderson <zanderso@users.noreply.github.com>
Co-authored-by: Brandon DeRosier <bdero@google.com>
Co-authored-by: Xilai Zhang <xilaizhang@google.com>
@bdero bdero mentioned this pull request Mar 14, 2024
Closed
DenisovAV pushed a commit to DenisovAV/flutter-tvos-engine that referenced this pull request May 5, 2024
@bdero @DenisovAV
[CP] [Impeller] Turn off Aiks bounds tracking for filtered SaveLayers…
4e8314e 
…. (#49198)

Cherry-pick for flutter/engine#49076.

For issue: flutter/flutter#139294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flar flar flar approved these changes

@jonahwilliams jonahwilliams Awaiting requested review from jonahwilliams

Assignees

@bdero bdero

Labels
e: impeller
Projects
No open projects
Impeller
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Impeller] ImageFiltered flickers when widget is rendered on top
2 participants
@bdero @flar