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.

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

[Impeller] Reland: Switch from transient stencil-only to depth+stencil buffer. #49838

Merged
merged 5 commits into from
Jan 22, 2024

Conversation

bdero
Copy link
Member

@bdero bdero commented Jan 17, 2024

Part of flutter/flutter#138460.

In preparation for draw order optimization and StC.

Use a transient depth+stencil texture instead of a stencil-only texture. Doing this in isolation to detect/weed out any bugs with handling the attachment.

@bdero
Copy link
Member Author

bdero commented Jan 18, 2024

Okay, this took me a bit longer to work out than I anticipated, but I think this is good to go now!

The main problems were:

  • Default pipelines weren't getting set up right, which caused the validation error that triggered the revert.
  • The root GL surface wasn't configuring a depth attachment, which I believe was being masked by the aforementioned issue.
  • With both a depth and stencil attachment, the VK backend was inserting a redundant/invalid memory barrier when the depth/stencil texture is shared.

@bdero bdero marked this pull request as ready for review January 18, 2024 10:42
@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 #49838 at sha c1d8935

@bdero
Copy link
Member Author

bdero commented Jan 18, 2024

This is definitely not good to go, from the look of those goldens.

@bdero bdero marked this pull request as draft January 18, 2024 19:44
@flutter-dashboard
Copy link

This pull request has been changed to a draft. The currently pending flutter-gold status will not be able to resolve until a new commit is pushed or the change is marked ready for review again.

@bdero bdero force-pushed the bdero/depth2 branch 2 times, most recently from aebeb34 to 9a2f762 Compare January 20, 2024 09:22
@bdero bdero marked this pull request as ready for review January 20, 2024 23:54
@flutter-dashboard
Copy link

Golden file changes are available for triage from new commit, Click here to view.

Changes reported for pull request #49838 at sha 9a2f762

@bdero
Copy link
Member Author

bdero commented Jan 22, 2024

This is working now, problems were silly. I'm intentionally adding the depth attachment alongside the stencil as a baby step to make sure there's no extreme perf regression and/or validation problems. But I was setting the compare op to not always pass -- causing clearing passes to sometimes z-fight and non-clearing passes to compare against garbage depth.

@bdero
Copy link
Member Author

bdero commented Jan 22, 2024

Took me a surprisingly long time to realize what was wrong since I was never managed to reproduce the bad behavior via local goldens or Wondrous on iOS.

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!

@bdero bdero merged commit 4a398d3 into flutter:main Jan 22, 2024
26 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 22, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 22, 2024
…141978)

flutter/engine@d653559...b2762f4

2024-01-22 32242716+ricardoamador@users.noreply.github.com Add Mac cache_builder back into Prod (flutter/engine#49936)
2024-01-22 skia-flutter-autoroll@skia.org Roll Skia from cc042f38288e to be066a6524ab (2 revisions) (flutter/engine#49937)
2024-01-22 bdero@google.com [Impeller] Reland: Switch from transient stencil-only to depth+stencil buffer. (flutter/engine#49838)

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 jonahwilliams@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 join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: ✅ Done
2 participants