[Impeller] disable entity culling by default. #48717

jonahwilliams · 2023-12-06T05:43:01Z

From local testing across both flutter galleries, wonderous, some other test apps, the only time this code successfully culls is during the stretch overscroll (and we cull 1 or so entries). The cost of this culling is approximately 20% of entity rendering time, and about 5% of overall raster time.

flutter-dashboard · 2023-12-06T16:35:44Z

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 #48717 at sha 6603682

jonahwilliams · 2023-12-06T16:38:49Z

Test failure is interesting, might need to also render in the case where clip coverage is empty:

engine/impeller/aiks/aiks_unittests.cc

Lines 371 to 392 in fa632d9

    
           TEST_P(AiksTest, CanRenderWithContiguousClipRestores) { 
        
             Canvas canvas; 
        
             // Cover the whole canvas with red. 
        
             canvas.DrawPaint({.color = Color::Red()}); 
        
             canvas.Save(); 
        
             // Append two clips, the second resulting in empty coverage. 
        
             canvas.ClipPath( 
        
                 PathBuilder{}.AddRect(Rect::MakeXYWH(100, 100, 100, 100)).TakePath()); 
        
             canvas.ClipPath( 
        
                 PathBuilder{}.AddRect(Rect::MakeXYWH(300, 300, 100, 100)).TakePath()); 
        
             // Restore to no clips. 
        
             canvas.Restore(); 
        
             // Replace the whole canvas with green. 
        
             canvas.DrawPaint({.color = Color::Green()}); 
        
             ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); 
        
           }

gaaclarke · 2023-12-06T16:40:37Z

From local testing across both flutter galleries, wonderous, some other test apps, the only time this code successfully culls is during the stretch overscroll (and we cull 1 or so entries).

Why only in that case? Is the framework performing its own culling? Or are display lists doing it?

gaaclarke · 2023-12-06T16:42:09Z

impeller/entity/entity.cc

+  if (!clip_coverage.has_value()) {
+    return false;
+  }
+#ifdef IMPELLER_CONTENT_CULLING


Do we have a planned end-of-line for this flag? If we decide we don't want ShouldRender, we should just rip it out. It's going to atrophy otherwise.

We're going to turn this on in impeller-cmake for now (which isn't yet located in the engine repo) since AAOS isn't using DisplayList and this is the only frustum culling mechanism in Impeller.

jonahwilliams · 2023-12-06T16:45:48Z

The display list does culling during recording, so we are currently attempting to cull twice

bdero

LGTM

gaaclarke

Do we have test coverage for the logic that we are now putting behind the a build compile flag? I don't want that to atrophy. We need to add one or make sure we have existing ones we are happy with.

jonahwilliams · 2023-12-06T17:38:48Z

Since this is ifdef'd out at the Entity level, the content level testing for culling is still valid:

https://github.com/flutter/engine/blob/main/impeller/entity/entity_unittests.cc#L1576-L1630

gaaclarke

lgtm! i just wanted to make sure we didn't accidentally break this while some people are using it but it is turned off by default, thanks.

…ne#48717)

…139716) flutter/engine@347f506...82de334 2023-12-07 [email protected] Revert "Replace use of Fontmgr::RefDefault with explicit creation calls" (flutter/engine#48755) 2023-12-07 [email protected] Roll Skia from 2abb01e18ab3 to 8ebf43ba1c09 (1 revision) (flutter/engine#48761) 2023-12-07 [email protected] Roll Dart SDK from dbfe4e3f867e to be8a95b6717d (1 revision) (flutter/engine#48757) 2023-12-06 [email protected] Remove obsolete properties. (flutter/engine#48753) 2023-12-06 [email protected] Roll Skia from 7ff0103760d0 to 2abb01e18ab3 (1 revision) (flutter/engine#48751) 2023-12-06 [email protected] Roll Skia from 570103e08087 to 7ff0103760d0 (1 revision) (flutter/engine#48748) 2023-12-06 [email protected] Roll Skia from 326bdc97ac40 to 570103e08087 (1 revision) (flutter/engine#48746) 2023-12-06 [email protected] Roll Dart SDK from 6eb13603c212 to dbfe4e3f867e (1 revision) (flutter/engine#48745) 2023-12-06 [email protected] [Impeller] Store Buffer/Texture bindings in vector instead of map. (flutter/engine#48719) 2023-12-06 [email protected] Roll Skia from 33cba437bf70 to 326bdc97ac40 (2 revisions) (flutter/engine#48743) 2023-12-06 [email protected] [Impeller] Provide the clear color to an advanced blend if it was optimized out (flutter/engine#48646) 2023-12-06 [email protected] [Windows] Set swap interval on raster thread after startup (flutter/engine#47787) 2023-12-06 [email protected] Roll Skia from 384d14063dc1 to 33cba437bf70 (3 revisions) (flutter/engine#48740) 2023-12-06 [email protected] [Windows] Refactor the GLES proc table (flutter/engine#48688) 2023-12-06 [email protected] Replace use of Fontmgr::RefDefault with explicit creation calls (flutter/engine#48571) 2023-12-06 [email protected] [Impeller] disable entity culling by default. (flutter/engine#48717) 2023-12-06 [email protected] Roll Skia from 23e1cb20a6b5 to 384d14063dc1 (1 revision) (flutter/engine#48738) 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

From local testing across both flutter galleries, wonderous, some other test apps, the only time this code successfully culls is during the stretch overscroll (and we cull 1 or so entries). The cost of this culling is approximately 20% of entity rendering time, and about 5% of overall raster time. ![image](https://github.com/flutter/engine/assets/8975114/ef85629c-48a8-457f-8385-0c8136404571)

[Impeller] disable entity culling by default.

bd1cedb

github-actions bot added the e: impeller label Dec 6, 2023

add test.

6603682

jonahwilliams marked this pull request as ready for review December 6, 2023 16:31

jonahwilliams requested review from bdero and gaaclarke December 6, 2023 16:31

flutter-dashboard bot added the will affect goldens label Dec 6, 2023

gaaclarke reviewed Dec 6, 2023

View reviewed changes

bdero approved these changes Dec 6, 2023

View reviewed changes

dont accidentally cull clips

7f4628a

gaaclarke suggested changes Dec 6, 2023

View reviewed changes

adjust so it can be tested still.

76184cc

gaaclarke approved these changes Dec 6, 2023

View reviewed changes

jonahwilliams added the autosubmit Merge PR when tree becomes green via auto submit App label Dec 6, 2023

auto-submit bot merged commit 2dbc5d2 into flutter:main Dec 6, 2023

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to ab89510f4e1e (3 revisions) flutter/flutter#139677

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

4f7cc97

…ne#48717)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to 05b4ac8a1070 (5 revisions) flutter/flutter#139680

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

2b74753

…ne#48717)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to c32eb048d55b (7 revisions) flutter/flutter#139681

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

42ff7d5

…ne#48717)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to 077e3b50ce72 (9 revisions) flutter/flutter#139684

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

f9b3755

…ne#48717)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to 64df738fdba0 (11 revisions) flutter/flutter#139686

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

9e25776

…ne#48717)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to 0e63da711f3f (12 revisions) flutter/flutter#139691

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

0b16d73

…ne#48717)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to a0d0515aa939 (13 revisions) flutter/flutter#139695

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

7380134

…ne#48717)

engine-flutter-autoroll mentioned this pull request Dec 6, 2023

Roll Flutter Engine from 347f506a341f to 6f5b6347d472 (14 revisions) flutter/flutter#139701

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 6, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

340dede

…ne#48717)

engine-flutter-autoroll mentioned this pull request Dec 7, 2023

Roll Flutter Engine from 347f506a341f to d16704bc9b10 (15 revisions) flutter/flutter#139711

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 7, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

877dbb3

…ne#48717)

engine-flutter-autoroll mentioned this pull request Dec 7, 2023

Roll Flutter Engine from 347f506a341f to 82de3347ccd7 (17 revisions) flutter/flutter#139716

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 7, 2023

2dbc5d2c5 [Impeller] disable entity culling by default. (flutter/engi…

eec47c4

…ne#48717)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Impeller] disable entity culling by default. #48717

[Impeller] disable entity culling by default. #48717

Uh oh!

jonahwilliams commented Dec 6, 2023 •

edited

Loading

Uh oh!

flutter-dashboard bot commented Dec 6, 2023

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

gaaclarke commented Dec 6, 2023

Uh oh!

gaaclarke Dec 6, 2023

Uh oh!

bdero Dec 6, 2023 •

edited

Loading

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

bdero left a comment

Uh oh!

gaaclarke left a comment

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

gaaclarke left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[Impeller] disable entity culling by default. #48717

[Impeller] disable entity culling by default. #48717

Uh oh!

Conversation

jonahwilliams commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flutter-dashboard bot commented Dec 6, 2023

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

gaaclarke commented Dec 6, 2023

Uh oh!

gaaclarke Dec 6, 2023

Choose a reason for hiding this comment

Uh oh!

bdero Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

bdero left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Dec 6, 2023

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonahwilliams commented Dec 6, 2023 •

edited

Loading

bdero Dec 6, 2023 •

edited

Loading