Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider adding a custom recording format #53501

Closed
liyuqian opened this issue Mar 28, 2020 · 10 comments
Closed

Consider adding a custom recording format #53501

liyuqian opened this issue Mar 28, 2020 · 10 comments
Labels
c: new feature Nothing broken; request for a new capability c: performance Relates to speed or footprint issues (see "perf:" labels) customer: product engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list perf: memory Performance issues related to memory

Comments

@liyuqian
Copy link
Contributor

liyuqian commented Mar 28, 2020

The custom recording format could be based on https://skia.googlesource.com/skia/+/refs/heads/android/10-release/src/core/SkLiteDL.h . We can use it to replace the SkPicture format to allow us to have more control. It could bring us

  1. More performance optimization opportunities regarding picture raster caches
  2. A potentially better way (than skp or sksl) to do shader warm-up
  3. A potentially better way to capture and analyze frame than skp.
@liyuqian liyuqian added engine flutter/engine repository. See also e: labels. c: performance Relates to speed or footprint issues (see "perf:" labels) labels Mar 28, 2020
@liyuqian
Copy link
Contributor Author

@chinmaygarde : I couldn't find such an issue in our database so I created this one. Please feel free to edit it.

@liyuqian liyuqian changed the title Consider add a custom recording format Consider adding a custom recording format Mar 28, 2020
@chinmaygarde chinmaygarde added P1 High-priority issues at the top of the work list c: new feature Nothing broken; request for a new capability labels Jun 15, 2020
@dnfield dnfield added the perf: memory Performance issues related to memory label Dec 16, 2020
@dnfield
Copy link
Contributor

dnfield commented Dec 16, 2020

For memory usage, it would be helpful if this could support:

  • Reusing objects that are the same/come from the same (unmutated) Dart object
  • Efficiently determine if an expensive resource (such as an image) can be disposed of early
  • Traceability for developers/devtools to know what code has instantiated or is retaining parts of a particular display list

@kf6gpe
Copy link
Contributor

kf6gpe commented Jan 11, 2021

@chinmaygarde Are we actively working on this? I don't think so. I'm tempted to downgrade this to a P5 if we're not.

@yjbanov
Copy link
Contributor

yjbanov commented Mar 10, 2021

This doesn't feel like a P3, unless perhaps if we decide that this is a blocker for solving jank. FWIW on the web we implemented this in order to manage picture memory on Safari without weak refs: https://github.com/flutter/engine/blob/dcb57f003bd3f5f8fbb3c75cd94443e8096ade94/lib/web_ui/lib/src/engine/canvaskit/canvas.dart#L269

We don't use it anywhere except Safari though, and we don't get the benefits described here.

@dnfield
Copy link
Contributor

dnfield commented Mar 10, 2021

This would solve a number of issues for us. I believe it's planned for the near term. Chinmay or @flar might know for sure

@flar
Copy link
Contributor

flar commented Mar 10, 2021

It has been moving up the priority list. Some of the not so distant term issues it could solve:

From there it could be a launching pad for other issues like:

  • Being able to split and merge pictures at the framework level when nodes contributing to a shader picture start/stop animating without having to repaint everything.
  • Minimize overhead of regenerating pictures when the contents are not changing by first comparing new operations to an existing picture and only forking it and creating a new picture when the contents change (this is an alternate solution to the previous issue, but with other benefits)

In addition to the list of benefits in the Description.

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2021
@flar flar reopened this Aug 27, 2021
@flar
Copy link
Contributor

flar commented Aug 27, 2021

While the mechanism has been added to the Flutter Engine, it is not presently the default mechanism to use for Picture recording. As we enable it as the default we discover new issues that need to be resolved and will disable it to fix those issues as needed. I'll link this issue to any PRs that enable or disable the DisplayList as the default and hopefully any issues that point out problems that cause us to disable it again and any PRs that fix such issues.

Note that while we work through these issues, the mechanism can be enabled or disabled on a run by run basis from the command line using the following flags (a short message will be emitted to confirm that the mechanism is enabled or disabled):
manually enable: flutter run --dart-flags=--enable-display-list ...
manually disable: flutter run --dart-flags=--no-enable-display-list ...

Mechanism added: flutter/engine#26928
Race condition fixed in: flutter/engine#27123

First attempt to enable it by default
Flag enabled: flutter/engine#27130
Flag disabled due to SkPathEffect issue: flutter/engine#27153
SkPathEffect issue fixed: flutter/engine#27155

Second attempt to enable it by default
Flag enabled: flutter/engine#27175
Flag disabled due to SVG and shadow rendering failures: flutter/engine#27230
(SVG rendering tests were rewritten to allow +/-1 pixel differences)
Fixes to store Shadows in DisplayList better: flutter/engine#27289

Third attempt to enable it by default
Flag enabled: flutter/engine#27407
Flag disabled again due to shadow rendering: flutter/engine#27475
Fixes to shadow rendering: flutter/engine#27124

Fourth attempt to enable it by default
Flag enabled: flutter/engine#27892
Memory usage issue discovered during internal testing: #88745
DisplayList leak fix: flutter/engine#28158
Flag disabled again because leak fix didn't resolve internal testing failures: flutter/engine#28308

Fifth attempt to enable it by default
Flag enabled: flutter/engine#28946

Current status: Enabled

@zanderso
Copy link
Member

This has landed and is now enabled by default with flutter/engine#28946. Work to clean up the previous approach is underway. Other follow on work is tracked under #85737, so I will go ahead and close this issue.

@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
c: new feature Nothing broken; request for a new capability c: performance Relates to speed or footprint issues (see "perf:" labels) customer: product engine flutter/engine repository. See also e: labels. P1 High-priority issues at the top of the work list perf: memory Performance issues related to memory
Projects
None yet
Development

No branches or pull requests

8 participants