-
Notifications
You must be signed in to change notification settings - Fork 6k
Create an ImageHandle wrapper #21057
Changes from 1 commit
aa5caba
df403f3
6b700aa
8537a70
b69d226
0cf431c
4f15600
9eef907
17254e3
cb29ca9
fdaeb11
f00dd18
31d47de
6bd80bb
03324dc
084796f
bf4cfeb
e38a09d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1566,13 +1566,25 @@ enum PixelFormat { | |
| /// To draw an [Image], use one of the methods on the [Canvas] class, such as | ||
| /// [Canvas.drawImage]. | ||
| /// | ||
| /// A class or method that recieves an image object should call [createHandle] | ||
| /// immediately and then call [dispose] on the handle when it is no longer | ||
| /// needed. The underlying image data will be released only when all outstanding | ||
| /// handles are disposed. | ||
| /// | ||
| /// It is also possible to call dispose directly on the image object received | ||
| /// from [FrameInfo.image]. Doing so will attempt to free any native resources | ||
| /// allocated for the object, but it will trigger an assert if there are any | ||
| /// oustanding handles created by [createHandle] for this image. | ||
| /// | ||
| /// Once all handles have been disposed of, the image object is no longer usable | ||
|
dnfield marked this conversation as resolved.
Outdated
|
||
| /// from Dart code, including for creating new handles. | ||
| /// | ||
| /// See also: | ||
| /// | ||
| /// * [Image](https://api.flutter.dev/flutter/widgets/Image-class.html), the class in the [widgets] library. | ||
| /// * [ImageDescriptor], which allows reading information about the image and | ||
| /// creating a codec to decode it. | ||
| /// * [instantiateImageCodec], a utility method that wraps [ImageDescriptor]. | ||
| /// | ||
| @pragma('vm:entry-point') | ||
| class Image extends NativeFieldWrapperClass2 { | ||
| // This class is created by the engine, and should not be instantiated | ||
|
|
@@ -1606,14 +1618,150 @@ class Image extends NativeFieldWrapperClass2 { | |
| /// Returns an error message on failure, null on success. | ||
| String? _toByteData(int format, _Callback<Uint8List?> callback) native 'Image_toByteData'; | ||
|
|
||
| bool _disposed = false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be named _debugDisposed since its only used within asserts and will only be valid in debug mode.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, move this closer to the dispose method?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved it, but also moved setting it out of asserts, since I think the runtime failure mode without it will be pretty confusing. |
||
| /// Release the resources used by this object. The object is no longer usable | ||
| /// after this method is called. | ||
| void dispose() native 'Image_dispose'; | ||
| /// | ||
| /// All outstanding handles from [createHandle] should be disposed before | ||
| /// calling this. Disposing all outstanding handles will automatically | ||
| /// dispose this object. | ||
| void dispose() { | ||
| assert(() { | ||
| assert(!_disposed); | ||
| assert( | ||
| _handles.isEmpty, | ||
| 'Attempted to dispose of an Image object that has ${_handles.length} open handles.', | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this print the (de-duped?) stack traces of the open handles?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds good.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is now just an assertion that should never fire, unless we introduce a bug into |
||
| ); | ||
| _disposed = true; | ||
| return true; | ||
| }()); | ||
| _dispose(); | ||
|
|
||
|
dnfield marked this conversation as resolved.
Outdated
|
||
| } | ||
|
|
||
| void _dispose() native 'Image_dispose'; | ||
|
|
||
| /// Returns the native wrapper of the Image. Any calls to `native` must use | ||
| /// this getter to avoid passing an [_ImageHandle] to native, which will | ||
| /// crash. | ||
| Image get _unwrapped => this; | ||
|
dnfield marked this conversation as resolved.
Outdated
|
||
|
|
||
| Set<_ImageHandle> _handles = <_ImageHandle>{}; | ||
|
|
||
| /// If asserts are enabled, returns the [StackTrace]s of each open handle from | ||
| /// [createHandle], in creation order. | ||
| /// | ||
| /// If asserts are disabled, this method always returns null. | ||
| List<StackTrace>? debugGetOpenHandleStackTraces() { | ||
| assert(!_disposed); | ||
| List<StackTrace>? stacks; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like there's an opportunity with this new API to avoid allowing nulls. How about initializing this to
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we usually make the Is there an advantage to this? This method will never return a meaningful value in release.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If there's already a convention around this, then sticking with that for this PR sgtm. The advantage is just avoiding mistakes around |
||
| assert(() { | ||
| stacks = _handles.map((_ImageHandle handle) => handle.debugStack!).toList(); | ||
| return true; | ||
| }()); | ||
| return stacks; | ||
| } | ||
|
|
||
| /// 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. docs need updating ("except" is no longer accurate)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| /// is the last remaining handle. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe add some additional details to describe when an additional handle should be created? And that it needs to be disposed to avoid leaks?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done and added a code sample. |
||
| Image createHandle() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clone SGTM |
||
| if (_disposed) { | ||
| throw StateError('Object disposed'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we come up with a better error message?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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."
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| } | ||
|
Comment on lines
+1731
to
+1738
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be wrapped in an assert? _dispose is only ever set to true when asserts are enabled.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| final _ImageHandle handle = _ImageHandle(this); | ||
| _handles.add(handle); | ||
| return handle; | ||
| } | ||
|
|
||
| @override | ||
| String toString() => '[$width\u00D7$height]'; | ||
| } | ||
|
|
||
| /// A disposable handle to an [Image]. | ||
| /// | ||
| /// Handles can create more handles as long as they (and the underlying image) | ||
| /// are not disposed. | ||
| class _ImageHandle implements Image { | ||
| _ImageHandle(this._image) { | ||
| assert(() { | ||
| debugStack = StackTrace.current; | ||
| return true; | ||
| }()); | ||
| } | ||
|
|
||
| final Image _image; | ||
|
|
||
| StackTrace? debugStack; | ||
|
|
||
| @override | ||
| int get width { | ||
| assert(!_disposed && !_image._disposed); | ||
| return _image.width; | ||
| } | ||
|
|
||
| @override | ||
| int get height { | ||
| assert(!_disposed && !_image._disposed); | ||
| return _image.height; | ||
| } | ||
|
|
||
| @override | ||
| Future<ByteData?> toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) { | ||
| assert(!_disposed && !_image._disposed); | ||
| return _image.toByteData(format: format); | ||
| } | ||
|
|
||
| @override | ||
| Image get _unwrapped { | ||
| assert(!_disposed && !_image._disposed); | ||
| return _image; | ||
| } | ||
|
|
||
| @override | ||
| List<StackTrace>? debugGetOpenHandleStackTraces() { | ||
| assert(!_disposed && !_image._disposed); | ||
| return _image.debugGetOpenHandleStackTraces(); | ||
| } | ||
|
|
||
|
|
||
| @override | ||
| Image createHandle() { | ||
| assert(!_disposed && !_image._disposed); | ||
| return _image.createHandle(); | ||
| } | ||
|
|
||
| @override | ||
| bool _disposed = false; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since this is only set in asserts, call it _debugDispose? |
||
|
|
||
| @override | ||
| void dispose() { | ||
| assert(() { | ||
| assert(!_disposed && !_image._disposed); | ||
| assert(_image._handles.contains(this)); | ||
| _disposed = true; | ||
| return true; | ||
| }()); | ||
| _image._handles.remove(this); | ||
| if (_image._handles.isEmpty) { | ||
| _image.dispose(); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I suspect having both
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @goderbauer and I were talking about this and were a little concerned about what this does in It makes the ownership model slightly more confusing, since
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And in case it's not clear, if we leave
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Put another way, if you want to give an image reference to someone, you need to call |
||
| } | ||
|
|
||
| /// Unused private implementation of [Image] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| @override | ||
| Set<_ImageHandle> _handles = <_ImageHandle>{}; | ||
|
|
||
| @override | ||
| void _dispose() => throw UnimplementedError(); | ||
|
|
||
| @override | ||
| String? _toByteData(int format, _Callback<Uint8List?> callback) => throw UnimplementedError(); | ||
| } | ||
|
|
||
| /// Callback signature for [decodeImageFromList]. | ||
| typedef ImageDecoderCallback = void Function(Image result); | ||
|
|
||
|
|
@@ -3239,7 +3387,7 @@ class ImageShader extends Shader { | |
| if (matrix4.length != 16) | ||
| throw ArgumentError('"matrix4" must have 16 entries.'); | ||
| _constructor(); | ||
| _initWithImage(image, tmx.index, tmy.index, matrix4); | ||
| _initWithImage(image._unwrapped, tmx.index, tmy.index, matrix4); | ||
| } | ||
| void _constructor() native 'ImageShader_constructor'; | ||
| void _initWithImage(Image image, int tmx, int tmy, Float64List matrix4) native 'ImageShader_initWithImage'; | ||
|
|
@@ -3843,7 +3991,7 @@ class Canvas extends NativeFieldWrapperClass2 { | |
| assert(image != null); // image is checked on the engine side | ||
| assert(_offsetIsValid(offset)); | ||
| assert(paint != null); // ignore: unnecessary_null_comparison | ||
| _drawImage(image, offset.dx, offset.dy, paint._objects, paint._data); | ||
| _drawImage(image._unwrapped, offset.dx, offset.dy, paint._objects, paint._data); | ||
| } | ||
| void _drawImage(Image image, | ||
| double x, | ||
|
|
@@ -3866,7 +4014,7 @@ class Canvas extends NativeFieldWrapperClass2 { | |
| assert(_rectIsValid(src)); | ||
| assert(_rectIsValid(dst)); | ||
| assert(paint != null); // ignore: unnecessary_null_comparison | ||
| _drawImageRect(image, | ||
| _drawImageRect(image._unwrapped, | ||
| src.left, | ||
| src.top, | ||
| src.right, | ||
|
|
@@ -3909,7 +4057,7 @@ class Canvas extends NativeFieldWrapperClass2 { | |
| assert(_rectIsValid(center)); | ||
| assert(_rectIsValid(dst)); | ||
| assert(paint != null); // ignore: unnecessary_null_comparison | ||
| _drawImageNine(image, | ||
| _drawImageNine(image._unwrapped, | ||
| center.left, | ||
| center.top, | ||
| center.right, | ||
|
|
@@ -4198,7 +4346,7 @@ class Canvas extends NativeFieldWrapperClass2 { | |
| final Float32List? cullRectBuffer = cullRect?._value32; | ||
|
|
||
| _drawAtlas( | ||
| paint._objects, paint._data, atlas, rstTransformBuffer, rectBuffer, | ||
| paint._objects, paint._data, atlas._unwrapped, rstTransformBuffer, rectBuffer, | ||
| colorBuffer, (blendMode ?? BlendMode.src).index, cullRectBuffer | ||
| ); | ||
| } | ||
|
|
@@ -4367,7 +4515,7 @@ class Canvas extends NativeFieldWrapperClass2 { | |
| throw ArgumentError('If non-null, "colors" length must be one fourth the length of "rstTransforms" and "rects".'); | ||
|
|
||
| _drawAtlas( | ||
| paint._objects, paint._data, atlas, rstTransforms, rects, | ||
| paint._objects, paint._data, atlas._unwrapped, rstTransforms, rects, | ||
| colors, (blendMode ?? BlendMode.src).index, cullRect?._value32 | ||
| ); | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| // Copyright 2019 The Flutter Authors. All rights reserved. | ||
| // Use of this source code is governed by a BSD-style license that can be | ||
| // found in the LICENSE file. | ||
|
|
||
| // @dart = 2.6 | ||
| import 'dart:io'; | ||
| import 'dart:typed_data'; | ||
| import 'dart:ui'; | ||
|
|
||
| import 'package:path/path.dart' as path; | ||
| import 'package:test/test.dart'; | ||
|
|
||
| void main() { | ||
| bool assertsEnabled = false; | ||
| assert(() { | ||
| assertsEnabled = true; | ||
| return true; | ||
| }()); | ||
| final Matcher throwsAssertionError = throwsA(const TypeMatcher<AssertionError>()); | ||
|
|
||
| test('Image.dispose asserts if handles are active', () async { | ||
| final Uint8List bytes = await readFile('2x2.png'); | ||
| final Codec codec = await instantiateImageCodec(bytes); | ||
| final FrameInfo frame = await codec.getNextFrame(); | ||
|
|
||
| expect(frame.image.width, 2); | ||
| expect(frame.image.height, 2); | ||
| final Image handle1 = frame.image.createHandle(); | ||
| expect(handle1.width, frame.image.width); | ||
| expect(handle1.height, frame.image.height); | ||
|
|
||
| final Image handle2 = handle1.createHandle(); | ||
| expect(handle1 != handle2, true); | ||
|
|
||
| if (assertsEnabled) { | ||
| expect(() => frame.image.dispose(), throwsAssertionError); | ||
| handle1.dispose(); | ||
| expect(() => frame.image.dispose(), throwsAssertionError); | ||
| handle2.dispose(); | ||
| } | ||
| frame.image.dispose(); | ||
| }); | ||
|
|
||
| test('Canvas can paint image from handle and byte data from handle', () async { | ||
| final Uint8List bytes = await readFile('2x2.png'); | ||
| final Codec codec = await instantiateImageCodec(bytes); | ||
| final FrameInfo frame = await codec.getNextFrame(); | ||
|
|
||
| expect(frame.image.width, 2); | ||
| expect(frame.image.height, 2); | ||
| final Image handle1 = frame.image.createHandle(); | ||
|
|
||
| final PictureRecorder recorder = PictureRecorder(); | ||
| final Canvas canvas = Canvas(recorder); | ||
|
|
||
| const Rect rect = Rect.fromLTRB(0, 0, 2, 2); | ||
| canvas.drawImage(handle1, Offset.zero, Paint()); | ||
| canvas.drawImageRect(handle1, rect, rect, Paint()); | ||
| canvas.drawImageNine(handle1, rect, rect, Paint()); | ||
| canvas.drawAtlas(handle1, <RSTransform>[], <Rect>[], <Color>[], BlendMode.src, rect, Paint()); | ||
| canvas.drawRawAtlas(handle1, Float32List(0), Float32List(0), Int32List(0), BlendMode.src, rect, Paint()); | ||
|
|
||
| final Picture picture = recorder.endRecording(); | ||
|
|
||
| final Image rasterizedImage = await picture.toImage(2, 2); | ||
| final ByteData sourceData = await frame.image.toByteData(); | ||
| final ByteData handleData = await handle1.toByteData(); | ||
| final ByteData rasterizedData = await rasterizedImage.toByteData(); | ||
|
|
||
| expect(sourceData.buffer.asUint8List(), equals(handleData.buffer.asUint8List())); | ||
| expect(sourceData.buffer.asUint8List(), equals(rasterizedData.buffer.asUint8List())); | ||
| }); | ||
|
|
||
| test('Records stack traces', () async { | ||
| final Uint8List bytes = await readFile('2x2.png'); | ||
| final Codec codec = await instantiateImageCodec(bytes); | ||
| final FrameInfo frame = await codec.getNextFrame(); | ||
|
|
||
| final Image handle1 = frame.image.createHandle(); | ||
| final Image handle2 = handle1.createHandle(); | ||
|
|
||
| List<StackTrace> stackTraces = frame.image.debugGetOpenHandleStackTraces(); | ||
| expect(stackTraces.length, 2); | ||
| expect(stackTraces, equals(handle2.debugGetOpenHandleStackTraces())); | ||
|
|
||
| handle1.dispose(); | ||
| stackTraces = frame.image.debugGetOpenHandleStackTraces(); | ||
| expect(stackTraces.length, 1); | ||
| expect(stackTraces, equals(handle2.debugGetOpenHandleStackTraces())); | ||
|
|
||
| handle2.dispose(); | ||
| stackTraces = frame.image.debugGetOpenHandleStackTraces(); | ||
| expect(stackTraces.length, 0); | ||
| }, skip: !assertsEnabled); | ||
| } | ||
|
|
||
| Future<Uint8List> readFile(String fileName) async { | ||
| final File file = File(path.join( | ||
| 'flutter', | ||
| 'testing', | ||
| 'resources', | ||
| fileName, | ||
| )); | ||
| return await file.readAsBytes(); | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.