Calling dispose on a ui.Image should release native resources

[dnfield]

Currently, ui.Image.dispose tries to clear the Dart wrapper, but does not release the underlying Skia resources until a GC is performed. It should be possible to do this more efficiently/quickly. See more discussion at #56482

A dispose method on a Dart object that allocates significant native resources should at least make an effort to dispose them. One challenge I'm seeing with this is that something else might still be referrring to the underlying SkImage, and I'm still trying to track down what that is or if we can do anything about it.

/cc @jason-simmons @chinmaygarde

[chinmaygarde]

Just clearing the image reference in CanvasImage::dispose() should suffice.

One challenge I'm seeing with this is that something else might still be referrring to the underlying SkImage.

This is not an issue. If the image is no longer referenced by any object, the wrapper around the sk_sp will send the object to the unref queue for collection. If it is used in a layer tree, it will be released when the layer tree is released. Since layer trees are released on the raster thread with a GPU context, the collection will be fine. The image cannot be referenced on the UI thread because that is the thread that just called dispose on the image.

[dnfield]

I've tried clearing the image reference but I believe the raster cache is sometimes holding it, and we should look to evict it there as well if we can.

There is also something else holding it, but I don't know what yet.

Basically, what I see is that even when you release the image reference, it still requires a GC to run before the actual native textures are released.

[dnfield]

Found the leaky ref: https://github.com/flutter/engine/blob/363050d4f65cf3392b26492403f8cee314285f2d/lib/ui/painting/image_decoder.cc#L296 (and a similar one a few lines up). Needs to be a std::move for the texture image there.

[chinmaygarde]

Not sure how you are characterizing that as leaky. The sk_sp will be collected with it moves out of the scope. The std::move will only avoid the ref count churn right?

[dnfield]

Hmmm. Yeah I think there's still something I'm missing.

I'm seeing some sequence of calls that I haven't fully identified yet that is increasing the ref count for reasons I'mf ailing to grasp. I believe it's coming from us though.

I was seeing that call unexpectedly increase the refcount while I was checking things, but it's not fully it.

[chinmaygarde]

I believe it's coming from us though.

It could be referenced still in the picture which has a Dart VM reference.

[dnfield]

Yeah, looking to see what I can do about that.

[dnfield]

Argh. Here's what I'm pretty convinced I'm seeing at this point:

1. We can make Image::dispose release its reference on the SkImage
2. Other things, in particular, SkPicture's we've created in the framework, have references to it to.
3. We could in theory make Picture::dispose release its ref on SkPicture. Finding the right SkPicture for the image is probably possible but it's harder. I'll try to explore that a little further.

Calling Dart_NotifyLowMemory from Image::dispose 100% clears up this issue, presumably because it forces a GC to happen ASAP. But I do not believe that's a good solution for this problem.

[dnfield]

I have some good ideas and a patch that should be pretty close to fixing this, but it doesn't get us everything I hoped for. The basic idea of my patch is:

In dispose, release the native reference.
Whenever we draw an image to a canvas, make sure the size of that gets added to the allocation size of the resulting picture.

However, this still doesn't result in the desired effect if the Picture(s) referencing the SkImage are collectable but not collected.

[github-actions]

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.