From 7582476ace1becdb370afcbee618ff647ce09e44 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Fri, 20 Oct 2023 16:45:21 -0700 Subject: [PATCH 1/9] Add FilterContents::GetSourceCoverage to enable filtered saveLayer clipping --- .../border_mask_blur_filter_contents.cc | 11 ++++++ .../border_mask_blur_filter_contents.h | 5 +++ .../contents/filters/color_filter_contents.cc | 6 ++++ .../contents/filters/color_filter_contents.h | 5 +++ ...rectional_gaussian_blur_filter_contents.cc | 11 ++++++ .../contents/filters/filter_contents.cc | 22 ++++++++++++ .../entity/contents/filters/filter_contents.h | 18 ++++++++++ .../inputs/filter_contents_filter_input.cc | 6 ++++ .../inputs/filter_contents_filter_input.h | 5 +++ .../contents/filters/inputs/filter_input.cc | 6 ++++ .../contents/filters/inputs/filter_input.h | 3 ++ .../filters/local_matrix_filter_contents.cc | 11 ++++++ .../filters/local_matrix_filter_contents.h | 5 +++ .../filters/matrix_filter_contents.cc | 11 ++++++ .../contents/filters/matrix_filter_contents.h | 5 +++ .../filters/morphology_filter_contents.cc | 14 ++++++++ .../filters/morphology_filter_contents.h | 5 +++ .../filters/yuv_to_rgb_filter_contents.cc | 6 ++++ .../filters/yuv_to_rgb_filter_contents.h | 5 +++ impeller/entity/entity_pass.cc | 5 ++- impeller/geometry/rect.h | 36 +++++++++++++++++++ 21 files changed, 200 insertions(+), 1 deletion(-) diff --git a/impeller/entity/contents/filters/border_mask_blur_filter_contents.cc b/impeller/entity/contents/filters/border_mask_blur_filter_contents.cc index 3da610b90bd3f..849ffa7b5be53 100644 --- a/impeller/entity/contents/filters/border_mask_blur_filter_contents.cc +++ b/impeller/entity/contents/filters/border_mask_blur_filter_contents.cc @@ -169,4 +169,15 @@ std::optional BorderMaskBlurFilterContents::GetFilterCoverage( Size(extent.x, extent.y)); } +std::optional BorderMaskBlurFilterContents::GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + auto transformed_blur_vector = + effect_transform.TransformDirection(Vector2(Radius{sigma_x_}.radius, 0)) + .Abs() + + effect_transform.TransformDirection(Vector2(0, Radius{sigma_y_}.radius)) + .Abs(); + return output_limit.Expand(transformed_blur_vector); +} + } // namespace impeller diff --git a/impeller/entity/contents/filters/border_mask_blur_filter_contents.h b/impeller/entity/contents/filters/border_mask_blur_filter_contents.h index d41b4a47672e9..90b8e2a44c1a2 100644 --- a/impeller/entity/contents/filters/border_mask_blur_filter_contents.h +++ b/impeller/entity/contents/filters/border_mask_blur_filter_contents.h @@ -27,6 +27,11 @@ class BorderMaskBlurFilterContents final : public FilterContents { const Entity& entity, const Matrix& effect_transform) const override; + // |FilterContents| + std::optional GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const override; + private: // |FilterContents| std::optional RenderFilter( diff --git a/impeller/entity/contents/filters/color_filter_contents.cc b/impeller/entity/contents/filters/color_filter_contents.cc index f5a36456a451f..489cc4b0cd6b5 100644 --- a/impeller/entity/contents/filters/color_filter_contents.cc +++ b/impeller/entity/contents/filters/color_filter_contents.cc @@ -99,4 +99,10 @@ std::optional ColorFilterContents::GetAlpha() const { return alpha_; } +std::optional ColorFilterContents::GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + return output_limit; +} + } // namespace impeller diff --git a/impeller/entity/contents/filters/color_filter_contents.h b/impeller/entity/contents/filters/color_filter_contents.h index 07506d2c3808e..7fc77d7e32666 100644 --- a/impeller/entity/contents/filters/color_filter_contents.h +++ b/impeller/entity/contents/filters/color_filter_contents.h @@ -44,6 +44,11 @@ class ColorFilterContents : public FilterContents { std::optional GetAlpha() const; + // |FilterContents| + std::optional GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const override; + private: AbsorbOpacity absorb_opacity_ = AbsorbOpacity::kNo; std::optional alpha_; diff --git a/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.cc b/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.cc index 7aad9e863f958..a03abb2eb2069 100644 --- a/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.cc +++ b/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.cc @@ -273,6 +273,17 @@ std::optional DirectionalGaussianBlurFilterContents::RenderFilter( entity.GetBlendMode(), entity.GetClipDepth()); } +std::optional +DirectionalGaussianBlurFilterContents::GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + auto transform = effect_transform.Basis(); + auto transformed_blur_vector = + transform.TransformDirection(blur_direction_ * Radius{blur_sigma_}.radius) + .Abs(); + return output_limit.Expand(transformed_blur_vector); +} + std::optional DirectionalGaussianBlurFilterContents::GetFilterCoverage( const FilterInput::Vector& inputs, const Entity& entity, diff --git a/impeller/entity/contents/filters/filter_contents.cc b/impeller/entity/contents/filters/filter_contents.cc index 617a2a62934e6..25eb7f1bd8603 100644 --- a/impeller/entity/contents/filters/filter_contents.cc +++ b/impeller/entity/contents/filters/filter_contents.cc @@ -229,6 +229,28 @@ std::optional FilterContents::GetFilterCoverage( return result; } +std::optional FilterContents::GetSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + auto filter_input_coverage = + GetFilterSourceCoverage(effect_transform_, output_limit); + + if (!filter_input_coverage.has_value()) { + return std::nullopt; + } + + std::optional inputs_coverage; + for (auto input : inputs_) { + auto input_coverage = input->GetSourceCoverage( + effect_transform, filter_input_coverage.value()); + if (!input_coverage.has_value()) { + return std::nullopt; + } + inputs_coverage = Union(inputs_coverage, input_coverage.value()); + } + return inputs_coverage; +} + std::optional FilterContents::GetEntity( const ContentContext& renderer, const Entity& entity, diff --git a/impeller/entity/contents/filters/filter_contents.h b/impeller/entity/contents/filters/filter_contents.h index f61d7262d896d..f06c17c19364c 100644 --- a/impeller/entity/contents/filters/filter_contents.h +++ b/impeller/entity/contents/filters/filter_contents.h @@ -132,6 +132,20 @@ class FilterContents : public Contents { // |Contents| const FilterContents* AsFilter() const override; + /// @brief Determines the coverage of source pixels that will be needed + /// to apply this filter under the given transform and produce + /// results anywhere within the indicated coverage limit. + /// + /// This is useful for subpass rendering scenarios where a filter + /// will be applied to the output of the subpass and we need to + /// determine how large of a render target to allocate in order + /// to collect all pixels that might affect the supplied output + /// coverage limit. While we might clip the rendering of the subpass, + /// we want to avoid clipping out any pixels that contribute to + /// the output limit via the filtering operation. + std::optional GetSourceCoverage(const Matrix& effect_transform, + const Rect& output_limit) const; + virtual Matrix GetLocalTransform(const Matrix& parent_transform) const; Matrix GetTransform(const Matrix& parent_transform) const; @@ -169,6 +183,10 @@ class FilterContents : public Contents { const Entity& entity, const Matrix& effect_transform) const; + virtual std::optional GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const = 0; + /// @brief Converts zero or more filter inputs into a render instruction. virtual std::optional RenderFilter( const FilterInput::Vector& inputs, diff --git a/impeller/entity/contents/filters/inputs/filter_contents_filter_input.cc b/impeller/entity/contents/filters/inputs/filter_contents_filter_input.cc index 3e43132c1320d..eabefd380d882 100644 --- a/impeller/entity/contents/filters/inputs/filter_contents_filter_input.cc +++ b/impeller/entity/contents/filters/inputs/filter_contents_filter_input.cc @@ -43,6 +43,12 @@ std::optional FilterContentsFilterInput::GetCoverage( return filter_->GetCoverage(entity); } +std::optional FilterContentsFilterInput::GetSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + return filter_->GetSourceCoverage(effect_transform, output_limit); +} + Matrix FilterContentsFilterInput::GetLocalTransform( const Entity& entity) const { return filter_->GetLocalTransform(entity.GetTransformation()); diff --git a/impeller/entity/contents/filters/inputs/filter_contents_filter_input.h b/impeller/entity/contents/filters/inputs/filter_contents_filter_input.h index e21cb7acc451c..ca3137ac4615a 100644 --- a/impeller/entity/contents/filters/inputs/filter_contents_filter_input.h +++ b/impeller/entity/contents/filters/inputs/filter_contents_filter_input.h @@ -25,6 +25,11 @@ class FilterContentsFilterInput final : public FilterInput { // |FilterInput| std::optional GetCoverage(const Entity& entity) const override; + // |FilterInput| + std::optional GetSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const override; + // |FilterInput| Matrix GetLocalTransform(const Entity& entity) const override; diff --git a/impeller/entity/contents/filters/inputs/filter_input.cc b/impeller/entity/contents/filters/inputs/filter_input.cc index b3ecc69895b92..34df9ed19b8c0 100644 --- a/impeller/entity/contents/filters/inputs/filter_input.cc +++ b/impeller/entity/contents/filters/inputs/filter_input.cc @@ -66,6 +66,12 @@ std::optional FilterInput::GetLocalCoverage(const Entity& entity) const { return GetCoverage(local_entity); } +std::optional FilterInput::GetSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + return output_limit; +} + Matrix FilterInput::GetTransform(const Entity& entity) const { return entity.GetTransformation() * GetLocalTransform(entity); } diff --git a/impeller/entity/contents/filters/inputs/filter_input.h b/impeller/entity/contents/filters/inputs/filter_input.h index 955932885ea07..125a2eba3329a 100644 --- a/impeller/entity/contents/filters/inputs/filter_input.h +++ b/impeller/entity/contents/filters/inputs/filter_input.h @@ -56,6 +56,9 @@ class FilterInput { virtual std::optional GetCoverage(const Entity& entity) const = 0; + virtual std::optional GetSourceCoverage(const Matrix& effect_transform, + const Rect& output_limit) const; + /// @brief Get the local transform of this filter input. This transform is /// relative to the `Entity` transform space. virtual Matrix GetLocalTransform(const Entity& entity) const; diff --git a/impeller/entity/contents/filters/local_matrix_filter_contents.cc b/impeller/entity/contents/filters/local_matrix_filter_contents.cc index 7f96fd1c7ba1e..e30edbd04e8d0 100644 --- a/impeller/entity/contents/filters/local_matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/local_matrix_filter_contents.cc @@ -19,6 +19,17 @@ Matrix LocalMatrixFilterContents::GetLocalTransform( return matrix_; } +std::optional LocalMatrixFilterContents::GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + auto matrix = matrix_.Basis(); + if (matrix.GetDeterminant() == 0.0) { + return std::nullopt; + } + auto inverse = matrix.Invert(); + return output_limit.TransformBounds(inverse); +} + std::optional LocalMatrixFilterContents::RenderFilter( const FilterInput::Vector& inputs, const ContentContext& renderer, diff --git a/impeller/entity/contents/filters/local_matrix_filter_contents.h b/impeller/entity/contents/filters/local_matrix_filter_contents.h index 72f77be74aab7..5d9e547a35e5b 100644 --- a/impeller/entity/contents/filters/local_matrix_filter_contents.h +++ b/impeller/entity/contents/filters/local_matrix_filter_contents.h @@ -20,6 +20,11 @@ class LocalMatrixFilterContents final : public FilterContents { // |FilterContents| Matrix GetLocalTransform(const Matrix& parent_transform) const override; + // |FilterContents| + std::optional GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const override; + private: // |FilterContents| std::optional RenderFilter( diff --git a/impeller/entity/contents/filters/matrix_filter_contents.cc b/impeller/entity/contents/filters/matrix_filter_contents.cc index c16bcdb88d698..f8a43c19b6f78 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents.cc @@ -68,6 +68,17 @@ std::optional MatrixFilterContents::RenderFilter( entity.GetClipDepth()); } +std::optional MatrixFilterContents::GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + auto matrix = matrix_.Basis(); + if (matrix.GetDeterminant() == 0.0) { + return std::nullopt; + } + auto inverse = matrix.Invert(); + return output_limit.TransformBounds(inverse); +} + std::optional MatrixFilterContents::GetFilterCoverage( const FilterInput::Vector& inputs, const Entity& entity, diff --git a/impeller/entity/contents/filters/matrix_filter_contents.h b/impeller/entity/contents/filters/matrix_filter_contents.h index 3a9591fe480d5..bb498ea402443 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.h +++ b/impeller/entity/contents/filters/matrix_filter_contents.h @@ -41,6 +41,11 @@ class MatrixFilterContents final : public FilterContents { const Rect& coverage, const std::optional& coverage_hint) const override; + // |FilterContents| + std::optional GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const override; + Matrix matrix_; SamplerDescriptor sampler_descriptor_ = {}; Entity::RenderingMode rendering_mode_ = Entity::RenderingMode::kDirect; diff --git a/impeller/entity/contents/filters/morphology_filter_contents.cc b/impeller/entity/contents/filters/morphology_filter_contents.cc index bdbe3266c4088..79175b044351e 100644 --- a/impeller/entity/contents/filters/morphology_filter_contents.cc +++ b/impeller/entity/contents/filters/morphology_filter_contents.cc @@ -192,4 +192,18 @@ std::optional DirectionalMorphologyFilterContents::GetFilterCoverage( return Rect(origin, Size(size.x, size.y)); } +std::optional +DirectionalMorphologyFilterContents::GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + auto transformed_vector = + effect_transform.TransformDirection(direction_ * radius_.radius).Abs(); + switch (morph_type_) { + case FilterContents::MorphType::kDilate: + return output_limit.Expand(-transformed_vector); + case FilterContents::MorphType::kErode: + return output_limit.Expand(transformed_vector); + } +} + } // namespace impeller diff --git a/impeller/entity/contents/filters/morphology_filter_contents.h b/impeller/entity/contents/filters/morphology_filter_contents.h index 60118e8ae284d..91482f4673312 100644 --- a/impeller/entity/contents/filters/morphology_filter_contents.h +++ b/impeller/entity/contents/filters/morphology_filter_contents.h @@ -29,6 +29,11 @@ class DirectionalMorphologyFilterContents final : public FilterContents { const Entity& entity, const Matrix& effect_transform) const override; + // |FilterContents| + std::optional GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const override; + private: // |FilterContents| std::optional RenderFilter( diff --git a/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.cc b/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.cc index 29098fb37bb5c..00f37d0824261 100644 --- a/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.cc +++ b/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.cc @@ -137,4 +137,10 @@ std::optional YUVToRGBFilterContents::RenderFilter( return sub_entity; } +std::optional YUVToRGBFilterContents::GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const { + return output_limit; +} + } // namespace impeller diff --git a/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.h b/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.h index 0e6d23c2a1868..ef3ae2e522c93 100644 --- a/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.h +++ b/impeller/entity/contents/filters/yuv_to_rgb_filter_contents.h @@ -26,6 +26,11 @@ class YUVToRGBFilterContents final : public FilterContents { const Rect& coverage, const std::optional& coverage_hint) const override; + // |FilterContents| + std::optional GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const override; + YUVColorSpace yuv_color_space_ = YUVColorSpace::kBT601LimitedRange; YUVToRGBFilterContents(const YUVToRGBFilterContents&) = delete; diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index d85e9e18d49ff..1415554e7bc78 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -195,7 +195,10 @@ std::optional EntityPass::GetSubpassCoverage( // If the subpass has an image filter, then its coverage space may deviate // from the parent pass and make intersecting with the pass coverage limit // unsafe. - coverage_limit = image_filter ? std::nullopt : coverage_limit; + if (image_filter && coverage_limit.has_value()) { + coverage_limit = image_filter->GetSourceCoverage(subpass.xformation_, + coverage_limit.value()); + } auto entities_coverage = subpass.GetElementsCoverage(coverage_limit); // The entities don't cover anything. There is nothing to do. diff --git a/impeller/geometry/rect.h b/impeller/geometry/rect.h index 0bda6140d95b9..585afbd906f9d 100644 --- a/impeller/geometry/rect.h +++ b/impeller/geometry/rect.h @@ -292,6 +292,15 @@ struct TRect { size.height + amount * 2); } + /// @brief Returns a rectangle with expanded edges in all directions. + /// Negative expansion results in shrinking. + constexpr TRect Expand(TPoint amount) const { + return TRect(origin.x - amount.x, // + origin.y - amount.y, // + size.width + amount.x * 2, // + size.height + amount.y * 2); + } + /// @brief Returns a new rectangle that represents the projection of the /// source rectangle onto this rectangle. In other words, the source /// rectangle is redefined in terms of the corrdinate space of this @@ -306,6 +315,33 @@ struct TRect { using Rect = TRect; using IRect = TRect; +constexpr std::optional Union(const Rect& a, + const std::optional b) { + if (!b.has_value()) { + return a; + } + return a.Union(b.value()); +} + +constexpr std::optional Union(const std::optional a, + const Rect& b) { + if (!a.has_value()) { + return b; + } + return a.value().Union(b); +} + +constexpr std::optional Union(const std::optional a, + const std::optional b) { + if (!a.has_value()) { + return b; + } + if (!b.has_value()) { + return a; + } + return a.value().Union(b.value()); +} + } // namespace impeller namespace std { From 821e7b1601c4f9590c53fbbcce3d3ce924be8629 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Mon, 23 Oct 2023 17:16:54 -0700 Subject: [PATCH 2/9] add test case and fix subpass coverage limits --- impeller/aiks/aiks_unittests.cc | 30 ++++++++++++++++++++++ impeller/entity/entity_pass.cc | 17 +++++-------- impeller/geometry/rect.h | 45 ++++++++++++++++++--------------- 3 files changed, 61 insertions(+), 31 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 628b6df1029b3..d1bd34755127f 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -2760,6 +2760,36 @@ TEST_P(AiksTest, TranslucentSaveLayerWithColorAndImageFilterDrawsCorrectly) { ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } +TEST_P(AiksTest, ImageFilteredSaveLayerWithUnboundedContents) { + Canvas canvas; + + auto texture = CreateTextureForFixture("airplane.jpg"); + auto blur_filter = ImageFilter::MakeBlur(Sigma{5.0}, Sigma{5.0}, + FilterContents::BlurStyle::kNormal, + Entity::TileMode::kClamp); + auto image_source = ColorSource::MakeImage(texture, Entity::TileMode::kRepeat, + Entity::TileMode::kRepeat, {}, {}); + + canvas.SaveLayer({.image_filter = blur_filter}, + Rect::MakeLTRB(100, 100, 200, 200)); + + canvas.DrawPaint({.color_source = image_source}); + + Paint blue = {.color = Color::Blue()}; + Paint green = {.color = Color::Green()}; + + canvas.DrawRect(Rect::MakeLTRB(125, 125, 175, 175), blue); + + canvas.DrawRect(Rect::MakeLTRB(125, 50, 175, 98), green); + canvas.DrawRect(Rect::MakeLTRB(202, 125, 250, 175), green); + canvas.DrawRect(Rect::MakeLTRB(125, 202, 175, 250), green); + canvas.DrawRect(Rect::MakeLTRB(50, 125, 98, 175), green); + + canvas.Restore(); + + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} + TEST_P(AiksTest, TranslucentSaveLayerImageDrawsCorrectly) { Canvas canvas; diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 1415554e7bc78..673d04661fc98 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -192,6 +192,12 @@ std::optional EntityPass::GetSubpassCoverage( std::shared_ptr image_filter = subpass.delegate_->WithImageFilter(Rect(), subpass.xformation_); + if (subpass.bounds_limit_.has_value()) { + auto user_bounds_coverage = + subpass.bounds_limit_->TransformBounds(subpass.xformation_); + coverage_limit = Intersection(user_bounds_coverage, coverage_limit); + } + // If the subpass has an image filter, then its coverage space may deviate // from the parent pass and make intersecting with the pass coverage limit // unsafe. @@ -201,17 +207,8 @@ std::optional EntityPass::GetSubpassCoverage( } auto entities_coverage = subpass.GetElementsCoverage(coverage_limit); - // The entities don't cover anything. There is nothing to do. - if (!entities_coverage.has_value()) { - return std::nullopt; - } - if (!subpass.bounds_limit_.has_value()) { - return entities_coverage; - } - auto user_bounds_coverage = - subpass.bounds_limit_->TransformBounds(subpass.xformation_); - return entities_coverage->Intersection(user_bounds_coverage); + return entities_coverage; } EntityPass* EntityPass::GetSuperpass() const { diff --git a/impeller/geometry/rect.h b/impeller/geometry/rect.h index 585afbd906f9d..3dc60ea26b7e2 100644 --- a/impeller/geometry/rect.h +++ b/impeller/geometry/rect.h @@ -315,31 +315,34 @@ struct TRect { using Rect = TRect; using IRect = TRect; -constexpr std::optional Union(const Rect& a, - const std::optional b) { - if (!b.has_value()) { - return a; - } - return a.Union(b.value()); +constexpr inline std::optional Union(const Rect& a, + const std::optional b) { + return b.has_value() ? a.Union(b.value()) : a; } -constexpr std::optional Union(const std::optional a, - const Rect& b) { - if (!a.has_value()) { - return b; - } - return a.value().Union(b); +constexpr inline std::optional Union(const std::optional a, + const Rect& b) { + return Union(b, a); } -constexpr std::optional Union(const std::optional a, - const std::optional b) { - if (!a.has_value()) { - return b; - } - if (!b.has_value()) { - return a; - } - return a.value().Union(b.value()); +constexpr inline std::optional Union(const std::optional a, + const std::optional b) { + return a.has_value() ? Union(a.value(), b) : b; +} + +constexpr inline std::optional Intersection(const Rect& a, + const std::optional b) { + return b.has_value() ? a.Intersection(b.value()) : a; +} + +constexpr inline std::optional Intersection(const std::optional a, + const Rect& b) { + return Intersection(b, a); +} + +constexpr inline std::optional Intersection(const std::optional a, + const std::optional b) { + return a.has_value() ? Intersection(a.value(), b) : b; } } // namespace impeller From f16d5d1913b78b5df27e048c4c89c8f3a6617812 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Tue, 24 Oct 2023 14:51:38 -0700 Subject: [PATCH 3/9] unit tests for new optional functions --- impeller/geometry/geometry_unittests.cc | 104 ++++++++++++++++++++++++ 1 file changed, 104 insertions(+) diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index 9c4d7a77f05ec..26fb5f8304a62 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -1644,6 +1644,58 @@ TEST(GeometryTest, RectUnion) { } } +TEST(GeometryTest, OptRectUnion) { + Rect a = Rect::MakeLTRB(0, 0, 100, 100); + Rect b = Rect::MakeLTRB(100, 100, 200, 200); + Rect c = Rect::MakeLTRB(100, 0, 200, 100); + + // NullOpt, NullOpt + EXPECT_FALSE(Union(std::nullopt, std::nullopt).has_value()); + EXPECT_EQ(Union(std::nullopt, std::nullopt), std::nullopt); + + auto test1 = [](const Rect& r) { + // Rect, NullOpt + EXPECT_TRUE(Union(r, std::nullopt).has_value()); + EXPECT_EQ(Union(r, std::nullopt).value(), r); + + // OptRect, NullOpt + EXPECT_TRUE(Union(std::optional(r), std::nullopt).has_value()); + EXPECT_EQ(Union(std::optional(r), std::nullopt).value(), r); + + // NullOpt, Rect + EXPECT_TRUE(Union(std::nullopt, r).has_value()); + EXPECT_EQ(Union(std::nullopt, r).value(), r); + + // NullOpt, OptRect + EXPECT_TRUE(Union(std::nullopt, std::optional(r)).has_value()); + EXPECT_EQ(Union(std::nullopt, std::optional(r)).value(), r); + }; + + test1(a); + test1(b); + test1(c); + + auto test2 = [](const Rect& a, const Rect& b, const Rect& u) { + ASSERT_EQ(a.Union(b), u); + + // Rect, OptRect + EXPECT_TRUE(Union(a, std::optional(b)).has_value()); + EXPECT_EQ(Union(a, std::optional(b)).value(), u); + + // OptRect, Rect + EXPECT_TRUE(Union(std::optional(a), b).has_value()); + EXPECT_EQ(Union(std::optional(a), b).value(), u); + + // OptRect, OptRect + EXPECT_TRUE(Union(std::optional(a), std::optional(b)).has_value()); + EXPECT_EQ(Union(std::optional(a), std::optional(b)).value(), u); + }; + + test2(a, b, Rect::MakeLTRB(0, 0, 200, 200)); + test2(a, c, Rect::MakeLTRB(0, 0, 200, 100)); + test2(b, c, Rect::MakeLTRB(100, 0, 200, 200)); +} + TEST(GeometryTest, RectIntersection) { { Rect a(100, 100, 100, 100); @@ -1693,6 +1745,58 @@ TEST(GeometryTest, RectIntersection) { } } +TEST(GeometryTest, OptRectIntersection) { + Rect a = Rect::MakeLTRB(0, 0, 110, 110); + Rect b = Rect::MakeLTRB(100, 100, 200, 200); + Rect c = Rect::MakeLTRB(100, 0, 200, 110); + + // NullOpt, NullOpt + EXPECT_FALSE(Intersection(std::nullopt, std::nullopt).has_value()); + EXPECT_EQ(Intersection(std::nullopt, std::nullopt), std::nullopt); + + auto test1 = [](const Rect& r) { + // Rect, NullOpt + EXPECT_TRUE(Intersection(r, std::nullopt).has_value()); + EXPECT_EQ(Intersection(r, std::nullopt).value(), r); + + // OptRect, NullOpt + EXPECT_TRUE(Intersection(std::optional(r), std::nullopt).has_value()); + EXPECT_EQ(Intersection(std::optional(r), std::nullopt).value(), r); + + // NullOpt, Rect + EXPECT_TRUE(Intersection(std::nullopt, r).has_value()); + EXPECT_EQ(Intersection(std::nullopt, r).value(), r); + + // NullOpt, OptRect + EXPECT_TRUE(Intersection(std::nullopt, std::optional(r)).has_value()); + EXPECT_EQ(Intersection(std::nullopt, std::optional(r)).value(), r); + }; + + test1(a); + test1(b); + test1(c); + + auto test2 = [](const Rect& a, const Rect& b, const Rect& i) { + ASSERT_EQ(a.Intersection(b), i); + + // Rect, OptRect + EXPECT_TRUE(Intersection(a, std::optional(b)).has_value()); + EXPECT_EQ(Intersection(a, std::optional(b)).value(), i); + + // OptRect, Rect + EXPECT_TRUE(Intersection(std::optional(a), b).has_value()); + EXPECT_EQ(Intersection(std::optional(a), b).value(), i); + + // OptRect, OptRect + EXPECT_TRUE(Intersection(std::optional(a), std::optional(b)).has_value()); + EXPECT_EQ(Intersection(std::optional(a), std::optional(b)).value(), i); + }; + + test2(a, b, Rect::MakeLTRB(100, 100, 110, 110)); + test2(a, c, Rect::MakeLTRB(100, 0, 110, 110)); + test2(b, c, Rect::MakeLTRB(100, 100, 200, 110)); +} + TEST(GeometryTest, RectIntersectsWithRect) { { Rect a(100, 100, 100, 100); From a387ec21647ad13502e04c4b228912d8b171a0c3 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Tue, 24 Oct 2023 19:59:19 -0700 Subject: [PATCH 4/9] adjust clipping of savelayer content and improve unit tests --- impeller/aiks/aiks_unittests.cc | 54 +++++++++++++++++++++++++-------- impeller/entity/entity_pass.cc | 17 ++++++----- 2 files changed, 51 insertions(+), 20 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index d1bd34755127f..6b6910f3b790e 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -2763,28 +2763,56 @@ TEST_P(AiksTest, TranslucentSaveLayerWithColorAndImageFilterDrawsCorrectly) { TEST_P(AiksTest, ImageFilteredSaveLayerWithUnboundedContents) { Canvas canvas; - auto texture = CreateTextureForFixture("airplane.jpg"); - auto blur_filter = ImageFilter::MakeBlur(Sigma{5.0}, Sigma{5.0}, + auto blur_filter = ImageFilter::MakeBlur(Sigma{10.0}, Sigma{10.0}, FilterContents::BlurStyle::kNormal, - Entity::TileMode::kClamp); - auto image_source = ColorSource::MakeImage(texture, Entity::TileMode::kRepeat, - Entity::TileMode::kRepeat, {}, {}); + Entity::TileMode::kDecal); + + auto DrawLine = [&canvas](const Point& p0, const Point& p1, const Paint& p) { + auto path = PathBuilder{} + .AddLine(p0, p1) + .SetConvexity(Convexity::kConvex) + .TakePath(); + Paint paint = p; + paint.style = Paint::Style::kStroke; + canvas.DrawPath(path, paint); + }; + // Registration marks for the edge of the SaveLayer + DrawLine(Point(75, 100), Point(225, 100), {.color = Color::White()}); + DrawLine(Point(75, 200), Point(225, 200), {.color = Color::White()}); + DrawLine(Point(100, 75), Point(100, 225), {.color = Color::White()}); + DrawLine(Point(200, 75), Point(200, 225), {.color = Color::White()}); canvas.SaveLayer({.image_filter = blur_filter}, Rect::MakeLTRB(100, 100, 200, 200)); + { + // DrawPaint to verify correct behavior when the contents are unbounded. + canvas.DrawPaint({.color = Color::Yellow()}); + + // Contrasting rectangle to see interior blurring + canvas.DrawRect(Rect::MakeLTRB(125, 125, 175, 175), + {.color = Color::Blue()}); + } + canvas.Restore(); - canvas.DrawPaint({.color_source = image_source}); + ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); +} - Paint blue = {.color = Color::Blue()}; - Paint green = {.color = Color::Green()}; +TEST_P(AiksTest, ImageFilteredUnboundedSaveLayerWithUnboundedContents) { + Canvas canvas; - canvas.DrawRect(Rect::MakeLTRB(125, 125, 175, 175), blue); + auto blur_filter = ImageFilter::MakeBlur(Sigma{10.0}, Sigma{10.0}, + FilterContents::BlurStyle::kNormal, + Entity::TileMode::kDecal); - canvas.DrawRect(Rect::MakeLTRB(125, 50, 175, 98), green); - canvas.DrawRect(Rect::MakeLTRB(202, 125, 250, 175), green); - canvas.DrawRect(Rect::MakeLTRB(125, 202, 175, 250), green); - canvas.DrawRect(Rect::MakeLTRB(50, 125, 98, 175), green); + canvas.SaveLayer({.image_filter = blur_filter}, std::nullopt); + { + // DrawPaint to verify correct behavior when the contents are unbounded. + canvas.DrawPaint({.color = Color::Yellow()}); + // Contrasting rectangle to see interior blurring + canvas.DrawRect(Rect::MakeLTRB(125, 125, 175, 175), + {.color = Color::Blue()}); + } canvas.Restore(); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 673d04661fc98..1415554e7bc78 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -192,12 +192,6 @@ std::optional EntityPass::GetSubpassCoverage( std::shared_ptr image_filter = subpass.delegate_->WithImageFilter(Rect(), subpass.xformation_); - if (subpass.bounds_limit_.has_value()) { - auto user_bounds_coverage = - subpass.bounds_limit_->TransformBounds(subpass.xformation_); - coverage_limit = Intersection(user_bounds_coverage, coverage_limit); - } - // If the subpass has an image filter, then its coverage space may deviate // from the parent pass and make intersecting with the pass coverage limit // unsafe. @@ -207,8 +201,17 @@ std::optional EntityPass::GetSubpassCoverage( } auto entities_coverage = subpass.GetElementsCoverage(coverage_limit); + // The entities don't cover anything. There is nothing to do. + if (!entities_coverage.has_value()) { + return std::nullopt; + } - return entities_coverage; + if (!subpass.bounds_limit_.has_value()) { + return entities_coverage; + } + auto user_bounds_coverage = + subpass.bounds_limit_->TransformBounds(subpass.xformation_); + return entities_coverage->Intersection(user_bounds_coverage); } EntityPass* EntityPass::GetSuperpass() const { From d12bd4ee6c30797bf43a8398dbcde6e8d2c20f1e Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 25 Oct 2023 13:48:04 -0700 Subject: [PATCH 5/9] fix MatrixFilterContents::GetFilterSourceCoverage --- .../entity/contents/filters/matrix_filter_contents.cc | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/impeller/entity/contents/filters/matrix_filter_contents.cc b/impeller/entity/contents/filters/matrix_filter_contents.cc index f8a43c19b6f78..d5a5c911c6f11 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents.cc @@ -71,11 +71,10 @@ std::optional MatrixFilterContents::RenderFilter( std::optional MatrixFilterContents::GetFilterSourceCoverage( const Matrix& effect_transform, const Rect& output_limit) const { - auto matrix = matrix_.Basis(); - if (matrix.GetDeterminant() == 0.0) { - return std::nullopt; - } - auto inverse = matrix.Invert(); + auto transform = effect_transform * // + matrix_ * // + effect_transform.Invert(); // + auto inverse = transform.Invert(); return output_limit.TransformBounds(inverse); } From 9f532382cf172788d9c922272ec5750182096d09 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 25 Oct 2023 13:53:14 -0700 Subject: [PATCH 6/9] non-invertible matrices should produce unbounded results --- impeller/entity/contents/filters/matrix_filter_contents.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/impeller/entity/contents/filters/matrix_filter_contents.cc b/impeller/entity/contents/filters/matrix_filter_contents.cc index d5a5c911c6f11..ee566d49b431a 100644 --- a/impeller/entity/contents/filters/matrix_filter_contents.cc +++ b/impeller/entity/contents/filters/matrix_filter_contents.cc @@ -74,6 +74,9 @@ std::optional MatrixFilterContents::GetFilterSourceCoverage( auto transform = effect_transform * // matrix_ * // effect_transform.Invert(); // + if (transform.GetDeterminant() == 0.0) { + return std::nullopt; + } auto inverse = transform.Invert(); return output_limit.TransformBounds(inverse); } From eed0e0b0f400fbd4df3c3468092201fa72be3f2f Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Wed, 25 Oct 2023 16:18:46 -0700 Subject: [PATCH 7/9] round out bounds for subpass render target --- impeller/entity/entity_pass.cc | 2 ++ impeller/geometry/geometry_unittests.cc | 11 +++++++++++ impeller/geometry/rect.h | 5 +++++ 3 files changed, 18 insertions(+) diff --git a/impeller/entity/entity_pass.cc b/impeller/entity/entity_pass.cc index 1415554e7bc78..e79b191da5ef8 100644 --- a/impeller/entity/entity_pass.cc +++ b/impeller/entity/entity_pass.cc @@ -616,6 +616,8 @@ EntityPass::EntityResult EntityPass::GetEntityForElement( return EntityPass::EntityResult::Skip(); } + subpass_coverage = RoundOut(subpass_coverage.value()); + auto subpass_size = ISize(subpass_coverage->size); if (subpass_size.IsEmpty()) { capture.CreateChild("Subpass Entity (Skipped: Empty subpass coverage B)"); diff --git a/impeller/geometry/geometry_unittests.cc b/impeller/geometry/geometry_unittests.cc index 26fb5f8304a62..2b80e64b5a25e 100644 --- a/impeller/geometry/geometry_unittests.cc +++ b/impeller/geometry/geometry_unittests.cc @@ -2114,6 +2114,17 @@ TEST(GeometryTest, RectProject) { } } +TEST(GeometryTest, RectRoundOut) { + { + auto r = Rect::MakeLTRB(-100, -100, 100, 100); + ASSERT_EQ(RoundOut(r), r); + } + { + auto r = Rect::MakeLTRB(-100.1, -100.1, 100.1, 100.1); + ASSERT_EQ(RoundOut(r), Rect::MakeLTRB(-101, -101, 101, 101)); + } +} + TEST(GeometryTest, CubicPathComponentPolylineDoesNotIncludePointOne) { CubicPathComponent component({10, 10}, {20, 35}, {35, 20}, {40, 40}); auto polyline = component.CreatePolyline(1.0f); diff --git a/impeller/geometry/rect.h b/impeller/geometry/rect.h index 3dc60ea26b7e2..5b8805504ac44 100644 --- a/impeller/geometry/rect.h +++ b/impeller/geometry/rect.h @@ -315,6 +315,11 @@ struct TRect { using Rect = TRect; using IRect = TRect; +constexpr inline Rect RoundOut(const Rect& r) { + return Rect::MakeLTRB(floor(r.GetLeft()), floor(r.GetTop()), + ceil(r.GetRight()), ceil(r.GetBottom())); +} + constexpr inline std::optional Union(const Rect& a, const std::optional b) { return b.has_value() ? a.Union(b.value()) : a; From 51356d25b9a4be758ed8c5b03a7084a93ed35080 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 26 Oct 2023 15:34:26 -0700 Subject: [PATCH 8/9] scale the new unit tests and test each type of ImageFilter --- impeller/aiks/aiks_unittests.cc | 107 ++++++++++++++++++++++++-------- 1 file changed, 80 insertions(+), 27 deletions(-) diff --git a/impeller/aiks/aiks_unittests.cc b/impeller/aiks/aiks_unittests.cc index 6b6910f3b790e..1ee111f138dc4 100644 --- a/impeller/aiks/aiks_unittests.cc +++ b/impeller/aiks/aiks_unittests.cc @@ -2762,43 +2762,96 @@ TEST_P(AiksTest, TranslucentSaveLayerWithColorAndImageFilterDrawsCorrectly) { TEST_P(AiksTest, ImageFilteredSaveLayerWithUnboundedContents) { Canvas canvas; + canvas.Scale(GetContentScale()); - auto blur_filter = ImageFilter::MakeBlur(Sigma{10.0}, Sigma{10.0}, - FilterContents::BlurStyle::kNormal, - Entity::TileMode::kDecal); + auto test = [&canvas](const std::shared_ptr& filter) { + auto DrawLine = [&canvas](const Point& p0, const Point& p1, + const Paint& p) { + auto path = PathBuilder{} + .AddLine(p0, p1) + .SetConvexity(Convexity::kConvex) + .TakePath(); + Paint paint = p; + paint.style = Paint::Style::kStroke; + canvas.DrawPath(path, paint); + }; + // Registration marks for the edge of the SaveLayer + DrawLine(Point(75, 100), Point(225, 100), {.color = Color::White()}); + DrawLine(Point(75, 200), Point(225, 200), {.color = Color::White()}); + DrawLine(Point(100, 75), Point(100, 225), {.color = Color::White()}); + DrawLine(Point(200, 75), Point(200, 225), {.color = Color::White()}); + + canvas.SaveLayer({.image_filter = filter}, + Rect::MakeLTRB(100, 100, 200, 200)); + { + // DrawPaint to verify correct behavior when the contents are unbounded. + canvas.DrawPaint({.color = Color::Yellow()}); - auto DrawLine = [&canvas](const Point& p0, const Point& p1, const Paint& p) { - auto path = PathBuilder{} - .AddLine(p0, p1) - .SetConvexity(Convexity::kConvex) - .TakePath(); - Paint paint = p; - paint.style = Paint::Style::kStroke; - canvas.DrawPath(path, paint); + // Contrasting rectangle to see interior blurring + canvas.DrawRect(Rect::MakeLTRB(125, 125, 175, 175), + {.color = Color::Blue()}); + } + canvas.Restore(); }; - // Registration marks for the edge of the SaveLayer - DrawLine(Point(75, 100), Point(225, 100), {.color = Color::White()}); - DrawLine(Point(75, 200), Point(225, 200), {.color = Color::White()}); - DrawLine(Point(100, 75), Point(100, 225), {.color = Color::White()}); - DrawLine(Point(200, 75), Point(200, 225), {.color = Color::White()}); - - canvas.SaveLayer({.image_filter = blur_filter}, - Rect::MakeLTRB(100, 100, 200, 200)); - { - // DrawPaint to verify correct behavior when the contents are unbounded. - canvas.DrawPaint({.color = Color::Yellow()}); - // Contrasting rectangle to see interior blurring - canvas.DrawRect(Rect::MakeLTRB(125, 125, 175, 175), - {.color = Color::Blue()}); - } - canvas.Restore(); + test(ImageFilter::MakeBlur(Sigma{10.0}, Sigma{10.0}, + FilterContents::BlurStyle::kNormal, + Entity::TileMode::kDecal)); + + canvas.Translate({200.0, 0.0}); + + test(ImageFilter::MakeDilate(Radius{10.0}, Radius{10.0})); + + canvas.Translate({200.0, 0.0}); + + test(ImageFilter::MakeErode(Radius{10.0}, Radius{10.0})); + + canvas.Translate({-400.0, 200.0}); + + auto rotate_filter = + ImageFilter::MakeMatrix(Matrix::MakeTranslation({150, 150}) * + Matrix::MakeRotationZ(Degrees{10.0}) * + Matrix::MakeTranslation({-150, -150}), + SamplerDescriptor{}); + test(rotate_filter); + + canvas.Translate({200.0, 0.0}); + + auto rgb_swap_filter = ImageFilter::MakeFromColorFilter( + *ColorFilter::MakeMatrix({.array = { + 0, 1, 0, 0, 0, // + 0, 0, 1, 0, 0, // + 1, 0, 0, 0, 0, // + 0, 0, 0, 1, 0 // + }})); + test(rgb_swap_filter); + + canvas.Translate({200.0, 0.0}); + + test(ImageFilter::MakeCompose(*rotate_filter, *rgb_swap_filter)); + + canvas.Translate({-400.0, 200.0}); + + test(ImageFilter::MakeLocalMatrix(Matrix::MakeTranslation({25.0, 25.0}), + *rotate_filter)); + + canvas.Translate({200.0, 0.0}); + + test(ImageFilter::MakeLocalMatrix(Matrix::MakeTranslation({25.0, 25.0}), + *rgb_swap_filter)); + + canvas.Translate({200.0, 0.0}); + + test(ImageFilter::MakeLocalMatrix( + Matrix::MakeTranslation({25.0, 25.0}), + *ImageFilter::MakeCompose(*rotate_filter, *rgb_swap_filter))); ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture())); } TEST_P(AiksTest, ImageFilteredUnboundedSaveLayerWithUnboundedContents) { Canvas canvas; + canvas.Scale(GetContentScale()); auto blur_filter = ImageFilter::MakeBlur(Sigma{10.0}, Sigma{10.0}, FilterContents::BlurStyle::kNormal, From 3c6c6c19fca488c322df15ae6869a7f3fcccd747 Mon Sep 17 00:00:00 2001 From: Jim Graham Date: Thu, 26 Oct 2023 15:52:39 -0700 Subject: [PATCH 9/9] updates for merge conflict --- .../filters/directional_gaussian_blur_filter_contents.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.h b/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.h index 2fbf984a749ce..ff15c388c713b 100644 --- a/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.h +++ b/impeller/entity/contents/filters/directional_gaussian_blur_filter_contents.h @@ -69,6 +69,11 @@ class DirectionalGaussianBlurFilterContents final : public FilterContents { /// beginning of the chain. void SetIsSecondPass(bool is_second_pass); + // |FilterContents| + std::optional GetFilterSourceCoverage( + const Matrix& effect_transform, + const Rect& output_limit) const override; + // |FilterContents| std::optional GetFilterCoverage( const FilterInput::Vector& inputs,