[Impeller] dont unnecessarily copy point data out of display list. (#56492)

Display list now stores impeller::Point objects, so have the PointFieldGeometry reference these points directly. As the dispatching/recording is immediate, there is no need to copy to secondary storage.

Author: jonahwilliams

impeller/display_list/canvas.cc

@@ -638,7 +638,8 @@ void Canvas::ClipGeometry(const Geometry& geometry,
       clip_depth);
 }
 
-void Canvas::DrawPoints(std::vector<Point> points,
+void Canvas::DrawPoints(const Point points[],
+                        uint32_t count,
                         Scalar radius,
                         const Paint& paint,
                         PointStyle point_style) {
@@ -650,7 +651,7 @@ void Canvas::DrawPoints(std::vector<Point> 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);
 }

impeller/display_list/canvas.h

@@ -200,7 +200,8 @@ class Canvas {
 
   void DrawCircle(const Point& center, Scalar radius, const Paint& paint);
 
-  void DrawPoints(std::vector<Point> points,
+  void DrawPoints(const Point points[],
+                  uint32_t count,
                   Scalar radius,
                   const Paint& paint,
                   PointStyle point_style);

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) {

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<Point> 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));
 }
 

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,12 +62,6 @@ std::unique_ptr<Geometry> Geometry::MakeFillPath(
   return std::make_unique<FillPathGeometry>(path, inner_rect);
 }
 
-std::unique_ptr<Geometry> Geometry::MakePointField(std::vector<Point> points,
-                                                   Scalar radius,
-                                                   bool round) {
-  return std::make_unique<PointFieldGeometry>(std::move(points), radius, round);
-}
-
 std::unique_ptr<Geometry> Geometry::MakeStrokePath(const Path& path,
                                                    Scalar stroke_width,
                                                    Scalar miter_limit,

impeller/entity/geometry/geometry.h

@@ -83,10 +83,6 @@ class Geometry {
   static std::unique_ptr<Geometry> MakeRoundRect(const Rect& rect,
                                                  const Size& radii);
 
-  static std::unique_ptr<Geometry> MakePointField(std::vector<Point> points,
-                                                  Scalar radius,
-                                                  bool round);
-
   virtual GeometryResult GetPositionBuffer(const ContentContext& renderer,
                                            const Entity& entity,
                                            RenderPass& pass) const = 0;

impeller/entity/geometry/point_field_geometry.cc

@@ -12,18 +12,22 @@
 
 namespace impeller {
 
-PointFieldGeometry::PointFieldGeometry(std::vector<Point> 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;
 
 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<Point*>(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<Point*>(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,22 +133,21 @@ GeometryResult PointFieldGeometry::GetPositionBuffer(
 // |Geometry|
 std::optional<Rect> 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();
-    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);
     }
-    auto coverage = Rect::MakeLTRB(left - radius_, top - radius_,
+    Rect coverage = Rect::MakeLTRB(left - radius_, top - radius_,
                                    right + radius_, bottom + radius_);
     return coverage.TransformBounds(transform);
   }

impeller/entity/geometry/point_field_geometry.h

@@ -10,24 +10,31 @@
 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<Point> points, Scalar radius, bool round);
+  PointFieldGeometry(const Point* points,
+                     size_t point_count,
+                     Scalar radius,
+                     bool round);
 
   ~PointFieldGeometry() override;
 
+  // |Geometry|
+  std::optional<Rect> GetCoverage(const Matrix& transform) const override;
+
  private:
   // |Geometry|
   GeometryResult GetPositionBuffer(const ContentContext& renderer,
                                    const Entity& entity,
                                    RenderPass& pass) const override;
 
-  // |Geometry|
-  std::optional<Rect> GetCoverage(const Matrix& transform) const override;
-
-  std::vector<Point> points_;
+  size_t point_count_;
   Scalar radius_;
   bool round_;
+  const Point* points_;
 
   PointFieldGeometry(const PointFieldGeometry&) = delete;