Skip to content

Commit

Permalink
Reland path volatility tracker (flutter#23063)
Browse files Browse the repository at this point in the history 
* Revert "Revert "Set SkPath::setIsVolatile based on whether the path survives at least two frames (flutter#22620)" (flutter#23044)"

This reverts commit 4f91425.

* Fix tracing
  • Loading branch information
@dnfield
dnfield authored Dec 15, 2020
1 parent f37c8c5 commit 205d2b8
Show file tree
Hide file tree
Showing 28 changed files with 606 additions and 121 deletions.
3 changes: 3 additions & 0 deletions ci/licenses_golden/licenses_flutter
View file
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
13 changes: 13 additions & 0 deletions fml/trace_event.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,19 @@
::fml::tracing::TraceCounter((category_group), (name), (counter_id), (arg1), \
__VA_ARGS__);

// Avoid using the same `name` and `argX_name` for nested traces, which can
// lead to double free errors. E.g. the following code should be avoided:
//
// ```cpp
// {
// TRACE_EVENT1("flutter", "Foo::Bar", "count", "initial_count_value");
// ...
// TRACE_EVENT_INSTANT1("flutter", "Foo::Bar",
// "count", "updated_count_value");
// }
// ```
//
// Instead, either use different `name` or `arg1` parameter names.
#define FML_TRACE_EVENT(category_group, name, ...) \
::fml::tracing::TraceEvent((category_group), (name), __VA_ARGS__); \
__FML__AUTO_TRACE_END(name)
Expand Down
3 changes: 3 additions & 0 deletions lib/ui/BUILD.gn
View file
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
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
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() {
if (!tracked_path_->tracking_volatility) {
mutable_path().setIsVolatile(true);
tracked_path_->frame_count = 0;
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
Loading

0 comments on commit 205d2b8

Please sign in to comment.