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

[Impeller] Check the correct stencil coverage when deciding whether to elide a restore #39023

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

bdero
Copy link
Member

@bdero bdero commented Jan 20, 2023

Just noticed this while carefully reviewing the stencil coverage stack again today. This is much more rare/a subset of the original issue that was fixed by #38964, but could still result in lots of draw calls getting culled. Slightly modified the playground to detect this case too.

Before
Screenshot 2023-01-19 at 5 49 35 PM

After
Screenshot 2023-01-19 at 5 51 42 PM

@bdero bdero self-assigned this Jan 20, 2023
@bdero bdero added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 20, 2023
@bdero
Copy link
Member Author

bdero commented Jan 20, 2023

Correction: This wouldn't result in excess culling of draw calls -- it actually results in some stuff getting inverse clipped by anything above the restored value because the stencil check uses strict equality (as can be seen in the playground screenshot).

Mercury really needs to be in retrograde for an app to be affected by this, so it's not as much of an emergency, but could cause big weird problems for the poor soul that runs into it:

  1. We can only hit this if the stencil stack prior to restore happened to have empty coverage, and
  2. just like the original bug, multiple clips need to be getting restored by one restore entity. -- this isn't true. Actually any restore from empty coverage may break subsequent draws.

@auto-submit auto-submit bot merged commit 730e88f into flutter:main Jan 20, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 20, 2023
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Jan 20, 2023
…118869)

* 7bbe79e10 Extract WideToUTF16String/UTF16StringToWide to FML (flutter/engine#39006)

* acca56ce0 Revert "Extract WideToUTF16String/UTF16StringToWide to FML (#39006)" (flutter/engine#39019)

* 730e88fb6 [Impeller] Check the correct stencil coverage when deciding whether to elide a restore (flutter/engine#39023)
@bdero
Copy link
Member Author

bdero commented Jan 24, 2023

@zanderso Should we do a cherry pick for this one too? This isn't present on gallery/wondrous, but I won't say it doesn't worry me.

@zanderso
Copy link
Member

@bdero Yeah, we can. I only hadn't because I interpreted #39023 (comment) as saying that it wasn't worth a CP. I'll draw up the issue.

@bdero
Copy link
Member Author

bdero commented Jan 24, 2023

@zanderso Makes sense, thanks. When this came across my mind today I realized this doesn't seem terribly rare. Any restore from empty stencil coverage will break subsequent draws (including clips). The stencil buffer will basically be in a bad state that will only recover the next time a restore happens from non-empty stencil coverage.

The fact that it'll only happen when restoring from an empty clip is what's saving us. I bet stuff that regularly animates clips (e.g. rive animations) will trigger this.

ricardoamador pushed a commit to ricardoamador/engine that referenced this pull request Jan 25, 2023
XilaiZhang pushed a commit that referenced this pull request Jan 31, 2023
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
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants