Create an ImageHandle wrapper #21057

dnfield · 2020-09-09T21:47:15Z

Description

Allows for reference counting of images before disposal.

This will allow multiple callers to hold a reference to an image and dispose of their reference without disposing the underlying image until all handles have been disposed.

This will be used by the framework to help resolve some of the kludge I was trying to introduce in flutter/flutter#64582

Related Issues

flutter/flutter#56482

Tests

I added the following tests:

Tests that getting and disposing of an image handle works, and that it's possible to debug track handle creation.

Breaking Change

Did any tests fail when you ran them? Please read handling breaking changes.

No, no existing tests failed, so this is not a breaking change.

lib/ui/painting.dart

Hixie · 2020-09-09T22:28:15Z

lib/ui/painting.dart

+  /// is the last remaining handle.
+  Image createHandle() {
+    if (_disposed) {
+      throw StateError('Object disposed');


can we come up with a better error message?

I believe this is what is used in other cases when a native object is used that is disposed, but I'm open to suggestions.

Something like "Cannot clone a disposed image.\nThe clone() method of a previously-disposed Image was called. Once an Image object has been disposed, it can no longer be used to create handles, as the underlying data may have been released."

Hixie · 2020-09-09T22:40:57Z

lib/ui/painting.dart

+    _image._handles.remove(this);
+    if (_image._handles.isEmpty) {
+      _image.dispose();
+    }


this means that if you get a dart:ui.Image, you must get a handle to it, you can just vent handles to it, because if you give someone a handle and they dispose it, your object is killed. That seems weird. Would it make sense to always vend an ImageHandle, and just make the current dart:ui.Image private to dart:ui?

This would have another advantage, which is that this patch has a hidden cost; we're making all uses of dart:ui.Image use a more expensive polymorphic dispatch whereas before they were single-dispatch (which is much cheaper). By having two unrelated classes, we would avoid this.

Yeah, I suspect having both Image and ImageHandle public will create confusion.

@goderbauer and I were talking about this and were a little concerned about what this does in FrameInfo.image, since we'll have to cache the _ImageHandle in there.

It makes the ownership model slightly more confusing, since FrameInfo doesn't get disposed. The real question becomes "do I have to call createHandle on the result of FrameInfo.image before using it, and who has to dispose the result of it?".

And in case it's not clear, if we leave FrameInfo.image alone, it all sort of works. When you get the image there, you immediately create a handle from it, and whoever else needs one does the same, and when all of you are done the underlying image gets disposed and you're not left wondering whether you're supposed to dispose the property of a class that was given to you.

I would have preferred to only expose Handles to the framework, but the ownership of the very first handle (provided in the FrameInfo) is kinda strange. Who is in charge of disposing that one? And when would that happen?

I've pushed up some changes that I think make sense, although I'm now realizing I forgot to update documents.

I think the way to reason about this should be if you can access an Image, you should assume it's yours and you must dispose it.

Put another way, if you want to give an image reference to someone, you need to call createHandle on your handle, you should not expect them to. You can then also decide whether you want to "move" the reference to them (give it without calling createHandle, and now they must call dispose and you must not) or "copy" it (call createHandle, you each dispose your instance).

zanderso · 2020-09-09T23:08:20Z

lib/ui/painting.dart

+    _image._handles.remove(this);
+    if (_image._handles.isEmpty) {
+      _image.dispose();
+    }


Yeah, I suspect having both Image and ImageHandle public will create confusion.

zanderso · 2020-09-09T23:08:58Z

lib/ui/painting.dart

+    }
+  }
+
+  /// Unused private implementation of [Image]


// rather than /// I think.

I did this so the link to Image would work in the IDE, but can remove it. I think DartDoc ignores it since there's only private members after it anyway.

zanderso · 2020-09-09T23:12:08Z

lib/ui/painting.dart

+      assert(!_disposed);
+      assert(
+        _handles.isEmpty,
+        'Attempted to dispose of an Image object that has ${_handles.length} open handles.',


Should this print the (de-duped?) stack traces of the open handles?

I think that will become pretty overwhelming - for instance, in testing it's not terribly uncommon to have 4 or 5 open handles (between ImageCache in a couple spots, the ImageStream, the widget actually using the image). The stacks might be hundreds of lines long a piece.

In the Framework we can use the Diagnosticable stuff and some stack parsing logic to make it a bit more manageable via the debug property. Maybe we could make this error message suggest using that if you're hitting this?

Maybe we could make this error message suggest using that if you're hitting this?

Sounds good.

This is now just an assertion that should never fire, unless we introduce a bug into dart:ui. I've updated the text to recommend filing a bug.

dnfield · 2020-09-11T00:53:10Z

I've updated docs, and reworked this patch so that dart:ui only exposes image handles (although called Image still), and now there's no polymorphic dispatch.

Unfortunately, this patch will fail until https://dart-review.googlesource.com/c/sdk/+/162460 rolls in to the engine (/cc @jason-simmons who might have a good idea about how to work around that).

/cc @yjbanov - I'm only adding stubs for the web implementation, but it looks like for Skia we might be able to do this. I'm hoping you'll know whether we should or not - my assumption is that on web, the browser is managing most of this anyway (for sure on the HTML backend), but I'm not as clear about what we're doing with the Skia backend for images.

Hixie · 2020-09-11T07:04:59Z

lib/ui/painting.dart

+  /// The returned object behaves identically to this image, except calling
+  /// [dispose] on it will only dispose the underlying native resources if it
+  /// is the last remaining handle.
+  Image createHandle() {


"duplicateHandle" or "clone" or something might be better than "createHandle". The latter makes it sound like this isn't a handle and the thing returned is different.

Hixie · 2020-09-11T07:05:10Z

lib/ui/painting.dart

+  /// Creates a disposable handle to this image.
+  ///
+  /// The returned object behaves identically to this image, except calling
+  /// [dispose] on it will only dispose the underlying native resources if it


docs need updating ("except" is no longer accurate)

Hixie · 2020-09-11T07:10:53Z

lib/ui/painting.dart

  /// The [Image] object for this frame.
-  Image get image native 'FrameInfo_image';
+  Image get image => _cachedImage ??= Image._(_image);
+  _Image get _image native 'FrameInfo_image';
 }


we should definitely update the docs for FrameInfo, FrameInfo.image, and any APIs that return FrameInfo to say that the Image inside the returned object needs to be explicitly disposed and that if a handle to that image is passed it must first be cloned and that a handle to the FrameInfo itself must never be passed.

...which altogether sounds rather frightening. Should FrameInfo similarly be made cloneable to at least make it possible to create a new copy rather than making it a hot potato object? Having to dispose a property of a returned object is pretty weird as an API, I'm sure that would be a cause of leaks.

Why is FrameInfo a NativeFieldWrapperClass2? It looks like it could just be created with a reference to _image and _durationMillis and be a pure Dart object.

The only problem I can think of with making FrameInfo not a NativeFieldWrapperClass2 is that image will no longer be lazy, so someone who is trying to "seek" to a particular frame will have to decode all the images between (whereas now they might not have to).

However, as implemented, image is not actually lazy, and making this a "regular" Dart object we could probably still keep it lazy by just pointing it to a private getter on the Codec that does the native work.

I think if we do make it lazy, we don't have to tell people to dispose it unless they access it.

I'm not a huge fan of cloning FrameInfo. You should either use the image and then dispose it, or you should pass it on without touching the image and tell the next person to dispose it.

Hm. Making image lazy is probably something we should do in a separate patch, if at all. It'd shift decode workloads from when you call getNextFrame to whenyou first access the image, and we'd have to make the image getter async to make it really make sense.

Added some more documentation on FrameInfo, FrameInfo.image, and Codec.getNextFrame.

dnfield · 2020-09-11T17:56:10Z

Latest push:

Makes FrameInfo a regular Dart object
Updates some C++ tests to access the "real" native _Image.
Renamed createHandle to clone.
Updated some docs/exception messages.

goderbauer · 2020-09-11T22:43:59Z

lib/ui/painting.dart

+/// that handle with other callers, it is critical that [clone] is called
+/// _before_ [dispose]. A handle that has been disposed cannot create new


maybe just simplify: "If dart:ui passes an Image object and the recipient wishes to share that handle with other callers, [clone] must be called before [dispose]."

(The "is is critical that" sounds like empty prose)

goderbauer · 2020-09-11T22:45:49Z

lib/ui/painting.dart

+    return _image.toByteData(format: format);
+  }
+
+  bool _disposed = false;


Should be named _debugDisposed since its only used within asserts and will only be valid in debug mode.

Also, move this closer to the dispose method?

Moved it, but also moved setting it out of asserts, since I think the runtime failure mode without it will be pretty confusing.

goderbauer · 2020-09-11T22:48:05Z

lib/ui/painting.dart

+    if (_disposed) {
+      throw StateError(
+        'Cannot clone a disposed image.\n'
+        'The clone() method of a previously-disposed Image was called. Once an '
+        'Image object has been disposed, it can no longer be used to create '
+        'handles, as the underlying data may have been released.'
+      );
+    }


Should this be wrapped in an assert? _dispose is only ever set to true when asserts are enabled.

I think instead we should set _disposed for real.

My main concern here is that this is going to cause a runtime failure, but it'd be better to have that runtime failure happen here rather than later whensomeone actually goes to use the image.

goderbauer · 2020-09-11T22:50:15Z

lib/ui/painting.dart

+  // This class is created by the engine, and should not be instantiated
+  // or extended directly.
+  //
+  // To obtain an [Image] object, use [instantiateImageCodec].


This comment referencing Image on _Image seems confusing...

Should this instead be on the private constructor of Image?

I'll reword this. It's leftover.

goderbauer · 2020-09-11T22:50:55Z

lib/ui/painting.dart

-  /// Release the resources used by this object. The object is no longer usable
-  /// after this method is called.
-  void dispose() native 'Image_dispose';
+  bool _disposed = false;


since this is only set in asserts, call it _debugDispose?

goderbauer · 2020-09-11T22:54:43Z

lib/ui/painting.dart

-@pragma('vm:entry-point')
-class FrameInfo extends NativeFieldWrapperClass2 {
+///
+/// The recipient of this class is responsible for calling [Image.dispose] on


Should we say "The recipient of this class, who obtained it from [Codec.getNextFrame], becomes the owner of it and is responsible for ..."

Should we also say that you shouldn't share the FrameInfo object with anybody else and instead hand out handles to the wrapped image?

Part of what I'm struggling with on this one is that we don't have any clear request for sharing/reusing frame info. If we do, the right thing to do is add a clone method here.

It's not so much that I'm opposed to adding that as that I'm reluctant to do it here without a clearer idea of why you would do it.

I still think that this comment should get a little more details about the image handle it contains (or link to documentation that better described how to deal with it).

I think [Image.clone] is the right place to link to for additional information.

I've added a couple code samples ("BAD" and "GOOD") to demonstrate usage.

goderbauer · 2020-09-11T22:55:28Z

lib/ui/painting.dart

-    return _futurize(_getNextFrame);
+  ///
+  /// The caller of this method is responsible for disposing the
+  /// [FrameInfo.image] on the returned object.


Should this also mention that you should create additional handles to the image if you pass it around?

goderbauer · 2020-09-11T22:56:37Z

lib/ui/painting.dart


  /// The [Image] object for this frame.
-  Image get image native 'FrameInfo_image';
+  ///
+  /// This object must be disposed by the recipient of this frame info.


Maybe include here that instead of passing this object around, consider creating additional clones? And describe why that may be appropriate?

goderbauer · 2020-09-11T22:57:32Z

lib/ui/painting.dart

+  ///
+  /// The returned object behaves identically to this image. Calling
+  /// [dispose] on it will only dispose the underlying native resources if it
+  /// is the last remaining handle.


Maybe add some additional details to describe when an additional handle should be created? And that it needs to be disposed to avoid leaks?

Done and added a code sample.

goderbauer

In general, this looks good to me now, but somebody from the engine side should also give it another review.

Also, do you have a framework-side PR that shows how this will be used over there now?

Last, but not least: the tests seem unhappy.

goderbauer · 2020-09-14T17:00:47Z

lib/ui/compositing.dart

@@ -27,10 +27,13 @@ class Scene extends NativeFieldWrapperClass2 {
    if (width <= 0 || height <= 0) {
      throw Exception('Invalid image dimensions.');
    }
-    return _futurize((_Callback<Image> callback) => _toImage(width, height, callback));
+    return _futurize((_Callback<Image> callback) => _toImage(width, height, (_Image image) {


The documentation on the method toImage needs updating explaining that you get a handle to an image that needs to be disposed when you're done with it.

Added some more details here.

goderbauer · 2020-09-14T17:07:20Z

lib/ui/painting.dart

-@pragma('vm:entry-point')
-class FrameInfo extends NativeFieldWrapperClass2 {
+///
+/// The recipient of this class is responsible for calling [Image.dispose] on


I still think that this comment should get a little more details about the image handle it contains (or link to documentation that better described how to deal with it).

dnfield · 2020-09-14T17:21:17Z

@goderbauer - tests will fail until https://dart-review.googlesource.com/c/sdk/+/162460 rolls into the engine.

I don't have a PR on the framework side, but it will end up looking similar to flutter/flutter#64582, but more sound now.

zanderso · 2020-09-15T18:05:35Z

lib/ui/painting.dart

+  ///
+  /// If asserts are disabled, this method always returns null.
+  List<StackTrace>? debugGetOpenHandleStackTraces() {
+    List<StackTrace>? stacks;


It looks like there's an opportunity with this new API to avoid allowing nulls. How about initializing this to List.empty() and getting rid of the ? from the return type?

I think we usually make the debug* methods return null in the framework, but I'm not opposed to this.

Is there an advantage to this? This method will never return a meaningful value in release.

If there's already a convention around this, then sticking with that for this PR sgtm.

The advantage is just avoiding mistakes around null and having the ?'s propagate around everywhere.

dnfield · 2020-09-17T23:29:16Z

Blocked on flutter/flutter#65876

dnfield · 2020-09-22T18:59:10Z

Framework side tests that were breaking for this have been updated, this should all be green now.

dnfield · 2020-09-22T19:00:07Z

To clarify my last comment - I've removed all fake implementations of ui.Image from the framework test suites, which was dangerous for two reasons:

It broke if the contract for ui.Image changed, but ui.Image is documented as something you should not extend or implement.
It crashed the tester process if one of those fake implementations ever managed to make it to C++ code, where C++ would look for native pointers that didn't exist.

dnfield · 2020-09-22T20:28:37Z

There is one remaining test file to fix in the framework for this :(

I also have to figure out what to do with whether we tree shake _image from Image now, since we run tests in release mode for this.

dnfield · 2020-09-22T22:25:20Z

All blocking PRs have landed.

* b49de93 Create an ImageHandle wrapper (flutter/engine#21057) * b0fb2c8 Roll Skia from 7b97b3cb2bd0 to 59b2a92c96ba (4 revisions) (flutter/engine#21365)

Create an ImageHandle wrapper

aa5caba

dnfield requested review from goderbauer and zanderso September 9, 2020 21:47

googlebot added the cla: yes label Sep 9, 2020