-
Notifications
You must be signed in to change notification settings - Fork 6k
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] Fix nullopt access and simplify coverage computation in GetSubpassCoverage. #47347
Conversation
impeller/entity/entity_pass.cc
Outdated
if (backdrop_coverage.has_value()) { | ||
if (unfiltered_coverage.has_value()) { | ||
unfiltered_coverage = coverage->Union(*backdrop_coverage); | ||
unfiltered_coverage = coverage.has_value() |
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.
I might be missing something, but I don't see how coverage.has_value()
could ever be true here.
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.
Also, this tests backdrop_coverage.has_value()
after it already used the ->
operator on it...
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.
Is the bug here that the calculation should have been unfiltered_coverage->Union(*backdrop_coverage)
all along?
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.
Fixed, yeah this was supposed to union with unfiltered_coverage.
impeller/entity/entity_pass.cc
Outdated
if (backdrop_coverage.has_value()) { | ||
if (unfiltered_coverage.has_value()) { | ||
unfiltered_coverage = coverage->Union(*backdrop_coverage); | ||
unfiltered_coverage = coverage.has_value() | ||
? coverage->Union(*backdrop_coverage) |
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.
I have new functions in #47183 that perform nullopt aware Union and Intersection that might simplify this code.
a3d1a82
to
86535a6
Compare
86535a6
to
b290657
Compare
@@ -122,17 +121,18 @@ std::optional<Rect> EntityPass::GetElementsCoverage( | |||
// If the current pass elements have any coverage so far and there's a | |||
// backdrop filter, then incorporate the backdrop filter in the | |||
// pre-filtered coverage of the subpass. | |||
if (result.has_value() && subpass.backdrop_filter_proc_) { | |||
if (accumulated_coverage.has_value() && subpass.backdrop_filter_proc_) { |
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.
Why is this based on accumulated_coverage
? If there is a backdrop filter on one of the elements, then the subpass coverage should be automatically expanded to the coverage_limit, shouldn't it?
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.
This is wrong when backdrop filters are destructive color filters, but don't detect this right now. This is already being tracked here: flutter/flutter#137090
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.
Oh, I'm forgetting that BDF only operates on the current layer so it will depend on the accumulated coverage...
In the DiffContext partial repaint code BDF always accumulates the coverage_limit which is probably conservative for one that appears inside another layer (or one that appears with no previous "clear" command).
impeller/entity/entity_pass.cc
Outdated
if (backdrop_coverage.has_value()) { | ||
backdrop_coverage->origin += accumulated_coverage->origin; |
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.
I'm not sure why this is done...? A BDF is independent of the prior content in the pass.
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.
I think this is a bug and removed it... Let's see those goldens. The filter's coverage is computed above with an identity transform, but the placeholder rect given to the filter input should already contain the offset.
impeller/entity/entity_pass.cc
Outdated
if (unfiltered_coverage.has_value()) { | ||
unfiltered_coverage = coverage->Union(*backdrop_coverage); | ||
unfiltered_coverage = | ||
unfiltered_coverage->Union(*backdrop_coverage); |
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.
This if/else can be replaced with unfiltered_coverage = Union(backdrop_coverage.value(), unfiltered_coverage)
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.
Done.
impeller/entity/entity_pass.cc
Outdated
continue; | ||
} | ||
result = result->Union(coverage.value()); | ||
accumulated_coverage = | ||
accumulated_coverage->Union(element_coverage.value()); |
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.
All of these lines (178-187) => accumulated_coverage = Union(accumulated_coverage, element_coverage)
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.
Done.
impeller/entity/entity_pass.cc
Outdated
(!image_filter || image_filter->IsTranslationOnly())) { | ||
coverage = coverage->Intersection(coverage_limit.value()); | ||
element_coverage = |
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.
My head is spinning a little here - why does image_filter disqualify this intersection?
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.
We shouldn't need to guard against this anymore now that the coverage_limit incorporates image filters as we walk the subpasses (not that it was a correct fix to begin with). Removed it.
b290657
to
0fccf6f
Compare
Whoops, didn't push my changes until now 😅 |
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 if clang-tidy is happy and @flar's comments are addressed.
0fccf6f
to
8e6b898
Compare
impeller/entity/entity_pass.cc
Outdated
unfiltered_coverage = backdrop_coverage; | ||
} | ||
unfiltered_coverage = | ||
Union(backdrop_coverage.value(), unfiltered_coverage); |
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.
This can be further simplified to Union(unfiltered_coverage, backdrop_coverage)
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.
(No need to test backdrop_coverage.has_value()
around it as the Union
overrides will do that for you.)
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.
Done.
impeller/entity/entity_pass.cc
Outdated
current_clip_coverage->Intersection( | ||
element_entity.GetContents()->GetCoverageHint().value())); | ||
element_coverage_hint.value().Intersection( | ||
current_clip_coverage.value())); |
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.
Can all of this be replaced by
element_entity.GetContents()->SetCoverageHint(Intersection(element_coverage_hint, current_clip_coverage)
?
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.
Sure can. Done.
Appreciate you taking a look for spots we can simplify with the new utils. I think it removes quite a bit of cognitive overhead while reading through this part of the codebase. |
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.
Looks good. We should look at using GetSourceCoverage in there at some point.
const auto* filter = entity->GetContents()->AsFilter(); | ||
if (!filter || filter->IsTranslationOnly()) { | ||
coverage = coverage->Intersection(coverage_limit.value()); | ||
element_coverage = | ||
element_coverage->Intersection(coverage_limit.value()); |
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.
This looks like a good place to use filter->GetSourceCoverage, maybe as a follow-on?
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.
Yes!
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie or stuartmorgan on the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!). If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. |
db240bf
to
c26fd1b
Compare
…tion in GetSubpassCoverage. (flutter/engine#47347)
…137656) flutter/engine@db06c2e...a0ac6b4 2023-11-01 [email protected] [Impeller] Include cstdint everywhere that uint32_t is used. (flutter/engine#47533) 2023-11-01 [email protected] [Impeller] Fix nullopt access and simplify coverage computation in GetSubpassCoverage. (flutter/engine#47347) 2023-11-01 [email protected] [Impeller] OpenGLES: Ensure frag/vert textures are bound with unique texture units. (flutter/engine#47218) 2023-11-01 [email protected] Roll Fuchsia Linux SDK from LCfhx_lTRJI51G0zc... to _TyF0etsONe5aqCbM... (flutter/engine#47532) 2023-11-01 [email protected] [Impeller] stencil buffer record/replay instead of MSAA storage. (flutter/engine#47397) 2023-11-01 [email protected] [macOS] Delete FlutterCompositor tests (flutter/engine#47527) 2023-10-31 [email protected] [Impeller] Place Rect statics under the Rect template. (flutter/engine#47529) 2023-10-31 [email protected] Roll Skia from aaa225e0cc6d to 34ef20100acc (1 revision) (flutter/engine#47530) Also rolling transitive DEPS: fuchsia/sdk/core/linux-amd64 from LCfhx_lTRJI5 to _TyF0etsONe5 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] 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
Follow up for comments in #46130.
This case shouldn't actually be possible today, but we should be able to make this reasonably testable without goldens... added an issue to follow-up here: flutter/flutter#137356
This branch noise will also melt away with: flutter/flutter#137306