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 all 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
3 changes: 3 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 @@ -361,6 +361,7 @@ FILE: ../../../flutter/lib/ui/painting/path.cc
FILE: ../../../flutter/lib/ui/painting/path.h
FILE: ../../../flutter/lib/ui/painting/path_measure.cc
FILE: ../../../flutter/lib/ui/painting/path_measure.h
FILE: ../../../flutter/lib/ui/painting/path_unittests.cc
FILE: ../../../flutter/lib/ui/painting/picture.cc
FILE: ../../../flutter/lib/ui/painting/picture.h
FILE: ../../../flutter/lib/ui/painting/picture_recorder.cc
Expand Down Expand Up @@ -404,6 +405,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
3 changes: 3 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 Expand Up @@ -188,6 +190,7 @@ if (enable_unittests) {
sources = [
"painting/image_dispose_unittests.cc",
"painting/image_encoding_unittests.cc",
"painting/path_unittests.cc",
"painting/vertices_unittests.cc",
"window/platform_configuration_unittests.cc",
"window/pointer_data_packet_converter_unittests.cc",
Expand Down
13 changes: 12 additions & 1 deletion lib/ui/fixtures/ui_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,20 @@ void createVertices() {
);
_validateVertices(vertices);
}

void _validateVertices(Vertices vertices) native 'ValidateVertices';

@pragma('vm:entry-point')
void createPath() {
final Path path = Path()..lineTo(10, 10);
_validatePath(path);
// Arbitrarily hold a reference to the path to make sure it does not get
// garbage collected.
Future<void>.delayed(const Duration(days: 100)).then((_) {
path.lineTo(100, 100);
});
}
void _validatePath(Path path) native 'ValidatePath';

@pragma('vm:entry-point')
void frameCallback(FrameInfo info) {
print('called back');
Expand Down
130 changes: 89 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,69 @@ 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::TrackedPath>()) {
FML_DCHECK(path_tracker_);
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) {
mutable_path().setIsVolatile(true);
tracked_path_->frame_count = 0;
Copy link
Member

@chinmaygarde chinmaygarde Dec 10, 2020

Choose a reason for hiding this comment

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

Seems redundant.

Copy link
Contributor Author

@dnfield dnfield Dec 10, 2020

Choose a reason for hiding this comment

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

If this path had previously been marked as non-volatile, but now has been mutated and is volatile again, we have to reset the frame count so that we don't immediately mark it volatile if it is only drawn for e.g. 1 frame.

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_);
}

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 +138,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 +148,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 +173,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 +191,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 +207,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 +228,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 +266,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 +277,8 @@ 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);
mutable_path().addPath(path->path(), dx, dy, SkPath::kExtend_AddPathMode);
resetVolatility();
}

void CanvasPath::extendWithPathAndMatrix(CanvasPath* path,
Expand All @@ -253,37 +294,43 @@ 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);
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();
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();
}

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 +340,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->mutable_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;
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::TrackedPath> 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
Loading