Skip to content

Commit 377050b

Browse files
author
Jonah Williams
authored
[Impeller] Remove impeller::Path copy constructor. (flutter#48616)
From looking at profiles, we're always copying paths at least once when recording commands. By deleting the copy constructor, I cna ensure that we're always either moving or explicitly cloning the Path object. Or, now that I fixed all the moves I could add the copy constructor back.
1 parent cb133d4 commit 377050b

23 files changed

Lines changed: 130 additions & 73 deletions

‎impeller/aiks/aiks_unittests.cc‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -80,8 +80,8 @@ TEST_P(AiksTest, RotateColorFilteredPath) {
8080
ColorFilter::MakeBlend(BlendMode::kSourceIn, Color::AliceBlue()),
8181
};
8282

83-
canvas.DrawPath(arrow_stem, paint);
84-
canvas.DrawPath(arrow_head, paint);
83+
canvas.DrawPath(std::move(arrow_stem), paint);
84+
canvas.DrawPath(std::move(arrow_head), paint);
8585
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
8686
}
8787

@@ -1266,7 +1266,7 @@ TEST_P(AiksTest, CanRenderRoundedRectWithNonUniformRadii) {
12661266
.AddRoundedRect(Rect::MakeXYWH(100, 100, 500, 500), radii)
12671267
.TakePath();
12681268

1269-
canvas.DrawPath(path, paint);
1269+
canvas.DrawPath(std::move(path), paint);
12701270

12711271
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
12721272
}
@@ -1323,7 +1323,7 @@ TEST_P(AiksTest, CanRenderDifferencePaths) {
13231323
canvas.DrawImage(
13241324
std::make_shared<Image>(CreateTextureForFixture("boston.jpg")), {10, 10},
13251325
Paint{});
1326-
canvas.DrawPath(path, paint);
1326+
canvas.DrawPath(std::move(path), paint);
13271327

13281328
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
13291329
}
@@ -1984,7 +1984,7 @@ TEST_P(AiksTest, SolidStrokesRenderCorrectly) {
19841984
paint.stroke_join = join;
19851985
for (auto cap : {Cap::kButt, Cap::kSquare, Cap::kRound}) {
19861986
paint.stroke_cap = cap;
1987-
canvas.DrawPath(path, paint);
1987+
canvas.DrawPath(path.Clone(), paint);
19881988
canvas.Translate({80, 0});
19891989
}
19901990
canvas.Translate({-240, 60});
@@ -2248,7 +2248,7 @@ TEST_P(AiksTest, GradientStrokesRenderCorrectly) {
22482248
paint.stroke_join = join;
22492249
for (auto cap : {Cap::kButt, Cap::kSquare, Cap::kRound}) {
22502250
paint.stroke_cap = cap;
2251-
canvas.DrawPath(path, paint);
2251+
canvas.DrawPath(path.Clone(), paint);
22522252
canvas.Translate({80, 0});
22532253
}
22542254
canvas.Translate({-240, 60});
@@ -2980,7 +2980,7 @@ TEST_P(AiksTest, ImageFilteredSaveLayerWithUnboundedContents) {
29802980
.TakePath();
29812981
Paint paint = p;
29822982
paint.style = Paint::Style::kStroke;
2983-
canvas.DrawPath(path, paint);
2983+
canvas.DrawPath(std::move(path), paint);
29842984
};
29852985
// Registration marks for the edge of the SaveLayer
29862986
DrawLine(Point(75, 100), Point(225, 100), {.color = Color::White()});

‎impeller/aiks/canvas.cc‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -171,12 +171,13 @@ void Canvas::RestoreToCount(size_t count) {
171171
}
172172
}
173173

174-
void Canvas::DrawPath(const Path& path, const Paint& paint) {
174+
void Canvas::DrawPath(Path path, const Paint& paint) {
175175
Entity entity;
176176
entity.SetTransform(GetCurrentTransform());
177177
entity.SetClipDepth(GetClipDepth());
178178
entity.SetBlendMode(paint.blend_mode);
179-
entity.SetContents(paint.WithFilters(paint.CreateContentsForEntity(path)));
179+
entity.SetContents(
180+
paint.WithFilters(paint.CreateContentsForEntity(std::move(path))));
180181

181182
GetCurrentPass().AddEntity(entity);
182183
}
@@ -277,13 +278,13 @@ void Canvas::DrawRRect(Rect rect, Point corner_radii, const Paint& paint) {
277278
entity.SetTransform(GetCurrentTransform());
278279
entity.SetClipDepth(GetClipDepth());
279280
entity.SetBlendMode(paint.blend_mode);
280-
entity.SetContents(paint.WithFilters(
281-
paint.CreateContentsForGeometry(Geometry::MakeFillPath(path))));
281+
entity.SetContents(paint.WithFilters(paint.CreateContentsForGeometry(
282+
Geometry::MakeFillPath(std::move(path)))));
282283

283284
GetCurrentPass().AddEntity(entity);
284285
return;
285286
}
286-
DrawPath(path, paint);
287+
DrawPath(std::move(path), paint);
287288
}
288289

289290
void Canvas::DrawCircle(Point center, Scalar radius, const Paint& paint) {
@@ -308,10 +309,10 @@ void Canvas::DrawCircle(Point center, Scalar radius, const Paint& paint) {
308309
GetCurrentPass().AddEntity(entity);
309310
}
310311

311-
void Canvas::ClipPath(const Path& path, Entity::ClipOperation clip_op) {
312-
ClipGeometry(Geometry::MakeFillPath(path), clip_op);
312+
void Canvas::ClipPath(Path path, Entity::ClipOperation clip_op) {
313+
auto bounds = path.GetBoundingBox();
314+
ClipGeometry(Geometry::MakeFillPath(std::move(path)), clip_op);
313315
if (clip_op == Entity::ClipOperation::kIntersect) {
314-
auto bounds = path.GetBoundingBox();
315316
if (bounds.has_value()) {
316317
IntersectCulling(bounds.value());
317318
}
@@ -355,7 +356,7 @@ void Canvas::ClipRRect(const Rect& rect,
355356
std::optional<Rect> inner_rect = (flat_on_LR && flat_on_TB)
356357
? rect.Expand(-corner_radii)
357358
: std::make_optional<Rect>();
358-
auto geometry = Geometry::MakeFillPath(path, inner_rect);
359+
auto geometry = Geometry::MakeFillPath(std::move(path), inner_rect);
359360
auto& cull_rect = transform_stack_.back().cull_rect;
360361
if (clip_op == Entity::ClipOperation::kIntersect && //
361362
cull_rect.has_value() && //

‎impeller/aiks/canvas.h‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ class Canvas {
9696

9797
void Rotate(Radians radians);
9898

99-
void DrawPath(const Path& path, const Paint& paint);
99+
void DrawPath(Path path, const Paint& paint);
100100

101101
void DrawPaint(const Paint& paint);
102102

@@ -125,7 +125,7 @@ class Canvas {
125125
SamplerDescriptor sampler = {});
126126

127127
void ClipPath(
128-
const Path& path,
128+
Path path,
129129
Entity::ClipOperation clip_op = Entity::ClipOperation::kIntersect);
130130

131131
void ClipRect(

‎impeller/aiks/canvas_recorder.h‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,16 @@ class CanvasRecorder {
9191
return (canvas_.*canvasMethod)(std::forward<Args>(args)...);
9292
}
9393

94+
template <typename FuncType, typename... Args>
95+
auto ExecuteAndSkipArgSerialize(CanvasRecorderOp op,
96+
FuncType canvasMethod,
97+
Args&&... args)
98+
-> decltype((std::declval<Canvas>().*
99+
canvasMethod)(std::forward<Args>(args)...)) {
100+
serializer_.Write(op);
101+
return (canvas_.*canvasMethod)(std::forward<Args>(args)...);
102+
}
103+
94104
//////////////////////////////////////////////////////////////////////////////
95105
// Canvas Static Polymorphism
96106
// ////////////////////////////////////////////////
@@ -169,9 +179,11 @@ class CanvasRecorder {
169179
return ExecuteAndSerialize(FLT_CANVAS_RECORDER_OP_ARG(Rotate), radians);
170180
}
171181

172-
void DrawPath(const Path& path, const Paint& paint) {
173-
return ExecuteAndSerialize(FLT_CANVAS_RECORDER_OP_ARG(DrawPath), path,
174-
paint);
182+
void DrawPath(Path path, const Paint& paint) {
183+
serializer_.Write(path);
184+
serializer_.Write(paint);
185+
return ExecuteAndSkipArgSerialize(FLT_CANVAS_RECORDER_OP_ARG(DrawPath),
186+
std::move(path), paint);
175187
}
176188

177189
void DrawPaint(const Paint& paint) {
@@ -219,10 +231,12 @@ class CanvasRecorder {
219231
}
220232

221233
void ClipPath(
222-
const Path& path,
234+
Path path,
223235
Entity::ClipOperation clip_op = Entity::ClipOperation::kIntersect) {
224-
return ExecuteAndSerialize(FLT_CANVAS_RECORDER_OP_ARG(ClipPath), path,
225-
clip_op);
236+
serializer_.Write(path);
237+
serializer_.Write(clip_op);
238+
return ExecuteAndSkipArgSerialize(FLT_CANVAS_RECORDER_OP_ARG(ClipPath),
239+
std::move(path), clip_op);
226240
}
227241

228242
void ClipRect(

‎impeller/aiks/canvas_unittests.cc‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ TEST(AiksCanvasTest, PathClipIntersectAgainstEmptyCullRect) {
267267
Rect rect_clip = Rect::MakeXYWH(5, 5, 10, 10);
268268

269269
Canvas canvas;
270-
canvas.ClipPath(path, Entity::ClipOperation::kIntersect);
270+
canvas.ClipPath(std::move(path), Entity::ClipOperation::kIntersect);
271271

272272
ASSERT_TRUE(canvas.GetCurrentLocalCullingBounds().has_value());
273273
ASSERT_EQ(canvas.GetCurrentLocalCullingBounds().value(), rect_clip);
@@ -282,7 +282,7 @@ TEST(AiksCanvasTest, PathClipDiffAgainstEmptyCullRect) {
282282
Path path = builder.TakePath();
283283

284284
Canvas canvas;
285-
canvas.ClipPath(path, Entity::ClipOperation::kDifference);
285+
canvas.ClipPath(std::move(path), Entity::ClipOperation::kDifference);
286286

287287
ASSERT_FALSE(canvas.GetCurrentLocalCullingBounds().has_value());
288288
}
@@ -298,7 +298,7 @@ TEST(AiksCanvasTest, PathClipIntersectAgainstCullRect) {
298298
Rect result_cull = Rect::MakeXYWH(5, 5, 5, 5);
299299

300300
Canvas canvas(initial_cull);
301-
canvas.ClipPath(path, Entity::ClipOperation::kIntersect);
301+
canvas.ClipPath(std::move(path), Entity::ClipOperation::kIntersect);
302302

303303
ASSERT_TRUE(canvas.GetCurrentLocalCullingBounds().has_value());
304304
ASSERT_EQ(canvas.GetCurrentLocalCullingBounds().value(), result_cull);
@@ -315,7 +315,7 @@ TEST(AiksCanvasTest, PathClipDiffAgainstNonCoveredCullRect) {
315315
Rect result_cull = Rect::MakeXYWH(0, 0, 10, 10);
316316

317317
Canvas canvas(initial_cull);
318-
canvas.ClipPath(path, Entity::ClipOperation::kDifference);
318+
canvas.ClipPath(std::move(path), Entity::ClipOperation::kDifference);
319319

320320
ASSERT_TRUE(canvas.GetCurrentLocalCullingBounds().has_value());
321321
ASSERT_EQ(canvas.GetCurrentLocalCullingBounds().value(), result_cull);
@@ -330,7 +330,7 @@ TEST(AiksCanvasTest, PathClipDiffAgainstFullyCoveredCullRect) {
330330
Rect result_cull = Rect::MakeXYWH(5, 5, 10, 10);
331331

332332
Canvas canvas(initial_cull);
333-
canvas.ClipPath(path, Entity::ClipOperation::kDifference);
333+
canvas.ClipPath(std::move(path), Entity::ClipOperation::kDifference);
334334

335335
ASSERT_TRUE(canvas.GetCurrentLocalCullingBounds().has_value());
336336
ASSERT_EQ(canvas.GetCurrentLocalCullingBounds().value(), result_cull);

‎impeller/aiks/paint.cc‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -26,18 +26,19 @@ constexpr ColorMatrix kColorInversion = {
2626
};
2727
// clang-format on
2828

29-
std::shared_ptr<Contents> Paint::CreateContentsForEntity(const Path& path,
29+
std::shared_ptr<Contents> Paint::CreateContentsForEntity(Path path,
3030
bool cover) const {
3131
std::shared_ptr<Geometry> geometry;
3232
switch (style) {
3333
case Style::kFill:
34-
geometry = cover ? Geometry::MakeCover() : Geometry::MakeFillPath(path);
34+
geometry = cover ? Geometry::MakeCover()
35+
: Geometry::MakeFillPath(std::move(path));
3536
break;
3637
case Style::kStroke:
37-
geometry =
38-
cover ? Geometry::MakeCover()
39-
: Geometry::MakeStrokePath(path, stroke_width, stroke_miter,
40-
stroke_cap, stroke_join);
38+
geometry = cover ? Geometry::MakeCover()
39+
: Geometry::MakeStrokePath(std::move(path), stroke_width,
40+
stroke_miter, stroke_cap,
41+
stroke_join);
4142
break;
4243
}
4344
return CreateContentsForGeometry(geometry);

‎impeller/aiks/paint.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ struct Paint {
8484
std::shared_ptr<Contents> input,
8585
const Matrix& effect_transform = Matrix()) const;
8686

87-
std::shared_ptr<Contents> CreateContentsForEntity(const Path& path = {},
87+
std::shared_ptr<Contents> CreateContentsForEntity(Path path = {},
8888
bool cover = false) const;
8989

9090
std::shared_ptr<Contents> CreateContentsForGeometry(

‎impeller/display_list/dl_dispatcher.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -778,7 +778,7 @@ void DlDispatcher::drawOval(const SkRect& bounds) {
778778
.AddOval(skia_conversions::ToRect(bounds))
779779
.SetConvexity(Convexity::kConvex)
780780
.TakePath();
781-
canvas_.DrawPath(path, paint_);
781+
canvas_.DrawPath(std::move(path), paint_);
782782
}
783783
}
784784

‎impeller/entity/contents/solid_color_contents.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -86,10 +86,10 @@ bool SolidColorContents::Render(const ContentContext& renderer,
8686
return true;
8787
}
8888

89-
std::unique_ptr<SolidColorContents> SolidColorContents::Make(const Path& path,
89+
std::unique_ptr<SolidColorContents> SolidColorContents::Make(Path path,
9090
Color color) {
9191
auto contents = std::make_unique<SolidColorContents>();
92-
contents->SetGeometry(Geometry::MakeFillPath(path));
92+
contents->SetGeometry(Geometry::MakeFillPath(std::move(path)));
9393
contents->SetColor(color);
9494
return contents;
9595
}

‎impeller/entity/contents/solid_color_contents.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,7 @@ class SolidColorContents final : public ColorSourceContents {
2727

2828
~SolidColorContents() override;
2929

30-
static std::unique_ptr<SolidColorContents> Make(const Path& path,
31-
Color color);
30+
static std::unique_ptr<SolidColorContents> Make(Path path, Color color);
3231

3332
void SetColor(Color color);
3433

0 commit comments

Comments
 (0)