Skip to content

Commit

Permalink
Revert "cc/paint: Use std::unique_ptr to wrap paths in PaintOps"
Browse files Browse the repository at this point in the history 
This reverts commit 85aae9f.

Reason for revert: ~3.3% motionmark regression - see crbug.com/1347691

Original change's description:
> cc/paint: Use std::unique_ptr to wrap paths in PaintOps
>
> We want to statically assert that PaintOps are trivially relocatable so
> that the PaintOpBuffer memcpy on realloc optimization is safe. One way
> of doing that is to mark PaintOps with clang's trivial_abi attribute
> which only works if the types contained are also trivially relocatable.
>
> ThreadsafePath is a thin subclass of SkPath contained in ClipPathOp and
> DrawPathOp. Since SkPath contains std::atomics which lack copy ctors, it
> cannot be simply marked as trivial_abi so the next best thing is to wrap
> paths in a std::unique_ptr which is what this CL does. Also add move
> constructors since these will be required when the types are marked as
> trivial_abi in a subsequent CL.
>
> Bug: 1302745
> Change-Id: I4879a6ade6620bea344f61cbf046e4a9e6b6a9c9
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3787235
> Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
> Commit-Queue: Peter Boström <pbos@chromium.org>
> Auto-Submit: Sunny Sachanandani <sunnyps@chromium.org>
> Reviewed-by: Peter Boström <pbos@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1028444}

Bug: 1302745, 1347691
Change-Id: I2e9ab91ab687bb725140130f2086a32f8c24ba19
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3825577
Reviewed-by: Vasiliy Telezhnikov <vasilyt@chromium.org>
Reviewed-by: Peter Boström <pbos@chromium.org>
Commit-Queue: Peter Boström <pbos@chromium.org>
Auto-Submit: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033961}
  • Loading branch information
@sunnyps
sunnyps authored and Chromium LUCI CQ committed Aug 11, 2022
1 parent 59752be commit b280704
Show file tree
Hide file tree
Showing 5 changed files with 52 additions and 75 deletions.
76 changes: 18 additions & 58 deletions cc/paint/paint_op_buffer.cc
View file
Expand Up @@ -120,14 +120,6 @@ bool GrSlugAreEqual(sk_sp<GrSlug> left, sk_sp<GrSlug> right) {
return false;
}

class ThreadsafePath : public SkPath {
public:
explicit ThreadsafePath(const SkPath& path) : SkPath(path) {
updateBoundsCache();
}
ThreadsafePath() { updateBoundsCache(); }
};

} // namespace

#define TYPES(M) \
Expand Down Expand Up @@ -469,7 +461,7 @@ size_t ClipPathOp::Serialize(const PaintOp* base_op,
const SkM44& original_ctm) {
auto* op = static_cast<const ClipPathOp*>(base_op);
PaintOpWriter helper(memory, size, options);
helper.Write(*op->path, op->use_cache);
helper.Write(op->path, op->use_cache);
helper.Write(op->op);
helper.Write(op->antialias);
return helper.size();
Expand Down Expand Up @@ -685,7 +677,7 @@ size_t DrawPathOp::Serialize(const PaintOp* base_op,
if (!flags_to_serialize)
flags_to_serialize = &op->flags;
helper.Write(*flags_to_serialize, current_ctm);
helper.Write(*op->path, op->use_cache);
helper.Write(op->path, op->use_cache);
helper.Write(op->sk_path_fill_type);
return helper.size();
}
Expand Down Expand Up @@ -1056,7 +1048,7 @@ PaintOp* ClipPathOp::Deserialize(const volatile void* input,
PaintOpDeserializer<ClipPathOp> deserializer(input, input_size, options,
new (output) ClipPathOp);

deserializer.Read(deserializer->path.get());
deserializer.Read(&deserializer->path);
deserializer.Read(&deserializer->op);
deserializer.Read(&deserializer->antialias);
return deserializer.FinalizeOp();
Expand Down Expand Up @@ -1236,9 +1228,9 @@ PaintOp* DrawPathOp::Deserialize(const volatile void* input,
PaintOpDeserializer<DrawPathOp> deserializer(input, input_size, options,
new (output) DrawPathOp);
deserializer.Read(&deserializer->flags);
deserializer.Read(deserializer->path.get());
deserializer.Read(&deserializer->path);
deserializer.Read(&deserializer->sk_path_fill_type);
deserializer->path->setFillType(
deserializer->path.setFillType(
static_cast<SkPathFillType>(deserializer->sk_path_fill_type));
return deserializer.FinalizeOp();
}
Expand Down Expand Up @@ -1545,7 +1537,7 @@ void AnnotateOp::Raster(const AnnotateOp* op,
void ClipPathOp::Raster(const ClipPathOp* op,
SkCanvas* canvas,
const PlaybackParams& params) {
canvas->clipPath(*op->path, op->op, op->antialias);
canvas->clipPath(op->path, op->op, op->antialias);
}

void ClipRectOp::Raster(const ClipRectOp* op,
Expand All @@ -1561,8 +1553,8 @@ void ClipRRectOp::Raster(const ClipRRectOp* op,
}

void ConcatOp::Raster(const ConcatOp* op,
SkCanvas* canvas,
const PlaybackParams& params) {
SkCanvas* canvas,
const PlaybackParams& params) {
canvas->concat(op->matrix);
}

Expand Down Expand Up @@ -1760,7 +1752,7 @@ void DrawPathOp::RasterWithFlags(const DrawPathOp* op,
SkCanvas* canvas,
const PlaybackParams& params) {
flags->DrawToSk(canvas, [op](SkCanvas* c, const SkPaint& p) {
c->drawPath(*op->path, p);
c->drawPath(op->path, p);
});
}

Expand Down Expand Up @@ -1939,8 +1931,8 @@ void ScaleOp::Raster(const ScaleOp* op,
}

void SetMatrixOp::Raster(const SetMatrixOp* op,
SkCanvas* canvas,
const PlaybackParams& params) {
SkCanvas* canvas,
const PlaybackParams& params) {
canvas->setMatrix(params.original_ctm * op->matrix);
}

Expand Down Expand Up @@ -2073,7 +2065,7 @@ bool ClipPathOp::AreEqual(const PaintOp* base_left, const PaintOp* base_right) {
auto* right = static_cast<const ClipPathOp*>(base_right);
DCHECK(left->IsValid());
DCHECK(right->IsValid());
if (*left->path != *right->path)
if (left->path != right->path)
return false;
if (left->op != right->op)
return false;
Expand Down Expand Up @@ -2240,7 +2232,7 @@ bool DrawPathOp::AreEqual(const PaintOp* base_left, const PaintOp* base_right) {
DCHECK(right->IsValid());
if (left->flags != right->flags)
return false;
if (*left->path != *right->path)
if (left->path != right->path)
return false;
return true;
}
Expand Down Expand Up @@ -2555,7 +2547,7 @@ bool PaintOp::GetBounds(const PaintOp* op, SkRect* rect) {
}
case PaintOpType::DrawPath: {
auto* path_op = static_cast<const DrawPathOp*>(op);
*rect = path_op->path->getBounds();
*rect = path_op->path.getBounds();
rect->sort();
return true;
}
Expand Down Expand Up @@ -2706,7 +2698,7 @@ void PaintOpWithFlags::RasterWithFlags(SkCanvas* canvas,
}

int ClipPathOp::CountSlowPaths() const {
return antialias && !path->isConvex() ? 1 : 0;
return antialias && !path.isConvex() ? 1 : 0;
}

int DrawLineOp::CountSlowPaths() const {
Expand All @@ -2726,17 +2718,17 @@ int DrawLineOp::CountSlowPaths() const {
int DrawPathOp::CountSlowPaths() const {
// This logic is copied from SkPathCounter instead of attempting to expose
// that from Skia.
if (!flags.isAntiAlias() || path->isConvex())
if (!flags.isAntiAlias() || path.isConvex())
return 0;

PaintFlags::Style paintStyle = flags.getStyle();
const SkRect& pathBounds = path->getBounds();
const SkRect& pathBounds = path.getBounds();
if (paintStyle == PaintFlags::kStroke_Style && flags.getStrokeWidth() == 0) {
// AA hairline concave path is not slow.
return 0;
} else if (paintStyle == PaintFlags::kFill_Style &&
pathBounds.width() < 64.f && pathBounds.height() < 64.f &&
!path->isVolatile()) {
!path.isVolatile()) {
// AADF eligible concave path is not slow.
return 0;
} else {
Expand Down Expand Up @@ -2780,23 +2772,6 @@ AnnotateOp::AnnotateOp(PaintCanvas::AnnotationType annotation_type,

AnnotateOp::~AnnotateOp() = default;

ClipPathOp::ClipPathOp()
: PaintOp(kType), path(std::make_unique<ThreadsafePath>()) {}

ClipPathOp::ClipPathOp(SkPath path,
SkClipOp op,
bool antialias,
UsePaintCache use_paint_cache)
: PaintOp(kType),
path(std::make_unique<ThreadsafePath>(path)),
op(op),
antialias(antialias),
use_cache(use_paint_cache) {}

ClipPathOp::ClipPathOp(ClipPathOp&&) = default;

ClipPathOp::~ClipPathOp() = default;

DrawImageOp::DrawImageOp() : PaintOpWithFlags(kType) {}

DrawImageOp::DrawImageOp(const PaintImage& image, SkScalar left, SkScalar top)
Expand Down Expand Up @@ -2855,21 +2830,6 @@ bool DrawImageRectOp::HasDiscardableImages() const {

DrawImageRectOp::~DrawImageRectOp() = default;

DrawPathOp::DrawPathOp()
: PaintOpWithFlags(kType), path(std::make_unique<ThreadsafePath>()) {}

DrawPathOp::DrawPathOp(const SkPath& path,
const PaintFlags& flags,
UsePaintCache use_paint_cache)
: PaintOpWithFlags(kType, flags),
path(std::make_unique<ThreadsafePath>(path)),
sk_path_fill_type(static_cast<uint8_t>(path.getFillType())),
use_cache(use_paint_cache) {}

DrawPathOp::DrawPathOp(DrawPathOp&&) = default;

DrawPathOp::~DrawPathOp() = default;

DrawRecordOp::DrawRecordOp(sk_sp<const PaintRecord> record)
: PaintOp(kType), record(std::move(record)) {}

Expand Down
37 changes: 25 additions & 12 deletions cc/paint/paint_op_buffer.h
View file
Expand Up @@ -57,6 +57,14 @@ class SkottieSerializationHistory;
class TransferCacheDeserializeHelper;
class TransferCacheSerializeHelper;

class CC_PAINT_EXPORT ThreadsafePath : public SkPath {
public:
explicit ThreadsafePath(const SkPath& path) : SkPath(path) {
updateBoundsCache();
}
ThreadsafePath() { updateBoundsCache(); }
};

class CC_PAINT_EXPORT SharedImageProvider {
public:
enum class Error {
Expand Down Expand Up @@ -408,25 +416,28 @@ class CC_PAINT_EXPORT ClipPathOp final : public PaintOp {
ClipPathOp(SkPath path,
SkClipOp op,
bool antialias,
UsePaintCache use_paint_cache = UsePaintCache::kEnabled);
ClipPathOp(ClipPathOp&&);
~ClipPathOp();
UsePaintCache use_paint_cache = UsePaintCache::kEnabled)
: PaintOp(kType),
path(path),
op(op),
antialias(antialias),
use_cache(use_paint_cache) {}
static void Raster(const ClipPathOp* op,
SkCanvas* canvas,
const PlaybackParams& params);
bool IsValid() const { return IsValidSkClipOp(op) && IsValidPath(*path); }
bool IsValid() const { return IsValidSkClipOp(op) && IsValidPath(path); }
static bool AreEqual(const PaintOp* left, const PaintOp* right);
int CountSlowPaths() const;
bool HasNonAAPaint() const { return !antialias; }
HAS_SERIALIZATION_FUNCTIONS();

std::unique_ptr<SkPath> path;
ThreadsafePath path;
SkClipOp op;
bool antialias;
UsePaintCache use_cache;

private:
ClipPathOp();
ClipPathOp() : PaintOp(kType) {}
};

class CC_PAINT_EXPORT ClipRectOp final : public PaintOp {
Expand Down Expand Up @@ -708,19 +719,21 @@ class CC_PAINT_EXPORT DrawPathOp final : public PaintOpWithFlags {
static constexpr bool kIsDrawOp = true;
DrawPathOp(const SkPath& path,
const PaintFlags& flags,
UsePaintCache use_paint_cache = UsePaintCache::kEnabled);
DrawPathOp(DrawPathOp&&);
~DrawPathOp();
UsePaintCache use_paint_cache = UsePaintCache::kEnabled)
: PaintOpWithFlags(kType, flags),
path(path),
sk_path_fill_type(static_cast<uint8_t>(path.getFillType())),
use_cache(use_paint_cache) {}
static void RasterWithFlags(const DrawPathOp* op,
const PaintFlags* flags,
SkCanvas* canvas,
const PlaybackParams& params);
bool IsValid() const { return flags.IsValid() && IsValidPath(*path); }
bool IsValid() const { return flags.IsValid() && IsValidPath(path); }
static bool AreEqual(const PaintOp* left, const PaintOp* right);
int CountSlowPaths() const;
HAS_SERIALIZATION_FUNCTIONS();

std::unique_ptr<SkPath> path;
ThreadsafePath path;

// Changing the fill type on an SkPath does not change the
// generation id. This can lead to caching issues so we explicitly
Expand All @@ -730,7 +743,7 @@ class CC_PAINT_EXPORT DrawPathOp final : public PaintOpWithFlags {
UsePaintCache use_cache;

private:
DrawPathOp();
DrawPathOp() : PaintOpWithFlags(kType) {}
};

class CC_PAINT_EXPORT DrawRecordOp final : public PaintOp {
Expand Down
2 changes: 1 addition & 1 deletion cc/paint/paint_op_buffer_unittest.cc
View file
Expand Up @@ -2827,7 +2827,7 @@ TEST(PaintOpBufferTest, BoundingRect_DrawPathOp) {
auto* op = static_cast<DrawPathOp*>(base_op);

ASSERT_TRUE(PaintOp::GetBounds(op, &rect));
EXPECT_EQ(rect, op->path->getBounds().makeSorted());
EXPECT_EQ(rect, op->path.getBounds().makeSorted());
}
}

Expand Down
8 changes: 6 additions & 2 deletions cc/test/paint_op_helper.h
View file
Expand Up @@ -35,7 +35,7 @@ class PaintOpHelper {
}
case PaintOpType::ClipPath: {
const auto* op = static_cast<const ClipPathOp*>(base_op);
str << "ClipPathOp(path=" << PaintOpHelper::SkiaTypeToString(*op->path)
str << "ClipPathOp(path=" << PaintOpHelper::SkiaTypeToString(op->path)
<< ", op=" << PaintOpHelper::SkiaTypeToString(op->op)
<< ", antialias=" << op->antialias
<< ", use_cache=" << (op->use_cache == UsePaintCache::kEnabled)
Expand Down Expand Up @@ -125,7 +125,7 @@ class PaintOpHelper {
}
case PaintOpType::DrawPath: {
const auto* op = static_cast<const DrawPathOp*>(base_op);
str << "DrawPathOp(path=" << PaintOpHelper::SkiaTypeToString(*op->path)
str << "DrawPathOp(path=" << PaintOpHelper::SkiaTypeToString(op->path)
<< ", flags=" << PaintOpHelper::FlagsToString(op->flags)
<< ", use_cache=" << (op->use_cache == UsePaintCache::kEnabled)
<< ")";
Expand Down Expand Up @@ -358,6 +358,10 @@ class PaintOpHelper {
return data ? "<SkData>" : "(nil)";
}

static std::string SkiaTypeToString(const ThreadsafePath& path) {
return SkiaTypeToString(static_cast<const SkPath&>(path));
}

static std::string SkiaTypeToString(const SkPath& path) {
// TODO(vmpstr): SkPath has a dump function which we can use here?
return "<SkPath>";
Expand Down
4 changes: 2 additions & 2 deletions ui/views/animation/ink_drop_mask_unittest.cc
View file
Expand Up @@ -37,10 +37,10 @@ TEST(InkDropMaskTest, PathInkDropMaskPaintsTriangle) {
sk_sp<cc::PaintRecord> record = list->ReleaseAsRecord();
const auto* draw_op = record->GetOpAtForTesting<cc::DrawPathOp>(0);
ASSERT_NE(nullptr, draw_op);
ASSERT_EQ(3, draw_op->path->countPoints());
ASSERT_EQ(3, draw_op->path.countPoints());

SkPoint points[3];
ASSERT_EQ(3, draw_op->path->getPoints(points, 3));
ASSERT_EQ(3, draw_op->path.getPoints(points, 3));
std::sort(points, points + 3,
[](const SkPoint& a, const SkPoint& b) { return a.x() < b.x(); });
EXPECT_EQ(p1, points[0]);
Expand Down

0 comments on commit b280704

Please sign in to comment.