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

[Impeller] Reland: Use the scissor to limit all draws by clip coverage. #51731

Merged
merged 4 commits into from
Mar 28, 2024

Conversation

bdero
Copy link
Member

@bdero bdero commented Mar 27, 2024

Reland #51698.

This reverts commit 73c145c.

Attempts to improve flutter/flutter#145274.

Our new clipping technique paints walls on the depth buffer "in front" of the Entities that will be affected by said clips. So when an intersect clip is drawn (the common case), the clip will cover the whole framebuffer.

Depth is divvied up such that deeper clips get drawn behind shallower clips, and so many clips actually don't end up drawing depth across the whole framebuffer. However, if the app does a lot of transitioning from a huge clips to a small clips, a lot of unnecessary depth churn occurs (very common in Flutter -- this happens with both the app bar and floating action button in the counter template, for example).

Since progressively deeper layers in the clip coverage stack always subset their parent layers, we can reduce waste for small intersect clips by setting the scissor to the clip coverage rect instead of drawing the clip to the whole screen.

Note that this change does not help much with huge/fullscreen clips.

Also, we could potentially improve this further by computing much stricter bounds. Rather than just using clip coverage for the scissor, we could intersect it with the union of all draws affected by the clip at the cost of a bit more CPU churn per draw. I don't think that's enough juice for the squeeze though.

Before (Play/AiksTest.CanRenderNestedClips/Metal):

Screen.Recording.2024-03-26.at.7.34.29.PM.mov

After (Play/AiksTest.CanRenderNestedClips/Metal):

Screen.Recording.2024-03-26.at.7.38.33.PM.mov

@bdero
Copy link
Member Author

bdero commented Mar 27, 2024

The new golden reproduces the issue that triggered the revert.

The problem was that the MSAA backdrop restore was getting erroneously limited by the clip coverage scissor. The fix is to just draw the restore before replaying the clips.

Before:

Screen.Recording.2024-03-27.at.3.13.55.PM.mov

After:

Screen.Recording.2024-03-27.at.3.14.29.PM.mov

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 added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2024
@bdero bdero removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 27, 2024
@bdero
Copy link
Member Author

bdero commented Mar 27, 2024

The new golden is missing for some reason. I'm going to push a blank commit to see if it shows up...

@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 #51731 at sha 638dabb

@bdero bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 28, 2024
@auto-submit auto-submit bot merged commit 918bf19 into flutter:main Mar 28, 2024
29 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 28, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 28, 2024
…145877)

flutter/engine@c602abd...922c7b1

2024-03-28 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from Lk8KBU-c97ROj-YHm... to uzI3wnbEGlZ_dtO0Z... (flutter/engine#51737)
2024-03-28 skia-flutter-autoroll@skia.org Roll Skia from b6160ffc0b96 to 66241be7c81c (16 revisions) (flutter/engine#51733)
2024-03-28 bdero@google.com [Impeller] Reland: Use the scissor to limit all draws by clip coverage.  (flutter/engine#51731)
2024-03-27 30870216+gaaclarke@users.noreply.github.com [Impeller] adds a `plus` advanced blend for f16 pixel formats (flutter/engine#51589)
2024-03-27 maRci002@users.noreply.github.com Platform channel for predictive back in route transitions on android (flutter/engine#49093)
2024-03-27 jonahwilliams@google.com [Impeller] render empty filled paths without crashing. (flutter/engine#51713)
2024-03-27 jonahwilliams@google.com [Impeller] use optimal depth attachment, remove useless barrier. (flutter/engine#51723)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Lk8KBU-c97RO to uzI3wnbEGlZ_

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 jacksongardner@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
auto-submit bot added a commit to flutter/flutter that referenced this pull request Mar 28, 2024
…isions) (#145877)" (#145901)

Reverts: #145877
Initiated by: zanderso
Reason for reverting: #145899
Original PR Author: engine-flutter-autoroll

Reviewed By: {fluttergithubbot}

This change reverts the following previous change:

flutter/engine@c602abd...922c7b1

2024-03-28 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from Lk8KBU-c97ROj-YHm... to uzI3wnbEGlZ_dtO0Z... (flutter/engine#51737)
2024-03-28 skia-flutter-autoroll@skia.org Roll Skia from b6160ffc0b96 to 66241be7c81c (16 revisions) (flutter/engine#51733)
2024-03-28 bdero@google.com [Impeller] Reland: Use the scissor to limit all draws by clip coverage.  (flutter/engine#51731)
2024-03-27 30870216+gaaclarke@users.noreply.github.com [Impeller] adds a `plus` advanced blend for f16 pixel formats (flutter/engine#51589)
2024-03-27 maRci002@users.noreply.github.com Platform channel for predictive back in route transitions on android (flutter/engine#49093)
2024-03-27 jonahwilliams@google.com [Impeller] render empty filled paths without crashing. (flutter/engine#51713)
2024-03-27 jonahwilliams@google.com [Impeller] use optimal depth attachment, remove useless barrier. (flutter/engine#51723)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Lk8KBU-c97RO to uzI3wnbEGlZ_

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 jacksongardner@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
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 28, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 28, 2024
…sions) (#145903)

Manual roll requested by zra@google.com

flutter/engine@c602abd...9df2d3a

2024-03-28 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Impeller] adds a `plus` advanced blend for f16 pixel formats (#51589)" (flutter/engine#51741)
2024-03-28 jonahwilliams@google.com [Impeller] correct multisample resolve/store configuration for Vulkan. (flutter/engine#51740)
2024-03-28 matanlurey@users.noreply.github.com Remove Impeller/OpenGLES from CI branch for Android e2e tests. (flutter/engine#51734)
2024-03-28 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from Lk8KBU-c97ROj-YHm... to uzI3wnbEGlZ_dtO0Z... (flutter/engine#51737)
2024-03-28 skia-flutter-autoroll@skia.org Roll Skia from b6160ffc0b96 to 66241be7c81c (16 revisions) (flutter/engine#51733)
2024-03-28 bdero@google.com [Impeller] Reland: Use the scissor to limit all draws by clip coverage.  (flutter/engine#51731)
2024-03-27 30870216+gaaclarke@users.noreply.github.com [Impeller] adds a `plus` advanced blend for f16 pixel formats (flutter/engine#51589)
2024-03-27 maRci002@users.noreply.github.com Platform channel for predictive back in route transitions on android (flutter/engine#49093)
2024-03-27 jonahwilliams@google.com [Impeller] render empty filled paths without crashing. (flutter/engine#51713)
2024-03-27 jonahwilliams@google.com [Impeller] use optimal depth attachment, remove useless barrier. (flutter/engine#51723)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from Lk8KBU-c97RO to uzI3wnbEGlZ_

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 jacksongardner@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 subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

2 participants