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

Conversation

@dnfield
Copy link
Contributor

@dnfield dnfield commented May 11, 2021

Today, SceneBuilder.addPicture creates an engine side layer but does not return a handle to that layer.

As part of flutter/flutter#81514, we'll want to be able to tell these layers when we are done with them framework side so the picture can get disposed of properly - otherwise, even if the framework tries to dispose its copy of the Picture object, picture layers will still retain it until they are disposed, which will require a full GC. This will help both VM and web. This PR does not add the dispose method, but instead just makes the smaller change to return the layer for now.

I did all the checkbox things.

@flutter-dashboard flutter-dashboard bot added the platform-web Code specifically for the web engine label May 11, 2021
@google-cla google-cla bot added the cla: yes label May 11, 2021
Copy link
Contributor

@yjbanov yjbanov left a comment

Choose a reason for hiding this comment

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

lgtm


abstract class PhysicalShapeEngineLayer implements EngineLayer {}

abstract class PictureEngineLayer implements EngineLayer {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please leave a TODO(yjbanov) here and a Github issue for me to incorporate this into our DOM diffing algorithm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Picture picture, {
bool isComplexHint = false,
bool willChangeHint = false,
PictureEngineLayer? oldLayer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Docs for the new argument? I think the dart.ui.sceneBuilder.oldLayer macro should be reusable here. There's also the dart.ui.sceneBuilder.oldLayerVsRetained macro but I'm not sure it applies to pictures. Picture layer is a leaf layer and the same picture can be reused multiple time in the same scene.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@flar
Copy link
Contributor

flar commented May 11, 2021

This will have some minor conflicts with #25234 depending on which goes in first. "Pictures" will no longer always be a Skia-oriented thing.

@dnfield
Copy link
Contributor Author

dnfield commented May 11, 2021

This will have some minor conflicts with #25234 depending on which goes in first. "Pictures" will no longer always be a Skia-oriented thing.

That's why I'm copying you for review :)

@dnfield dnfield merged commit 8e72d7b into flutter:master May 12, 2021
@dnfield dnfield deleted the picture_layer branch May 12, 2021 03:11
@dnfield dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label May 12, 2021
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 12, 2021
dnfield added a commit that referenced this pull request May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes platform-web Code specifically for the web engine waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants