[web] Add clone method to LayerPicture and dispose pictures in PictureLayer

[harryterkelsen]

Fixes #82878

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, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• I followed the breaking change policy and added Data Driven Fixes where supported.
• All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

[gemini-code-assist]

Code Review

This pull request introduces a clone method for LayerPicture and updates PictureLayer to dispose of its picture, addressing a memory management issue on the web. By cloning Picture objects when they are added to a LayerSceneBuilder, the application can safely dispose of its original Picture handle while the engine retains its own copy. The underlying graphics resources are kept alive via reference counting and are only released when the PictureLayer itself is disposed. The changes are consistently applied across both the CanvasKit and Skwasm backends and are accompanied by a relevant test case. The overall approach is sound and effectively resolves the issue. I've added a couple of minor suggestions to add documentation for new functions to improve maintainability.

[gemini-code-assist]

Per the Flutter Style Guide, all public members should have documentation.¹ Please add a doc comment for the pictureRef function to describe its purpose.

/// Increments the reference count of the underlying native picture object.
@Native<Void Function(PictureHandle)>(symbol: 'picture_ref', isLeaf: true)
external void pictureRef(PictureHandle handle);

Style Guide References

Footnotes

1. All public members should have documentation. ↩

[gemini-code-assist]

For better maintainability and clarity, especially for exported functions, please add a comment explaining what picture_ref does. It's helpful to describe that it increments the reference count of the DisplayList.

// Increments the reference count of the given DisplayList object. This is used
// when a picture is cloned on the Dart side.
SKWASM_EXPORT void picture_ref(DisplayList* picture) {
  livePictureCount++;
  picture->ref();
}

[mdebbar]

It really feels like a picture should be a CountedRef, and PictureLayer should somehow do picture.ref(this);. That would avoid the unnecessary cloning of pictures, and the _isClone thing.

But (ugh!) CountedRef is designed to only work with C++/JSObject, so... probably outside the scope of this PR.

[harryterkelsen]

Yeah, there's a bit of clunkiness because we use our implementations of the dart:ui classes to be both how we represent the Skia objects to the framework but also to be how the engine holds onto the Skia objects itself. In the native engine there is another layer of abstraction in the C++ UI library between the dart:ui types and the underlying Skia objects.

[mdebbar]

By doing it this way, we are reporting the disposal of the picture as soon as the framework calls dispose on the original picture, even though the "picture" is still alive in the engine. This probably defeats the purpose of the callback.

I'm guessing the purpose of the callback is to report when the picture is actually disposed, i.e. when all clones have been disposed and the native object is released.

[harryterkelsen]

This matches how the native engine does it:

flutter/engine/src/flutter/lib/ui/painting.dart

Line 7658 in d57f6f8

Picture.onDispose?.call(this);

Once Picture.dispose() is called, the framework has no way to access the picture anymore.

The reason the native engine can call _dispose right away and doesn't need to create a clone of the image is that the native implementation of Picture.dispose just derefs the backing display_list, and when addPicture is called, it creates a new reference to the backing display_list. On the web, the Picture object is both how we represent the underlying Skia picture to the framework and how we hold the reference to the Skia picture ourselves, so we clone the Picture

[mdebbar]

Got it, I see now. Okay if it matches the native behavior, then sounds good!

[mdebbar]

[mdebbar]

Got it, I see now. Okay if it matches the native behavior, then sounds good!

[mdebbar]

This should've been failing all along. According to the SceneBuilder.build docs:

After calling this function, the scene builder object is invalid and cannot be used further.

Can we make LayerSceneBuilder.build explicitly throw when it's called more than once? (not necessarily in this PR).

[harryterkelsen]

Yeah. Let's make it so that the SceneBuilder disposes itself after build() is called, which is basically what the native engine does

flutter/engine/src/flutter/lib/ui/compositing/scene_builder.cc

Line 298 in 8e952d6

layer_stack_.clear();

I'll add a TODO

[mdebbar]

I'm not seeing ui.EngineLayer.dispose being called inside the engine, so I'm assuming that the framework is calling it when it's done rendering the scene.

If that's the case, why not override RootLayer.dispose and do the clonedPictures cleanup there?

[harryterkelsen]

That's basically what I changed it to, except I overrode dispose() in ContainerLayer and dispose of any straggling PictureLayers there.

[mdebbar]

Good candidate for whenComplete?

[harryterkelsen]

Done

[harryterkelsen]

Thanks Mouad! PTAL

[mdebbar]

LGTM!

[mdebbar]

This is not necessary here since it's already done in whenComplete

[harryterkelsen]

Oops! Good catch

[mdebbar]

I think this isn't needed anymore?

[harryterkelsen]

Right! Done

[auto-submit]

autosubmit label was removed for flutter/flutter/179162, because - The status or check suite Google testing has failed. Please fix the issues identified (or deflake) before re-applying this label.