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

Rework simpler conditional offscreen for screen readback support.#14104

Merged
flar merged 8 commits intoflutter:masterfrom
flar:layertree-negotiate-save-layer-with-surface
Dec 11, 2019
Merged

Rework simpler conditional offscreen for screen readback support.#14104
flar merged 8 commits intoflutter:masterfrom
flar:layertree-negotiate-save-layer-with-surface

Conversation

@flar
Copy link
Contributor

@flar flar commented Dec 4, 2019

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-logs

Things TBD:

  • run more tests (apps and examples to make sure they render correctly)

Former TBD things completed:

  • specifically run the BackdropFilter devicelab benchmark before/after to verify no loss of performance
  • specifically run the LinearProgressApp and check its energy usage in Xcode and CPU/GPU usage with the gauge in flutter-packages
  • write (and then run) unit tests
  • remove offscreen support from gpu_surface_gl (currently it is just bypassed, but it can be deleted)
  • my Fuchsia build seems to be broken now so I can only marginally claim that it still compiles

@flar
Copy link
Contributor Author

flar commented Dec 4, 2019

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?

@flar
Copy link
Contributor Author

flar commented Dec 4, 2019

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.

@flar flar added the Work in progress (WIP) Not ready (yet) for review! label Dec 4, 2019
@flar flar force-pushed the layertree-negotiate-save-layer-with-surface branch from a43665f to 15e96be Compare December 5, 2019 00:24
@flar
Copy link
Contributor Author

flar commented Dec 5, 2019

backdrop_filter_perf.dart benchmark results:

                             before fix      after fix
                             ----------      ---------

avg build time run 1:         0.692 ms        0.690 ms
avg build time run 2:         0.672 ms        0.684 ms
avg build time run 3:         0.711 ms        0.697 ms
overall avg build time:       0.6917          0.6903    (<.2% reduction)

avg raster time run 1:        145.8 ms        146.8 ms
avg raster time run 2:        145.7 ms        148.2 ms
avg raster time run 3:        145.7 ms        147.7 ms
overall avg raster time:      145.7           147.6     (1.25% increase)

Running the LinearProgressApp:

                         before fix      after fix
                         ----------      ---------
Xcode energy rating:     Very High          Low
(within/for)              < 40sec         > 10min

GPU gauge run 1:           16.8%           8.8%
GPU gauge run 2:           15.2%           8.4%
GPU gauge run 3:           15.4%           8.6%
GPU gauge run 4:           15.0%           8.4%
GPU gauge average:         15.6%           8.6%    (45% reduction)

CPU gauge run 1:           19.0%           16.0%
CPU gauge run 2:           19.6%           15.4%
CPU gauge run 3:           19.8%           16.9%
CPU gauge run 4:           18.8%           17.3%
CPU gauge average:         19.3%           16.4%   (15% reduction)

Combined GPU/CPU                                   (53% reduction)

Real-life battery drain:    40%             32%    (20% reduction
Compared to blank screen:   16%              8%    (50% reduction)

Source for the app:

    return MaterialApp(
      title: 'Demo',
      home: Scaffold(
        body: Center(
          child: LinearProgressIndicator(),
        ),
      ),
    );

@flar
Copy link
Contributor Author

flar commented Dec 5, 2019

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).

@flar flar force-pushed the layertree-negotiate-save-layer-with-surface branch from 8e9a948 to 45d59bb Compare December 5, 2019 04:46
@flar flar removed the Work in progress (WIP) Not ready (yet) for review! label Dec 5, 2019
@flar
Copy link
Contributor Author

flar commented Dec 5, 2019

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.

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

Thank you for the tests! Left some minor comments on naming and code styles.

@flar flar force-pushed the layertree-negotiate-save-layer-with-surface branch from 8f61b31 to dbb58d2 Compare December 6, 2019 11:43

void BackdropFilterLayer::Preroll(PrerollContext* context,
const SkMatrix& matrix) {
Layer::AutoPrerollSaveLayer save =
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: AutoPrerollSaveLayer seems to be very similar to AutoSaveLayer but they do very different things. Maybe rename it to something like AutoRestoreReadbackFlag?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

@liyuqian liyuqian left a comment

Choose a reason for hiding this comment

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

LGTM with a few naming nits.

@liyuqian
Copy link
Contributor

liyuqian commented Dec 7, 2019

BTW, before merging, please make sure to put flutter/flutter#31865 in the PR/commit description for future reference.

@flar flar merged commit 8595361 into flutter:master Dec 11, 2019
@flar
Copy link
Contributor Author

flar commented Dec 11, 2019

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants