Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Impeller] Absorb DrawPaints at the beginning of EntityPasses #40675

Merged
merged 3 commits into from
Mar 28, 2023
Merged

[Impeller] Absorb DrawPaints at the beginning of EntityPasses #40675

merged 3 commits into from
Mar 28, 2023

Conversation

bdero
Copy link
Member

@bdero bdero commented Mar 27, 2023

No description provided.

@bdero bdero requested a review from jonahwilliams March 27, 2023 19:59
@flutter-dashboard

This comment was marked as outdated.

Sign in to view
jonahwilliams
jonahwilliams approved these changes Mar 27, 2023
View reviewed changes
Copy link
Member

@jonahwilliams jonahwilliams 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 mentioned this pull request Mar 27, 2023
Closed
@jonahwilliams
Copy link
Member

This could probably do with a unit test. Can we detect that nothing has been drawn on an aiks canvas?

@chinmaygarde
Copy link
Member

I don' think this is quite right. It depends on the pass load action being something specific (Color::BlackTransparent). Which I suppose is true now for Aiks. We should be able to handle all clears (multiple redundant clears or calls before clears) one level below this.

@bdero
Copy link
Member Author

bdero commented Mar 27, 2023 •
edited
Loading

I don't think this PR will produce incorrect rendering results and I verified that it removes the first redundant fill, but I'm refactoring it to make it more aggressive/remove multiple fills and set the clear color.

@bdero bdero force-pushed the bdero/skip-black branch from 053c240 to d903a46 Compare March 28, 2023 02:25
@bdero
Copy link
Member Author

bdero commented Mar 28, 2023

Alright, I fixed this up to absorb back-to-back clears regardless of their color. Also managed to sneak in a test.

@bdero bdero force-pushed the bdero/skip-black branch from d903a46 to d46f3bd Compare March 28, 2023 03:01
@bdero bdero force-pushed the bdero/skip-black branch from d46f3bd to 59acbc8 Compare March 28, 2023 03:05
@bdero bdero changed the title [Impeller] Skip the first drawPaint if its transparent black [Impeller] Absorb DrawPaints at the beginning of EntityPasses Mar 28, 2023
@bdero bdero self-assigned this Mar 28, 2023
@bdero bdero merged commit 29c2845 into flutter:main Mar 28, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 28, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 28, 2023
@engine-flutter-autoroll
29c2845fd [Impeller] Absorb DrawPaints at the beginning of EntityPass…
2ee52bb 
…es (flutter/engine#40675)
@zanderso
Copy link
Member

SkiaPerf is showing this was a nice improvement:

https://flutter-flutter-perf.skia.org/e/?begin=1680019824&end=1680068938&keys=Xb61f60087363c82f825669970a96f51d&num_commits=50&request_type=1&xbaroffset=34046

https://flutter-flutter-perf.skia.org/e/?begin=1680019824&end=1680068938&keys=Xf0f0f379a989b7b62fec6f5b22baf3ee&num_commits=50&request_type=1&xbaroffset=34046

bdero added a commit that referenced this pull request Mar 30, 2023
@bdero
Revert "[Impeller] Absorb DrawPaints at the beginning of EntityPasses (…
4a4ef50 
…#40675)"

This reverts commit 29c2845.
@bdero bdero mentioned this pull request Mar 30, 2023
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jonahwilliams jonahwilliams Awaiting requested review from jonahwilliams

@chinmaygarde chinmaygarde Awaiting requested review from chinmaygarde

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.
4 participants
@bdero @jonahwilliams @chinmaygarde @zanderso