Reland Fixes MatrixFilterContents rendering/coverage #52880
#52939
Conversation
fixes: flutter/flutter#147807 relands flutter#43943 (with fixes that hopefully avoid it being reverted again) [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
|
I'm pulling this back into a draft, it doesn't fix the reproduction code. I thought |
impeller/entity/entity_pass.cc
Outdated
| // When the subpass has a translation that means the math with | ||
| // the snapshot has to be different. | ||
| subpass_transform_basis == subpass->transform_ | ||
| ? Entity::RenderingMode::kSubpassAppendSnapshotTransform | ||
| : Entity::RenderingMode::kSubpassPrependSnapshotTransform); |
There was a problem hiding this comment.
@bdero this is the big change between when it landed last time. I've also renamed the RenderModes to match how they are used in the filter. I'm open to better naming. My intuition of the problem is that sometimes the snapshot is clipped and sometimes it's not which requires this check.
There was a problem hiding this comment.
Hmm, this seems like we might be skirting by with the current set of tests, but seems unlikely to be correct in all cases. Do you have an intuition for why we'd need to suddenly switch the ordering when a translation is present?
There was a problem hiding this comment.
I'm trying to play with this locally to see if I can understand what's going on here. We have three space cases that make this whole thing kind of confusing to deal with...
- Apply filter to a regular Entity. (kDirect)
- Apply filter to a subpass texture.
- Apply filter to a backdrop of the parent subpass.
The complexity comes from subpass textures/backdrops being drawn relative to screen space, so their entity transforms end up being different than usual. For subpass textures this is a problem because the filter needs to be applied in CTM space, similar to kDirect.
The subpass texture is the least special of the two and we might be able to get rid of that special case altogether.
But for backdrop filters, I don't remember what space their effect is supposed to be applied in. We gotta document it.
There was a problem hiding this comment.
This being correct isn't just asserted in our tests, it is also asserted as correct for the Flutter user's reproduction code. All PR's contain a risk of having bugs. The status quo is demonstrably incorrect this is demonstrably more correct.
Here's my intuition as to why this is likely correct:
- It has been demonstrated that there are 2 different approaches to the subpass math in MatrixContentFilter as evidenced by your original PR (and its revert).
- This code path is different than the others since it takes the effect transform's basis whereas the others don't.
We are essentially throwing away context before handing it down to lower levels of the code. - Before we take the basis, if you look at the subpass'
bounds_limit_and thetransform_you can see there are 2 clear cases, one where thebounds_limit_is based at the origin and the effect transform has translation, and case wherebounds_limit_isn't at the origin and the effect transform has no translation. There is clearly 2 different approaches to the subpass's math happening here. That context was being thrown away with the basis call, the different render modes maintains that proper context so we can make the correct decision. - I dug a bit into why the subpasses math appears different in 2 different cases. I traced it back to
SaveLayerOptions::content_is_clipped(). If the subpass is clipped that suggests that the math would have to be handled differently.
I think we should investigate eliminating this discrepancy, we should be eliminating that call to Basis(). I tried doing that but was unsuccessful. Hopefully that would eventually allow us to collapse the subpass render modes.
There was a problem hiding this comment.
Sure, I don't think it makes sense for me to block this while playing with it and trying to simplify. THanks for working on this :)
There was a problem hiding this comment.
Thanks @bdero. I'm going to still make the change for the HasTranslation() before landing. I'm hoping that this can help lead to a more wholistic solution. I think it may be the case that this is some integration problem / miscommunication that is going to take some detangling beyond what one person can do though. I'm happy to help with that if I can.
|
I was tagged in a comment that I can't find here, but related to We can determine if it definitely does not clip the content, but we can't prove the opposite... The hint disables opacity peephole which can only be applied if the content of the SaveLayer is unclipped. |
Yea, sorry Jim. I deleted it, hoping not to bother you since it wasn't 100% correct and I had to change the code to cover all cases. This may still be related to that feature but that check on the promise wasn't handling all cases (maybe because the "may" clause?). Now I look at the effect transform's translation (which I think may be a downstream effect of the promise and clipping). |
|
FYI, this is on my radar this morning and I'll review it after some sheriff stuff. |
impeller/entity/entity_pass.cc
Outdated
| // When the subpass has a translation that means the math with | ||
| // the snapshot has to be different. | ||
| subpass_transform_basis == subpass->transform_ | ||
| ? Entity::RenderingMode::kSubpassAppendSnapshotTransform | ||
| : Entity::RenderingMode::kSubpassPrependSnapshotTransform); |
There was a problem hiding this comment.
Hmm, this seems like we might be skirting by with the current set of tests, but seems unlikely to be correct in all cases. Do you have an intuition for why we'd need to suddenly switch the ordering when a translation is present?
gaaclarke
added
the
autosubmit
…148802) flutter/engine@a8872c8...bc1345b 2024-05-21 chinmaygarde@google.com [Impeller] Remove unused GN flags. (flutter/engine#52962) 2024-05-21 30870216+gaaclarke@users.noreply.github.com Reland `Fixes MatrixFilterContents rendering/coverage #52880` (flutter/engine#52939) 2024-05-21 skia-flutter-autoroll@skia.org Roll Dart SDK from dd768e0d1890 to f0ac30bbc63e (1 revision) (flutter/engine#52958) 2024-05-21 skia-flutter-autoroll@skia.org Roll Skia from 4f6568ba7051 to df78435c2f26 (4 revisions) (flutter/engine#52959) 2024-05-21 chinmaygarde@google.com [Impeller] Refactor impeller.gni (flutter/engine#52942) 2024-05-21 skia-flutter-autoroll@skia.org Roll Skia from ca98796cc19e to 4f6568ba7051 (2 revisions) (flutter/engine#52956) 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
fixes flutter/flutter#147807
relands #52880
relands #43943
This was previously reverted because of the golden test failure
widgets.widgets.magnifier.styled. This fixes that problem by instead of focusing on ImageFilter and BackdropFilter subpasses, instead focuses on wether a subpass is clipped or not and by extension how the math should be handled.widgets.widgets.magnifier.styledafter diff:Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.