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

separate saveLayer events into record and execute variants and trace more of the execution calls #29598

Merged
fluttergithubbot merged 2 commits into from
Nov 10, 2021
Merged

separate saveLayer events into record and execute variants and trace more of the execution calls #29598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
370d76c
separate record and execute saveLayer events and trace more saveLayer…
flar Nov 9, 2021
4fb97e9
review feedback, trace savePaint calls that use saveLayer
flar Nov 9, 2021
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
19 changes: 16 additions & 3 deletions flow/display_list_canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ void DisplayListCanvasDispatcher::restore() {
}
void DisplayListCanvasDispatcher::saveLayer(const SkRect* bounds,
bool restore_with_paint) {
TRACE_EVENT0("flutter", "Canvas::saveLayer");
canvas_->saveLayer(bounds, restore_with_paint ? &paint() : nullptr);
}

Expand Down Expand Up @@ -76,7 +77,14 @@ void DisplayListCanvasDispatcher::clipPath(const SkPath& path,
}

void DisplayListCanvasDispatcher::drawPaint() {
canvas_->drawPaint(paint());
const SkPaint& sk_paint = paint();
SkImageFilter* filter = sk_paint.getImageFilter();
if (filter && !filter->asColorFilter(nullptr)) {
// drawPaint does an implicit saveLayer if an SkImageFilter is
// present that cannot be replaced by an SkColorFilter.
TRACE_EVENT0("flutter", "Canvas::saveLayer");
}
canvas_->drawPaint(sk_paint);
}
void DisplayListCanvasDispatcher::drawColor(SkColor color, SkBlendMode mode) {
canvas_->drawColor(color, mode);
Expand Down Expand Up @@ -170,8 +178,13 @@ void DisplayListCanvasDispatcher::drawAtlas(const sk_sp<SkImage> atlas,
void DisplayListCanvasDispatcher::drawPicture(const sk_sp<SkPicture> picture,
const SkMatrix* matrix,
bool render_with_attributes) {
canvas_->drawPicture(picture, matrix,
render_with_attributes ? &paint() : nullptr);
if (render_with_attributes) {
// drawPicture does an implicit saveLayer if an SkPaint is supplied.
TRACE_EVENT0("flutter", "Canvas::saveLayer");
canvas_->drawPicture(picture, matrix, &paint());
} else {
canvas_->drawPicture(picture, matrix, nullptr);
}
Copy link
Contributor

@dnfield dnfield Nov 9, 2021

Choose a reason for hiding this comment

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

We may also want to capture when drawPaint and draw color will insert a save layer.

Copy link
Contributor Author

@flar flar Nov 9, 2021

Choose a reason for hiding this comment

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

It looks like there is a public way to determine if the drawPaint will use a saveLayer so I'm adding that. (It's only when there is an ImageFilter, though, so it probably won't happen much in practice.)

drawColor doesn't show any signs of using a saveLayer, though. The operation becomes a drawPaint with no ImageFilter which never generates a saveLayer in the Skia code.

Copy link
Contributor Author

@flar flar Nov 9, 2021

Choose a reason for hiding this comment

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

I should probably also mark it as recorded when I record the drawPaint. Should that be the same event name as when we record saveLayer? Or modified slightly - like "(drawPaint Recorded)" to distinguish it so that developers can know to look for a drawPaint call instead of a saveLayer. Or "ui.Canvas::drawPaint (Recorded needing saveLayer)" or something?

Copy link
Contributor

@dnfield dnfield Nov 9, 2021

Choose a reason for hiding this comment

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

Hmm. I know the method name has something about ImageFilters in its return type, but when I was tracing through the code it looked like the surface could decide to trigger a savelayer with or without an image filter... Specifically I'm looking at these sites:

https://github.com/google/skia/blob/6a5f772ea6f0f689b32e9dbdb78f6a24963f524a/src/core/SkCanvas.cpp#L1750 eventually ends up in https://github.com/google/skia/blob/6a5f772ea6f0f689b32e9dbdb78f6a24963f524a/src/core/SkCanvas.cpp#L170 and then https://github.com/google/skia/blob/6a5f772ea6f0f689b32e9dbdb78f6a24963f524a/src/core/SkCanvas.cpp#L170

What's the part that I'm missing?

Copy link
Contributor Author

@flar flar Nov 9, 2021

Choose a reason for hiding this comment

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

Those last 2 lines are the same. The one that determines whether to add a saveLayer is:

https://github.com/google/skia/blob/6a5f772ea6f0f689b32e9dbdb78f6a24963f524a/src/core/SkCanvas.cpp#L311

And that's the one I model in the (new) code.

Copy link
Contributor

@dnfield dnfield Nov 9, 2021

Choose a reason for hiding this comment

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

Sorry, tha tlast link was supposed to be https://github.com/google/skia/blob/main/src/image/SkSurface.cpp#L120

But now I see the part you linked and it all makes sense. Ok.

Copy link
Contributor Author

@flar flar Nov 9, 2021

Choose a reason for hiding this comment

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

Yeah, I think that call can reject the operation entirely, but the actual saveLayer happens in their ...Auto... object.

}
void DisplayListCanvasDispatcher::drawDisplayList(
const sk_sp<DisplayList> display_list) {
Expand Down
2 changes: 2 additions & 0 deletions flow/layers/layer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ Layer::AutoSaveLayer::AutoSaveLayer(const PaintContext& paint_context,
canvas_(save_mode == SaveMode::kInternalNodesCanvas
? *(paint_context.internal_nodes_canvas)
: *(paint_context.leaf_nodes_canvas)) {
TRACE_EVENT0("flutter", "Canvas::saveLayer");
canvas_.saveLayer(bounds_, paint);
}

Expand All @@ -76,6 +77,7 @@ Layer::AutoSaveLayer::AutoSaveLayer(const PaintContext& paint_context,
canvas_(save_mode == SaveMode::kInternalNodesCanvas
? *(paint_context.internal_nodes_canvas)
: *(paint_context.leaf_nodes_canvas)) {
TRACE_EVENT0("flutter", "Canvas::saveLayer");
canvas_.saveLayer(layer_rec);
}

Expand Down
11 changes: 9 additions & 2 deletions lib/ui/painting/canvas.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ void Canvas::saveLayerWithoutBounds(const Paint& paint,
if (!canvas_) {
return;
}
TRACE_EVENT0("flutter", "Canvas::saveLayer");
TRACE_EVENT0("flutter", "ui.Canvas::saveLayer (Recorded)");
canvas_->saveLayer(nullptr, paint.paint());
}

Expand All @@ -118,7 +118,7 @@ void Canvas::saveLayer(double left,
if (!canvas_) {
return;
}
TRACE_EVENT0("flutter", "Canvas::saveLayer");
TRACE_EVENT0("flutter", "ui.Canvas::saveLayer (Recorded)");
SkRect bounds = SkRect::MakeLTRB(left, top, right, bottom);
canvas_->saveLayer(&bounds, paint.paint());
}
Expand Down Expand Up @@ -230,6 +230,13 @@ void Canvas::drawPaint(const Paint& paint, const PaintData& paint_data) {
if (!canvas_) {
return;
}
const SkPaint* sk_paint = paint.paint();
SkImageFilter* filter = sk_paint->getImageFilter();
if (filter && !filter->asColorFilter(nullptr)) {
// drawPaint does an implicit saveLayer if an SkImageFilter is
// present that cannot be replaced by an SkColorFilter.
TRACE_EVENT0("flutter", "ui.Canvas::saveLayer (Recorded)");
}
canvas_->drawPaint(*paint.paint());
}

Expand Down
19 changes: 14 additions & 5 deletions testing/dart/observatory/tracing_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,11 @@ void main() {
);

final Completer<void> completer = Completer<void>();
window.onBeginFrame = (Duration timeStamp) {
window.onBeginFrame = (Duration timeStamp) async {
final PictureRecorder recorder = PictureRecorder();
final Canvas canvas = Canvas(recorder);
canvas.drawColor(const Color(0xff0000ff), BlendMode.srcOut);
canvas.drawPaint(Paint()..imageFilter = ImageFilter.blur(sigmaX: 2, sigmaY: 3));
canvas.saveLayer(null, Paint());
canvas.saveLayer(const Rect.fromLTWH(0, 0, 100, 100), Paint());
canvas.drawRect(const Rect.fromLTRB(10, 10, 20, 20), Paint());
Expand All @@ -37,7 +39,7 @@ void main() {
builder.addPicture(Offset.zero, picture);
final Scene scene = builder.build();

window.render(scene);
await scene.toImage(100, 100);
Copy link
Contributor

@dnfield dnfield Nov 9, 2021

Choose a reason for hiding this comment

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

nit: add a comment about why we're doing this

scene.dispose();
completer.complete();
};
Expand All @@ -47,13 +49,20 @@ void main() {
final vms.Timeline timeline = await vmService.getVMTimeline();
await vmService.dispose();

int saveLayerRecordCount = 0;
int saveLayerCount = 0;
for (final vms.TimelineEvent event in timeline.traceEvents!) {
final Map<String, dynamic> json = event.json!;
if (json['name'] == 'Canvas::saveLayer' && json['ph'] == 'B') {
saveLayerCount += 1;
if (json['ph'] == 'B') {
if (json['name'] == 'ui.Canvas::saveLayer (Recorded)') {
saveLayerRecordCount += 1;
}
if (json['name'] == 'Canvas::saveLayer') {
saveLayerCount += 1;
}
}
}
expect(saveLayerCount, 2);
expect(saveLayerRecordCount, 3);
expect(saveLayerCount, 3);
});
}