From 2bc22270d72296943f5bfe811148f76e03cf9586 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 10 Nov 2024 12:46:20 -0800 Subject: [PATCH 1/4] [Impeller] dont unnecessarily copy point data out of display list. --- impeller/display_list/canvas.cc | 5 ++-- impeller/display_list/canvas.h | 3 ++- impeller/display_list/dl_dispatcher.cc | 11 ++++---- impeller/entity/geometry/geometry.cc | 3 ++- .../entity/geometry/point_field_geometry.cc | 26 +++++++++++-------- .../entity/geometry/point_field_geometry.h | 11 ++++++-- 6 files changed, 36 insertions(+), 23 deletions(-) diff --git a/impeller/display_list/canvas.cc b/impeller/display_list/canvas.cc index dfca9f39bdd17..ba53272912ef4 100644 --- a/impeller/display_list/canvas.cc +++ b/impeller/display_list/canvas.cc @@ -635,7 +635,8 @@ void Canvas::ClipGeometry(const Geometry& geometry, clip_depth); } -void Canvas::DrawPoints(std::vector points, +void Canvas::DrawPoints(const Point points[], + uint32_t count, Scalar radius, const Paint& paint, PointStyle point_style) { @@ -647,7 +648,7 @@ void Canvas::DrawPoints(std::vector points, entity.SetTransform(GetCurrentTransform()); entity.SetBlendMode(paint.blend_mode); - PointFieldGeometry geom(std::move(points), radius, + PointFieldGeometry geom(points, count, radius, /*round=*/point_style == PointStyle::kRound); AddRenderEntityWithFiltersToCurrentPass(entity, &geom, paint); } diff --git a/impeller/display_list/canvas.h b/impeller/display_list/canvas.h index 59887dd635e45..b3d336cd49f43 100644 --- a/impeller/display_list/canvas.h +++ b/impeller/display_list/canvas.h @@ -197,7 +197,8 @@ class Canvas { void DrawCircle(const Point& center, Scalar radius, const Paint& paint); - void DrawPoints(std::vector points, + void DrawPoints(const Point points[], + uint32_t count, Scalar radius, const Paint& paint, PointStyle point_style); diff --git a/impeller/display_list/dl_dispatcher.cc b/impeller/display_list/dl_dispatcher.cc index b754c721f804c..8e035961c2727 100644 --- a/impeller/display_list/dl_dispatcher.cc +++ b/impeller/display_list/dl_dispatcher.cc @@ -27,7 +27,6 @@ #include "impeller/entity/entity.h" #include "impeller/entity/geometry/ellipse_geometry.h" #include "impeller/entity/geometry/fill_path_geometry.h" -#include "impeller/entity/geometry/geometry.h" #include "impeller/entity/geometry/rect_geometry.h" #include "impeller/entity/geometry/round_rect_geometry.h" #include "impeller/geometry/color.h" @@ -663,14 +662,14 @@ void DlDispatcherBase::drawPoints(PointMode mode, switch (mode) { case flutter::DlCanvas::PointMode::kPoints: { // Cap::kButt is also treated as a square. - auto point_style = paint.stroke_cap == Cap::kRound ? PointStyle::kRound - : PointStyle::kSquare; - auto radius = paint.stroke_width; + PointStyle point_style = paint.stroke_cap == Cap::kRound + ? PointStyle::kRound + : PointStyle::kSquare; + Scalar radius = paint.stroke_width; if (radius > 0) { radius /= 2.0; } - GetCanvas().DrawPoints(skia_conversions::ToPoints(points, count), radius, - paint, point_style); + GetCanvas().DrawPoints(points, count, radius, paint, point_style); } break; case flutter::DlCanvas::PointMode::kLines: for (uint32_t i = 1; i < count; i += 2) { diff --git a/impeller/entity/geometry/geometry.cc b/impeller/entity/geometry/geometry.cc index 3e9df2cf2ad00..9656da1d29dfb 100644 --- a/impeller/entity/geometry/geometry.cc +++ b/impeller/entity/geometry/geometry.cc @@ -66,7 +66,8 @@ std::unique_ptr Geometry::MakeFillPath( std::unique_ptr Geometry::MakePointField(std::vector points, Scalar radius, bool round) { - return std::make_unique(std::move(points), radius, round); + return std::make_unique(points.data(), points.size(), + radius, round); } std::unique_ptr Geometry::MakeStrokePath(const Path& path, diff --git a/impeller/entity/geometry/point_field_geometry.cc b/impeller/entity/geometry/point_field_geometry.cc index 0548d61dbb37d..626b2469e6d1a 100644 --- a/impeller/entity/geometry/point_field_geometry.cc +++ b/impeller/entity/geometry/point_field_geometry.cc @@ -12,10 +12,14 @@ namespace impeller { -PointFieldGeometry::PointFieldGeometry(std::vector points, +PointFieldGeometry::PointFieldGeometry(const Point* points, + size_t point_count, Scalar radius, bool round) - : points_(std::move(points)), radius_(radius), round_(round) {} + : point_count_(point_count), + radius_(radius), + round_(round), + points_(points) {} PointFieldGeometry::~PointFieldGeometry() = default; @@ -23,7 +27,7 @@ GeometryResult PointFieldGeometry::GetPositionBuffer( const ContentContext& renderer, const Entity& entity, RenderPass& pass) const { - if (radius_ < 0.0 || points_.empty()) { + if (radius_ < 0.0 || point_count_ == 0) { return {}; } const Matrix& transform = entity.GetTransform(); @@ -52,7 +56,7 @@ GeometryResult PointFieldGeometry::GetPositionBuffer( }); FML_DCHECK(circle_vertices.size() == generator.GetVertexCount()); - vertex_count = (circle_vertices.size() + 2) * points_.size() - 2; + vertex_count = (circle_vertices.size() + 2) * point_count_ - 2; buffer_view = host_buffer.Emplace( vertex_count * sizeof(Point), alignof(Point), [&](uint8_t* data) { Point* output = reinterpret_cast(data); @@ -66,7 +70,7 @@ GeometryResult PointFieldGeometry::GetPositionBuffer( // the strip. This could be optimized out if we switched to using // primitive restart. Point last_point = circle_vertices.back() + center; - for (size_t i = 1; i < points_.size(); i++) { + for (size_t i = 1; i < point_count_; i++) { Point center = points_[i]; output[offset++] = last_point; output[offset++] = Point(center + circle_vertices[0]); @@ -77,7 +81,7 @@ GeometryResult PointFieldGeometry::GetPositionBuffer( } }); } else { - vertex_count = 6 * points_.size() - 2; + vertex_count = 6 * point_count_ - 2; buffer_view = host_buffer.Emplace( vertex_count * sizeof(Point), alignof(Point), [&](uint8_t* data) { Point* output = reinterpret_cast(data); @@ -97,7 +101,7 @@ GeometryResult PointFieldGeometry::GetPositionBuffer( // For all subequent points, insert a degenerate triangle to break // the strip. This could be optimized out if we switched to using // primitive restart. - for (size_t i = 1; i < points_.size(); i++) { + for (size_t i = 1; i < point_count_; i++) { Point point = points_[i]; Point first = Point(point.x - radius, point.y - radius); @@ -129,11 +133,11 @@ GeometryResult PointFieldGeometry::GetPositionBuffer( // |Geometry| std::optional PointFieldGeometry::GetCoverage( const Matrix& transform) const { - if (points_.size() > 0) { + if (point_count_ > 0) { // Doesn't use MakePointBounds as this isn't resilient to points that // all lie along the same axis. - auto first = points_.begin(); - auto last = points_.end(); + const Point* first = points_; + const Point* last = points_ + point_count_; auto left = first->x; auto top = first->y; auto right = first->x; @@ -144,7 +148,7 @@ std::optional PointFieldGeometry::GetCoverage( right = std::max(right, it->x); bottom = std::max(bottom, it->y); } - auto coverage = Rect::MakeLTRB(left - radius_, top - radius_, + Rect coverage = Rect::MakeLTRB(left - radius_, top - radius_, right + radius_, bottom + radius_); return coverage.TransformBounds(transform); } diff --git a/impeller/entity/geometry/point_field_geometry.h b/impeller/entity/geometry/point_field_geometry.h index cdbb19288671d..7d3eedf7504d2 100644 --- a/impeller/entity/geometry/point_field_geometry.h +++ b/impeller/entity/geometry/point_field_geometry.h @@ -10,9 +10,15 @@ namespace impeller { /// @brief A geometry class specialized for Canvas::DrawPoints. +/// +/// Does not hold ownership of the allocated point data, which is expected to be +/// maintained via the display list structure. class PointFieldGeometry final : public Geometry { public: - PointFieldGeometry(std::vector points, Scalar radius, bool round); + PointFieldGeometry(const Point* points, + size_t point_count, + Scalar radius, + bool round); ~PointFieldGeometry() override; @@ -25,9 +31,10 @@ class PointFieldGeometry final : public Geometry { // |Geometry| std::optional GetCoverage(const Matrix& transform) const override; - std::vector points_; + size_t point_count_; Scalar radius_; bool round_; + const Point* points_; PointFieldGeometry(const PointFieldGeometry&) = delete; From 82995cf30fb454b8cd44ec8c2f8f2c9b48f84c80 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 10 Nov 2024 14:59:44 -0800 Subject: [PATCH 2/4] Remove factory. --- impeller/entity/entity_unittests.cc | 7 ++++--- impeller/entity/geometry/geometry.cc | 8 ------- impeller/entity/geometry/geometry.h | 4 ---- .../entity/geometry/point_field_geometry.cc | 21 +++++++++---------- .../entity/geometry/point_field_geometry.h | 6 +++--- 5 files changed, 17 insertions(+), 29 deletions(-) diff --git a/impeller/entity/entity_unittests.cc b/impeller/entity/entity_unittests.cc index 1b1a9a903913c..cccc436bc7357 100644 --- a/impeller/entity/entity_unittests.cc +++ b/impeller/entity/entity_unittests.cc @@ -35,6 +35,7 @@ #include "impeller/entity/entity.h" #include "impeller/entity/entity_playground.h" #include "impeller/entity/geometry/geometry.h" +#include "impeller/entity/geometry/point_field_geometry.h" #include "impeller/entity/geometry/stroke_path_geometry.h" #include "impeller/entity/geometry/superellipse_geometry.h" #include "impeller/geometry/color.h" @@ -2095,9 +2096,9 @@ TEST_P(EntityTest, TiledTextureContentsIsOpaque) { TEST_P(EntityTest, PointFieldGeometryCoverage) { std::vector points = {{10, 20}, {100, 200}}; - auto geometry = Geometry::MakePointField(points, 5.0, false); - ASSERT_EQ(*geometry->GetCoverage(Matrix()), Rect::MakeLTRB(5, 15, 105, 205)); - ASSERT_EQ(*geometry->GetCoverage(Matrix::MakeTranslation({30, 0, 0})), + PointFieldGeometry geometry(points.data(), 2, 5.0, false); + ASSERT_EQ(geometry.GetCoverage(Matrix()), Rect::MakeLTRB(5, 15, 105, 205)); + ASSERT_EQ(geometry.GetCoverage(Matrix::MakeTranslation({30, 0, 0})), Rect::MakeLTRB(35, 15, 135, 205)); } diff --git a/impeller/entity/geometry/geometry.cc b/impeller/entity/geometry/geometry.cc index 9656da1d29dfb..0f3185fce940e 100644 --- a/impeller/entity/geometry/geometry.cc +++ b/impeller/entity/geometry/geometry.cc @@ -13,7 +13,6 @@ #include "impeller/entity/geometry/ellipse_geometry.h" #include "impeller/entity/geometry/fill_path_geometry.h" #include "impeller/entity/geometry/line_geometry.h" -#include "impeller/entity/geometry/point_field_geometry.h" #include "impeller/entity/geometry/rect_geometry.h" #include "impeller/entity/geometry/round_rect_geometry.h" #include "impeller/entity/geometry/stroke_path_geometry.h" @@ -63,13 +62,6 @@ std::unique_ptr Geometry::MakeFillPath( return std::make_unique(path, inner_rect); } -std::unique_ptr Geometry::MakePointField(std::vector points, - Scalar radius, - bool round) { - return std::make_unique(points.data(), points.size(), - radius, round); -} - std::unique_ptr Geometry::MakeStrokePath(const Path& path, Scalar stroke_width, Scalar miter_limit, diff --git a/impeller/entity/geometry/geometry.h b/impeller/entity/geometry/geometry.h index 3884551fe5fe5..d5c504d79e844 100644 --- a/impeller/entity/geometry/geometry.h +++ b/impeller/entity/geometry/geometry.h @@ -83,10 +83,6 @@ class Geometry { static std::unique_ptr MakeRoundRect(const Rect& rect, const Size& radii); - static std::unique_ptr MakePointField(std::vector points, - Scalar radius, - bool round); - virtual GeometryResult GetPositionBuffer(const ContentContext& renderer, const Entity& entity, RenderPass& pass) const = 0; diff --git a/impeller/entity/geometry/point_field_geometry.cc b/impeller/entity/geometry/point_field_geometry.cc index 626b2469e6d1a..2a8429a2d7ac5 100644 --- a/impeller/entity/geometry/point_field_geometry.cc +++ b/impeller/entity/geometry/point_field_geometry.cc @@ -136,17 +136,16 @@ std::optional PointFieldGeometry::GetCoverage( if (point_count_ > 0) { // Doesn't use MakePointBounds as this isn't resilient to points that // all lie along the same axis. - const Point* first = points_; - const Point* last = points_ + point_count_; - auto left = first->x; - auto top = first->y; - auto right = first->x; - auto bottom = first->y; - for (auto it = first + 1; it < last; ++it) { - left = std::min(left, it->x); - top = std::min(top, it->y); - right = std::max(right, it->x); - bottom = std::max(bottom, it->y); + Scalar left = points_[0].x; + Scalar top = points_[0].y; + Scalar right = points_[0].x; + Scalar bottom = points_[0].y; + for (auto i = 1u; i < point_count_; i++) { + const Point point = points_[i]; + left = std::min(left, point.x); + top = std::min(top, point.y); + right = std::max(right, point.x); + bottom = std::max(bottom, point.y); } Rect coverage = Rect::MakeLTRB(left - radius_, top - radius_, right + radius_, bottom + radius_); diff --git a/impeller/entity/geometry/point_field_geometry.h b/impeller/entity/geometry/point_field_geometry.h index 7d3eedf7504d2..8ab8e8b4ae0bb 100644 --- a/impeller/entity/geometry/point_field_geometry.h +++ b/impeller/entity/geometry/point_field_geometry.h @@ -22,15 +22,15 @@ class PointFieldGeometry final : public Geometry { ~PointFieldGeometry() override; + // |Geometry| + std::optional GetCoverage(const Matrix& transform) const override; + private: // |Geometry| GeometryResult GetPositionBuffer(const ContentContext& renderer, const Entity& entity, RenderPass& pass) const override; - // |Geometry| - std::optional GetCoverage(const Matrix& transform) const override; - size_t point_count_; Scalar radius_; bool round_; From 7a01644234cd3cf1e922bbae57e732a24887d2e7 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 10 Nov 2024 19:04:44 -0800 Subject: [PATCH 3/4] add txt. --- testing/impeller_golden_tests_output.txt | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index fddecde08074b..f7f9601873af6 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -575,6 +575,12 @@ impeller_Play_AiksTest_CoordinateConversionsAreCorrect_Vulkan.png impeller_Play_AiksTest_CoverageOriginShouldBeAccountedForInSubpasses_Metal.png impeller_Play_AiksTest_CoverageOriginShouldBeAccountedForInSubpasses_OpenGLES.png impeller_Play_AiksTest_CoverageOriginShouldBeAccountedForInSubpasses_Vulkan.png +impeller_Play_AiksTest_DepthValuesForLineMode_Metal.png +impeller_Play_AiksTest_DepthValuesForLineMode_OpenGLES.png +impeller_Play_AiksTest_DepthValuesForLineMode_Vulkan.png +impeller_Play_AiksTest_DepthValuesForPolygonMode_Metal.png +impeller_Play_AiksTest_DepthValuesForPolygonMode_OpenGLES.png +impeller_Play_AiksTest_DepthValuesForPolygonMode_Vulkan.png impeller_Play_AiksTest_DestructiveBlendColorFilterFloodsClip_Metal.png impeller_Play_AiksTest_DestructiveBlendColorFilterFloodsClip_OpenGLES.png impeller_Play_AiksTest_DestructiveBlendColorFilterFloodsClip_Vulkan.png From 95dfaa1ed5197e4fe6e3c9edcff166e27ad54e30 Mon Sep 17 00:00:00 2001 From: jonahwilliams Date: Sun, 10 Nov 2024 19:32:28 -0800 Subject: [PATCH 4/4] Revert "add txt." This reverts commit 7a01644234cd3cf1e922bbae57e732a24887d2e7. --- testing/impeller_golden_tests_output.txt | 6 ------ 1 file changed, 6 deletions(-) diff --git a/testing/impeller_golden_tests_output.txt b/testing/impeller_golden_tests_output.txt index f7f9601873af6..fddecde08074b 100644 --- a/testing/impeller_golden_tests_output.txt +++ b/testing/impeller_golden_tests_output.txt @@ -575,12 +575,6 @@ impeller_Play_AiksTest_CoordinateConversionsAreCorrect_Vulkan.png impeller_Play_AiksTest_CoverageOriginShouldBeAccountedForInSubpasses_Metal.png impeller_Play_AiksTest_CoverageOriginShouldBeAccountedForInSubpasses_OpenGLES.png impeller_Play_AiksTest_CoverageOriginShouldBeAccountedForInSubpasses_Vulkan.png -impeller_Play_AiksTest_DepthValuesForLineMode_Metal.png -impeller_Play_AiksTest_DepthValuesForLineMode_OpenGLES.png -impeller_Play_AiksTest_DepthValuesForLineMode_Vulkan.png -impeller_Play_AiksTest_DepthValuesForPolygonMode_Metal.png -impeller_Play_AiksTest_DepthValuesForPolygonMode_OpenGLES.png -impeller_Play_AiksTest_DepthValuesForPolygonMode_Vulkan.png impeller_Play_AiksTest_DestructiveBlendColorFilterFloodsClip_Metal.png impeller_Play_AiksTest_DestructiveBlendColorFilterFloodsClip_OpenGLES.png impeller_Play_AiksTest_DestructiveBlendColorFilterFloodsClip_Vulkan.png