Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 156 additions & 8 deletions lib/ui/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// 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
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, move this closer to the dispose method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sounds good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 dart:ui. I've updated the text to recommend filing a bug.

);
_disposed = true;
return true;
}());
_dispose();

}

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;

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;
Copy link
Member

Choose a reason for hiding this comment

The 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 List.empty() and getting rid of the ? from the return type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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 null and having the ?'s propagate around everywhere.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

/// is the last remaining handle.
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done and added a code sample.

Image createHandle() {
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clone SGTM

if (_disposed) {
throw StateError('Object disposed');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we come up with a better error message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

}
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;
Copy link
Member

Choose a reason for hiding this comment

The 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();
}
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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?".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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).

}

/// Unused private implementation of [Image]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// rather than /// I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);

Expand Down Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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
);
}
Expand Down Expand Up @@ -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
);
}
Expand Down
4 changes: 4 additions & 0 deletions lib/web_ui/lib/src/ui/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,10 @@ abstract class Image {
Future<ByteData?> toByteData({ImageByteFormat format = ImageByteFormat.rawRgba});
void dispose();

Image createHandle() => this;

List<StackTrace>? debugGetOpenHandleStackTraces() => null;

@override
String toString() => '[$width\u00D7$height]';
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,11 @@ class TestImage implements Image {

@override
void dispose() {}

@override
Image createHandle() => this;

List<StackTrace>? debugGetOpenHandleStackTraces() => <StackTrace>[];
}

Paragraph createTestParagraph() {
Expand Down
105 changes: 105 additions & 0 deletions testing/dart/image_dispose_test.dart
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();
}