-
Notifications
You must be signed in to change notification settings - Fork 6.1k
[Impeller] Reland: Use the scissor to limit all draws by clip coverage. #51731
Conversation
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.movAfter: Screen.Recording.2024-03-27.at.3.14.29.PM.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
The new golden is missing for some reason. I'm going to push a blank commit to see if it shows up... |
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. |
…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
At least on the flutter gallery benchmark, we're back to the pre StC baseline: |
…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
…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
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