From e3cbaa705a25057b07d6d38eabd2c649949c5d69 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 10:24:54 -0700 Subject: [PATCH 1/9] [Impeller] render empty filled paths without crashing. --- impeller/entity/entity_unittests.cc | 25 ++++++++++++++++ .../entity/geometry/fill_path_geometry.cc | 29 ++++++++++++++++++- .../entity/geometry/geometry_unittests.cc | 3 ++ impeller/tessellator/tessellator.cc | 3 ++ 4 files changed, 59 insertions(+), 1 deletion(-) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 7dcebc01f8117..216fa2104fb36 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2812,6 +2812,31 @@ TEST_P(EntityTest, FailOnValidationError) { ""); } +TEST_P(EntityTest, CanRenderEmptyPathsWithoutCrashing) { + PathBuilder builder = {}; + builder.AddRect(Rect::MakeLTRB(0, 0, 0, 0)); + Path path = builder.TakePath(); + + EXPECT_TRUE(path.GetBoundingBox()->IsEmpty()); + + auto geom = Geometry::MakeFillPath(path); + + Entity entity; + RenderTarget target = + GetContentContext()->GetRenderTargetCache()->CreateOffscreen( + *GetContext(), {1, 1}, 1u); + testing::MockRenderPass render_pass(GetContext(), target); + auto position_result = + geom->GetPositionBuffer(*GetContentContext(), entity, render_pass); + + auto uv_result = + geom->GetPositionUVBuffer(Rect::MakeLTRB(0, 0, 100, 100), Matrix(), + *GetContentContext(), entity, render_pass); + + EXPECT_EQ(position_result.vertex_buffer.vertex_count, 0u); + EXPECT_EQ(uv_result.vertex_buffer.vertex_count, 0u); +} + } // namespace testing } // namespace impeller diff --git a/impeller/entity/geometry/fill_path_geometry.cc b/impeller/entity/geometry/fill_path_geometry.cc index e187659ce04b1..d23fa39683795 100644 --- a/impeller/entity/geometry/fill_path_geometry.cc +++ b/impeller/entity/geometry/fill_path_geometry.cc @@ -6,6 +6,7 @@ #include "fml/logging.h" #include "impeller/core/formats.h" +#include "impeller/core/vertex_buffer.h" #include "impeller/entity/contents/content_context.h" #include "impeller/entity/geometry/geometry.h" @@ -20,8 +21,21 @@ GeometryResult FillPathGeometry::GetPositionBuffer( const Entity& entity, RenderPass& pass) const { auto& host_buffer = renderer.GetTransientsBuffer(); - VertexBuffer vertex_buffer; + if (path_.GetBoundingBox()->IsEmpty()) { + return GeometryResult{ + .type = PrimitiveType::kTriangle, + .vertex_buffer = + VertexBuffer{ + .vertex_buffer = {}, + .vertex_count = 0, + .index_type = IndexType::k16bit, + }, + .transform = pass.GetOrthographicTransform() * entity.GetTransform(), + }; + } + + VertexBuffer vertex_buffer; if constexpr (!ContentContext::kEnableStencilThenCover) { if (!path_.IsConvex()) { auto tesselation_result = renderer.GetTessellator()->Tessellate( @@ -79,6 +93,19 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( RenderPass& pass) const { using VS = TextureFillVertexShader; + if (path_.GetBoundingBox()->IsEmpty()) { + return GeometryResult{ + .type = PrimitiveType::kTriangle, + .vertex_buffer = + VertexBuffer{ + .vertex_buffer = {}, + .vertex_count = 0, + .index_type = IndexType::k16bit, + }, + .transform = pass.GetOrthographicTransform() * entity.GetTransform(), + }; + } + auto uv_transform = texture_coverage.GetNormalizingTransform() * effect_transform; diff --git a/impeller/entity/geometry/geometry_unittests.cc b/impeller/entity/geometry/geometry_unittests.cc index c56c16613bd57..990b1419659ed 100644 --- a/impeller/entity/geometry/geometry_unittests.cc +++ b/impeller/entity/geometry/geometry_unittests.cc @@ -2,11 +2,14 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +#include #include "flutter/testing/testing.h" +#include "impeller/entity/contents/content_context.h" #include "impeller/entity/geometry/geometry.h" #include "impeller/entity/geometry/stroke_path_geometry.h" #include "impeller/geometry/geometry_asserts.h" #include "impeller/geometry/path_builder.h" +#include "impeller/renderer/testing/mocks.h" inline ::testing::AssertionResult SolidVerticesNear( std::vector a, diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index 8f5edc61374d5..2c4078d2b4897 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -58,6 +58,7 @@ static int ToTessWindingRule(FillType fill_type) { Tessellator::Result Tessellator::Tessellate(const Path& path, Scalar tolerance, const BuilderCallback& callback) { + FML_DCHECK(!path.GetBoundingBox()->IsEmpty()); if (!callback) { return Result::kInputError; } @@ -178,7 +179,9 @@ Path::Polyline Tessellator::CreateTempPolyline(const Path& path, std::vector Tessellator::TessellateConvex(const Path& path, Scalar tolerance) { + FML_DCHECK(!path.GetBoundingBox()->IsEmpty()); FML_DCHECK(point_buffer_); + std::vector output; point_buffer_->clear(); auto polyline = From 5fbc9532abe583083f0bfa222cea44fe0585cae7 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 10:53:15 -0700 Subject: [PATCH 2/9] clang tidy --- impeller/entity/geometry/fill_path_geometry.cc | 6 ++++-- impeller/tessellator/tessellator_unittests.cc | 4 +++- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/impeller/entity/geometry/fill_path_geometry.cc b/impeller/entity/geometry/fill_path_geometry.cc index d23fa39683795..711bbe93d970e 100644 --- a/impeller/entity/geometry/fill_path_geometry.cc +++ b/impeller/entity/geometry/fill_path_geometry.cc @@ -22,7 +22,8 @@ GeometryResult FillPathGeometry::GetPositionBuffer( RenderPass& pass) const { auto& host_buffer = renderer.GetTransientsBuffer(); - if (path_.GetBoundingBox()->IsEmpty()) { + const auto& bounding_box = path_.GetBoundingBox(); + if (bounding_box.has_value() && bounding_box->IsEmpty()) { return GeometryResult{ .type = PrimitiveType::kTriangle, .vertex_buffer = @@ -93,7 +94,8 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( RenderPass& pass) const { using VS = TextureFillVertexShader; - if (path_.GetBoundingBox()->IsEmpty()) { + const auto& bounding_box = path_.GetBoundingBox(); + if (bounding_box.has_value() && bounding_box->IsEmpty()) { return GeometryResult{ .type = PrimitiveType::kTriangle, .vertex_buffer = diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 772a794b85f87..a984bac2f52fa 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -17,7 +17,9 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { // Zero points. { Tessellator t; - auto path = PathBuilder{}.TakePath(FillType::kOdd); + auto path = PathBuilder{} + .AddRect(Rect::MakeLTRB(0, 0, 100, 100)) + .TakePath(FillType::kOdd); Tessellator::Result result = t.Tessellate( path, 1.0f, [](const float* vertices, size_t vertices_count, From fc62291f15f9639417b3a9c1cf4d3b057a117d41 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 11:18:32 -0700 Subject: [PATCH 3/9] early return if there is no geometry. --- .../entity/contents/color_source_contents.h | 6 +++++ impeller/entity/entity_unittests.cc | 22 ++++++++++++++++++- .../entity/geometry/fill_path_geometry.cc | 4 +++- 3 files changed, 30 insertions(+), 2 deletions(-) diff --git a/impeller/entity/contents/color_source_contents.h b/impeller/entity/contents/color_source_contents.h index a23fae582ab86..d5a7263e932e5 100644 --- a/impeller/entity/contents/color_source_contents.h +++ b/impeller/entity/contents/color_source_contents.h @@ -137,6 +137,9 @@ class ColorSourceContents : public Contents { GeometryResult stencil_geometry_result = GetGeometry()->GetPositionBuffer(renderer, entity, pass); + if (stencil_geometry_result.vertex_buffer.vertex_count == 0u) { + return true; + } pass.SetVertexBuffer(std::move(stencil_geometry_result.vertex_buffer)); options.primitive_type = stencil_geometry_result.type; @@ -182,6 +185,9 @@ class ColorSourceContents : public Contents { ? geometry.GetPositionUVBuffer(texture_coverage, effect_transform, renderer, entity, pass) : geometry.GetPositionBuffer(renderer, entity, pass); + if (geometry_result.vertex_buffer.vertex_count == 0u) { + return true; + } pass.SetVertexBuffer(std::move(geometry_result.vertex_buffer)); options.primitive_type = geometry_result.type; diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 216fa2104fb36..9b2f7bdc53595 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2812,7 +2812,7 @@ TEST_P(EntityTest, FailOnValidationError) { ""); } -TEST_P(EntityTest, CanRenderEmptyPathsWithoutCrashing) { +TEST_P(EntityTest, CanComputeGeometryForEmptyPathsWithoutCrashing) { PathBuilder builder = {}; builder.AddRect(Rect::MakeLTRB(0, 0, 0, 0)); Path path = builder.TakePath(); @@ -2835,6 +2835,26 @@ TEST_P(EntityTest, CanRenderEmptyPathsWithoutCrashing) { EXPECT_EQ(position_result.vertex_buffer.vertex_count, 0u); EXPECT_EQ(uv_result.vertex_buffer.vertex_count, 0u); + + EXPECT_EQ(geom->GetResultMode(), GeometryResult::Mode::kNormal); +} + +TEST_P(EntityTest, CanRenderEmptyPathsWithoutCrashing) { + PathBuilder builder = {}; + builder.AddRect(Rect::MakeLTRB(0, 0, 0, 0)); + Path path = builder.TakePath(); + + EXPECT_TRUE(path.GetBoundingBox()->IsEmpty()); + + auto contents = std::make_shared(); + contents->SetGeometry(Geometry::MakeFillPath(path)); + contents->SetColor(Color::Red()); + + Entity entity; + entity.SetTransform(Matrix::MakeScale(GetContentScale())); + entity.SetContents(contents); + + ASSERT_TRUE(OpenPlaygroundHere(std::move(entity))); } } // namespace testing diff --git a/impeller/entity/geometry/fill_path_geometry.cc b/impeller/entity/geometry/fill_path_geometry.cc index 711bbe93d970e..0d4f1782423ff 100644 --- a/impeller/entity/geometry/fill_path_geometry.cc +++ b/impeller/entity/geometry/fill_path_geometry.cc @@ -168,7 +168,9 @@ GeometryResult FillPathGeometry::GetPositionUVBuffer( } GeometryResult::Mode FillPathGeometry::GetResultMode() const { - if (!ContentContext::kEnableStencilThenCover || path_.IsConvex()) { + const auto& bounding_box = path_.GetBoundingBox(); + if (!ContentContext::kEnableStencilThenCover || path_.IsConvex() || + (bounding_box.has_value() && bounding_box->IsEmpty())) { return GeometryResult::Mode::kNormal; } From e509527b8fe2737b17b1b3aa1d0ac580936dfc7f Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 27 Mar 2024 11:47:28 -0700 Subject: [PATCH 4/9] Update tessellator_unittests.cc --- impeller/tessellator/tessellator_unittests.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index a984bac2f52fa..75d3745f3c7f7 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -16,16 +16,17 @@ namespace testing { TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { // Zero points. { + // This will hit a DHCECK so the test can only run in non-debug builds. +#ifndef NDEBUG Tessellator t; - auto path = PathBuilder{} - .AddRect(Rect::MakeLTRB(0, 0, 100, 100)) - .TakePath(FillType::kOdd); + auto path = PathBuilder{}.TakePath(FillType::kOdd); Tessellator::Result result = t.Tessellate( path, 1.0f, [](const float* vertices, size_t vertices_count, const uint16_t* indices, size_t indices_count) { return true; }); ASSERT_EQ(result, Tessellator::Result::kInputError); +#endif // NDEBUG } // One point. From 53af2e8be256b4fffea91ca4bdaca2a063d60f38 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Wed, 27 Mar 2024 12:15:39 -0700 Subject: [PATCH 5/9] Update tessellator_unittests.cc --- impeller/tessellator/tessellator_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 75d3745f3c7f7..fa590dcfba4c7 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -26,7 +26,7 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { const uint16_t* indices, size_t indices_count) { return true; }); ASSERT_EQ(result, Tessellator::Result::kInputError); -#endif // NDEBUG +#endif // NDEBUG } // One point. From 182998d82c5fce5a474a4608cd35f890e617d57c Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 12:58:10 -0700 Subject: [PATCH 6/9] fix ndebug condition. --- impeller/tessellator/tessellator_unittests.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index fa590dcfba4c7..83221a358517b 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -17,7 +17,7 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { // Zero points. { // This will hit a DHCECK so the test can only run in non-debug builds. -#ifndef NDEBUG +#ifdef NDEBUG Tessellator t; auto path = PathBuilder{}.TakePath(FillType::kOdd); Tessellator::Result result = t.Tessellate( From fbd1b3a4be0eb96cddfe82403a6ee0fc2db7f1de Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 13:31:45 -0700 Subject: [PATCH 7/9] ++ --- impeller/tessellator/tessellator_unittests.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 83221a358517b..2921115dcc579 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -17,7 +17,8 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { // Zero points. { // This will hit a DHCECK so the test can only run in non-debug builds. -#ifdef NDEBUG +#ifndef NDEBUG +#else Tessellator t; auto path = PathBuilder{}.TakePath(FillType::kOdd); Tessellator::Result result = t.Tessellate( From 67a668f5e3dde5470468b345f18cdd64ef68f2ea Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 14:02:31 -0700 Subject: [PATCH 8/9] ++ --- impeller/tessellator/tessellator_unittests.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 2921115dcc579..3e69b88d41b9c 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -17,8 +17,7 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { // Zero points. { // This will hit a DHCECK so the test can only run in non-debug builds. -#ifndef NDEBUG -#else +#if NDEBUG Tessellator t; auto path = PathBuilder{}.TakePath(FillType::kOdd); Tessellator::Result result = t.Tessellate( @@ -32,6 +31,7 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { // One point. { +#if NDEBUG Tessellator t; auto path = PathBuilder{}.LineTo({0, 0}).TakePath(FillType::kOdd); Tessellator::Result result = t.Tessellate( @@ -40,6 +40,7 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { const uint16_t* indices, size_t indices_count) { return true; }); ASSERT_EQ(result, Tessellator::Result::kSuccess); +#endif // NDEBUG } // Two points. From 59bd52c2ca56e09874dfc1cbc25bdd82eb541f71 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 27 Mar 2024 14:28:39 -0700 Subject: [PATCH 9/9] remove dchecks --- impeller/tessellator/tessellator.cc | 2 -- impeller/tessellator/tessellator_unittests.cc | 5 ----- 2 files changed, 7 deletions(-) diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index 2c4078d2b4897..665a6248217cb 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -58,7 +58,6 @@ static int ToTessWindingRule(FillType fill_type) { Tessellator::Result Tessellator::Tessellate(const Path& path, Scalar tolerance, const BuilderCallback& callback) { - FML_DCHECK(!path.GetBoundingBox()->IsEmpty()); if (!callback) { return Result::kInputError; } @@ -179,7 +178,6 @@ Path::Polyline Tessellator::CreateTempPolyline(const Path& path, std::vector Tessellator::TessellateConvex(const Path& path, Scalar tolerance) { - FML_DCHECK(!path.GetBoundingBox()->IsEmpty()); FML_DCHECK(point_buffer_); std::vector output; diff --git a/impeller/tessellator/tessellator_unittests.cc b/impeller/tessellator/tessellator_unittests.cc index 3e69b88d41b9c..772a794b85f87 100644 --- a/impeller/tessellator/tessellator_unittests.cc +++ b/impeller/tessellator/tessellator_unittests.cc @@ -16,8 +16,6 @@ namespace testing { TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { // Zero points. { - // This will hit a DHCECK so the test can only run in non-debug builds. -#if NDEBUG Tessellator t; auto path = PathBuilder{}.TakePath(FillType::kOdd); Tessellator::Result result = t.Tessellate( @@ -26,12 +24,10 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { const uint16_t* indices, size_t indices_count) { return true; }); ASSERT_EQ(result, Tessellator::Result::kInputError); -#endif // NDEBUG } // One point. { -#if NDEBUG Tessellator t; auto path = PathBuilder{}.LineTo({0, 0}).TakePath(FillType::kOdd); Tessellator::Result result = t.Tessellate( @@ -40,7 +36,6 @@ TEST(TessellatorTest, TessellatorBuilderReturnsCorrectResultStatus) { const uint16_t* indices, size_t indices_count) { return true; }); ASSERT_EQ(result, Tessellator::Result::kSuccess); -#endif // NDEBUG } // Two points.