From 8554733c97785c97599e1eafe46016e48c93a3d3 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Sat, 28 Oct 2023 23:53:26 -0700 Subject: [PATCH 1/3] [Impeller] Fix nullopt access in GetSubpassCoverage. --- impeller/entity/entity_pass.cc | 56 +++++++++++++++++++--------------- 1 file changed, 31 insertions(+), 25 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 1948a72a69d3d..8ac58137881d7 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -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 @@ -97,19 +95,20 @@ size_t EntityPass::GetSubpassesDepth() const { std::optional EntityPass::GetElementsCoverage( std::optional coverage_limit) const { - std::optional result; + std::optional accumulated_coverage; for (const auto& element : elements_) { - std::optional coverage; + std::optional element_coverage; if (auto entity = std::get_if(&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()); } } } else if (auto subpass_ptr = @@ -122,17 +121,18 @@ std::optional 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_) { std::shared_ptr 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; if (unfiltered_coverage.has_value()) { - unfiltered_coverage = coverage->Union(*backdrop_coverage); + unfiltered_coverage = + unfiltered_coverage->Union(*backdrop_coverage); } else { unfiltered_coverage = backdrop_coverage; } @@ -161,29 +161,31 @@ std::optional 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 = + 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()); } - return result; + return accumulated_coverage; } std::optional EntityPass::GetSubpassCoverage( @@ -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())); } else { element_entity.GetContents()->SetCoverageHint(current_clip_coverage); } From 9e6eab5cefff116c7a6baf4e1c279255639e7a35 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 31 Oct 2023 11:26:33 -0700 Subject: [PATCH 2/3] Address comments --- impeller/entity/entity_pass.cc | 26 +++++--------------------- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 8ac58137881d7..ed8a8123ba63f 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -23,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 @@ -129,13 +130,8 @@ std::optional EntityPass::GetElementsCoverage( if (backdrop_filter) { auto backdrop_coverage = backdrop_filter->GetCoverage({}); if (backdrop_coverage.has_value()) { - backdrop_coverage->origin += accumulated_coverage->origin; - if (unfiltered_coverage.has_value()) { - unfiltered_coverage = - unfiltered_coverage->Union(*backdrop_coverage); - } else { - unfiltered_coverage = backdrop_coverage; - } + unfiltered_coverage = + Union(backdrop_coverage.value(), unfiltered_coverage); } } else { VALIDATION_LOG << "The EntityPass backdrop filter proc didn't return " @@ -166,24 +162,12 @@ std::optional EntityPass::GetElementsCoverage( element_coverage = unfiltered_coverage; } - if (element_coverage.has_value() && coverage_limit.has_value() && - (!image_filter || image_filter->IsTranslationOnly())) { - element_coverage = - element_coverage->Intersection(coverage_limit.value()); - } + element_coverage = Intersection(element_coverage, coverage_limit); } else { FML_UNREACHABLE(); } - if (!accumulated_coverage.has_value() && element_coverage.has_value()) { - accumulated_coverage = element_coverage; - continue; - } - if (!element_coverage.has_value()) { - continue; - } - accumulated_coverage = - accumulated_coverage->Union(element_coverage.value()); + accumulated_coverage = Union(accumulated_coverage, element_coverage); } return accumulated_coverage; } From c26fd1bcff1b911427288d6ad2cd97953738a1a2 Mon Sep 17 00:00:00 2001 From: Brandon DeRosier Date: Tue, 31 Oct 2023 16:25:44 -0700 Subject: [PATCH 3/3] Simplify --- impeller/entity/entity_pass.cc | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index ed8a8123ba63f..e1de81cf5a67b 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -129,10 +129,8 @@ std::optional EntityPass::GetElementsCoverage( subpass.xformation_, Entity::RenderingMode::kSubpass); if (backdrop_filter) { auto backdrop_coverage = backdrop_filter->GetCoverage({}); - if (backdrop_coverage.has_value()) { - unfiltered_coverage = - Union(backdrop_coverage.value(), unfiltered_coverage); - } + unfiltered_coverage = + Rect::Union(unfiltered_coverage, backdrop_coverage); } else { VALIDATION_LOG << "The EntityPass backdrop filter proc didn't return " "a valid filter."; @@ -162,12 +160,12 @@ std::optional EntityPass::GetElementsCoverage( element_coverage = unfiltered_coverage; } - element_coverage = Intersection(element_coverage, coverage_limit); + element_coverage = Rect::Intersection(element_coverage, coverage_limit); } else { FML_UNREACHABLE(); } - accumulated_coverage = Union(accumulated_coverage, element_coverage); + accumulated_coverage = Rect::Union(accumulated_coverage, element_coverage); } return accumulated_coverage; } @@ -747,19 +745,8 @@ bool EntityPass::RenderElement(Entity& element_entity, // clip coverage (which is the max clip bounds). The contents may // optionally use this hint to avoid unnecessary rendering work. 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( - element_coverage_hint.value().Intersection( - current_clip_coverage.value())); - } else { - element_entity.GetContents()->SetCoverageHint(current_clip_coverage); - } + element_entity.GetContents()->SetCoverageHint( + Rect::Intersection(element_coverage_hint, current_clip_coverage)); switch (clip_coverage.type) { case Contents::ClipCoverage::Type::kNoChange: