Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

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

Merged
merged 4 commits into from
Nov 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions impeller/display_list/canvas.cc
Original file line number Diff line number Diff line change
Expand Up @@ -635,7 +635,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) {
Expand All @@ -647,7 +648,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);
}
Expand Down
3 changes: 2 additions & 1 deletion impeller/display_list/canvas.h
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,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);
Expand Down
11 changes: 5 additions & 6 deletions impeller/display_list/dl_dispatcher.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand Down
7 changes: 4 additions & 3 deletions impeller/entity/entity_unittests.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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));
}

Expand Down
7 changes: 0 additions & 7 deletions impeller/entity/geometry/geometry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 0 additions & 4 deletions impeller/entity/geometry/geometry.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
43 changes: 23 additions & 20 deletions impeller/entity/geometry/point_field_geometry.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
Expand All @@ -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]);
Expand All @@ -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);
Expand All @@ -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);

Expand Down Expand Up @@ -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);
}
Expand Down
17 changes: 12 additions & 5 deletions impeller/entity/geometry/point_field_geometry.h
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down