[WIP] Recator API 'SceneBulder.addPicture' to return 'PictureEngineLayer' #32738

ColdPaleLight · 2022-04-18T11:30:49Z

framework change probably looks like this

diff --git a/packages/flutter/lib/src/rendering/layer.dart b/packages/flutter/lib/src/rendering/layer.dart
index 4644180ddf..13115dafc8 100644
--- a/packages/flutter/lib/src/rendering/layer.dart
+++ b/packages/flutter/lib/src/rendering/layer.dart
@@ -711,7 +711,7 @@ class PictureLayer extends Layer {
   @override
   void addToScene(ui.SceneBuilder builder) {
     assert(picture != null);
-    builder.addPicture(Offset.zero, picture!, isComplexHint: isComplexHint, willChangeHint: willChangeHint);
+    engineLayer = builder.addPicture(Offset.zero, picture!, isComplexHint: isComplexHint, willChangeHint: willChangeHint, oldLayer: _engineLayer as ui.PictureEngineLayer?);
   }

Pre-launch Checklist

I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
I read the [Tree Hygiene] wiki page, which explains my responsibilities.
I read and followed the [Flutter Style Guide] and the [C++, Objective-C, Java style guides].
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See [testing the engine] for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the [CLA].
All existing and new tests are passing.

…dLayer'

…n web_ui

ColdPaleLight · 2022-04-18T13:52:35Z

lib/ui/compositing.dart

    Picture picture, {
    bool isComplexHint = false,
    bool willChangeHint = false,
+    PictureEngineLayer? oldLayer,


I'm not sure if oldLayer is needed in this place, it doesn't look useful. Probability it makes more sense not to add it?

ColdPaleLight · 2022-04-18T14:29:17Z

lib/ui/compositing.dart

+  /// {@macro dart.ui.sceneBuilder.oldLayer}
+  ///
+  /// {@macro dart.ui.sceneBuilder.oldLayerVsRetained}
+  PictureEngineLayer addPicture(


Since PictureEngineLayer is returned here and can't be null, the related logic in web_ui must also be implemented in this PR. And I'm not familiar with web_ui, so I'm not sure if my implementation is correct.

In addition, when this PR prepare to land, we need to temporarily disable some tests in packages/flutter/test/rendering/layers_test.dart, Because the FakeSceneBuilder.addPicture returns null
c.f.
https://github.com/flutter/flutter/blob/94d7f5e6724d3e0c627a9ceb27e294c5c12ddebb/packages/flutter/test/rendering/layers_test.dart#L751-L781

Maybe we can consider changing the return value here to PictureEngineLayer?, so that we don't have to change the implementation of web_ui, and we don't have to disable layers_test.dart

jonahwilliams · 2022-04-18T15:45:13Z

I think the previous change you proposed would be much, much less intrusive than this change

ColdPaleLight · 2022-04-18T16:51:28Z

I think the previous change you proposed would be much, much less intrusive than this change

The API changing in this proposal should not be too big, although it can't do the same as the previous proposal without change the API at all. But in this proposal, we only need to change the return value of SceneBuilder.addPicture from void to PictureEngineLayer or PictureEngineLayer?, and do not have to change the required arguments or optional arguments in SceneBuilder.addPicture . (oldLayer doesn't seem to be useful here, maybe we don't need to add it to the API, So we just need to change the return value type)

I verified that this proposal also works fine like the previous one.

jonahwilliams · 2022-04-18T16:58:20Z

But compared to not having to update the API at all, it is a big change. Also consider that all code that manages pictures in the framework will now need to track the engine layer too.

flar · 2022-04-18T23:15:22Z

Your suggestion for the framework change sounds like you haven't actually tested if this can work?

In particular, if the picture has been rebuilt then it should not be treated as an "oldLayer" when added to the tree. Where do you set engineLayer to null when the picture is rebuilt?

flar · 2022-04-18T23:16:57Z

The RepaintBoundary solves that issue by actually not rebuilding its layers unless a child is modified. Solving this for non-RepaintBoundary objects would require any RenderObject that holds on to a picture to do such tracking - which means it needs to be a RepaintBoundary, or at least participate in the concept of which parts of the tree to rebuild to the same extent as RPB does.

ColdPaleLight · 2022-04-19T00:07:01Z

Your suggestion for the framework change sounds like you haven't actually tested if this can work?

In particular, if the picture has been rebuilt then it should not be treated as an "oldLayer" when added to the tree. Where do you set engineLayer to null when the picture is rebuilt?

@flar I tested it and it works. In the current implementation of flutter, the PictureLayer of the framework does not hold the engineLayer, which results in that when its parent's addToScene is called, even if the PictureLayer's flag _needsAddToScene is false, its addToScene will also be called. And if PictureLayer also has engineLayer, then addRetained will be called in this case. So the key to making it work is to let PictureLayer hold its own engineLayer, so that the DisplayListLayer of the engine layer can be stable.Of course, like the previous proposal, this also requires flutter/flutter#101952

c.f.
https://github.com/flutter/flutter/blob/fd360c4a1d2418ad2d1ad83f7b5cc125696837f9/packages/flutter/lib/src/rendering/layer.dart#L532

jonahwilliams · 2022-04-19T00:15:10Z

I sort of disagree with this implementation - we don't need the display list layer to be stable if the picture is stable. Assuming the user or the framework is keeping the picture stable, then the kLayer caching strategy should work. To do so, it must use the display list ID as the cache key.

This sort of approach seems like way more work.

ColdPaleLight · 2022-04-19T01:06:53Z

@jonahwilliams @flar It seems that none of you like this proposal. Maybe we can move on to the previous proposal :)

flutter-dashboard · 2022-05-17T14:19:03Z

This pull request executed golden file tests, but it has not been updated in a while (20+ days). Test results from Gold expire after as many days, so this pull request will need to be updated with a fresh commit in order to get results from Gold.

ColdPaleLight added 3 commits April 18, 2022 19:29

Recator API 'SceneBulder.addPicture' to return 'PictureEngineLayer'

ba58242

Fix test 'ImageDisposeTest.ImageReleasedAfterFrameAndDisposePictureAn…

6683be0

…dLayer'

Recator API 'SceneBulder.addPicture' to return 'PictureEngineLayer' i…

56ca836

…n web_ui

ColdPaleLight changed the title ~~Recator API 'SceneBulder.addPicture' to return 'PictureEngineLayer'~~ [WIP] Recator API 'SceneBulder.addPicture' to return 'PictureEngineLayer' Apr 18, 2022

flutter-dashboard bot added the platform-web Code specifically for the web engine label Apr 18, 2022

ColdPaleLight commented Apr 18, 2022

View reviewed changes

ColdPaleLight mentioned this pull request Apr 18, 2022

Tweak the logic about generating 'RasterCacheKey' of the type 'kLayerChildren' #32676

Merged

8 tasks

ColdPaleLight commented Apr 18, 2022

View reviewed changes

ColdPaleLight added the Work in progress (WIP) Not ready (yet) for review! label Apr 27, 2022

ColdPaleLight closed this May 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[WIP] Recator API 'SceneBulder.addPicture' to return 'PictureEngineLayer' #32738

[WIP] Recator API 'SceneBulder.addPicture' to return 'PictureEngineLayer' #32738

Uh oh!

ColdPaleLight commented Apr 18, 2022 •

edited

Loading

Uh oh!

ColdPaleLight Apr 18, 2022

Uh oh!

ColdPaleLight Apr 18, 2022

Uh oh!

jonahwilliams commented Apr 18, 2022

Uh oh!

ColdPaleLight commented Apr 18, 2022 •

edited

Loading

Uh oh!

jonahwilliams commented Apr 18, 2022

Uh oh!

flar commented Apr 18, 2022

Uh oh!

flar commented Apr 18, 2022

Uh oh!

ColdPaleLight commented Apr 19, 2022

Uh oh!

jonahwilliams commented Apr 19, 2022

Uh oh!

ColdPaleLight commented Apr 19, 2022

Uh oh!

flutter-dashboard bot commented May 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[WIP] Recator API 'SceneBulder.addPicture' to return 'PictureEngineLayer' #32738

[WIP] Recator API 'SceneBulder.addPicture' to return 'PictureEngineLayer' #32738

Uh oh!

Conversation

ColdPaleLight commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

ColdPaleLight Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

ColdPaleLight Apr 18, 2022

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Apr 18, 2022

Uh oh!

ColdPaleLight commented Apr 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonahwilliams commented Apr 18, 2022

Uh oh!

flar commented Apr 18, 2022

Uh oh!

flar commented Apr 18, 2022

Uh oh!

ColdPaleLight commented Apr 19, 2022

Uh oh!

jonahwilliams commented Apr 19, 2022

Uh oh!

ColdPaleLight commented Apr 19, 2022

Uh oh!

flutter-dashboard bot commented May 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ColdPaleLight commented Apr 18, 2022 •

edited

Loading

ColdPaleLight commented Apr 18, 2022 •

edited

Loading