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

[Impeller] Fix nullopt access and simplify coverage computation in GetSubpassCoverage. #47347

Merged
bdero merged 3 commits into from
Nov 1, 2023
Merged

[Impeller] Fix nullopt access and simplify coverage computation in GetSubpassCoverage. #47347

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8554733
[Impeller] Fix nullopt access in GetSubpassCoverage.
bdero Oct 29, 2023
9e6eab5
Address comments
bdero Oct 31, 2023
c26fd1b
Simplify
bdero Oct 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Impeller] Fix nullopt access in GetSubpassCoverage.
  • Loading branch information
@bdero
bdero committed Nov 1, 2023
commit 8554733c97785c97599e1eafe46016e48c93a3d3
56 changes: 31 additions & 25 deletions impeller/entity/entity_pass.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

// FLUTTER_NOLINT: https://github.com/flutter/flutter/issues/132129

#include "impeller/entity/entity_pass.h"

#include <memory>
Expand Down Expand Up @@ -97,19 +95,20 @@ size_t EntityPass::GetSubpassesDepth() const {

std::optional<Rect> EntityPass::GetElementsCoverage(
std::optional<Rect> coverage_limit) const {
std::optional<Rect> result;
std::optional<Rect> accumulated_coverage;
for (const auto& element : elements_) {
std::optional<Rect> coverage;
std::optional<Rect> element_coverage;

if (auto entity = std::get_if<Entity>(&element)) {
coverage = entity->GetCoverage();
element_coverage = entity->GetCoverage();

// When the coverage limit is std::nullopt, that means there is no limit,
// as opposed to empty coverage.
if (coverage.has_value() && coverage_limit.has_value()) {
if (element_coverage.has_value() && coverage_limit.has_value()) {
const auto* filter = entity->GetContents()->AsFilter();
if (!filter || filter->IsTranslationOnly()) {
coverage = coverage->Intersection(coverage_limit.value());
element_coverage =
element_coverage->Intersection(coverage_limit.value());
Copy link
Contributor

@flar flar Oct 31, 2023

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?

Copy link
Member Author

@bdero bdero Nov 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

}
}
} else if (auto subpass_ptr =
Expand All @@ -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_) {
Copy link
Contributor

@flar flar Oct 30, 2023

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?

Copy link
Member Author

@bdero bdero Oct 31, 2023

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

Copy link
Contributor

@flar flar Oct 31, 2023

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).

std::shared_ptr<FilterContents> backdrop_filter =
subpass.backdrop_filter_proc_(FilterInput::Make(result.value()),
subpass.xformation_,
Entity::RenderingMode::kSubpass);
subpass.backdrop_filter_proc_(
FilterInput::Make(accumulated_coverage.value()),
subpass.xformation_, Entity::RenderingMode::kSubpass);
if (backdrop_filter) {
auto backdrop_coverage = backdrop_filter->GetCoverage({});
backdrop_coverage->origin += result->origin;
if (backdrop_coverage.has_value()) {
backdrop_coverage->origin += accumulated_coverage->origin;
Copy link
Contributor

@flar flar Oct 30, 2023

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.

Copy link
Member Author

@bdero bdero Oct 31, 2023 •
edited
Loading

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.

if (unfiltered_coverage.has_value()) {
unfiltered_coverage = coverage->Union(*backdrop_coverage);
unfiltered_coverage =
unfiltered_coverage->Union(*backdrop_coverage);
Copy link
Contributor

@flar flar Oct 30, 2023

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)

Copy link
Member Author

@bdero bdero Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

} else {
unfiltered_coverage = backdrop_coverage;
}
Expand Down Expand Up @@ -161,29 +161,31 @@ std::optional<Rect> EntityPass::GetElementsCoverage(
if (image_filter) {
Entity subpass_entity;
subpass_entity.SetTransformation(subpass.xformation_);
coverage = image_filter->GetCoverage(subpass_entity);
element_coverage = image_filter->GetCoverage(subpass_entity);
} else {
coverage = unfiltered_coverage;
element_coverage = unfiltered_coverage;
}

if (coverage.has_value() && coverage_limit.has_value() &&
if (element_coverage.has_value() && coverage_limit.has_value() &&
(!image_filter || image_filter->IsTranslationOnly())) {
coverage = coverage->Intersection(coverage_limit.value());
element_coverage =
Copy link
Contributor

@flar flar Oct 30, 2023

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?

Copy link
Member Author

@bdero bdero Oct 31, 2023

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.

element_coverage->Intersection(coverage_limit.value());
}
} else {
FML_UNREACHABLE();
}

if (!result.has_value() && coverage.has_value()) {
result = coverage;
if (!accumulated_coverage.has_value() && element_coverage.has_value()) {
accumulated_coverage = element_coverage;
continue;
}
if (!coverage.has_value()) {
if (!element_coverage.has_value()) {
continue;
}
result = result->Union(coverage.value());
accumulated_coverage =
accumulated_coverage->Union(element_coverage.value());
Copy link
Contributor

@flar flar Oct 30, 2023

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)

Copy link
Member Author

@bdero bdero Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

}
return result;
return accumulated_coverage;
}

std::optional<Rect> EntityPass::GetSubpassCoverage(
Expand Down Expand Up @@ -760,13 +762,17 @@ bool EntityPass::RenderElement(Entity& element_entity,
// rendered output will actually be used, and so we set this to the current
// clip coverage (which is the max clip bounds). The contents may
// optionally use this hint to avoid unnecessary rendering work.
if (element_entity.GetContents()->GetCoverageHint().has_value()) {
auto element_coverage_hint = element_entity.GetContents()->GetCoverageHint();
if (element_coverage_hint.has_value() &&
// If the `current_clip_coverage` is `std::nullopt`, just fall into the
// else case and let std::nullopt get assigned as the coverage hint.
current_clip_coverage.has_value()) {
// If the element already has a coverage hint (because its an advanced
// blend), then we need to intersect the clip coverage hint with the
// existing coverage hint.
element_entity.GetContents()->SetCoverageHint(
current_clip_coverage->Intersection(
element_entity.GetContents()->GetCoverageHint().value()));
element_coverage_hint.value().Intersection(
current_clip_coverage.value()));
Copy link
Contributor

@flar flar Oct 31, 2023

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)?

Copy link
Member Author

@bdero bdero Oct 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure can. Done.

} else {
element_entity.GetContents()->SetCoverageHint(current_clip_coverage);
}
Expand Down