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 6 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
2 changes: 2 additions & 0 deletions ci/licenses_golden/licenses_flutter
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,8 @@ FILE: ../../../flutter/lib/ui/ui.dart
FILE: ../../../flutter/lib/ui/ui_benchmarks.cc
FILE: ../../../flutter/lib/ui/ui_dart_state.cc
FILE: ../../../flutter/lib/ui/ui_dart_state.h
FILE: ../../../flutter/lib/ui/volatile_path_tracker.cc
FILE: ../../../flutter/lib/ui/volatile_path_tracker.h
FILE: ../../../flutter/lib/ui/window.dart
FILE: ../../../flutter/lib/ui/window/platform_configuration.cc
FILE: ../../../flutter/lib/ui/window/platform_configuration.h
Expand Down
2 changes: 2 additions & 0 deletions lib/ui/BUILD.gn
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,8 @@ source_set("ui") {
"text/text_box.h",
"ui_dart_state.cc",
"ui_dart_state.h",
"volatile_path_tracker.cc",
"volatile_path_tracker.h",
"window/platform_configuration.cc",
"window/platform_configuration.h",
"window/platform_message.cc",
Expand Down
132 changes: 91 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,68 @@ void CanvasPath::RegisterNatives(tonic::DartLibraryNatives* natives) {
FOR_EACH_BINDING(DART_REGISTER_NATIVE)});
}

CanvasPath::CanvasPath() {}
CanvasPath::CanvasPath()
: path_tracker_(UIDartState::Current()->GetVolatilePathTracker()),
tracked_path_(std::make_shared<VolatilePathTracker::Path>()) {
FML_DCHECK(path_tracker_);
resetVolatility();
}

CanvasPath::~CanvasPath() = default;

void CanvasPath::resetVolatility() {

@liyuqian liyuqian Nov 28, 2020

Copy link
Copy Markdown
Contributor

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.

@dnfield dnfield Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

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) {
mutable_path().setIsVolatile(true);
tracked_path_->tracking_volatility = true;
path_tracker_->insert(tracked_path_);
}
}

CanvasPath::~CanvasPath() {}
void CanvasPath::ReleaseDartWrappableReference() const {
FML_DCHECK(path_tracker_);
path_tracker_->erase(tracked_path_);

@liyuqian liyuqian Nov 28, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we do the following?

if (tracked_path_->tracking_volatility) {
  path_tracker_->erase(tracked_path_);
}

This seems to be able to save some fml::TaskRunner::RunNowOrPostTask calls when the path is non-volatile.

@dnfield dnfield Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nice, done

}

int CanvasPath::getFillType() {
return static_cast<int>(path_.getFillType());
return static_cast<int>(path().getFillType());
}

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

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

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

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

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

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

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

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

void CanvasPath::relativeCubicTo(float x1,
Expand All @@ -121,19 +147,22 @@ void CanvasPath::relativeCubicTo(float x1,
float y2,
float x3,
float y3) {
path_.rCubicTo(x1, y1, x2, y2, x3, y3);
mutable_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);
mutable_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);
mutable_path().rConicTo(x1, y1, x2, y2, w);
resetVolatility();
}

void CanvasPath::arcTo(float left,
Expand All @@ -143,9 +172,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);
mutable_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 +190,9 @@ void CanvasPath::arcToPoint(float arcEndX,
const auto direction =
isClockwiseDirection ? SkPathDirection::kCW : SkPathDirection::kCCW;

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

void CanvasPath::relativeArcToPoint(float arcEndDeltaX,
Expand All @@ -175,16 +206,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);
mutable_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));
mutable_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));
mutable_path().addOval(SkRect::MakeLTRB(left, top, right, bottom));
resetVolatility();
}

void CanvasPath::addArc(float left,
Expand All @@ -193,25 +227,29 @@ 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);
mutable_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);
mutable_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);
mutable_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);
mutable_path().addPath(path->path(), dx, dy, SkPath::kAppend_AddPathMode);
resetVolatility();
}

void CanvasPath::addPathWithMatrix(CanvasPath* path,
Expand All @@ -227,8 +265,9 @@ 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);
mutable_path().addPath(path->path(), matrix, SkPath::kAppend_AddPathMode);
matrix4.Release();
resetVolatility();
}

void CanvasPath::extendWithPath(CanvasPath* path, double dx, double dy) {
Expand All @@ -237,7 +276,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->mutable_path().addPath(path->path(), dx, dy,
SkPath::kExtend_AddPathMode);
resetVolatility();
}

void CanvasPath::extendWithPathAndMatrix(CanvasPath* path,
Expand All @@ -253,37 +294,45 @@ 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->mutable_path().addPath(path->path(), matrix,
SkPath::kExtend_AddPathMode);
matrix4.Release();
resetVolatility();
}

void CanvasPath::close() {
path_.close();
mutable_path().close();
resetVolatility();
}

void CanvasPath::reset() {
path_.reset();
mutable_path().reset();
resetVolatility();
}

bool CanvasPath::contains(double x, double y) {
return path_.contains(x, y);
return 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_);
auto other_mutable_path = path->mutable_path();

@jason-simmons jason-simmons Dec 1, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this should be auto&

@dnfield dnfield Dec 1, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You are right - adding a test to cover this

mutable_path().offset(dx, dy, &other_mutable_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_);
auto other_mutable_path = path->mutable_path();
mutable_path().transform(ToSkMatrix(matrix4), &other_mutable_path);
matrix4.Release();
resetVolatility();

@liyuqian liyuqian Nov 28, 2020

Copy link
Copy Markdown
Contributor

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.

@dnfield dnfield Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

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 = path().getBounds();
rect[0] = bounds.left();
rect[1] = bounds.top();
rect[2] = bounds.right();
Expand All @@ -293,21 +342,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_ = this->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) + path().approximateBytesUsed();
}

} // namespace flutter
15 changes: 12 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 @@ -7,6 +7,7 @@

#include "flutter/lib/ui/dart_wrapper.h"
#include "flutter/lib/ui/painting/rrect.h"
#include "flutter/lib/ui/volatile_path_tracker.h"
#include "third_party/skia/include/core/SkPath.h"
#include "third_party/skia/include/pathops/SkPathOps.h"
#include "third_party/tonic/typed_data/typed_list.h"
Expand Down Expand Up @@ -36,7 +37,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;

@liyuqian liyuqian Nov 28, 2020

Copy link
Copy Markdown
Contributor

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()

@dnfield dnfield Nov 30, 2020

Copy link
Copy Markdown
Contributor Author

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 +109,24 @@ 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);

virtual void ReleaseDartWrappableReference() const override;

private:
CanvasPath();

SkPath path_;
std::shared_ptr<VolatilePathTracker> path_tracker_;
std::shared_ptr<VolatilePathTracker::Path> tracked_path_;

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

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

} // namespace flutter
Expand Down
9 changes: 8 additions & 1 deletion lib/ui/ui_dart_state.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ UIDartState::UIDartState(
std::string logger_prefix,
UnhandledExceptionCallback unhandled_exception_callback,
std::shared_ptr<IsolateNameServer> isolate_name_server,
bool is_root_isolate)
bool is_root_isolate,
std::shared_ptr<VolatilePathTracker> volatile_path_tracker)
: task_runners_(std::move(task_runners)),
add_callback_(std::move(add_callback)),
remove_callback_(std::move(remove_callback)),
Expand All @@ -36,6 +37,7 @@ UIDartState::UIDartState(
io_manager_(std::move(io_manager)),
skia_unref_queue_(std::move(skia_unref_queue)),
image_decoder_(std::move(image_decoder)),
volatile_path_tracker_(std::move(volatile_path_tracker)),
advisory_script_uri_(std::move(advisory_script_uri)),
advisory_script_entrypoint_(std::move(advisory_script_entrypoint)),
logger_prefix_(std::move(logger_prefix)),
Expand Down Expand Up @@ -106,6 +108,11 @@ fml::RefPtr<flutter::SkiaUnrefQueue> UIDartState::GetSkiaUnrefQueue() const {
return skia_unref_queue_;
}

std::shared_ptr<VolatilePathTracker> UIDartState::GetVolatilePathTracker()
const {
return volatile_path_tracker_;
}

void UIDartState::ScheduleMicrotask(Dart_Handle closure) {
if (tonic::LogIfError(closure) || !Dart_IsClosure(closure)) {
return;
Expand Down
Loading