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
merged 3 commits into from
Nov 1, 2023
Merged

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

Changes from all commits
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
65 changes: 21 additions & 44 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 All @@ -25,6 +23,7 @@
#include "impeller/entity/entity.h"
#include "impeller/entity/inline_pass_context.h"
#include "impeller/geometry/color.h"
#include "impeller/geometry/rect.h"
#include "impeller/renderer/command_buffer.h"

#ifdef IMPELLER_DEBUG
Expand Down Expand Up @@ -97,19 +96,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,21 +122,15 @@ 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()) {
if (unfiltered_coverage.has_value()) {
unfiltered_coverage = coverage->Union(*backdrop_coverage);
} else {
unfiltered_coverage = backdrop_coverage;
}
}
unfiltered_coverage =
Rect::Union(unfiltered_coverage, backdrop_coverage);
} else {
VALIDATION_LOG << "The EntityPass backdrop filter proc didn't return "
"a valid filter.";
Expand All @@ -161,29 +155,19 @@ 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() &&
(!image_filter || image_filter->IsTranslationOnly())) {
coverage = coverage->Intersection(coverage_limit.value());
}
element_coverage = Rect::Intersection(element_coverage, coverage_limit);
} else {
FML_UNREACHABLE();
}

if (!result.has_value() && coverage.has_value()) {
result = coverage;
continue;
}
if (!coverage.has_value()) {
continue;
}
result = result->Union(coverage.value());
accumulated_coverage = Rect::Union(accumulated_coverage, element_coverage);
}
return result;
return accumulated_coverage;
}

std::optional<Rect> EntityPass::GetSubpassCoverage(
Expand Down Expand Up @@ -760,16 +744,9 @@ 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()) {
// 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()));
} else {
element_entity.GetContents()->SetCoverageHint(current_clip_coverage);
}
auto element_coverage_hint = element_entity.GetContents()->GetCoverageHint();
element_entity.GetContents()->SetCoverageHint(
Rect::Intersection(element_coverage_hint, current_clip_coverage));

switch (clip_coverage.type) {
case Contents::ClipCoverage::Type::kNoChange:
Expand Down