[Impeller] Fix clip management for DrawPicture. #44835
Conversation
|
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. |
|
|
||
| if (auto subpass = std::get_if<std::unique_ptr<EntityPass>>(&element)) { | ||
| subpass->get()->SetStencilDepth(subpass->get()->GetStencilDepth() + | ||
| GetStencilDepth()); |
There was a problem hiding this comment.
So this is the current stencil depth of the canvas that we are rendering the new picture into?
…132921) flutter/engine@394554b...8c7ce6d 2023-08-20 [email protected] Roll Skia from 4f6b9d08b6d1 to d2369dac4a1d (6 revisions) (flutter/engine#44883) 2023-08-20 [email protected] [Impeller] Fix clip management for DrawPicture. (flutter/engine#44835) 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 [email protected],[email protected],[email protected] 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://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
| GetCurrentPass().AddSubpassInline(std::move(pass)); | ||
|
|
||
| RestoreToCount(save_count); | ||
| RestoreClip(); |
There was a problem hiding this comment.
I think this is what I was missing in my attempt - why is this right and the RestoreToCount wasn't?
There was a problem hiding this comment.
Because in-between the save and restore, we just merge the two passes without changing anything about the canvas transformation stack, and so the RestoreToCount doesn't result in any clip restores getting appended because it thinks we're still at the same stencil depth.
RestoreClip always inserts a clip restore that will flatten to the currently tracked stencil depth, which gets the pass back into alignment with the canvas state without us having to add (normally unnecessary) stencil cleanup at the end of the base pass.
Solves two problems: 1. Fix the debug crash by restoring the clip after the `EntityPass` merge. I initially surrounded the merge with a `Save` and `Restore`, which was a mistake and is a no-op in this situation. This should have no excess impact on performance, since we ignore clips in `EntityPass` when we detect that they won't do anything. 2. Picture subpasses weren't getting their stencil depth adjusted during the merge, which can result in subpass entities potentially receiving a clip depth that's too high.
3 participants
Solves two problems:
EntityPassmerge. I initially surrounded the merge with aSaveandRestore, which was a mistake and is a no-op in this situation. This should have no excess impact on performance, since we ignore clips inEntityPasswhen we detect that they won't do anything.Before:
After: