From fed2d653da01a2df7bcaa63257bb77dda2daca88 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Nov 2024 14:28:12 -0800 Subject: [PATCH 1/7] [Impeller] generate vertices into point arena. --- .../entity/geometry/geometry_unittests.cc | 20 +- .../entity/geometry/stroke_path_geometry.cc | 246 ++++++++++-------- .../entity/geometry/stroke_path_geometry.h | 14 +- impeller/geometry/geometry_benchmarks.cc | 16 +- impeller/tessellator/tessellator.cc | 3 +- impeller/tessellator/tessellator.h | 4 + 6 files changed, 168 insertions(+), 135 deletions(-) diff --git a/impeller/entity/geometry/geometry_unittests.cc b/impeller/entity/geometry/geometry_unittests.cc index 4b29450bf595d..8889ab141f14b 100644 --- a/impeller/entity/geometry/geometry_unittests.cc +++ b/impeller/entity/geometry/geometry_unittests.cc @@ -14,13 +14,13 @@ #include "impeller/renderer/testing/mocks.h" inline ::testing::AssertionResult SolidVerticesNear( - std::vector a, - std::vector b) { + std::vector a, + std::vector b) { if (a.size() != b.size()) { return ::testing::AssertionFailure() << "Colors length does not match"; } for (auto i = 0u; i < b.size(); i++) { - if (!PointNear(a[i].position, b[i].position)) { + if (!PointNear(a[i], b[i])) { return ::testing::AssertionFailure() << "Positions are not equal."; } } @@ -53,13 +53,13 @@ namespace impeller { class ImpellerEntityUnitTestAccessor { public: - static std::vector - GenerateSolidStrokeVertices(const Path::Polyline& polyline, - Scalar stroke_width, - Scalar miter_limit, - Join stroke_join, - Cap stroke_cap, - Scalar scale) { + static std::vector GenerateSolidStrokeVertices( + const Path::Polyline& polyline, + Scalar stroke_width, + Scalar miter_limit, + Join stroke_join, + Cap stroke_cap, + Scalar scale) { return StrokePathGeometry::GenerateSolidStrokeVertices( polyline, stroke_width, miter_limit, stroke_join, stroke_cap, scale); } diff --git a/impeller/entity/geometry/stroke_path_geometry.cc b/impeller/entity/geometry/stroke_path_geometry.cc index a7020a87c3528..3f4f3d6d3ec2c 100644 --- a/impeller/entity/geometry/stroke_path_geometry.cc +++ b/impeller/entity/geometry/stroke_path_geometry.cc @@ -11,49 +11,56 @@ #include "impeller/geometry/path_builder.h" #include "impeller/geometry/path_component.h" #include "impeller/geometry/separated_vector.h" +#include "impeller/geometry/wangs_formula.h" namespace impeller { -using VS = SolidFillVertexShader; namespace { -template -using CapProc = std::function& points) + : points_(points), oversized_(0) {} + + void AppendVertex(const Point& point) { + if (offset_ >= 4096) { + oversized_.push_back(point); + } + points_[offset_++] = point; + } + + size_t GetUsedSize() const { return offset_; } + + bool HasOversizedBuffer() const { return !oversized_.empty(); } + + const std::vector& GetOveriszedBuffer() const { return oversized_; } + + private: + std::vector& points_; + std::vector oversized_; + size_t offset_ = 0u; +}; + +using CapProc = std::function; -template -using JoinProc = std::function; -class PositionWriter { - public: - void AppendVertex(const Point& point) { - data_.emplace_back(SolidFillVertexShader::PerVertexData{.position = point}); - } - - const std::vector& GetData() const { - return data_; - } - - private: - std::vector data_ = {}; -}; - -template class StrokeGenerator { public: StrokeGenerator(const Path::Polyline& p_polyline, const Scalar p_stroke_width, const Scalar p_scaled_miter_limit, - const JoinProc& p_join_proc, - const CapProc& p_cap_proc, + const JoinProc& p_join_proc, + const CapProc& p_cap_proc, const Scalar p_scale) : polyline(p_polyline), stroke_width(p_stroke_width), @@ -62,7 +69,7 @@ class StrokeGenerator { cap_proc(p_cap_proc), scale(p_scale) {} - void Generate(VertexWriter& vtx_builder) { + void Generate(PositionWriter& vtx_builder) { for (size_t contour_i = 0; contour_i < polyline.contours.size(); contour_i++) { const Path::PolylineContour& contour = polyline.contours[contour_i]; @@ -70,7 +77,7 @@ class StrokeGenerator { std::tie(contour_start_point_i, contour_end_point_i) = polyline.GetContourPointBounds(contour_i); - auto contour_delta = contour_end_point_i - contour_start_point_i; + size_t contour_delta = contour_end_point_i - contour_start_point_i; if (contour_delta == 1) { Point p = polyline.GetPoint(contour_start_point_i); cap_proc(vtx_builder, p, {-stroke_width * 0.5f, 0}, scale, @@ -177,7 +184,7 @@ class StrokeGenerator { stroke_width * 0.5f); } - void AddVerticesForLinearComponent(VertexWriter& vtx_builder, + void AddVerticesForLinearComponent(PositionWriter& vtx_builder, const size_t component_start_index, const size_t component_end_index, const size_t contour_start_point_i, @@ -216,7 +223,7 @@ class StrokeGenerator { } } - void AddVerticesForCurveComponent(VertexWriter& vtx_builder, + void AddVerticesForCurveComponent(PositionWriter& vtx_builder, const size_t component_start_index, const size_t component_end_index, const size_t contour_start_point_i, @@ -296,8 +303,8 @@ class StrokeGenerator { const Path::Polyline& polyline; const Scalar stroke_width; const Scalar scaled_miter_limit; - const JoinProc& join_proc; - const CapProc& cap_proc; + const JoinProc& join_proc; + const CapProc& cap_proc; const Scalar scale; SeparatedVector2 previous_offset; @@ -305,22 +312,17 @@ class StrokeGenerator { SolidFillVertexShader::PerVertexData vtx; }; -template -void CreateButtCap(VertexWriter& vtx_builder, +void CreateButtCap(PositionWriter& vtx_builder, const Point& position, const Point& offset, Scalar scale, bool reverse) { Point orientation = offset * (reverse ? -1 : 1); - VS::PerVertexData vtx; - vtx.position = position + orientation; - vtx_builder.AppendVertex(vtx.position); - vtx.position = position - orientation; - vtx_builder.AppendVertex(vtx.position); + vtx_builder.AppendVertex(position + orientation); + vtx_builder.AppendVertex(position - orientation); } -template -void CreateRoundCap(VertexWriter& vtx_builder, +void CreateRoundCap(PositionWriter& vtx_builder, const Point& position, const Point& offset, Scalar scale, @@ -347,17 +349,18 @@ void CreateRoundCap(VertexWriter& vtx_builder, vtx = position - orientation; vtx_builder.AppendVertex(vtx); - arc.ToLinearPathComponents(scale, [&vtx_builder, &vtx, forward_normal, - position](const Point& point) { + Scalar line_count = std::ceilf(ComputeCubicSubdivisions(scale, arc)); + for (size_t i = 1; i < line_count; i++) { + Point point = arc.Solve(i / line_count); vtx = position + point; vtx_builder.AppendVertex(vtx); vtx = position + (-point).Reflect(forward_normal); vtx_builder.AppendVertex(vtx); - }); + } + vtx_builder.AppendVertex(arc.p2); } -template -void CreateSquareCap(VertexWriter& vtx_builder, +void CreateSquareCap(PositionWriter& vtx_builder, const Point& position, const Point& offset, Scalar scale, @@ -375,8 +378,7 @@ void CreateSquareCap(VertexWriter& vtx_builder, vtx_builder.AppendVertex(vtx); } -template -Scalar CreateBevelAndGetDirection(VertexWriter& vtx_builder, +Scalar CreateBevelAndGetDirection(PositionWriter& vtx_builder, const Point& position, const Point& start_offset, const Point& end_offset) { @@ -392,8 +394,7 @@ Scalar CreateBevelAndGetDirection(VertexWriter& vtx_builder, return dir; } -template -void CreateMiterJoin(VertexWriter& vtx_builder, +void CreateMiterJoin(PositionWriter& vtx_builder, const Point& position, const Point& start_offset, const Point& end_offset, @@ -417,13 +418,10 @@ void CreateMiterJoin(VertexWriter& vtx_builder, } // Outer miter point. - VS::PerVertexData vtx; - vtx.position = position + miter_point * direction; - vtx_builder.AppendVertex(vtx.position); + vtx_builder.AppendVertex(position + miter_point * direction); } -template -void CreateRoundJoin(VertexWriter& vtx_builder, +void CreateRoundJoin(PositionWriter& vtx_builder, const Point& position, const Point& start_offset, const Point& end_offset, @@ -452,19 +450,20 @@ void CreateRoundJoin(VertexWriter& vtx_builder, PathBuilder::kArcApproximationMagic * alignment * direction; - VS::PerVertexData vtx; - CubicPathComponent(start_offset, start_handle, middle_handle, middle) - .ToLinearPathComponents(scale, [&vtx_builder, direction, &vtx, position, - middle_normal](const Point& point) { - vtx.position = position + point * direction; - vtx_builder.AppendVertex(vtx.position); - vtx.position = position + (-point * direction).Reflect(middle_normal); - vtx_builder.AppendVertex(vtx.position); - }); + CubicPathComponent arc(start_offset, start_handle, middle_handle, middle); + Scalar line_count = std::ceilf(ComputeCubicSubdivisions(scale, arc)); + for (size_t i = 1; i < line_count; i++) { + Point point = arc.Solve(i / line_count); + vtx_builder.AppendVertex(position + point * direction); + vtx_builder.AppendVertex(position + + (-point * direction).Reflect(middle_normal)); + } + vtx_builder.AppendVertex(position + arc.p2 * direction); + vtx_builder.AppendVertex(position + + (-arc.p2 * direction).Reflect(middle_normal)); } -template -void CreateBevelJoin(VertexWriter& vtx_builder, +void CreateBevelJoin(PositionWriter& vtx_builder, const Point& position, const Point& start_offset, const Point& end_offset, @@ -473,13 +472,12 @@ void CreateBevelJoin(VertexWriter& vtx_builder, CreateBevelAndGetDirection(vtx_builder, position, start_offset, end_offset); } -template -void CreateSolidStrokeVertices(VertexWriter& vtx_builder, +void CreateSolidStrokeVertices(PositionWriter& vtx_builder, const Path::Polyline& polyline, Scalar stroke_width, Scalar scaled_miter_limit, - const JoinProc& join_proc, - const CapProc& cap_proc, + const JoinProc& join_proc, + const CapProc& cap_proc, Scalar scale) { StrokeGenerator stroke_generator(polyline, stroke_width, scaled_miter_limit, join_proc, cap_proc, scale); @@ -487,46 +485,46 @@ void CreateSolidStrokeVertices(VertexWriter& vtx_builder, } // static -template -JoinProc GetJoinProc(Join stroke_join) { + +JoinProc GetJoinProc(Join stroke_join) { switch (stroke_join) { case Join::kBevel: - return &CreateBevelJoin; + return &CreateBevelJoin; case Join::kMiter: - return &CreateMiterJoin; + return &CreateMiterJoin; case Join::kRound: - return &CreateRoundJoin; + return &CreateRoundJoin; } } -template -CapProc GetCapProc(Cap stroke_cap) { +CapProc GetCapProc(Cap stroke_cap) { switch (stroke_cap) { case Cap::kButt: - return &CreateButtCap; + return &CreateButtCap; case Cap::kRound: - return &CreateRoundCap; + return &CreateRoundCap; case Cap::kSquare: - return &CreateSquareCap; + return &CreateSquareCap; } } } // namespace -std::vector -StrokePathGeometry::GenerateSolidStrokeVertices(const Path::Polyline& polyline, - Scalar stroke_width, - Scalar miter_limit, - Join stroke_join, - Cap stroke_cap, - Scalar scale) { +std::vector StrokePathGeometry::GenerateSolidStrokeVertices( + const Path::Polyline& polyline, + Scalar stroke_width, + Scalar miter_limit, + Join stroke_join, + Cap stroke_cap, + Scalar scale) { auto scaled_miter_limit = stroke_width * miter_limit * 0.5f; - auto join_proc = GetJoinProc(stroke_join); - auto cap_proc = GetCapProc(stroke_cap); + JoinProc join_proc = GetJoinProc(stroke_join); + CapProc cap_proc = GetCapProc(stroke_cap); StrokeGenerator stroke_generator(polyline, stroke_width, scaled_miter_limit, join_proc, cap_proc, scale); - PositionWriter vtx_builder; + std::vector points(4096); + PositionWriter vtx_builder(points); stroke_generator.Generate(vtx_builder); - return vtx_builder.GetData(); + return points; } StrokePathGeometry::StrokePathGeometry(const Path& path, @@ -580,29 +578,61 @@ GeometryResult StrokePathGeometry::GetPositionBuffer( auto& host_buffer = renderer.GetTransientsBuffer(); auto scale = entity.GetTransform().GetMaxBasisLengthXY(); - PositionWriter position_writer; - auto polyline = renderer.GetTessellator().CreateTempPolyline(path_, scale); + PositionWriter position_writer( + renderer.GetTessellator().GetStrokePointCache()); + Path::Polyline polyline = + renderer.GetTessellator().CreateTempPolyline(path_, scale); + CreateSolidStrokeVertices(position_writer, polyline, stroke_width, miter_limit_ * stroke_width_ * 0.5f, - GetJoinProc(stroke_join_), - GetCapProc(stroke_cap_), scale); - - BufferView buffer_view = - host_buffer.Emplace(position_writer.GetData().data(), - position_writer.GetData().size() * - sizeof(SolidFillVertexShader::PerVertexData), - alignof(SolidFillVertexShader::PerVertexData)); - - return GeometryResult{ - .type = PrimitiveType::kTriangleStrip, - .vertex_buffer = - { - .vertex_buffer = buffer_view, - .vertex_count = position_writer.GetData().size(), - .index_type = IndexType::kNone, - }, - .transform = entity.GetShaderTransform(pass), - .mode = GeometryResult::Mode::kPreventOverdraw}; + GetJoinProc(stroke_join_), GetCapProc(stroke_cap_), + scale); + + if (!position_writer.HasOversizedBuffer()) { + BufferView buffer_view = host_buffer.Emplace( + renderer.GetTessellator().GetStrokePointCache().data(), + position_writer.GetUsedSize() * sizeof(Point), alignof(Point)); + + return GeometryResult{.type = PrimitiveType::kTriangleStrip, + .vertex_buffer = + { + .vertex_buffer = buffer_view, + .vertex_count = position_writer.GetUsedSize(), + .index_type = IndexType::kNone, + }, + .transform = entity.GetShaderTransform(pass), + .mode = GeometryResult::Mode::kPreventOverdraw}; + } + FML_LOG(ERROR) << "Oversized"; + const std::vector& oversized_data = + position_writer.GetOveriszedBuffer(); + BufferView buffer_view = host_buffer.Emplace( + nullptr, + (position_writer.GetUsedSize() + oversized_data.size()) * sizeof(Point), + alignof(Point)); + memcpy(buffer_view.GetBuffer()->OnGetContents() + + buffer_view.GetRange().offset, // + renderer.GetTessellator().GetStrokePointCache().data(), // + position_writer.GetUsedSize() * sizeof(Point) // + ); + memcpy( + buffer_view.GetBuffer()->OnGetContents() + buffer_view.GetRange().offset + + position_writer.GetUsedSize() * sizeof(Point), // + oversized_data.data(), // + oversized_data.size() * sizeof(Point) // + ); + buffer_view.GetBuffer()->Flush(buffer_view.GetRange()); + + return GeometryResult{.type = PrimitiveType::kTriangleStrip, + .vertex_buffer = + { + .vertex_buffer = buffer_view, + .vertex_count = position_writer.GetUsedSize() + + oversized_data.size(), + .index_type = IndexType::kNone, + }, + .transform = entity.GetShaderTransform(pass), + .mode = GeometryResult::Mode::kPreventOverdraw}; } GeometryResult::Mode StrokePathGeometry::GetResultMode() const { diff --git a/impeller/entity/geometry/stroke_path_geometry.h b/impeller/entity/geometry/stroke_path_geometry.h index 4265710e6941a..95970d9dec3ba 100644 --- a/impeller/entity/geometry/stroke_path_geometry.h +++ b/impeller/entity/geometry/stroke_path_geometry.h @@ -44,13 +44,13 @@ class StrokePathGeometry final : public Geometry { std::optional GetCoverage(const Matrix& transform) const override; // Private for benchmarking and debugging - static std::vector - GenerateSolidStrokeVertices(const Path::Polyline& polyline, - Scalar stroke_width, - Scalar miter_limit, - Join stroke_join, - Cap stroke_cap, - Scalar scale); + static std::vector GenerateSolidStrokeVertices( + const Path::Polyline& polyline, + Scalar stroke_width, + Scalar miter_limit, + Join stroke_join, + Cap stroke_cap, + Scalar scale); friend class ImpellerBenchmarkAccessor; friend class ImpellerEntityUnitTestAccessor; diff --git a/impeller/geometry/geometry_benchmarks.cc b/impeller/geometry/geometry_benchmarks.cc index 1df17cffd69ba..2c7b9fccf45c1 100644 --- a/impeller/geometry/geometry_benchmarks.cc +++ b/impeller/geometry/geometry_benchmarks.cc @@ -4,8 +4,6 @@ #include "flutter/benchmarking/benchmarking.h" -#include "flutter/impeller/entity/solid_fill.vert.h" - #include "impeller/entity/geometry/stroke_path_geometry.h" #include "impeller/geometry/path.h" #include "impeller/geometry/path_builder.h" @@ -15,13 +13,13 @@ namespace impeller { class ImpellerBenchmarkAccessor { public: - static std::vector - GenerateSolidStrokeVertices(const Path::Polyline& polyline, - Scalar stroke_width, - Scalar miter_limit, - Join stroke_join, - Cap stroke_cap, - Scalar scale) { + static const std::vector& GenerateSolidStrokeVertices( + const Path::Polyline& polyline, + Scalar stroke_width, + Scalar miter_limit, + Join stroke_join, + Cap stroke_cap, + Scalar scale) { return StrokePathGeometry::GenerateSolidStrokeVertices( polyline, stroke_width, miter_limit, stroke_join, stroke_cap, scale); } diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index 6a2fc8260bb9e..3cc43977d1dfe 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -12,7 +12,8 @@ namespace impeller { Tessellator::Tessellator() : point_buffer_(std::make_unique>()), - index_buffer_(std::make_unique>()) { + index_buffer_(std::make_unique>()), + stroke_points_(4096) { point_buffer_->reserve(2048); index_buffer_->reserve(2048); } diff --git a/impeller/tessellator/tessellator.h b/impeller/tessellator/tessellator.h index 18fb013b2b1c9..ac20c564da599 100644 --- a/impeller/tessellator/tessellator.h +++ b/impeller/tessellator/tessellator.h @@ -284,10 +284,14 @@ class Tessellator { const Rect& bounds, const Size& radii); + std::vector& GetStrokePointCache() { return stroke_points_; } + protected: /// Used for polyline generation. std::unique_ptr> point_buffer_; std::unique_ptr> index_buffer_; + /// Used for stroke path generation. + std::vector stroke_points_; private: // Data for various Circle/EllipseGenerator classes, cached per From 6ff974843792b15c93dc8c66ed1fc5d53a438487 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Nov 2024 15:19:33 -0800 Subject: [PATCH 2/7] fix return of temporary object. --- impeller/geometry/geometry_benchmarks.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/impeller/geometry/geometry_benchmarks.cc b/impeller/geometry/geometry_benchmarks.cc index 2c7b9fccf45c1..d59650be5c731 100644 --- a/impeller/geometry/geometry_benchmarks.cc +++ b/impeller/geometry/geometry_benchmarks.cc @@ -13,7 +13,7 @@ namespace impeller { class ImpellerBenchmarkAccessor { public: - static const std::vector& GenerateSolidStrokeVertices( + static std::vector GenerateSolidStrokeVertices( const Path::Polyline& polyline, Scalar stroke_width, Scalar miter_limit, From 8333074d81f5cb5d1e9a048dd4e6ca915f86fc06 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Nov 2024 16:00:31 -0800 Subject: [PATCH 3/7] Add unit tests and fix branch condition. --- .../entity/geometry/stroke_path_geometry.cc | 6 +-- impeller/renderer/renderer_unittests.cc | 41 +++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/impeller/entity/geometry/stroke_path_geometry.cc b/impeller/entity/geometry/stroke_path_geometry.cc index 3f4f3d6d3ec2c..721120415ac06 100644 --- a/impeller/entity/geometry/stroke_path_geometry.cc +++ b/impeller/entity/geometry/stroke_path_geometry.cc @@ -23,10 +23,11 @@ class PositionWriter { : points_(points), oversized_(0) {} void AppendVertex(const Point& point) { - if (offset_ >= 4096) { + if (offset_ >= 4096u) { oversized_.push_back(point); + } else { + points_[offset_++] = point; } - points_[offset_++] = point; } size_t GetUsedSize() const { return offset_; } @@ -603,7 +604,6 @@ GeometryResult StrokePathGeometry::GetPositionBuffer( .transform = entity.GetShaderTransform(pass), .mode = GeometryResult::Mode::kPreventOverdraw}; } - FML_LOG(ERROR) << "Oversized"; const std::vector& oversized_data = position_writer.GetOveriszedBuffer(); BufferView buffer_view = host_buffer.Emplace( diff --git a/impeller/renderer/renderer_unittests.cc b/impeller/renderer/renderer_unittests.cc index 39cb1a7f19c7b..699456267d093 100644 --- a/impeller/renderer/renderer_unittests.cc +++ b/impeller/renderer/renderer_unittests.cc @@ -8,6 +8,8 @@ #include "impeller/core/formats.h" #include "impeller/core/host_buffer.h" #include "impeller/core/sampler_descriptor.h" +#include "impeller/entity/contents/content_context.h" +#include "impeller/entity/geometry/geometry.h" #include "impeller/fixtures/array.frag.h" #include "impeller/fixtures/array.vert.h" #include "impeller/fixtures/baby.frag.h" @@ -1626,6 +1628,45 @@ TEST_P(RendererTest, CanSepiaToneThenSwizzleWithSubpasses) { OpenPlaygroundHere(callback); } +TEST_P(RendererTest, GiantStrokePathAllocation) { + PathBuilder builder{}; + for (int i = 0; i < 10000; i++) { + builder.LineTo(Point(i, i)); + } + Path path = builder.TakePath(); + auto geom = Geometry::MakeStrokePath(path, /*stroke_width=*/10); + + ContentContext content_context(GetContext(), nullptr); + Entity entity; + + auto cmd_buffer = content_context.GetContext()->CreateCommandBuffer(); + + RenderTargetAllocator allocator( + content_context.GetContext()->GetResourceAllocator()); + + auto render_target = + allocator.CreateOffscreen(*content_context.GetContext(), {10, 10}, 1); + auto pass = cmd_buffer->CreateRenderPass(render_target); + + GeometryResult result = + geom->GetPositionBuffer(content_context, entity, *pass); + + // Validate the buffer data overflowed the small buffer + EXPECT_GT(result.vertex_buffer.vertex_count, 4096u); + + // Validate that there are no uninitialized points near the gap. + Point* written_data = reinterpret_cast( + (result.vertex_buffer.vertex_buffer.GetBuffer()->OnGetContents() + + result.vertex_buffer.vertex_buffer.GetRange().offset)); + + EXPECT_NE(*(written_data + 4094), Point(0, 0)); + EXPECT_NE(*(written_data + 4095), Point(0, 0)); + EXPECT_NE(*(written_data + 4096), Point(0, 0)); + EXPECT_NE(*(written_data + 4097), Point(0, 0)); + EXPECT_NE(*(written_data + 4098), Point(0, 0)); + EXPECT_NE(*(written_data + 4099), Point(0, 0)); +} + } // namespace testing } // namespace impeller From 8de563101e25b45a393bb59ba7e1f22f318f2756 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Nov 2024 16:18:44 -0800 Subject: [PATCH 4/7] fix includes by moving to entity unit tests. --- impeller/entity/entity_unittests.cc | 39 +++++++++++++++++++++++ impeller/renderer/renderer_unittests.cc | 41 ------------------------- 2 files changed, 39 insertions(+), 41 deletions(-) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index d971baba9ad16..7b71eeb22e593 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2364,6 +2364,45 @@ APPLY_COLOR_FILTER_GRADIENT_TEST(Radial); APPLY_COLOR_FILTER_GRADIENT_TEST(Conical); APPLY_COLOR_FILTER_GRADIENT_TEST(Sweep); +TEST_P(EntityTest, GiantStrokePathAllocation) { + PathBuilder builder{}; + for (int i = 0; i < 10000; i++) { + builder.LineTo(Point(i, i)); + } + Path path = builder.TakePath(); + auto geom = Geometry::MakeStrokePath(path, /*stroke_width=*/10); + + ContentContext content_context(GetContext(), nullptr); + Entity entity; + + auto cmd_buffer = content_context.GetContext()->CreateCommandBuffer(); + + RenderTargetAllocator allocator( + content_context.GetContext()->GetResourceAllocator()); + + auto render_target = + allocator.CreateOffscreen(*content_context.GetContext(), {10, 10}, 1); + auto pass = cmd_buffer->CreateRenderPass(render_target); + + GeometryResult result = + geom->GetPositionBuffer(content_context, entity, *pass); + + // Validate the buffer data overflowed the small buffer + EXPECT_GT(result.vertex_buffer.vertex_count, 4096u); + + // Validate that there are no uninitialized points near the gap. + Point* written_data = reinterpret_cast( + (result.vertex_buffer.vertex_buffer.GetBuffer()->OnGetContents() + + result.vertex_buffer.vertex_buffer.GetRange().offset)); + + EXPECT_NE(*(written_data + 4094), Point(0, 0)); + EXPECT_NE(*(written_data + 4095), Point(0, 0)); + EXPECT_NE(*(written_data + 4096), Point(0, 0)); + EXPECT_NE(*(written_data + 4097), Point(0, 0)); + EXPECT_NE(*(written_data + 4098), Point(0, 0)); + EXPECT_NE(*(written_data + 4099), Point(0, 0)); +} + } // namespace testing } // namespace impeller diff --git a/impeller/renderer/renderer_unittests.cc b/impeller/renderer/renderer_unittests.cc index 699456267d093..39cb1a7f19c7b 100644 --- a/impeller/renderer/renderer_unittests.cc +++ b/impeller/renderer/renderer_unittests.cc @@ -8,8 +8,6 @@ #include "impeller/core/formats.h" #include "impeller/core/host_buffer.h" #include "impeller/core/sampler_descriptor.h" -#include "impeller/entity/contents/content_context.h" -#include "impeller/entity/geometry/geometry.h" #include "impeller/fixtures/array.frag.h" #include "impeller/fixtures/array.vert.h" #include "impeller/fixtures/baby.frag.h" @@ -1628,45 +1626,6 @@ TEST_P(RendererTest, CanSepiaToneThenSwizzleWithSubpasses) { OpenPlaygroundHere(callback); } -TEST_P(RendererTest, GiantStrokePathAllocation) { - PathBuilder builder{}; - for (int i = 0; i < 10000; i++) { - builder.LineTo(Point(i, i)); - } - Path path = builder.TakePath(); - auto geom = Geometry::MakeStrokePath(path, /*stroke_width=*/10); - - ContentContext content_context(GetContext(), nullptr); - Entity entity; - - auto cmd_buffer = content_context.GetContext()->CreateCommandBuffer(); - - RenderTargetAllocator allocator( - content_context.GetContext()->GetResourceAllocator()); - - auto render_target = - allocator.CreateOffscreen(*content_context.GetContext(), {10, 10}, 1); - auto pass = cmd_buffer->CreateRenderPass(render_target); - - GeometryResult result = - geom->GetPositionBuffer(content_context, entity, *pass); - - // Validate the buffer data overflowed the small buffer - EXPECT_GT(result.vertex_buffer.vertex_count, 4096u); - - // Validate that there are no uninitialized points near the gap. - Point* written_data = reinterpret_cast( - (result.vertex_buffer.vertex_buffer.GetBuffer()->OnGetContents() + - result.vertex_buffer.vertex_buffer.GetRange().offset)); - - EXPECT_NE(*(written_data + 4094), Point(0, 0)); - EXPECT_NE(*(written_data + 4095), Point(0, 0)); - EXPECT_NE(*(written_data + 4096), Point(0, 0)); - EXPECT_NE(*(written_data + 4097), Point(0, 0)); - EXPECT_NE(*(written_data + 4098), Point(0, 0)); - EXPECT_NE(*(written_data + 4099), Point(0, 0)); -} - } // namespace testing } // namespace impeller From 783c093bf7ba6f86639bc92897102aa71a234d6f Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Nov 2024 16:27:03 -0800 Subject: [PATCH 5/7] use constant for arena size. --- impeller/entity/entity_unittests.cc | 14 +++++++------- impeller/entity/geometry/stroke_path_geometry.cc | 2 +- impeller/tessellator/tessellator.cc | 6 +++++- impeller/tessellator/tessellator.h | 6 +++++- 4 files changed, 18 insertions(+), 10 deletions(-) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 7b71eeb22e593..a10db7579e20b 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2388,19 +2388,19 @@ TEST_P(EntityTest, GiantStrokePathAllocation) { geom->GetPositionBuffer(content_context, entity, *pass); // Validate the buffer data overflowed the small buffer - EXPECT_GT(result.vertex_buffer.vertex_count, 4096u); + EXPECT_GT(result.vertex_buffer.vertex_count, kPointArenaSize); // Validate that there are no uninitialized points near the gap. Point* written_data = reinterpret_cast( (result.vertex_buffer.vertex_buffer.GetBuffer()->OnGetContents() + result.vertex_buffer.vertex_buffer.GetRange().offset)); - EXPECT_NE(*(written_data + 4094), Point(0, 0)); - EXPECT_NE(*(written_data + 4095), Point(0, 0)); - EXPECT_NE(*(written_data + 4096), Point(0, 0)); - EXPECT_NE(*(written_data + 4097), Point(0, 0)); - EXPECT_NE(*(written_data + 4098), Point(0, 0)); - EXPECT_NE(*(written_data + 4099), Point(0, 0)); + EXPECT_NE(*(written_data + kPointArenaSize - 2), Point(0, 0)); + EXPECT_NE(*(written_data + kPointArenaSize - 1), Point(0, 0)); + EXPECT_NE(*(written_data + kPointArenaSize), Point(0, 0)); + EXPECT_NE(*(written_data + kPointArenaSize + 1), Point(0, 0)); + EXPECT_NE(*(written_data + kPointArenaSize + 2), Point(0, 0)); + EXPECT_NE(*(written_data + kPointArenaSize + 3), Point(0, 0)); } } // namespace testing diff --git a/impeller/entity/geometry/stroke_path_geometry.cc b/impeller/entity/geometry/stroke_path_geometry.cc index 721120415ac06..187d689beb191 100644 --- a/impeller/entity/geometry/stroke_path_geometry.cc +++ b/impeller/entity/geometry/stroke_path_geometry.cc @@ -23,7 +23,7 @@ class PositionWriter { : points_(points), oversized_(0) {} void AppendVertex(const Point& point) { - if (offset_ >= 4096u) { + if (offset_ >= kPointArenaSize) { oversized_.push_back(point); } else { points_[offset_++] = point; diff --git a/impeller/tessellator/tessellator.cc b/impeller/tessellator/tessellator.cc index 3cc43977d1dfe..85d3afb3587bf 100644 --- a/impeller/tessellator/tessellator.cc +++ b/impeller/tessellator/tessellator.cc @@ -13,13 +13,17 @@ namespace impeller { Tessellator::Tessellator() : point_buffer_(std::make_unique>()), index_buffer_(std::make_unique>()), - stroke_points_(4096) { + stroke_points_(kPointArenaSize) { point_buffer_->reserve(2048); index_buffer_->reserve(2048); } Tessellator::~Tessellator() = default; +std::vector& Tessellator::GetStrokePointCache() { + return stroke_points_; +} + Path::Polyline Tessellator::CreateTempPolyline(const Path& path, Scalar tolerance) { FML_DCHECK(point_buffer_); diff --git a/impeller/tessellator/tessellator.h b/impeller/tessellator/tessellator.h index ac20c564da599..053bcc3618bac 100644 --- a/impeller/tessellator/tessellator.h +++ b/impeller/tessellator/tessellator.h @@ -18,6 +18,9 @@ namespace impeller { +/// The size of the point arena buffer stored on the tessellator. +static constexpr size_t kPointArenaSize = 4096u; + //------------------------------------------------------------------------------ /// @brief A utility that generates triangles of the specified fill type /// given a polyline. This happens on the CPU. @@ -284,7 +287,8 @@ class Tessellator { const Rect& bounds, const Size& radii); - std::vector& GetStrokePointCache() { return stroke_points_; } + /// Retrieve a pre-allocated arena of kPointArenaSize points. + std::vector& GetStrokePointCache(); protected: /// Used for polyline generation. From b03c4e514356d19ffda10deb4b97f89061a5a40b Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Tue, 5 Nov 2024 18:56:00 -0800 Subject: [PATCH 6/7] fix round cap generation. --- impeller/entity/geometry/stroke_path_geometry.cc | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/impeller/entity/geometry/stroke_path_geometry.cc b/impeller/entity/geometry/stroke_path_geometry.cc index 187d689beb191..2b988f133f1a4 100644 --- a/impeller/entity/geometry/stroke_path_geometry.cc +++ b/impeller/entity/geometry/stroke_path_geometry.cc @@ -358,7 +358,12 @@ void CreateRoundCap(PositionWriter& vtx_builder, vtx = position + (-point).Reflect(forward_normal); vtx_builder.AppendVertex(vtx); } - vtx_builder.AppendVertex(arc.p2); + + Point point = arc.p2; + vtx = position + point; + vtx_builder.AppendVertex(position + point); + vtx = position + (-point).Reflect(forward_normal); + vtx_builder.AppendVertex(vtx); } void CreateSquareCap(PositionWriter& vtx_builder, @@ -604,6 +609,7 @@ GeometryResult StrokePathGeometry::GetPositionBuffer( .transform = entity.GetShaderTransform(pass), .mode = GeometryResult::Mode::kPreventOverdraw}; } + FML_LOG(ERROR) << "Oversized"; const std::vector& oversized_data = position_writer.GetOveriszedBuffer(); BufferView buffer_view = host_buffer.Emplace( From dd2bf075ade85d26804a4645b9641aa016943569 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Wed, 6 Nov 2024 12:36:44 -0800 Subject: [PATCH 7/7] aaron review. --- impeller/entity/entity_unittests.cc | 39 ++++++++++++++----- .../entity/geometry/stroke_path_geometry.cc | 38 ++++++++++-------- 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index a10db7579e20b..1b1a9a903913c 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -2372,7 +2372,7 @@ TEST_P(EntityTest, GiantStrokePathAllocation) { Path path = builder.TakePath(); auto geom = Geometry::MakeStrokePath(path, /*stroke_width=*/10); - ContentContext content_context(GetContext(), nullptr); + ContentContext content_context(GetContext(), /*typographer_context=*/nullptr); Entity entity; auto cmd_buffer = content_context.GetContext()->CreateCommandBuffer(); @@ -2380,8 +2380,8 @@ TEST_P(EntityTest, GiantStrokePathAllocation) { RenderTargetAllocator allocator( content_context.GetContext()->GetResourceAllocator()); - auto render_target = - allocator.CreateOffscreen(*content_context.GetContext(), {10, 10}, 1); + auto render_target = allocator.CreateOffscreen( + *content_context.GetContext(), /*size=*/{10, 10}, /*mip_count=*/1); auto pass = cmd_buffer->CreateRenderPass(render_target); GeometryResult result = @@ -2395,12 +2395,33 @@ TEST_P(EntityTest, GiantStrokePathAllocation) { (result.vertex_buffer.vertex_buffer.GetBuffer()->OnGetContents() + result.vertex_buffer.vertex_buffer.GetRange().offset)); - EXPECT_NE(*(written_data + kPointArenaSize - 2), Point(0, 0)); - EXPECT_NE(*(written_data + kPointArenaSize - 1), Point(0, 0)); - EXPECT_NE(*(written_data + kPointArenaSize), Point(0, 0)); - EXPECT_NE(*(written_data + kPointArenaSize + 1), Point(0, 0)); - EXPECT_NE(*(written_data + kPointArenaSize + 2), Point(0, 0)); - EXPECT_NE(*(written_data + kPointArenaSize + 3), Point(0, 0)); + std::vector expected = { + Point(1019.46, 1026.54), // + Point(1026.54, 1019.46), // + Point(1020.45, 1027.54), // + Point(1027.54, 1020.46), // + Point(1020.46, 1027.53) // + }; + + Point point = written_data[kPointArenaSize - 2]; + EXPECT_NEAR(point.x, expected[0].x, 0.1); + EXPECT_NEAR(point.y, expected[0].y, 0.1); + + point = written_data[kPointArenaSize - 1]; + EXPECT_NEAR(point.x, expected[1].x, 0.1); + EXPECT_NEAR(point.y, expected[1].y, 0.1); + + point = written_data[kPointArenaSize]; + EXPECT_NEAR(point.x, expected[2].x, 0.1); + EXPECT_NEAR(point.y, expected[2].y, 0.1); + + point = written_data[kPointArenaSize + 1]; + EXPECT_NEAR(point.x, expected[3].x, 0.1); + EXPECT_NEAR(point.y, expected[3].y, 0.1); + + point = written_data[kPointArenaSize + 2]; + EXPECT_NEAR(point.x, expected[4].x, 0.1); + EXPECT_NEAR(point.y, expected[4].y, 0.1); } } // namespace testing diff --git a/impeller/entity/geometry/stroke_path_geometry.cc b/impeller/entity/geometry/stroke_path_geometry.cc index 2b988f133f1a4..ec6178b6036ff 100644 --- a/impeller/entity/geometry/stroke_path_geometry.cc +++ b/impeller/entity/geometry/stroke_path_geometry.cc @@ -6,6 +6,7 @@ #include "impeller/core/buffer_view.h" #include "impeller/core/formats.h" +#include "impeller/core/host_buffer.h" #include "impeller/entity/geometry/geometry.h" #include "impeller/geometry/constants.h" #include "impeller/geometry/path_builder.h" @@ -20,7 +21,9 @@ namespace { class PositionWriter { public: explicit PositionWriter(std::vector& points) - : points_(points), oversized_(0) {} + : points_(points), oversized_() { + FML_DCHECK(points_.size() == kPointArenaSize); + } void AppendVertex(const Point& point) { if (offset_ >= kPointArenaSize) { @@ -30,7 +33,11 @@ class PositionWriter { } } - size_t GetUsedSize() const { return offset_; } + /// @brief Return the number of points used in the arena, followed by + /// the number of points allocated in the overized buffer. + std::pair GetUsedSize() const { + return std::make_pair(offset_, oversized_.size()); + } bool HasOversizedBuffer() const { return !oversized_.empty(); } @@ -594,38 +601,38 @@ GeometryResult StrokePathGeometry::GetPositionBuffer( GetJoinProc(stroke_join_), GetCapProc(stroke_cap_), scale); + const auto [arena_length, oversized_length] = position_writer.GetUsedSize(); if (!position_writer.HasOversizedBuffer()) { BufferView buffer_view = host_buffer.Emplace( renderer.GetTessellator().GetStrokePointCache().data(), - position_writer.GetUsedSize() * sizeof(Point), alignof(Point)); + arena_length * sizeof(Point), alignof(Point)); return GeometryResult{.type = PrimitiveType::kTriangleStrip, .vertex_buffer = { .vertex_buffer = buffer_view, - .vertex_count = position_writer.GetUsedSize(), + .vertex_count = arena_length, .index_type = IndexType::kNone, }, .transform = entity.GetShaderTransform(pass), .mode = GeometryResult::Mode::kPreventOverdraw}; } - FML_LOG(ERROR) << "Oversized"; const std::vector& oversized_data = position_writer.GetOveriszedBuffer(); BufferView buffer_view = host_buffer.Emplace( - nullptr, - (position_writer.GetUsedSize() + oversized_data.size()) * sizeof(Point), - alignof(Point)); + /*buffer=*/nullptr, // + (arena_length + oversized_length) * sizeof(Point), // + alignof(Point) // + ); memcpy(buffer_view.GetBuffer()->OnGetContents() + buffer_view.GetRange().offset, // renderer.GetTessellator().GetStrokePointCache().data(), // - position_writer.GetUsedSize() * sizeof(Point) // + arena_length * sizeof(Point) // ); - memcpy( - buffer_view.GetBuffer()->OnGetContents() + buffer_view.GetRange().offset + - position_writer.GetUsedSize() * sizeof(Point), // - oversized_data.data(), // - oversized_data.size() * sizeof(Point) // + memcpy(buffer_view.GetBuffer()->OnGetContents() + + buffer_view.GetRange().offset + arena_length * sizeof(Point), // + oversized_data.data(), // + oversized_data.size() * sizeof(Point) // ); buffer_view.GetBuffer()->Flush(buffer_view.GetRange()); @@ -633,8 +640,7 @@ GeometryResult StrokePathGeometry::GetPositionBuffer( .vertex_buffer = { .vertex_buffer = buffer_view, - .vertex_count = position_writer.GetUsedSize() + - oversized_data.size(), + .vertex_count = arena_length + oversized_length, .index_type = IndexType::kNone, }, .transform = entity.GetShaderTransform(pass),