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

[Impeller] Reduce extra allocation and shared_ptr ops during canvas recording. #48585

Closed
jonahwilliams wants to merge 5 commits into from
Closed

[Impeller] Reduce extra allocation and shared_ptr ops during canvas recording. #48585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e15b5f3
[Impeller] start simplification of canvas recording.
Dec 1, 2023
fa80c7b
test tweaks
Dec 1, 2023
0420c7c
Make geometries unique-ish.
Dec 2, 2023
9f2ef2d
Merge branch 'main' of github.com:flutter/engine into fix_with_filters
Dec 2, 2023
f35c00b
++
Dec 2, 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
8 changes: 4 additions & 4 deletions impeller/aiks/aiks_unittests.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3973,11 +3973,11 @@ TEST_P(AiksTest, VerticesGeometryUVPositionData) {
std::vector<uint16_t> indices = {0u, 1u, 2u};
std::vector<Point> texture_coordinates = {};
std::vector<Color> vertex_colors = {};
auto geometry = std::make_shared<VerticesGeometry>(
auto geometry = std::make_unique<VerticesGeometry>(
vertices, indices, texture_coordinates, vertex_colors,
Rect::MakeLTRB(0, 0, 1, 1), VerticesGeometry::VertexMode::kTriangleStrip);

canvas.DrawVertices(geometry, BlendMode::kSourceOver, paint);
canvas.DrawVertices(std::move(geometry), BlendMode::kSourceOver, paint);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

Expand All @@ -3996,11 +3996,11 @@ TEST_P(AiksTest, VerticesGeometryUVPositionDataWithTranslate) {
std::vector<uint16_t> indices = {0u, 1u, 2u};
std::vector<Point> texture_coordinates = {};
std::vector<Color> vertex_colors = {};
auto geometry = std::make_shared<VerticesGeometry>(
auto geometry = std::make_unique<VerticesGeometry>(
vertices, indices, texture_coordinates, vertex_colors,
Rect::MakeLTRB(0, 0, 1, 1), VerticesGeometry::VertexMode::kTriangleStrip);

canvas.DrawVertices(geometry, BlendMode::kSourceOver, paint);
canvas.DrawVertices(std::move(geometry), BlendMode::kSourceOver, paint);
ASSERT_TRUE(OpenPlaygroundHere(canvas.EndRecordingAsPicture()));
}

Expand Down
144 changes: 114 additions & 30 deletions impeller/aiks/canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "impeller/aiks/canvas.h"

#include <memory>
#include <optional>
#include <utility>

Expand All @@ -18,11 +19,86 @@
#include "impeller/entity/contents/texture_contents.h"
#include "impeller/entity/contents/vertices_contents.h"
#include "impeller/entity/geometry/geometry.h"
#include "impeller/entity/geometry/vertices_geometry.h"
#include "impeller/geometry/constants.h"
#include "impeller/geometry/path_builder.h"

namespace impeller {

namespace {

static std::shared_ptr<Contents> CreateContentsForGeometryWithFilters(
Copy link
Contributor Author

@jonahwilliams jonahwilliams Dec 1, 2023

Choose a reason for hiding this comment

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

While this code uses shared_ptr its careful to move when possible to avoid the shared ptr release ops.

Copy link
Member

@gaaclarke gaaclarke Dec 1, 2023

Choose a reason for hiding this comment

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

No action required: rvalue parameters are technically forbidden ( https://google.github.io/styleguide/cppguide.html#Rvalue_references ) but it would have been nice to statically assert that people are moving geometry into this. I see there is just one case were you'd have to manually copy it to get an rvalue.

This is beyond the scope of this PR but we could have created a wrapper for std::shared_ptr that doesn't have a copy constructor.

template<typename T>
class  move_ptr {
  public:
    move_ptr(const ptr<T>&) = delete;
    move_ptr(const ptr<T>&&) = default;
    move_ptr<T> Clone() const {...}
  private:
   std::shared_ptr<T> ptr_;
};

const Paint& paint,
std::unique_ptr<Geometry> geometry) {
std::shared_ptr<ColorSourceContents> contents =
paint.color_source.GetContents(paint);

// Attempt to apply the color filter on the CPU first.
// Note: This is not just an optimization; some color sources rely on
// CPU-applied color filters to behave properly.
bool needs_color_filter = paint.HasColorFilter();
if (needs_color_filter) {
auto color_filter = paint.GetColorFilter();
if (contents->ApplyColorFilter(color_filter->GetCPUColorFilterProc())) {
needs_color_filter = false;
}
}

contents->SetGeometry(std::move(geometry));
if (paint.mask_blur_descriptor.has_value()) {
// If there's a mask blur and we need to apply the color filter on the GPU,
// we need to be careful to only apply the color filter to the source
// colors. CreateMaskBlur is able to handle this case.
return paint.mask_blur_descriptor->CreateMaskBlur(
contents, needs_color_filter ? paint.GetColorFilter() : nullptr);
}

std::shared_ptr<Contents> contents_copy = std::move(contents);
// Image input types will directly set their color filter,
// if any. See `TiledTextureContents.SetColorFilter`.
if (needs_color_filter &&
paint.color_source.GetType() != ColorSource::Type::kImage) {
auto color_filter = paint.GetColorFilter();
contents_copy = color_filter->WrapWithGPUColorFilter(
FilterInput::Make(std::move(contents_copy)),
ColorFilterContents::AbsorbOpacity::kYes);
}

if (paint.image_filter) {
std::shared_ptr<FilterContents> filter = paint.image_filter->WrapInput(
FilterInput::Make(std::move(contents_copy)));
filter->SetRenderingMode(Entity::RenderingMode::kDirect);
return filter;
}

return contents_copy;
}

static std::shared_ptr<Contents> CreateContentsWithFilters(
const Paint& paint,
const Path& path = {},
bool cover = false) {
std::unique_ptr<Geometry> geometry;
if (cover) {
geometry = Geometry::MakeCover();
} else {
switch (paint.style) {
case Paint::Style::kFill:
geometry = Geometry::MakeFillPath(path);
break;
case Paint::Style::kStroke:
geometry = Geometry::MakeStrokePath(
path, paint.stroke_width, paint.stroke_miter, paint.stroke_cap,
paint.stroke_join);
break;
}
}

return CreateContentsForGeometryWithFilters(paint, std::move(geometry));
}

} // namespace

Canvas::Canvas() {
Initialize(std::nullopt);
}
Expand Down Expand Up @@ -176,7 +252,7 @@ void Canvas::DrawPath(const Path& path, const Paint& paint) {
entity.SetTransform(GetCurrentTransform());
entity.SetClipDepth(GetClipDepth());
entity.SetBlendMode(paint.blend_mode);
entity.SetContents(paint.WithFilters(paint.CreateContentsForEntity(path)));
entity.SetContents(CreateContentsWithFilters(paint, path));

GetCurrentPass().AddEntity(entity);
}
Expand All @@ -186,7 +262,7 @@ void Canvas::DrawPaint(const Paint& paint) {
entity.SetTransform(GetCurrentTransform());
entity.SetClipDepth(GetClipDepth());
entity.SetBlendMode(paint.blend_mode);
entity.SetContents(paint.CreateContentsForEntity({}, true));
entity.SetContents(CreateContentsWithFilters(paint, {}, true));

GetCurrentPass().AddEntity(entity);
}
Expand Down Expand Up @@ -236,8 +312,8 @@ void Canvas::DrawLine(const Point& p0, const Point& p1, const Paint& paint) {
entity.SetTransform(GetCurrentTransform());
entity.SetClipDepth(GetClipDepth());
entity.SetBlendMode(paint.blend_mode);
entity.SetContents(paint.WithFilters(paint.CreateContentsForGeometry(
Geometry::MakeLine(p0, p1, paint.stroke_width, paint.stroke_cap))));
entity.SetContents(CreateContentsForGeometryWithFilters(
paint, Geometry::MakeLine(p0, p1, paint.stroke_width, paint.stroke_cap)));

GetCurrentPass().AddEntity(entity);
}
Expand All @@ -256,8 +332,8 @@ void Canvas::DrawRect(Rect rect, const Paint& paint) {
entity.SetTransform(GetCurrentTransform());
entity.SetClipDepth(GetClipDepth());
entity.SetBlendMode(paint.blend_mode);
entity.SetContents(paint.WithFilters(
paint.CreateContentsForGeometry(Geometry::MakeRect(rect))));
entity.SetContents(
CreateContentsForGeometryWithFilters(paint, Geometry::MakeRect(rect)));

GetCurrentPass().AddEntity(entity);
}
Expand All @@ -277,8 +353,8 @@ void Canvas::DrawRRect(Rect rect, Point corner_radii, const Paint& paint) {
entity.SetTransform(GetCurrentTransform());
entity.SetClipDepth(GetClipDepth());
entity.SetBlendMode(paint.blend_mode);
entity.SetContents(paint.WithFilters(
paint.CreateContentsForGeometry(Geometry::MakeFillPath(path))));
entity.SetContents(CreateContentsForGeometryWithFilters(
paint, Geometry::MakeFillPath(path)));

GetCurrentPass().AddEntity(entity);
return;
Expand All @@ -303,7 +379,7 @@ void Canvas::DrawCircle(Point center, Scalar radius, const Paint& paint) {
? Geometry::MakeStrokedCircle(center, radius, paint.stroke_width)
: Geometry::MakeCircle(center, radius);
entity.SetContents(
paint.WithFilters(paint.CreateContentsForGeometry(geometry)));
CreateContentsForGeometryWithFilters(paint, std::move(geometry)));

GetCurrentPass().AddEntity(entity);
}
Expand All @@ -328,7 +404,7 @@ void Canvas::ClipRect(const Rect& rect, Entity::ClipOperation clip_op) {
return; // This clip will do nothing, so skip it.
}

ClipGeometry(geometry, clip_op);
ClipGeometry(std::move(geometry), clip_op);
switch (clip_op) {
case Entity::ClipOperation::kIntersect:
IntersectCulling(rect);
Expand Down Expand Up @@ -364,7 +440,7 @@ void Canvas::ClipRRect(const Rect& rect,
return; // This clip will do nothing, so skip it.
}

ClipGeometry(geometry, clip_op);
ClipGeometry(std::move(geometry), clip_op);
switch (clip_op) {
case Entity::ClipOperation::kIntersect:
IntersectCulling(rect);
Expand All @@ -389,10 +465,10 @@ void Canvas::ClipRRect(const Rect& rect,
}
}

void Canvas::ClipGeometry(const std::shared_ptr<Geometry>& geometry,
void Canvas::ClipGeometry(std::unique_ptr<Geometry> geometry,
Entity::ClipOperation clip_op) {
auto contents = std::make_shared<ClipContents>();
contents->SetGeometry(geometry);
contents->SetGeometry(std::move(geometry));
contents->SetClipOperation(clip_op);

Entity entity;
Expand Down Expand Up @@ -454,9 +530,10 @@ void Canvas::DrawPoints(std::vector<Point> points,
entity.SetTransform(GetCurrentTransform());
entity.SetClipDepth(GetClipDepth());
entity.SetBlendMode(paint.blend_mode);
entity.SetContents(paint.WithFilters(paint.CreateContentsForGeometry(
entity.SetContents(CreateContentsForGeometryWithFilters(
paint,
Geometry::MakePointField(std::move(points), radius,
/*round=*/point_style == PointStyle::kRound))));
/*round=*/point_style == PointStyle::kRound)));

GetCurrentPass().AddEntity(entity);
}
Expand Down Expand Up @@ -598,24 +675,23 @@ void Canvas::DrawTextFrame(const std::shared_ptr<TextFrame>& text_frame,
GetCurrentPass().AddEntity(entity);
}

static bool UseColorSourceContents(
const std::shared_ptr<VerticesGeometry>& vertices,
const Paint& paint) {
static bool UseColorSourceContents(const VerticesGeometry& vertices,
const Paint& paint) {
// If there are no vertex color or texture coordinates. Or if there
// are vertex coordinates then only if the contents are an image or
// a solid color.
if (vertices->HasVertexColors()) {
if (vertices.HasVertexColors()) {
return false;
}
if (vertices->HasTextureCoordinates() &&
if (vertices.HasTextureCoordinates() &&
(paint.color_source.GetType() == ColorSource::Type::kImage ||
paint.color_source.GetType() == ColorSource::Type::kColor)) {
return true;
}
return !vertices->HasTextureCoordinates();
return !vertices.HasTextureCoordinates();
}

void Canvas::DrawVertices(const std::shared_ptr<VerticesGeometry>& vertices,
void Canvas::DrawVertices(std::unique_ptr<VerticesGeometry> vertices,
BlendMode blend_mode,
const Paint& paint) {
// Override the blend mode with kDestination in order to match the behavior
Expand All @@ -632,19 +708,27 @@ void Canvas::DrawVertices(const std::shared_ptr<VerticesGeometry>& vertices,

// If there are no vertex color or texture coordinates. Or if there
// are vertex coordinates then only if the contents are an image.
if (UseColorSourceContents(vertices, paint)) {
auto contents = paint.CreateContentsForGeometry(vertices);
entity.SetContents(paint.WithFilters(std::move(contents)));
if (UseColorSourceContents(*vertices, paint)) {
entity.SetContents(
CreateContentsForGeometryWithFilters(paint, std::move(vertices)));
GetCurrentPass().AddEntity(entity);
return;
}

auto src_paint = paint;
src_paint.color = paint.color.WithAlpha(1.0);

// TODO(jonahwilliams): the UniqueVerticesWrapper is a bit of a hack
// to bridge using unique_ptr elsewhere and working around my mistakes
// when designing the vertices geometry. All of this logic should be
// refactored into the vertices contents so that we don't need to
// have a shared reference to the vertices object.
std::shared_ptr<VerticesBase> shared_wrapper =
std::make_shared<UniqueVerticesWrapper>(std::move(vertices));

std::shared_ptr<Contents> src_contents =
src_paint.CreateContentsForGeometry(vertices);
if (vertices->HasTextureCoordinates()) {
src_paint.CreateContentsForGeometry(shared_wrapper);
if (shared_wrapper->HasTextureCoordinates()) {
// If the color source has an intrinsic size, then we use that to
// create the src contents as a simplification. Otherwise we use
// the extent of the texture coordinates to determine how large
Expand All @@ -655,12 +739,12 @@ void Canvas::DrawVertices(const std::shared_ptr<VerticesGeometry>& vertices,
if (size.has_value()) {
src_coverage = Rect::MakeXYWH(0, 0, size->width, size->height);
} else {
auto cvg = vertices->GetCoverage(Matrix{});
auto cvg = shared_wrapper->GetCoverage(Matrix{});
FML_CHECK(cvg.has_value());
src_coverage =
// Covered by FML_CHECK.
// NOLINTNEXTLINE(bugprone-unchecked-optional-access)
vertices->GetTextureCoordinateCoverge().value_or(cvg.value());
shared_wrapper->GetTextureCoordinateCoverge().value_or(cvg.value());
}
src_contents =
src_paint.CreateContentsForGeometry(Geometry::MakeRect(src_coverage));
Expand All @@ -669,7 +753,7 @@ void Canvas::DrawVertices(const std::shared_ptr<VerticesGeometry>& vertices,
auto contents = std::make_shared<VerticesContents>();
contents->SetAlpha(paint.color.alpha);
contents->SetBlendMode(blend_mode);
contents->SetGeometry(vertices);
contents->SetGeometry(shared_wrapper);
contents->SetSourceContents(std::move(src_contents));
entity.SetContents(paint.WithFilters(std::move(contents)));

Expand Down
4 changes: 2 additions & 2 deletions impeller/aiks/canvas.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class Canvas {
Point position,
const Paint& paint);

void DrawVertices(const std::shared_ptr<VerticesGeometry>& vertices,
void DrawVertices(std::unique_ptr<VerticesGeometry> vertices,
BlendMode blend_mode,
const Paint& paint);

Expand Down Expand Up @@ -173,7 +173,7 @@ class Canvas {

size_t GetClipDepth() const;

void ClipGeometry(const std::shared_ptr<Geometry>& geometry,
void ClipGeometry(std::unique_ptr<Geometry> geometry,
Entity::ClipOperation clip_op);

void IntersectCulling(Rect clip_bounds);
Expand Down
17 changes: 14 additions & 3 deletions impeller/aiks/canvas_recorder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,16 @@ class CanvasRecorder {
return (canvas_.*canvasMethod)(std::forward<Args>(args)...);
}

template <typename FuncType, typename... Args>
auto ExecuteAndSerializeIgnoringArgs(CanvasRecorderOp op,
FuncType canvasMethod,
Args&&... args)
-> decltype((std::declval<Canvas>().*
canvasMethod)(std::forward<Args>(args)...)) {
serializer_.Write(op);
return (canvas_.*canvasMethod)(std::forward<Args>(args)...);
}

//////////////////////////////////////////////////////////////////////////////
// Canvas Static Polymorphism
// ////////////////////////////////////////////////
Expand Down Expand Up @@ -252,11 +262,12 @@ class CanvasRecorder {
text_frame, position, paint);
}

void DrawVertices(const std::shared_ptr<VerticesGeometry>& vertices,
void DrawVertices(std::unique_ptr<VerticesGeometry> vertices,
BlendMode blend_mode,
const Paint& paint) {
return ExecuteAndSerialize(FLT_CANVAS_RECORDER_OP_ARG(DrawVertices),
vertices, blend_mode, paint);
return ExecuteAndSerializeIgnoringArgs(
FLT_CANVAS_RECORDER_OP_ARG(DrawVertices), std::move(vertices),
blend_mode, paint);
}

void DrawAtlas(const std::shared_ptr<Image>& atlas,
Expand Down
Loading