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

[Impeller] store all path point data in single buffer. #47896

Merged
merged 11 commits into from
Nov 16, 2023
Merged

[Impeller] store all path point data in single buffer. #47896

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
f485ad2
[Impeller] just crazy enough to work.
jonahwilliams Nov 10, 2023
25e1900
++
jonahwilliams Nov 10, 2023
9085c49
++
jonahwilliams Nov 10, 2023
70cc0c3
++
jonahwilliams Nov 10, 2023
44c3f3b
++
jonahwilliams Nov 10, 2023
ec7478d
Merge branch 'main' into combine_components
jonahwilliams Nov 10, 2023
f525594
Merge branch 'main' of github.com:flutter/engine into combine_components
jonahwilliams Nov 11, 2023
7600750
Merge branch 'combine_components' of github.com:jonahwilliams/engine …
jonahwilliams Nov 11, 2023
e1c9f32
Merge branch 'main' of github.com:flutter/engine into combine_components
jonahwilliams Nov 16, 2023
7caf0c4
matanl review.
jonahwilliams Nov 16, 2023
fedb8a2
++
jonahwilliams Nov 16, 2023
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
1 change: 1 addition & 0 deletions impeller/display_list/skia_conversions.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ Path ToPath(const SkPath& path, Point shift) {

PathBuilder builder;
PathData data;
builder.Reserve(path.countPoints() + 16, path.countVerbs() + 16);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since our storage more closely aligns with the SkPath, we can guestimate more easily...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we're assuming that stays true.

I'd probably leave a 1-line explaining how you came to these numbers for the next shmuck.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a line here

auto verb = SkPath::Verb::kDone_Verb;
do {
verb = iterator.next(data.points);
Expand Down
49 changes: 49 additions & 0 deletions impeller/geometry/geometry_unittests.cc
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's time to bite the bullet and create a new file (path_unittests.cc)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to path_unittests.cc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This came up in another review I was on too. In impeller there is some non-standard organization for tests. It makes a big difference for reviewing if when a file is changed the associated test file is changed too. Thanks for cleaning this up.

Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,55 @@ TEST(GeometryTest, SimplePath) {
});
}

TEST(GeometryTest, PathHorizontalLine) {
PathBuilder builder;
auto path = builder.HorizontalLineTo(10).TakePath();

LinearPathComponent linear;
path.GetLinearComponentAtIndex(1, linear);

EXPECT_EQ(linear.p1, Point(0, 0));
EXPECT_EQ(linear.p2, Point(10, 0));
}

TEST(GeometryTest, PathVerticalLine) {
PathBuilder builder;
auto path = builder.VerticalLineTo(10).TakePath();

LinearPathComponent linear;
path.GetLinearComponentAtIndex(1, linear);

EXPECT_EQ(linear.p1, Point(0, 0));
EXPECT_EQ(linear.p2, Point(0, 10));
}

TEST(GeometryTest, QuadradicPath) {
PathBuilder builder;
auto path = builder.QuadraticCurveTo(Point(10, 10), Point(20, 20)).TakePath();

QuadraticPathComponent quad;
path.GetQuadraticComponentAtIndex(1, quad);

EXPECT_EQ(quad.p1, Point(0, 0));
EXPECT_EQ(quad.cp, Point(10, 10));
EXPECT_EQ(quad.p2, Point(20, 20));
}

TEST(GeometryTest, CubicPath) {
PathBuilder builder;
auto path =
builder.CubicCurveTo(Point(10, 10), Point(-10, -10), Point(20, 20))
.TakePath();

CubicPathComponent cubic;
path.GetCubicComponentAtIndex(1, cubic);

EXPECT_EQ(cubic.p1, Point(0, 0));
EXPECT_EQ(cubic.cp1, Point(10, 10));
EXPECT_EQ(cubic.cp2, Point(-10, -10));
EXPECT_EQ(cubic.p2, Point(20, 20));
}

TEST(GeometryTest, BoundingBoxCubic) {
PathBuilder builder;
auto path =
Expand Down
173 changes: 102 additions & 71 deletions impeller/geometry/path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,19 +32,20 @@ std::tuple<size_t, size_t> Path::Polyline::GetContourPointBounds(
}

size_t Path::GetComponentCount(std::optional<ComponentType> type) const {
if (type.has_value()) {
switch (type.value()) {
case ComponentType::kLinear:
return linears_.size();
case ComponentType::kQuadratic:
return quads_.size();
case ComponentType::kCubic:
return cubics_.size();
case ComponentType::kContour:
return contours_.size();
if (!type.has_value()) {
return components_.size();
}
auto type_value = type.value();
if (type_value == ComponentType::kContour) {
return contours_.size();
}
size_t count = 0u;
for (const auto& component : components_) {
if (component.type == type_value) {
count++;
}
}
return components_.size();
return count;
}

void Path::SetFillType(FillType fill) {
Expand All @@ -64,51 +65,47 @@ void Path::SetConvexity(Convexity value) {
}

void Path::Shift(Point shift) {
size_t currentIndex = 0;
for (const auto& component : components_) {
switch (component.type) {
case ComponentType::kLinear:
linears_[component.index].p1 += shift;
linears_[component.index].p2 += shift;
break;
case ComponentType::kQuadratic:
quads_[component.index].cp += shift;
quads_[component.index].p1 += shift;
quads_[component.index].p2 += shift;
break;
case ComponentType::kCubic:
cubics_[component.index].cp1 += shift;
cubics_[component.index].cp2 += shift;
cubics_[component.index].p1 += shift;
cubics_[component.index].p2 += shift;
break;
case ComponentType::kContour:
contours_[component.index].destination += shift;
break;
}
currentIndex++;
for (auto i = 0u; i < points_.size(); i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI this is where having a linear algrebra library would be nice. This sort of operation would be 10x faster if it was vectorized.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't really a bottleneck though!

points_[i] += shift;
}
for (auto& contour : contours_) {
contour.destination += shift;
}
}

Path& Path::AddLinearComponent(Point p1, Point p2) {
linears_.emplace_back(p1, p2);
components_.emplace_back(ComponentType::kLinear, linears_.size() - 1);
Path& Path::AddLinearComponent(const Point& p1, const Point& p2) {
auto index = points_.size();
points_.emplace_back(p1);
points_.emplace_back(p2);
components_.emplace_back(ComponentType::kLinear, index);
return *this;
}

Path& Path::AddQuadraticComponent(Point p1, Point cp, Point p2) {
quads_.emplace_back(p1, cp, p2);
components_.emplace_back(ComponentType::kQuadratic, quads_.size() - 1);
Path& Path::AddQuadraticComponent(const Point& p1,
const Point& cp,
const Point& p2) {
auto index = points_.size();
points_.emplace_back(p1);
points_.emplace_back(cp);
points_.emplace_back(p2);
components_.emplace_back(ComponentType::kQuadratic, index);
return *this;
}

Path& Path::AddCubicComponent(Point p1, Point cp1, Point cp2, Point p2) {
cubics_.emplace_back(p1, cp1, cp2, p2);
components_.emplace_back(ComponentType::kCubic, cubics_.size() - 1);
Path& Path::AddCubicComponent(const Point& p1,
const Point& cp1,
const Point& cp2,
const Point& p2) {
auto index = points_.size();
points_.emplace_back(p1);
points_.emplace_back(cp1);
points_.emplace_back(cp2);
points_.emplace_back(p2);
components_.emplace_back(ComponentType::kCubic, index);
return *this;
}

Path& Path::AddContourComponent(Point destination, bool is_closed) {
Path& Path::AddContourComponent(const Point& destination, bool is_closed) {
if (components_.size() > 0 &&
components_.back().type == ComponentType::kContour) {
// Never insert contiguous contours.
Expand All @@ -134,17 +131,26 @@ void Path::EnumerateComponents(
switch (component.type) {
case ComponentType::kLinear:
if (linear_applier) {
linear_applier(currentIndex, linears_[component.index]);
linear_applier(currentIndex,
LinearPathComponent(points_[component.index],
points_[component.index + 1]));
}
break;
case ComponentType::kQuadratic:
if (quad_applier) {
quad_applier(currentIndex, quads_[component.index]);
quad_applier(currentIndex,
QuadraticPathComponent(points_[component.index],
points_[component.index + 1],
points_[component.index + 2]));
}
break;
case ComponentType::kCubic:
if (cubic_applier) {
cubic_applier(currentIndex, cubics_[component.index]);
cubic_applier(currentIndex,
CubicPathComponent(points_[component.index],
points_[component.index + 1],
points_[component.index + 2],
points_[component.index + 3]));
}
break;
case ComponentType::kContour:
Expand All @@ -167,7 +173,8 @@ bool Path::GetLinearComponentAtIndex(size_t index,
return false;
}

linear = linears_[components_[index].index];
auto point_index = components_[index].index;
linear = LinearPathComponent(points_[point_index], points_[point_index + 1]);
return true;
}

Expand All @@ -182,7 +189,9 @@ bool Path::GetQuadraticComponentAtIndex(
return false;
}

quadratic = quads_[components_[index].index];
auto point_index = components_[index].index;
quadratic = QuadraticPathComponent(
points_[point_index], points_[point_index + 1], points_[point_index + 2]);
return true;
}

Expand All @@ -196,7 +205,10 @@ bool Path::GetCubicComponentAtIndex(size_t index,
return false;
}

cubic = cubics_[components_[index].index];
auto point_index = components_[index].index;
cubic =
CubicPathComponent(points_[point_index], points_[point_index + 1],
points_[point_index + 2], points_[point_index + 3]);
return true;
}

Expand Down Expand Up @@ -246,11 +258,14 @@ Path::Polyline Path::CreatePolyline(
const auto& component = components_[component_i];
switch (component.type) {
case ComponentType::kLinear:
return &linears_[component.index];
return reinterpret_cast<const LinearPathComponent*>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you could use a union here instead of having these casts.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A union would require us to align storage to the largest component size, effectively padding out linear components 4x. The cast approach means that we can store points inline/continuously, though we have to rely on the secondary vector of verbs to determine the correct offsets. This is approach is closer to what Skia uses and I think we should too (partially because it makes point/verb reserve work nicely), and partially because its closer to the format we need for compute anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, I think it would? Is that right or wrong. 😆

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, could be. I'm not familiar enough with the code to know if there are less items in the vector to account for the different type. If the counts are the same then you're right.

You could make your own cast with template specialization

case ComponentType::kLinear:
  return point_cast<ComponentType::kLinear>(&points_[component.index]);

That's a wee bit cleaner. I don't have another easy solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to punt on this for now.

&points_[component.index]);
case ComponentType::kQuadratic:
return &quads_[component.index];
return reinterpret_cast<const QuadraticPathComponent*>(
&points_[component.index]);
case ComponentType::kCubic:
return &cubics_[component.index];
return reinterpret_cast<const CubicPathComponent*>(
&points_[component.index]);
case ComponentType::kContour:
return std::monostate{};
}
Expand Down Expand Up @@ -319,23 +334,27 @@ Path::Polyline Path::CreatePolyline(
.component_start_index = polyline.points->size() - 1,
.is_curve = false,
});
linears_[component.index].AppendPolylinePoints(*polyline.points);
reinterpret_cast<const LinearPathComponent*>(&points_[component.index])
->AppendPolylinePoints(*polyline.points);
previous_path_component_index = component_i;
break;
case ComponentType::kQuadratic:
components.push_back({
.component_start_index = polyline.points->size() - 1,
.is_curve = true,
});
quads_[component.index].AppendPolylinePoints(scale, *polyline.points);
reinterpret_cast<const QuadraticPathComponent*>(
&points_[component.index])
->AppendPolylinePoints(scale, *polyline.points);
previous_path_component_index = component_i;
break;
case ComponentType::kCubic:
components.push_back({
.component_start_index = polyline.points->size() - 1,
.is_curve = true,
});
cubics_[component.index].AppendPolylinePoints(scale, *polyline.points);
reinterpret_cast<const CubicPathComponent*>(&points_[component.index])
->AppendPolylinePoints(scale, *polyline.points);
previous_path_component_index = component_i;
break;
case ComponentType::kContour:
Expand Down Expand Up @@ -387,7 +406,7 @@ std::optional<Rect> Path::GetTransformedBoundingBox(
}

std::optional<std::pair<Point, Point>> Path::GetMinMaxCoveragePoints() const {
if (linears_.empty() && quads_.empty() && cubics_.empty()) {
if (points_.empty()) {
return std::nullopt;
}

Expand All @@ -407,20 +426,32 @@ std::optional<std::pair<Point, Point>> Path::GetMinMaxCoveragePoints() const {
}
};

for (const auto& linear : linears_) {
clamp(linear.p1);
clamp(linear.p2);
}

for (const auto& quad : quads_) {
for (const Point& point : quad.Extrema()) {
clamp(point);
}
}

for (const auto& cubic : cubics_) {
for (const Point& point : cubic.Extrema()) {
clamp(point);
for (const auto& component : components_) {
switch (component.type) {
case ComponentType::kLinear: {
auto* linear = reinterpret_cast<const LinearPathComponent*>(
&points_[component.index]);
clamp(linear->p1);
clamp(linear->p2);
break;
}
case ComponentType::kQuadratic:
for (const auto& extrema :
reinterpret_cast<const QuadraticPathComponent*>(
&points_[component.index])
->Extrema()) {
clamp(extrema);
}
break;
case ComponentType::kCubic:
for (const auto& extrema : reinterpret_cast<const CubicPathComponent*>(
&points_[component.index])
->Extrema()) {
clamp(extrema);
}
break;
case ComponentType::kContour:
break;
}
}

Expand Down
17 changes: 10 additions & 7 deletions impeller/geometry/path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,13 +184,18 @@ class Path {

void SetBounds(Rect rect);

Path& AddLinearComponent(Point p1, Point p2);
Path& AddLinearComponent(const Point& p1, const Point& p2);

Path& AddQuadraticComponent(Point p1, Point cp, Point p2);
Path& AddQuadraticComponent(const Point& p1,
const Point& cp,
const Point& p2);

Path& AddCubicComponent(Point p1, Point cp1, Point cp2, Point p2);
Path& AddCubicComponent(const Point& p1,
const Point& cp1,
const Point& cp2,
const Point& p2);

Path& AddContourComponent(Point destination, bool is_closed = false);
Path& AddContourComponent(const Point& destination, bool is_closed = false);

/// @brief Called by `PathBuilder` to compute the bounds for certain paths.
///
Expand All @@ -215,9 +220,7 @@ class Path {
FillType fill_ = FillType::kNonZero;
Convexity convexity_ = Convexity::kUnknown;
std::vector<ComponentIndexPair> components_;
std::vector<LinearPathComponent> linears_;
std::vector<QuadraticPathComponent> quads_;
std::vector<CubicPathComponent> cubics_;
std::vector<Point> points_;
std::vector<ContourComponent> contours_;

std::optional<Rect> computed_bounds_;
Expand Down
5 changes: 5 additions & 0 deletions impeller/geometry/path_builder.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ Path PathBuilder::TakePath(FillType fill) {
return path;
}

void PathBuilder::Reserve(size_t point_size, size_t verb_size) {
prototype_.points_.reserve(point_size);
prototype_.points_.reserve(verb_size);
}

PathBuilder& PathBuilder::MoveTo(Point point, bool relative) {
current_ = relative ? current_ + point : point;
subpath_start_ = current_;
Expand Down
Loading