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

Set SkPath::setIsVolatile based on whether the path survives at least two frames #22620

Merged
dnfield merged 23 commits into from
Dec 10, 2020
Merged

Set SkPath::setIsVolatile based on whether the path survives at least two frames #22620

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
bc78eea
does not work yet
dnfield Nov 19, 2020
f2867c8
safe?
dnfield Nov 21, 2020
34e2523
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Nov 21, 2020
100b2ec
one more
dnfield Nov 21, 2020
6a84f9e
missing mutex
dnfield Nov 21, 2020
24df8c7
UIDartState/shell ownership
dnfield Nov 21, 2020
280b3c9
review and fixup errors introduced
dnfield Nov 30, 2020
07d2d99
test
dnfield Nov 30, 2020
88d4125
benchmark and improvement
dnfield Dec 1, 2020
62c238f
oops
dnfield Dec 1, 2020
9fc3504
..
dnfield Dec 1, 2020
c8171ad
more review
dnfield Dec 1, 2020
17eefed
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Dec 1, 2020
9cd0ac9
deflake tests
dnfield Dec 1, 2020
e087e3d
fix dangling pointer issue
dnfield Dec 1, 2020
50dadd3
missed one
dnfield Dec 1, 2020
38281c2
NO MORE FLAKES
dnfield Dec 2, 2020
7767698
unnecessary code
dnfield Dec 2, 2020
b1d122e
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Dec 2, 2020
847a998
update new test
dnfield Dec 3, 2020
cfaa5b0
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Dec 8, 2020
71153fe
Merge remote-tracking branch 'upstream/master' into path_vol
dnfield Dec 10, 2020
1eac95f
review/update tests so I can run them with null safety
dnfield Dec 10, 2020
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
158 changes: 117 additions & 41 deletions lib/ui/painting/path.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,43 +67,92 @@ void CanvasPath::RegisterNatives(tonic::DartLibraryNatives* natives) {
FOR_EACH_BINDING(DART_REGISTER_NATIVE)});
}

CanvasPath::CanvasPath() {}
CanvasPath::CanvasPath()
: tracked_path_(std::make_shared<TrackedVolatilePath>()) {
resetVolatility();
}

CanvasPath::~CanvasPath() = default;

void CanvasPath::resetVolatility() {
Copy link
Contributor

@liyuqian liyuqian Nov 28, 2020

Choose a reason for hiding this comment

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

The name resetVolatility sounds like it will also reset the volatility frame count but that does not seem to be the case. I believe resetting the count is the intended behavior as it's called in those methods that mutate the path.

Copy link
Contributor Author

@dnfield dnfield Nov 30, 2020

Choose a reason for hiding this comment

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

Done

if (!tracked_path_->tracking_volatility) {
tracked_path_->path_.setIsVolatile(true);
tracked_path_->tracking_volatility = true;
{
std::scoped_lock guard(volatile_paths_mutex_);
volatile_paths_.insert(tracked_path_);
}
}
}

CanvasPath::~CanvasPath() {}
// static
std::mutex CanvasPath::volatile_paths_mutex_;
// static
std::unordered_set<std::shared_ptr<CanvasPath::TrackedVolatilePath>>
CanvasPath::volatile_paths_;
// static
void CanvasPath::updatePathVolatility() {
TRACE_EVENT0("flutter", "updatePathVolatility");
std::scoped_lock guard(volatile_paths_mutex_);
for (auto it = volatile_paths_.begin(), last = volatile_paths_.end();
it != last;) {
auto path = *it;
path->frame_count++;
if (path->frame_count >= number_of_frames_until_non_volatile) {
path->path_.setIsVolatile(false);
path->tracking_volatility = false;
it = volatile_paths_.erase(it);
} else {
++it;
}
}
}

void CanvasPath::ReleaseDartWrappableReference() const {
std::scoped_lock guard(volatile_paths_mutex_);
volatile_paths_.erase(tracked_path_);
}

int CanvasPath::getFillType() {
return static_cast<int>(path_.getFillType());
return static_cast<int>(tracked_path_->path_.getFillType());
Copy link
Member

@jason-simmons jason-simmons Nov 21, 2020

Choose a reason for hiding this comment

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

Use path() to access the SkPath in these methods. Add a private non-const mutable_path() accessor for methods that modify the path.

}

void CanvasPath::setFillType(int fill_type) {
path_.setFillType(static_cast<SkPathFillType>(fill_type));
tracked_path_->path_.setFillType(static_cast<SkPathFillType>(fill_type));
resetVolatility();
}

void CanvasPath::moveTo(float x, float y) {
path_.moveTo(x, y);
tracked_path_->path_.moveTo(x, y);
resetVolatility();
}

void CanvasPath::relativeMoveTo(float x, float y) {
path_.rMoveTo(x, y);
tracked_path_->path_.rMoveTo(x, y);
resetVolatility();
}

void CanvasPath::lineTo(float x, float y) {
path_.lineTo(x, y);
tracked_path_->path_.lineTo(x, y);
resetVolatility();
}

void CanvasPath::relativeLineTo(float x, float y) {
path_.rLineTo(x, y);
tracked_path_->path_.rLineTo(x, y);
resetVolatility();
}

void CanvasPath::quadraticBezierTo(float x1, float y1, float x2, float y2) {
path_.quadTo(x1, y1, x2, y2);
tracked_path_->path_.quadTo(x1, y1, x2, y2);
resetVolatility();
}

void CanvasPath::relativeQuadraticBezierTo(float x1,
float y1,
float x2,
float y2) {
path_.rQuadTo(x1, y1, x2, y2);
tracked_path_->path_.rQuadTo(x1, y1, x2, y2);
resetVolatility();
}

void CanvasPath::cubicTo(float x1,
Expand All @@ -112,7 +161,8 @@ void CanvasPath::cubicTo(float x1,
float y2,
float x3,
float y3) {
path_.cubicTo(x1, y1, x2, y2, x3, y3);
tracked_path_->path_.cubicTo(x1, y1, x2, y2, x3, y3);
resetVolatility();
}

void CanvasPath::relativeCubicTo(float x1,
Expand All @@ -121,19 +171,22 @@ void CanvasPath::relativeCubicTo(float x1,
float y2,
float x3,
float y3) {
path_.rCubicTo(x1, y1, x2, y2, x3, y3);
tracked_path_->path_.rCubicTo(x1, y1, x2, y2, x3, y3);
resetVolatility();
}

void CanvasPath::conicTo(float x1, float y1, float x2, float y2, float w) {
path_.conicTo(x1, y1, x2, y2, w);
tracked_path_->path_.conicTo(x1, y1, x2, y2, w);
resetVolatility();
}

void CanvasPath::relativeConicTo(float x1,
float y1,
float x2,
float y2,
float w) {
path_.rConicTo(x1, y1, x2, y2, w);
tracked_path_->path_.rConicTo(x1, y1, x2, y2, w);
resetVolatility();
}

void CanvasPath::arcTo(float left,
Expand All @@ -143,9 +196,10 @@ void CanvasPath::arcTo(float left,
float startAngle,
float sweepAngle,
bool forceMoveTo) {
path_.arcTo(SkRect::MakeLTRB(left, top, right, bottom),
startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI,
forceMoveTo);
tracked_path_->path_.arcTo(SkRect::MakeLTRB(left, top, right, bottom),
startAngle * 180.0 / M_PI,
sweepAngle * 180.0 / M_PI, forceMoveTo);
resetVolatility();
}

void CanvasPath::arcToPoint(float arcEndX,
Expand All @@ -160,8 +214,9 @@ void CanvasPath::arcToPoint(float arcEndX,
const auto direction =
isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW;

path_.arcTo(radiusX, radiusY, xAxisRotation, arcSize, direction, arcEndX,
arcEndY);
tracked_path_->path_.arcTo(radiusX, radiusY, xAxisRotation, arcSize,
direction, arcEndX, arcEndY);
resetVolatility();
}

void CanvasPath::relativeArcToPoint(float arcEndDeltaX,
Expand All @@ -175,16 +230,19 @@ void CanvasPath::relativeArcToPoint(float arcEndDeltaX,
: SkPath::ArcSize::kSmall_ArcSize;
const auto direction =
isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW;
path_.rArcTo(radiusX, radiusY, xAxisRotation, arcSize, direction,
arcEndDeltaX, arcEndDeltaY);
tracked_path_->path_.rArcTo(radiusX, radiusY, xAxisRotation, arcSize,
direction, arcEndDeltaX, arcEndDeltaY);
resetVolatility();
}

void CanvasPath::addRect(float left, float top, float right, float bottom) {
path_.addRect(SkRect::MakeLTRB(left, top, right, bottom));
tracked_path_->path_.addRect(SkRect::MakeLTRB(left, top, right, bottom));
resetVolatility();
}

void CanvasPath::addOval(float left, float top, float right, float bottom) {
path_.addOval(SkRect::MakeLTRB(left, top, right, bottom));
tracked_path_->path_.addOval(SkRect::MakeLTRB(left, top, right, bottom));
resetVolatility();
}

void CanvasPath::addArc(float left,
Expand All @@ -193,25 +251,31 @@ void CanvasPath::addArc(float left,
float bottom,
float startAngle,
float sweepAngle) {
path_.addArc(SkRect::MakeLTRB(left, top, right, bottom),
startAngle * 180.0 / M_PI, sweepAngle * 180.0 / M_PI);
tracked_path_->path_.addArc(SkRect::MakeLTRB(left, top, right, bottom),
startAngle * 180.0 / M_PI,
sweepAngle * 180.0 / M_PI);
resetVolatility();
}

void CanvasPath::addPolygon(const tonic::Float32List& points, bool close) {
path_.addPoly(reinterpret_cast<const SkPoint*>(points.data()),
points.num_elements() / 2, close);
tracked_path_->path_.addPoly(reinterpret_cast<const SkPoint*>(points.data()),
points.num_elements() / 2, close);
resetVolatility();
}

void CanvasPath::addRRect(const RRect& rrect) {
path_.addRRect(rrect.sk_rrect);
tracked_path_->path_.addRRect(rrect.sk_rrect);
resetVolatility();
}

void CanvasPath::addPath(CanvasPath* path, double dx, double dy) {
if (!path) {
Dart_ThrowException(ToDart("Path.addPath called with non-genuine Path."));
return;
}
path_.addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode);
tracked_path_->path_.addPath(path->path(), dx, dy,
SkPath::kAppend_AddPathMode);
resetVolatility();
}

void CanvasPath::addPathWithMatrix(CanvasPath* path,
Expand All @@ -227,8 +291,10 @@ void CanvasPath::addPathWithMatrix(CanvasPath* path,
SkMatrix matrix = ToSkMatrix(matrix4);
matrix.setTranslateX(matrix.getTranslateX() + dx);
matrix.setTranslateY(matrix.getTranslateY() + dy);
path_.addPath(path->path(), matrix, SkPath::kAppend_AddPathMode);
tracked_path_->path_.addPath(path->path(), matrix,
SkPath::kAppend_AddPathMode);
matrix4.Release();
resetVolatility();
}

void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) {
Expand All @@ -237,7 +303,9 @@ void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) {
ToDart("Path.extendWithPath called with non-genuine Path."));
return;
}
path_.addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode);
path->tracked_path_->path_.addPath(path->path(), dx, dy,
SkPath::kExtend_AddPathMode);
resetVolatility();
}

void CanvasPath::extendWithPathAndMatrix(CanvasPath* path,
Expand All @@ -253,37 +321,44 @@ void CanvasPath::extendWithPathAndMatrix(CanvasPath* path,
SkMatrix matrix = ToSkMatrix(matrix4);
matrix.setTranslateX(matrix.getTranslateX() + dx);
matrix.setTranslateY(matrix.getTranslateY() + dy);
path_.addPath(path->path(), matrix, SkPath::kExtend_AddPathMode);
path->tracked_path_->path_.addPath(path->path(), matrix,
SkPath::kExtend_AddPathMode);
matrix4.Release();
resetVolatility();
}

void CanvasPath::close() {
path_.close();
tracked_path_->path_.close();
resetVolatility();
}

void CanvasPath::reset() {
path_.reset();
tracked_path_->path_.reset();
resetVolatility();
}

bool CanvasPath::contains(double x, double y) {
return path_.contains(x, y);
return tracked_path_->path_.contains(x, y);
}

void CanvasPath::shift(Dart_Handle path_handle, double dx, double dy) {
fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
path_.offset(dx, dy, &path->path_);
tracked_path_->path_.offset(dx, dy, &path->tracked_path_->path_);
resetVolatility();
}

void CanvasPath::transform(Dart_Handle path_handle,
tonic::Float64List& matrix4) {
fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
path_.transform(ToSkMatrix(matrix4), &path->path_);
tracked_path_->path_.transform(ToSkMatrix(matrix4),
&path->tracked_path_->path_);
matrix4.Release();
resetVolatility();
Copy link
Contributor

@liyuqian liyuqian Nov 28, 2020

Choose a reason for hiding this comment

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

This method does not seem to mutate the current path. Can we do the following?

  path().transform(ToSkMatrix(matrix4), &other_mutable_path);
  matrix4.Release();	  matrix4.Release();
  // resetVolatility() is not needed.

Copy link
Contributor Author

@dnfield dnfield Nov 30, 2020

Choose a reason for hiding this comment

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

Ahh it mutates the new path, got it. Done.

}

tonic::Float32List CanvasPath::getBounds() {
tonic::Float32List rect(Dart_NewTypedData(Dart_TypedData_kFloat32, 4));
const SkRect& bounds = path_.getBounds();
const SkRect& bounds = tracked_path_->path_.getBounds();
rect[0] = bounds.left();
rect[1] = bounds.top();
rect[2] = bounds.right();
Expand All @@ -293,21 +368,22 @@ tonic::Float32List CanvasPath::getBounds() {

bool CanvasPath::op(CanvasPath* path1, CanvasPath* path2, int operation) {
return Op(path1->path(), path2->path(), static_cast<SkPathOp>(operation),
&path_);
&tracked_path_->path_);
resetVolatility();
}

void CanvasPath::clone(Dart_Handle path_handle) {
fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
// per Skia docs, this will create a fast copy
// data is shared until the source path or dest path are mutated
path->path_ = path_;
path->tracked_path_->path_ = tracked_path_->path_;
}

// This is doomed to be called too early, since Paths are mutable.
// However, it can help for some of the clone/shift/transform type methods
// where the resultant path will initially have a meaningful size.
size_t CanvasPath::GetAllocationSize() const {
return sizeof(CanvasPath) + path_.approximateBytesUsed();
return sizeof(CanvasPath) + tracked_path_->path_.approximateBytesUsed();
}

} // namespace flutter
32 changes: 29 additions & 3 deletions lib/ui/painting/path.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
#ifndef FLUTTER_LIB_UI_PAINTING_PATH_H_
#define FLUTTER_LIB_UI_PAINTING_PATH_H_

#include <mutex>
#include <unordered_set>

#include "flutter/lib/ui/dart_wrapper.h"
#include "flutter/lib/ui/painting/rrect.h"
#include "third_party/skia/include/core/SkPath.h"
Expand Down Expand Up @@ -36,7 +39,7 @@ class CanvasPath : public RefCountedDartWrappable<CanvasPath> {
static fml::RefPtr<CanvasPath> CreateFrom(Dart_Handle path_handle,
const SkPath& src) {
fml::RefPtr<CanvasPath> path = CanvasPath::Create(path_handle);
path->path_ = src;
path->tracked_path_->path_ = src;
Copy link
Contributor

@liyuqian liyuqian Nov 28, 2020

Choose a reason for hiding this comment

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

Nit: path->tracked_path_->path_ -> path->mutable_path()

Copy link
Contributor Author

@dnfield dnfield Nov 30, 2020

Choose a reason for hiding this comment

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

Done.

return path;
}

Expand Down Expand Up @@ -108,16 +111,39 @@ class CanvasPath : public RefCountedDartWrappable<CanvasPath> {
bool op(CanvasPath* path1, CanvasPath* path2, int operation);
void clone(Dart_Handle path_handle);

const SkPath& path() const { return path_; }
const SkPath& path() const { return tracked_path_->path_; }

size_t GetAllocationSize() const override;

static void RegisterNatives(tonic::DartLibraryNatives* natives);

// Called by the shell at the end of a frame after notifying Dart about idle
// time.
//
// This method will flip the volatility bit to false for any paths that have
// survived the |number_of_frames_until_non_volatile|.
static void updatePathVolatility();

virtual void ReleaseDartWrappableReference() const override;

private:
struct TrackedVolatilePath {
bool tracking_volatility = false;
int frame_count = 0;
SkPath path_;
};

CanvasPath();

SkPath path_;
static constexpr int number_of_frames_until_non_volatile = 2;
static std::mutex volatile_paths_mutex_;
static std::unordered_set<std::shared_ptr<TrackedVolatilePath>>
Copy link
Member

@jason-simmons jason-simmons Nov 21, 2020

Choose a reason for hiding this comment

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

Can this be moved to an object that lives at Shell scope and can be accessed through the UIDataState?
(similar to something like IOManager)

That would have some advantages:

  • no need for a mutex if this object is only accessed on the UI thread
  • if multiple engine instances exist, then one engine's NotifyIdle will not affect other engines' paths
  • paths in the set will be cleaned up when the shell is deleted

Copy link
Contributor Author

@dnfield dnfield Nov 21, 2020

Choose a reason for hiding this comment

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

That SGTM, but I think we'd still need the mutex because the erase call could come from a Dart VM worker thread.

Copy link
Contributor Author

@dnfield dnfield Nov 21, 2020

Choose a reason for hiding this comment

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

Although I suppose we could just do a post task for it...

Copy link
Contributor Author

@dnfield dnfield Nov 21, 2020

Choose a reason for hiding this comment

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

The problem with this will be we don't know if we're in isolate scope or not when a path gets collected.

Copy link
Contributor Author

@dnfield dnfield Nov 21, 2020

Choose a reason for hiding this comment

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

Ok - I made this more or less work by sharing the task runner and just grabbing out the tracker when we create the path, at wihch point we'll always have isolate scope.

I think this should be correct.

volatile_paths_;

std::shared_ptr<TrackedVolatilePath> tracked_path_;

// Must be called whenever the path is created or mutated.
void resetVolatility();
};

} // namespace flutter
Expand Down
Loading