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

[flar]

This PR builds upon the earlier fix to start tracing saveLayer calls by separating the instances where we are recording a saveLayer during the build stage (which might help developers locate where these calls are coming from) from the instances where we are executing them while rasterizing a frame.

It also adds 3 more locations where we use saveLayer for better tracking:

• saveLayer calls embedded in DisplayLists
• saveLayer calls from the AutoSaveLayer mechanism
• saveLayer calls implicitly used by Skia during a drawPicture(..., paint) call

[dnfield]

You'll need to do something to flush the raster task runner or this will be flaky. You can, for example, await scene.toImage.

[flar]

Trying that.

[dnfield]

I'm not quite clear on the value of recording the display list operations, as opposed to recording the skia operations the gpu backend - specifically, OpsTask::onExecute

[dnfield]

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

[flar]

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.

[flar]

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?

[dnfield]

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?

[flar]

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.

[dnfield]

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.

[flar]

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

[flar]

I'm not quite clear on the value of recording the display list operations, as opposed to recording the skia operations the gpu backend - specifically, OpsTask::onExecute

How is that any different than the events added anywhere else in the layer painting code? A saveLayer (executed on a rendering SkCanvas) is a saveLayer wherever it happens. Are you suggesting that we delete all of the saveLayer events in both the layers and DL?

[dnfield]

I'm saying that these traces give a more accurate picture of where DisplayList is actually sending a saveLayer command to Skia, but not necessarily which of those (or whether other commands) result in a render target switch in the end.

What I'm hoping to expose to developers is when they explicitly ask for something that will result in a render target switch, and how many times Skia actually does something on the GPU that is a render target switch. When the DL calls saveLayer is probably also helpful, but more so to Flutter engine developers rather than application developers.

[flar]

I'm saying that these traces give a more accurate picture of where DisplayList is actually sending a saveLayer command to Skia, but not necessarily which of those (or whether other commands) result in a render target switch in the end.

What I'm hoping to expose to developers is when they explicitly ask for something that will result in a render target switch, and how many times Skia actually does something on the GPU that is a render target switch. When the DL calls saveLayer is probably also helpful, but more so to Flutter engine developers rather than application developers.

The calls in canvas.cc will show them when they request it. All of the other calls in all of the other files have the same impact - they are when our rendering code is requesting a saveLayer during rendering of the layer. If you only want them to know when they request it, we can rip out all of the code that isn't in canvas.cc then.

But, your original PR said that you want developers to also know how many saveLayer calls happened in the long frames, Since long frames aren't 1:1 with built frames then that speaks to a need for tracing in the rendering phase to show all of the places where saveLayer is called. Another reason is that a frame can be built with 10 saveLayer calls that happen inside ui.Canvas calls, but introduce 15 or 20 or 6000 saveLayers during the rendering. The number of saveLayers that happen during the build of a frame is independent of the number of saveLayers that happen when that exact same frame is rendered. For example:

• retained layers are not rebuilt and so do not result in any calls to ui.Canvas, but they can have saveLayers
• layers can be built that will be culled at a high level by the engine and so the saveLayers they invoke in ui.Canvas may never happen
• scenes can be built that will be dropped on the floor
• scenes can be built that will be re-rendered in some cases (wherever last_layer_tree is used, but I think that is only when we take a snapshot? The rasterization cycle can also be told to resubmit a frame which means it could be rendered twice due to internal conditions.)

[flar]

In the latest commit I added the toImage to force rasterization in the unit test and I added tracing of drawPaint variants that cause a saveLayer.

Is it worth using a different name for the drawPaint events (either in ui.Canvas or in DL)?

[dnfield]

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

[dnfield]

LGTM

At some point we'll probably want to add some kind of argument to these to make it easier for devtools to consume, but for now this is good