Rework simpler conditional offscreen for screen readback support.#14104
Rework simpler conditional offscreen for screen readback support.#14104flar merged 8 commits intoflutter:masterfrom
Conversation
|
There is boilerplate in a number of layers that use savelayer to save the flag, preroll on children, and restore it. I could either provide PrerollWithSavelayer() and PrerollChildrenWithSavelayer() methods/inlines in ContainerLayer, or modify the existing Preroll()/PrerollChildren() methods to conditionally provide this service - thoughts? |
|
We will need a further PR to investigate the best way to handle Metal surfaces. For this particular PR, I only want to focus on compatibility with what is working today and to enable the optimization for GL surfaces. I will file an Issue when this PR lands to investigate the Metal possibilities. To be clear, Metal has a mutable property indicating whether a surface is only used for rendering, or whether it can be used for other operations - including the readback we need for surface_supports_readback. As a result, we may want to upgrade this fix to a 2-way communication protocol that allows us to inform the Metal surface when the tree is in a state that requires readback and let it update its settings. For now, with this fix, Metal will have to choose one way and then set the appropriate value for whether the properties it chose allows readback or not and that decision will be static. For this PR, I'm making the assumption that the Metal properties were already requesting readback capabilities from the platform and I've set the flag to indicate that we can readback from a Metal surface. I want to verify that assumption, but if that isn't true then it means that our current Metal support has a bug that prevents BackdropFilters from working, so I doubt the assumption is incorrect. Tagging @chinmaygarde for specific review in this area. |
a43665f to
15e96be
Compare
|
backdrop_filter_perf.dart benchmark results: Running the LinearProgressApp: |
|
It is outside the scope of this PR, but it seems like all of the variants of ClipFooLayer could be rolled into a common parent class with extremely minor implementations for their differences (things like returning the clip bounds and installing the clip shape into a mutator stack and/or a canvas, everything else should be identical among them). |
8e9a948 to
45d59bb
Compare
|
I've removed the WIP label as this is pretty much complete and all that is happening now is running various apps for testing - and waiting for a 4 hour time slot when I can run the "real world battery rundown" tests. |
liyuqian
left a comment
There was a problem hiding this comment.
Thank you for the tests! Left some minor comments on naming and code styles.
Comment LayerTree::Preroll method. Rename GPUSurfaceGLDelegate::UseOffscreenSurface -> SurfaceSupportsReadback
New auto mechanism to save/restore the flag for saveLayer Layers
8f61b31 to
dbb58d2
Compare
flow/layers/backdrop_filter_layer.cc
Outdated
|
|
||
| void BackdropFilterLayer::Preroll(PrerollContext* context, | ||
| const SkMatrix& matrix) { | ||
| Layer::AutoPrerollSaveLayer save = |
There was a problem hiding this comment.
Nit: AutoPrerollSaveLayer seems to be very similar to AutoSaveLayer but they do very different things. Maybe rename it to something like AutoRestoreReadbackFlag?
There was a problem hiding this comment.
We currently have AutoSaveLayer which is a contextual object for wrapping a tree descent with code that needs to be done to set up and take down the save layer. It actually does more than that, but everything it does is related to managing a save layer.
The new similarly tasked object I added is doing the same thing during Preroll, it is managing the setup and take down of state that needs to be managed for a save layer during the Preroll operation. Perhaps having both names match more would help?
AutoPaintSaveLayer
AutoPrerollSaveLayer
AutoSaveLayer::Paint
AutoSaveLayer::Preroll
?
There was a problem hiding this comment.
I went with AutoPrerollSaveLayerState - we discussed ManagePrerollSaveLayerState, but I felt that Auto reflected its scoped actions. Manage sounds like a 1-time operation (and was creating awkward line lengths).
liyuqian
left a comment
There was a problem hiding this comment.
LGTM with a few naming nits.
|
BTW, before merging, please make sure to put flutter/flutter#31865 in the PR/commit description for future reference. |
…toPrerollSaveLayerState.
|
This change should help with flutter/flutter#31865 In the best case, with only a single animating LinearProgressIndicator on the screen the improvement can be as much as a 50% reduction in combined CPU/GPU usage, but larger apps should see less of that reduction as the cost of the extra offscreen layer will gradually be dominated by other work the application is doing. Cupertino apps will automatically have a BackdropFilter in a few places unless they take steps to disable them. For example, the header will have a translucent background color by default and a blur filter is applied under it. If you set an opaque background color then the backdrop will not be applied. |
This change should help with flutter/flutter#31865
This solution for the conditional offscreen allows us to check for layers that read from the surface in Preroll instead of in the constructors. There are no longer any flags propagating to parents or any other potentially quadratic recursions compared to the previous solution.
I have an FML_LOG(INFO) in the code that lets you check on the status of when we use an offscreen by running an app with
flutter run --verbose-system-logsThings TBD:
Former TBD things completed: