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 5 commits
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
7 changes: 5 additions & 2 deletions lib/ui/compositing.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added some more details here.

callback(Image._(image));
}),
);
}

String _toImage(int width, int height, _Callback<Image> callback) native 'Scene_toImage';
String _toImage(int width, int height, _Callback<_Image> callback) native 'Scene_toImage';

/// Releases the resources used by this scene.
///
Expand Down
173 changes: 145 additions & 28 deletions lib/ui/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1566,27 +1566,72 @@ enum PixelFormat {
/// To draw an [Image], use one of the methods on the [Canvas] class, such as
/// [Canvas.drawImage].
///
/// A class or method that receives an image object must call [dispose] on the
/// handle when it is no longer needed. To create a shareable reference to the
/// underlying image, call [createHandle]. The method or object that recieves
/// the new instance will then be responsible for disposing it, and the
/// underlying image itself will be disposed when all outstanding handles are
/// disposed.
///
/// If `dart:ui` passes an `Image` object and the recipient wishes to share
/// that handle with other callers, it is critical that [createHandle] is called
/// _before_ [dispose]. A handle that has been disposed cannot create new
/// handles anymore.
///
/// 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
// or extended directly.
//
// To obtain an [Image] object, use [instantiateImageCodec].
@pragma('vm:entry-point')
Image._();
class Image {
Image._(this._image) {
assert(() {
_debugStack = StackTrace.current;
return true;
}());
_image._handles.add(this);
}

final _Image _image;

StackTrace? _debugStack;

/// The number of image pixels along the image's horizontal axis.
int get width native 'Image_width';
int get width {
assert(!_disposed && !_image._disposed);
return _image.width;
}

/// The number of image pixels along the image's vertical axis.
int get height native 'Image_height';
int get height {
assert(!_disposed && !_image._disposed);
return _image.height;
}

/// Release this handle's claim on the underlying Image. This handle is no
/// longer usable after this method is called.
///
/// Once all outstanding handles have been disposed, the underlying image will
/// be disposed as well.
///
/// In debug mode, [debugGetOpenHandleStackTraces] will return a list of
/// [StackTrace] objects from all open handles' creation points. This is
/// useful when trying to determine what parts of the program are keeping an
/// image resident in memory.
void dispose() {
assert(() {
assert(!_disposed && !_image._disposed);
assert(_image._handles.contains(this));
_disposed = true;
return true;
}());
final bool removed = _image._handles.remove(this);
assert(removed);
if (_image._handles.isEmpty) {
_image.dispose();
}
}

/// Converts the [Image] object into a byte array.
///
Expand All @@ -1595,6 +1640,56 @@ class Image extends NativeFieldWrapperClass2 {
///
/// Returns a future that completes with the binary image data or an error
/// if encoding fails.
Future<ByteData?> toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) {
assert(!_disposed && !_image._disposed);
return _image.toByteData(format: format);
}

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.


/// 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 && !_image._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 = _image._handles.map((Image 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

}
return Image._(_image);
}

@override
String toString() => _image.toString();
}

@pragma('vm:entry-point')
class _Image extends NativeFieldWrapperClass2 {
// This class is created by the engine, and should not be instantiated
// or extended directly.
//
// To obtain an [Image] object, use [instantiateImageCodec].
Copy link
Member

Choose a reason for hiding this comment

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

This comment referencing Image on _Image seems confusing...

Should this instead be on the private constructor of Image?

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'll reword this. It's leftover.

@pragma('vm:entry-point')
_Image._();

int get width native 'Image_width';

int get height native 'Image_height';

Future<ByteData?> toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) {
return _futurize((_Callback<ByteData> callback) {
return _toByteData(format.index, (Uint8List? encoded) {
Expand All @@ -1606,9 +1701,26 @@ class Image extends NativeFieldWrapperClass2 {
/// Returns an error message on failure, null on success.
String? _toByteData(int format, _Callback<Uint8List?> callback) native 'Image_toByteData';

/// 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;
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?

void dispose() {
assert(() {
assert(!_disposed);
assert(
_handles.isEmpty,
'Attempted to dispose of an Image object that has ${_handles.length} '
'open handles.\n'
'If you see this, it is a bug in dart:ui. Please file an issue at '
'https://github.com/flutter/flutter/issues/new.',
);
_disposed = true;
return true;
}());
_dispose();
}

void _dispose() native 'Image_dispose';

Set<Image> _handles = <Image>{};

@override
String toString() => '[$width\u00D7$height]';
Expand All @@ -1635,8 +1747,11 @@ class FrameInfo extends NativeFieldWrapperClass2 {
Duration get duration => Duration(milliseconds: _durationMillis);
int get _durationMillis native 'FrameInfo_durationMillis';

Image? _cachedImage;

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


/// A handle to an image codec.
Expand Down Expand Up @@ -3239,10 +3354,10 @@ 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._image, tmx.index, tmy.index, matrix4);
}
void _constructor() native 'ImageShader_constructor';
void _initWithImage(Image image, int tmx, int tmy, Float64List matrix4) native 'ImageShader_initWithImage';
void _initWithImage(_Image image, int tmx, int tmy, Float64List matrix4) native 'ImageShader_initWithImage';
}

/// Defines how a list of points is interpreted when drawing a set of triangles.
Expand Down Expand Up @@ -3843,9 +3958,9 @@ 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._image, offset.dx, offset.dy, paint._objects, paint._data);
}
void _drawImage(Image image,
void _drawImage(_Image image,
double x,
double y,
List<dynamic>? paintObjects,
Expand All @@ -3866,7 +3981,7 @@ class Canvas extends NativeFieldWrapperClass2 {
assert(_rectIsValid(src));
assert(_rectIsValid(dst));
assert(paint != null); // ignore: unnecessary_null_comparison
_drawImageRect(image,
_drawImageRect(image._image,
src.left,
src.top,
src.right,
Expand All @@ -3878,7 +3993,7 @@ class Canvas extends NativeFieldWrapperClass2 {
paint._objects,
paint._data);
}
void _drawImageRect(Image image,
void _drawImageRect(_Image image,
double srcLeft,
double srcTop,
double srcRight,
Expand Down Expand Up @@ -3909,7 +4024,7 @@ class Canvas extends NativeFieldWrapperClass2 {
assert(_rectIsValid(center));
assert(_rectIsValid(dst));
assert(paint != null); // ignore: unnecessary_null_comparison
_drawImageNine(image,
_drawImageNine(image._image,
center.left,
center.top,
center.right,
Expand All @@ -3921,7 +4036,7 @@ class Canvas extends NativeFieldWrapperClass2 {
paint._objects,
paint._data);
}
void _drawImageNine(Image image,
void _drawImageNine(_Image image,
double centerLeft,
double centerTop,
double centerRight,
Expand Down Expand Up @@ -4198,7 +4313,7 @@ class Canvas extends NativeFieldWrapperClass2 {
final Float32List? cullRectBuffer = cullRect?._value32;

_drawAtlas(
paint._objects, paint._data, atlas, rstTransformBuffer, rectBuffer,
paint._objects, paint._data, atlas._image, rstTransformBuffer, rectBuffer,
colorBuffer, (blendMode ?? BlendMode.src).index, cullRectBuffer
);
}
Expand Down Expand Up @@ -4367,14 +4482,14 @@ 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._image, rstTransforms, rects,
colors, (blendMode ?? BlendMode.src).index, cullRect?._value32
);
}

void _drawAtlas(List<dynamic>? paintObjects,
ByteData paintData,
Image atlas,
_Image atlas,
Float32List rstTransforms,
Float32List rects,
Int32List? colors,
Expand Down Expand Up @@ -4428,11 +4543,13 @@ class Picture extends NativeFieldWrapperClass2 {
if (width <= 0 || height <= 0)
throw Exception('Invalid image dimensions.');
return _futurize(
(_Callback<Image> callback) => _toImage(width, height, callback)
(_Callback<Image> callback) => _toImage(width, height, (_Image image) {
callback(Image._(image));
}),
);
}

String _toImage(int width, int height, _Callback<Image> callback) native 'Picture_toImage';
String _toImage(int width, int height, _Callback<_Image> callback) native 'Picture_toImage';

/// Release the resources used by this object. The object is no longer usable
/// after this method is called.
Expand Down
9 changes: 8 additions & 1 deletion lib/ui/painting/image.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,14 @@ namespace flutter {

typedef CanvasImage Image;

IMPLEMENT_WRAPPERTYPEINFO(ui, Image);
// Since _Image is a private class, we can't use IMPLEMENT_WRAPPERTYPEINFO
static const tonic::DartWrapperInfo kDartWrapperInfo_ui_Image = {
"ui",
"_Image",
sizeof(Image),
};
const tonic::DartWrapperInfo& Image::dart_wrapper_info_ =
kDartWrapperInfo_ui_Image;

#define FOR_EACH_BINDING(V) \
V(Image, width) \
Expand Down
11 changes: 11 additions & 0 deletions lib/web_ui/lib/src/engine/canvaskit/image.dart
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ class CkAnimatedImage implements ui.Image {
box.delete();
}

@override
ui.Image createHandle() => this;

@override
List<StackTrace>? debugGetOpenHandleStackTraces() => null;
int get frameCount => _skAnimatedImage.getFrameCount();

/// Decodes the next frame and returns the frame duration.
Expand Down Expand Up @@ -114,6 +119,12 @@ class CkImage implements ui.Image {
box.delete();
}

@override
ui.Image createHandle() => this;

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

@override
int get width => skImage.width();

Expand Down
6 changes: 6 additions & 0 deletions lib/web_ui/lib/src/engine/html_image_codec.dart
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,12 @@ class HtmlImage implements ui.Image {
// releasing the object url.
}

@override
ui.Image createHandle() => this;

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

@override
final int width;

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
Loading