From aa5cabac200437ed9a37e1a465844e3fad697ebd Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 9 Sep 2020 14:44:25 -0700 Subject: [PATCH 01/14] Create an ImageHandle wrapper --- lib/ui/painting.dart | 164 +++++++++++++++++- lib/web_ui/lib/src/ui/painting.dart | 4 + .../engine/recording_canvas_golden_test.dart | 5 + testing/dart/image_dispose_test.dart | 105 +++++++++++ 4 files changed, 270 insertions(+), 8 deletions(-) create mode 100644 testing/dart/image_dispose_test.dart diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index cfafb039c428f..d7f83e0d4994b 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -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 @@ -1606,14 +1618,150 @@ class Image extends NativeFieldWrapperClass2 { /// Returns an error message on failure, null on success. String? _toByteData(int format, _Callback callback) native 'Image_toByteData'; + bool _disposed = false; /// 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.', + ); + _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? debugGetOpenHandleStackTraces() { + assert(!_disposed); + List? stacks; + 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 + /// is the last remaining handle. + Image createHandle() { + if (_disposed) { + throw StateError('Object disposed'); + } + 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 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? debugGetOpenHandleStackTraces() { + assert(!_disposed && !_image._disposed); + return _image.debugGetOpenHandleStackTraces(); + } + + + @override + Image createHandle() { + assert(!_disposed && !_image._disposed); + return _image.createHandle(); + } + + @override + bool _disposed = false; + + @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(); + } + } + + /// Unused private implementation of [Image] + + @override + Set<_ImageHandle> _handles = <_ImageHandle>{}; + + @override + void _dispose() => throw UnimplementedError(); + + @override + String? _toByteData(int format, _Callback 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 ); } diff --git a/lib/web_ui/lib/src/ui/painting.dart b/lib/web_ui/lib/src/ui/painting.dart index fef5fa67c7809..b55ea8ff895e8 100644 --- a/lib/web_ui/lib/src/ui/painting.dart +++ b/lib/web_ui/lib/src/ui/painting.dart @@ -330,6 +330,10 @@ abstract class Image { Future toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}); void dispose(); + Image createHandle() => this; + + List? debugGetOpenHandleStackTraces() => null; + @override String toString() => '[$width\u00D7$height]'; } diff --git a/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart b/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart index cdd6bfca11966..cee7059fd6035 100644 --- a/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart +++ b/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart @@ -718,6 +718,11 @@ class TestImage implements Image { @override void dispose() {} + + @override + Image createHandle() => this; + + List? debugGetOpenHandleStackTraces() => []; } Paragraph createTestParagraph() { diff --git a/testing/dart/image_dispose_test.dart b/testing/dart/image_dispose_test.dart new file mode 100644 index 0000000000000..73e7b77eeecd1 --- /dev/null +++ b/testing/dart/image_dispose_test.dart @@ -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()); + + 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, [], [], [], 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 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 readFile(String fileName) async { + final File file = File(path.join( + 'flutter', + 'testing', + 'resources', + fileName, + )); + return await file.readAsBytes(); +} From df403f30e564de15ddf0df2cccc3e5e38e498cf1 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Wed, 9 Sep 2020 17:03:09 -0700 Subject: [PATCH 02/14] review --- lib/ui/painting.dart | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index d7f83e0d4994b..6dc3d6bd377a2 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1566,7 +1566,7 @@ 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] +/// A class or method that receives 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. @@ -1576,7 +1576,7 @@ enum PixelFormat { /// 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 +/// Once all handles have been disposed, the image object is no longer usable /// from Dart code, including for creating new handles. /// /// See also: @@ -1636,7 +1636,6 @@ class Image extends NativeFieldWrapperClass2 { return true; }()); _dispose(); - } void _dispose() native 'Image_dispose'; From 6b700aaac507900aca1dc3bf9312cfd4057fd658 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 10 Sep 2020 17:38:57 -0700 Subject: [PATCH 03/14] Only handles, no virtual dispatch --- lib/ui/compositing.dart | 7 +- lib/ui/painting.dart | 213 +++++++++++---------------- lib/ui/painting/image.cc | 9 +- testing/dart/image_dispose_test.dart | 22 +-- 4 files changed, 114 insertions(+), 137 deletions(-) diff --git a/lib/ui/compositing.dart b/lib/ui/compositing.dart index 39b0d63fc2b36..3eccc80edf6e0 100644 --- a/lib/ui/compositing.dart +++ b/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 callback) => _toImage(width, height, callback)); + return _futurize((_Callback callback) => _toImage(width, height, (_Image image) { + callback(Image._(image)); + }), + ); } - String _toImage(int width, int height, _Callback callback) native 'Scene_toImage'; + String _toImage(int width, int height, _Callback<_Image> callback) native 'Scene_toImage'; /// Releases the resources used by this scene. /// diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 6dc3d6bd377a2..2fe91de1ab8a5 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1585,40 +1585,31 @@ enum PixelFormat { /// * [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); + } - /// The number of image pixels along the image's horizontal axis. - int get width native 'Image_width'; + final _Image _image; - /// The number of image pixels along the image's vertical axis. - int get height native 'Image_height'; + StackTrace? _debugStack; - /// Converts the [Image] object into a byte array. - /// - /// The [format] argument specifies the format in which the bytes will be - /// returned. - /// - /// Returns a future that completes with the binary image data or an error - /// if encoding fails. - Future toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) { - return _futurize((_Callback callback) { - return _toByteData(format.index, (Uint8List? encoded) { - callback(encoded!.buffer.asByteData()); - }); - }); + /// The number of image pixels along the image's horizontal axis. + int get width { + assert(!_disposed && !_image._disposed); + return _image.width; } - /// Returns an error message on failure, null on success. - String? _toByteData(int format, _Callback callback) native 'Image_toByteData'; + /// The number of image pixels along the image's vertical axis. + int get height { + assert(!_disposed && !_image._disposed); + return _image.height; + } - bool _disposed = false; /// Release the resources used by this object. The object is no longer usable /// after this method is called. /// @@ -1627,35 +1618,41 @@ class Image extends NativeFieldWrapperClass2 { /// dispose this object. void dispose() { assert(() { - assert(!_disposed); - assert( - _handles.isEmpty, - 'Attempted to dispose of an Image object that has ${_handles.length} open handles.', - ); + assert(!_disposed && !_image._disposed); + assert(_image._handles.contains(this)); _disposed = true; return true; }()); - _dispose(); + final bool removed = _image._handles.remove(this); + assert(removed); + if (_image._handles.isEmpty) { + _image.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; + /// Converts the [Image] object into a byte array. + /// + /// The [format] argument specifies the format in which the bytes will be + /// returned. + /// + /// Returns a future that completes with the binary image data or an error + /// if encoding fails. + Future toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}) { + assert(!_disposed && !_image._disposed); + return _image.toByteData(format: format); + } - Set<_ImageHandle> _handles = <_ImageHandle>{}; + bool _disposed = false; /// 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? debugGetOpenHandleStackTraces() { - assert(!_disposed); + assert(!_disposed && !_image._disposed); List? stacks; assert(() { - stacks = _handles.map((_ImageHandle handle) => handle.debugStack!).toList(); + stacks = _image._handles.map((Image handle) => handle._debugStack!).toList(); return true; }()); return stacks; @@ -1670,95 +1667,60 @@ class Image extends NativeFieldWrapperClass2 { if (_disposed) { throw StateError('Object disposed'); } - final _ImageHandle handle = _ImageHandle(this); - _handles.add(handle); - return handle; + return Image._(_image); } @override - String toString() => '[$width\u00D7$height]'; + String toString() => _image.toString(); } -/// 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; +@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._(); - @override - int get width { - assert(!_disposed && !_image._disposed); - return _image.width; - } + int get width native 'Image_width'; - @override - int get height { - assert(!_disposed && !_image._disposed); - return _image.height; - } + int get height native 'Image_height'; - @override Future 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? debugGetOpenHandleStackTraces() { - assert(!_disposed && !_image._disposed); - return _image.debugGetOpenHandleStackTraces(); + return _futurize((_Callback callback) { + return _toByteData(format.index, (Uint8List? encoded) { + callback(encoded!.buffer.asByteData()); + }); + }); } + /// Returns an error message on failure, null on success. + String? _toByteData(int format, _Callback callback) native 'Image_toByteData'; - @override - Image createHandle() { - assert(!_disposed && !_image._disposed); - return _image.createHandle(); - } - - @override bool _disposed = false; - - @override void dispose() { assert(() { - assert(!_disposed && !_image._disposed); - assert(_image._handles.contains(this)); + 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; }()); - _image._handles.remove(this); - if (_image._handles.isEmpty) { - _image.dispose(); - } + _dispose(); } - /// Unused private implementation of [Image] - - @override - Set<_ImageHandle> _handles = <_ImageHandle>{}; + void _dispose() native 'Image_dispose'; - @override - void _dispose() => throw UnimplementedError(); + Set _handles = {}; @override - String? _toByteData(int format, _Callback callback) => throw UnimplementedError(); + String toString() => '[$width\u00D7$height]'; } /// Callback signature for [decodeImageFromList]. @@ -1782,8 +1744,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'; } /// A handle to an image codec. @@ -3386,10 +3351,10 @@ class ImageShader extends Shader { if (matrix4.length != 16) throw ArgumentError('"matrix4" must have 16 entries.'); _constructor(); - _initWithImage(image._unwrapped, 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. @@ -3990,9 +3955,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._unwrapped, 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? paintObjects, @@ -4013,7 +3978,7 @@ class Canvas extends NativeFieldWrapperClass2 { assert(_rectIsValid(src)); assert(_rectIsValid(dst)); assert(paint != null); // ignore: unnecessary_null_comparison - _drawImageRect(image._unwrapped, + _drawImageRect(image._image, src.left, src.top, src.right, @@ -4025,7 +3990,7 @@ class Canvas extends NativeFieldWrapperClass2 { paint._objects, paint._data); } - void _drawImageRect(Image image, + void _drawImageRect(_Image image, double srcLeft, double srcTop, double srcRight, @@ -4056,7 +4021,7 @@ class Canvas extends NativeFieldWrapperClass2 { assert(_rectIsValid(center)); assert(_rectIsValid(dst)); assert(paint != null); // ignore: unnecessary_null_comparison - _drawImageNine(image._unwrapped, + _drawImageNine(image._image, center.left, center.top, center.right, @@ -4068,7 +4033,7 @@ class Canvas extends NativeFieldWrapperClass2 { paint._objects, paint._data); } - void _drawImageNine(Image image, + void _drawImageNine(_Image image, double centerLeft, double centerTop, double centerRight, @@ -4345,7 +4310,7 @@ class Canvas extends NativeFieldWrapperClass2 { final Float32List? cullRectBuffer = cullRect?._value32; _drawAtlas( - paint._objects, paint._data, atlas._unwrapped, rstTransformBuffer, rectBuffer, + paint._objects, paint._data, atlas._image, rstTransformBuffer, rectBuffer, colorBuffer, (blendMode ?? BlendMode.src).index, cullRectBuffer ); } @@ -4514,14 +4479,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._unwrapped, rstTransforms, rects, + paint._objects, paint._data, atlas._image, rstTransforms, rects, colors, (blendMode ?? BlendMode.src).index, cullRect?._value32 ); } void _drawAtlas(List? paintObjects, ByteData paintData, - Image atlas, + _Image atlas, Float32List rstTransforms, Float32List rects, Int32List? colors, @@ -4575,11 +4540,13 @@ class Picture extends NativeFieldWrapperClass2 { if (width <= 0 || height <= 0) throw Exception('Invalid image dimensions.'); return _futurize( - (_Callback callback) => _toImage(width, height, callback) + (_Callback callback) => _toImage(width, height, (_Image image) { + callback(Image._(image)); + }), ); } - String _toImage(int width, int height, _Callback 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. diff --git a/lib/ui/painting/image.cc b/lib/ui/painting/image.cc index 64474227404a9..1f813096b8e73 100644 --- a/lib/ui/painting/image.cc +++ b/lib/ui/painting/image.cc @@ -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) \ diff --git a/testing/dart/image_dispose_test.dart b/testing/dart/image_dispose_test.dart index 73e7b77eeecd1..ca7495ad7e9ed 100644 --- a/testing/dart/image_dispose_test.dart +++ b/testing/dart/image_dispose_test.dart @@ -18,7 +18,7 @@ void main() { }()); final Matcher throwsAssertionError = throwsA(const TypeMatcher()); - test('Image.dispose asserts if handles are active', () async { + test('Handles are distinct', () async { final Uint8List bytes = await readFile('2x2.png'); final Codec codec = await instantiateImageCodec(bytes); final FrameInfo frame = await codec.getNextFrame(); @@ -31,13 +31,9 @@ void main() { final Image handle2 = handle1.createHandle(); expect(handle1 != handle2, true); + expect(handle1 != frame.image, true); + expect(frame.image == frame.image, true); - if (assertsEnabled) { - expect(() => frame.image.dispose(), throwsAssertionError); - handle1.dispose(); - expect(() => frame.image.dispose(), throwsAssertionError); - handle2.dispose(); - } frame.image.dispose(); }); @@ -80,17 +76,21 @@ void main() { final Image handle2 = handle1.createHandle(); List stackTraces = frame.image.debugGetOpenHandleStackTraces(); - expect(stackTraces.length, 2); - expect(stackTraces, equals(handle2.debugGetOpenHandleStackTraces())); + expect(stackTraces.length, 3); + expect(stackTraces, equals(handle1.debugGetOpenHandleStackTraces())); handle1.dispose(); stackTraces = frame.image.debugGetOpenHandleStackTraces(); - expect(stackTraces.length, 1); + expect(stackTraces.length, 2); expect(stackTraces, equals(handle2.debugGetOpenHandleStackTraces())); handle2.dispose(); stackTraces = frame.image.debugGetOpenHandleStackTraces(); - expect(stackTraces.length, 0); + expect(stackTraces.length, 1); + expect(stackTraces, equals(frame.image.debugGetOpenHandleStackTraces())); + + frame.image.dispose(); + expect(() => frame.image.debugGetOpenHandleStackTraces(), throwsAssertionError); }, skip: !assertsEnabled); } From 8537a70cdb94256a2c72581d280f2e80891b7744 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 10 Sep 2020 17:49:01 -0700 Subject: [PATCH 04/14] docs --- lib/ui/painting.dart | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 2fe91de1ab8a5..5744b81250a36 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1566,18 +1566,17 @@ 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 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. +/// 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. /// -/// 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, the image object is no longer usable -/// from Dart code, including for creating new handles. +/// 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: /// @@ -1610,12 +1609,16 @@ class Image { return _image.height; } - /// Release the resources used by this object. The object is no longer usable - /// after this method is called. + /// 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. /// - /// All outstanding handles from [createHandle] should be disposed before - /// calling this. Disposing all outstanding handles will automatically - /// dispose this object. + /// 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); From b69d22688c7f8734276e9ea58bce5234aa8af634 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 10 Sep 2020 17:50:58 -0700 Subject: [PATCH 05/14] web stubs --- lib/web_ui/lib/src/engine/canvaskit/image.dart | 11 +++++++++++ lib/web_ui/lib/src/engine/html_image_codec.dart | 6 ++++++ 2 files changed, 17 insertions(+) diff --git a/lib/web_ui/lib/src/engine/canvaskit/image.dart b/lib/web_ui/lib/src/engine/canvaskit/image.dart index 8bcb210d961ef..200152b0e0e90 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/image.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/image.dart @@ -52,6 +52,11 @@ class CkAnimatedImage implements ui.Image { box.delete(); } + @override + ui.Image createHandle() => this; + + @override + List? debugGetOpenHandleStackTraces() => null; int get frameCount => _skAnimatedImage.getFrameCount(); /// Decodes the next frame and returns the frame duration. @@ -114,6 +119,12 @@ class CkImage implements ui.Image { box.delete(); } + @override + ui.Image createHandle() => this; + + @override + List? debugGetOpenHandleStackTraces() => null; + @override int get width => skImage.width(); diff --git a/lib/web_ui/lib/src/engine/html_image_codec.dart b/lib/web_ui/lib/src/engine/html_image_codec.dart index 6a0595d66ae50..afbb8cc42a32c 100644 --- a/lib/web_ui/lib/src/engine/html_image_codec.dart +++ b/lib/web_ui/lib/src/engine/html_image_codec.dart @@ -122,6 +122,12 @@ class HtmlImage implements ui.Image { // releasing the object url. } + @override + ui.Image createHandle() => this; + + @override + List? debugGetOpenHandleStackTraces() => null; + @override final int width; From 0cf431cce838c57e92c206c826f60ca3b76b8aa5 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 11 Sep 2020 10:46:02 -0700 Subject: [PATCH 06/14] Make FrameInfo a regular Dart class, update tests --- ci/licenses_golden/licenses_flutter | 2 - lib/ui/BUILD.gn | 2 - lib/ui/dart_ui.cc | 2 - lib/ui/painting.dart | 68 +++++++++++++------ lib/ui/painting/codec.h | 2 +- lib/ui/painting/frame_info.cc | 30 -------- lib/ui/painting/frame_info.h | 37 ---------- lib/ui/painting/image_dispose_unittests.cc | 17 +++-- lib/ui/painting/image_encoding_unittests.cc | 4 ++ lib/ui/painting/multi_frame_codec.cc | 29 ++++---- lib/ui/painting/single_frame_codec.cc | 17 +++-- lib/ui/painting/single_frame_codec.h | 4 +- .../lib/src/engine/canvaskit/image.dart | 4 +- .../lib/src/engine/html_image_codec.dart | 2 +- lib/web_ui/lib/src/ui/painting.dart | 2 +- .../engine/recording_canvas_golden_test.dart | 2 +- testing/dart/image_dispose_test.dart | 13 ++-- 17 files changed, 99 insertions(+), 138 deletions(-) delete mode 100644 lib/ui/painting/frame_info.cc delete mode 100644 lib/ui/painting/frame_info.h diff --git a/ci/licenses_golden/licenses_flutter b/ci/licenses_golden/licenses_flutter index 12cc313c5b583..29356e2af831f 100755 --- a/ci/licenses_golden/licenses_flutter +++ b/ci/licenses_golden/licenses_flutter @@ -327,8 +327,6 @@ FILE: ../../../flutter/lib/ui/painting/color_filter.cc FILE: ../../../flutter/lib/ui/painting/color_filter.h FILE: ../../../flutter/lib/ui/painting/engine_layer.cc FILE: ../../../flutter/lib/ui/painting/engine_layer.h -FILE: ../../../flutter/lib/ui/painting/frame_info.cc -FILE: ../../../flutter/lib/ui/painting/frame_info.h FILE: ../../../flutter/lib/ui/painting/gradient.cc FILE: ../../../flutter/lib/ui/painting/gradient.h FILE: ../../../flutter/lib/ui/painting/image.cc diff --git a/lib/ui/BUILD.gn b/lib/ui/BUILD.gn index 6c72102c2d9b8..6b1c5d44049f4 100644 --- a/lib/ui/BUILD.gn +++ b/lib/ui/BUILD.gn @@ -30,8 +30,6 @@ source_set("ui") { "painting/color_filter.h", "painting/engine_layer.cc", "painting/engine_layer.h", - "painting/frame_info.cc", - "painting/frame_info.h", "painting/gradient.cc", "painting/gradient.h", "painting/image.cc", diff --git a/lib/ui/dart_ui.cc b/lib/ui/dart_ui.cc index f9f89d6f22d67..c22b26b17d3f6 100644 --- a/lib/ui/dart_ui.cc +++ b/lib/ui/dart_ui.cc @@ -13,7 +13,6 @@ #include "flutter/lib/ui/painting/codec.h" #include "flutter/lib/ui/painting/color_filter.h" #include "flutter/lib/ui/painting/engine_layer.h" -#include "flutter/lib/ui/painting/frame_info.h" #include "flutter/lib/ui/painting/gradient.h" #include "flutter/lib/ui/painting/image.h" #include "flutter/lib/ui/painting/image_descriptor.h" @@ -70,7 +69,6 @@ void DartUI::InitForGlobal() { DartRuntimeHooks::RegisterNatives(g_natives); EngineLayer::RegisterNatives(g_natives); FontCollection::RegisterNatives(g_natives); - FrameInfo::RegisterNatives(g_natives); ImageDescriptor::RegisterNatives(g_natives); ImageFilter::RegisterNatives(g_natives); ImageShader::RegisterNatives(g_natives); diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 5744b81250a36..deeda7bbae31c 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1568,13 +1568,13 @@ enum PixelFormat { /// /// 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 +/// underlying image, call [clone]. 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 +/// that handle with other callers, it is critical that [clone] is called /// _before_ [dispose]. A handle that has been disposed cannot create new /// handles anymore. /// @@ -1648,11 +1648,10 @@ class Image { bool _disposed = false; /// If asserts are enabled, returns the [StackTrace]s of each open handle from - /// [createHandle], in creation order. + /// [clone], in creation order. /// /// If asserts are disabled, this method always returns null. List? debugGetOpenHandleStackTraces() { - assert(!_disposed && !_image._disposed); List? stacks; assert(() { stacks = _image._handles.map((Image handle) => handle._debugStack!).toList(); @@ -1663,12 +1662,17 @@ class Image { /// Creates a disposable handle to this image. /// - /// The returned object behaves identically to this image, except calling + /// 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. - Image createHandle() { + Image clone() { if (_disposed) { - throw StateError('Object 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.' + ); } return Image._(_image); } @@ -1733,25 +1737,24 @@ typedef ImageDecoderCallback = void Function(Image result); /// /// To obtain an instance of the [FrameInfo] interface, see /// [Codec.getNextFrame]. -@pragma('vm:entry-point') -class FrameInfo extends NativeFieldWrapperClass2 { +class FrameInfo { /// This class is created by the engine, and should not be instantiated /// or extended directly. /// /// To obtain an instance of the [FrameInfo] interface, see /// [Codec.getNextFrame]. - @pragma('vm:entry-point') - FrameInfo._(); + FrameInfo._({required this.duration, required this.image}); /// The duration this frame should be shown. - Duration get duration => Duration(milliseconds: _durationMillis); - int get _durationMillis native 'FrameInfo_durationMillis'; + /// + /// A zero duration indicates that the frame should be shown indefinitely. + final Duration duration; - Image? _cachedImage; /// The [Image] object for this frame. - Image get image => _cachedImage ??= Image._(_image); - _Image get _image native 'FrameInfo_image'; + /// + /// This object must be disposed by the recipient of this frame info. + final Image image; } /// A handle to an image codec. @@ -1772,26 +1775,49 @@ class Codec extends NativeFieldWrapperClass2 { @pragma('vm:entry-point') Codec._(); + int? _cachedFrameCount; /// Number of frames in this image. - int get frameCount native 'Codec_frameCount'; + int get frameCount => _cachedFrameCount ??= _frameCount; + int get _frameCount native 'Codec_frameCount'; + int? _cachedRepetitionCount; /// Number of times to repeat the animation. /// /// * 0 when the animation should be played once. /// * -1 for infinity repetitions. - int get repetitionCount native 'Codec_repetitionCount'; + int get repetitionCount => _cachedRepetitionCount ??= _repetitionCount; + int get _repetitionCount native 'Codec_repetitionCount'; + + FrameInfo? _cachedFrame; /// Fetches the next animation frame. /// /// Wraps back to the first frame after returning the last frame. /// /// The returned future can complete with an error if the decoding has failed. - Future getNextFrame() { - return _futurize(_getNextFrame); + Future getNextFrame() async { + if (_cachedFrame == null || frameCount != 1) { + final Completer completer = Completer.sync(); + final String? error = _getNextFrame((_Image? image, int durationMilliseconds) { + if (image == null) { + throw Exception('Codec failed to produce an image, possibly due to invalid image data.'); + } + _cachedFrame = FrameInfo._( + image: Image._(image), + duration: Duration(milliseconds: durationMilliseconds), + ); + completer.complete(); + }); + if (error != null) { + throw Exception(error); + } + await completer.future; + } + return _cachedFrame!; } /// Returns an error message on failure, null on success. - String _getNextFrame(_Callback callback) native 'Codec_getNextFrame'; + String? _getNextFrame(void Function(_Image?, int) callback) native 'Codec_getNextFrame'; /// Release the resources used by this object. The object is no longer usable /// after this method is called. diff --git a/lib/ui/painting/codec.h b/lib/ui/painting/codec.h index 2e0c746eac2d6..4740a3488d476 100644 --- a/lib/ui/painting/codec.h +++ b/lib/ui/painting/codec.h @@ -6,7 +6,7 @@ #define FLUTTER_LIB_UI_PAINTING_CODEC_H_ #include "flutter/lib/ui/dart_wrapper.h" -#include "flutter/lib/ui/painting/frame_info.h" +#include "flutter/lib/ui/ui_dart_state.h" #include "third_party/skia/include/codec/SkCodec.h" #include "third_party/skia/include/core/SkBitmap.h" #include "third_party/skia/include/core/SkImage.h" diff --git a/lib/ui/painting/frame_info.cc b/lib/ui/painting/frame_info.cc deleted file mode 100644 index ad77f7b50d8a4..0000000000000 --- a/lib/ui/painting/frame_info.cc +++ /dev/null @@ -1,30 +0,0 @@ - -// Copyright 2013 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. - -#include "flutter/lib/ui/painting/frame_info.h" - -#include "third_party/tonic/dart_binding_macros.h" -#include "third_party/tonic/dart_library_natives.h" - -namespace flutter { - -IMPLEMENT_WRAPPERTYPEINFO(ui, FrameInfo); - -#define FOR_EACH_BINDING(V) \ - V(FrameInfo, durationMillis) \ - V(FrameInfo, image) - -FOR_EACH_BINDING(DART_NATIVE_CALLBACK) - -FrameInfo::FrameInfo(fml::RefPtr image, int durationMillis) - : image_(std::move(image)), durationMillis_(durationMillis) {} - -FrameInfo::~FrameInfo(){}; - -void FrameInfo::RegisterNatives(tonic::DartLibraryNatives* natives) { - natives->Register({FOR_EACH_BINDING(DART_REGISTER_NATIVE)}); -} - -} // namespace flutter diff --git a/lib/ui/painting/frame_info.h b/lib/ui/painting/frame_info.h deleted file mode 100644 index 184b132d17791..0000000000000 --- a/lib/ui/painting/frame_info.h +++ /dev/null @@ -1,37 +0,0 @@ -// Copyright 2013 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. - -#ifndef FLUTTER_LIB_UI_PAINTING_FRAME_INFO_H_ -#define FLUTTER_LIB_UI_PAINTING_FRAME_INFO_H_ - -#include "flutter/lib/ui/dart_wrapper.h" -#include "flutter/lib/ui/painting/image.h" - -namespace flutter { - -// A single animation frame. -class FrameInfo final : public RefCountedDartWrappable { - DEFINE_WRAPPERTYPEINFO(); - - public: - int durationMillis() { return durationMillis_; } - fml::RefPtr image() { return image_; } - - static void RegisterNatives(tonic::DartLibraryNatives* natives); - - private: - FrameInfo(fml::RefPtr image, int durationMillis); - - ~FrameInfo() override; - - const fml::RefPtr image_; - const int durationMillis_; - - FML_FRIEND_MAKE_REF_COUNTED(FrameInfo); - FML_FRIEND_REF_COUNTED_THREAD_SAFE(FrameInfo); -}; - -} // namespace flutter - -#endif // FLUTTER_LIB_UI_PAINTING_FRAME_INFO_H_ diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index 3763d70539f95..34dfd3b89f621 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -19,11 +19,11 @@ namespace testing { class ImageDisposeTest : public ShellTest { public: template - T* GetNativePeer(Dart_NativeArguments args, int index) { - auto handle = Dart_GetNativeArgument(args, index); + T* GetNativePeer(Dart_Handle handle) { intptr_t peer = 0; - EXPECT_FALSE(Dart_IsError(Dart_GetNativeInstanceField( - handle, tonic::DartWrappable::kPeerIndex, &peer))); + auto native_handle = Dart_GetNativeInstanceField( + handle, tonic::DartWrappable::kPeerIndex, &peer); + EXPECT_FALSE(Dart_IsError(native_handle)) << Dart_GetError(native_handle); return reinterpret_cast(peer); } @@ -42,8 +42,13 @@ class ImageDisposeTest : public ShellTest { TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { auto native_capture_image_and_picture = [&](Dart_NativeArguments args) { - CanvasImage* image = GetNativePeer(args, 0); - Picture* picture = GetNativePeer(args, 1); + auto image_handle = Dart_GetNativeArgument(args, 0); + auto native_image_handle = + Dart_GetField(image_handle, Dart_NewStringFromCString("_image")); + ASSERT_FALSE(Dart_IsError(native_image_handle)); + ASSERT_FALSE(Dart_IsNull(native_image_handle)); + CanvasImage* image = GetNativePeer(native_image_handle); + Picture* picture = GetNativePeer(Dart_GetNativeArgument(args, 1)); ASSERT_FALSE(image->image()->unique()); ASSERT_FALSE(picture->picture()->unique()); current_image_ = image->image(); diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc index f40e09b44b815..4fd38d20d581d 100644 --- a/lib/ui/painting/image_encoding_unittests.cc +++ b/lib/ui/painting/image_encoding_unittests.cc @@ -20,6 +20,10 @@ fml::AutoResetWaitableEvent message_latch; TEST_F(ShellTest, EncodeImageGivesExternalTypedData) { auto nativeEncodeImage = [&](Dart_NativeArguments args) { auto image_handle = Dart_GetNativeArgument(args, 0); + image_handle = + Dart_GetField(image_handle, Dart_NewStringFromCString("_image")); + ASSERT_FALSE(Dart_IsError(image_handle)); + ASSERT_FALSE(Dart_IsNull(image_handle)); auto format_handle = Dart_GetNativeArgument(args, 1); auto callback_handle = Dart_GetNativeArgument(args, 2); diff --git a/lib/ui/painting/multi_frame_codec.cc b/lib/ui/painting/multi_frame_codec.cc index c40561836011a..2ab5ce6d108c4 100644 --- a/lib/ui/painting/multi_frame_codec.cc +++ b/lib/ui/painting/multi_frame_codec.cc @@ -5,6 +5,7 @@ #include "flutter/lib/ui/painting/multi_frame_codec.h" #include "flutter/fml/make_copyable.h" +#include "flutter/lib/ui/painting/image.h" #include "third_party/dart/runtime/include/dart_api.h" #include "third_party/skia/include/core/SkPixelRef.h" #include "third_party/tonic/logging/dart_invoke.h" @@ -24,7 +25,8 @@ MultiFrameCodec::State::State(std::shared_ptr generator) nextFrameIndex_(0) {} static void InvokeNextFrameCallback( - fml::RefPtr frameInfo, + fml::RefPtr image, + int duration, std::unique_ptr callback, size_t trace_id) { std::shared_ptr dart_state = callback->dart_state().lock(); @@ -34,11 +36,8 @@ static void InvokeNextFrameCallback( return; } tonic::DartState::Scope scope(dart_state); - if (!frameInfo) { - tonic::DartInvoke(callback->value(), {Dart_Null()}); - } else { - tonic::DartInvoke(callback->value(), {ToDart(frameInfo)}); - } + tonic::DartInvoke(callback->value(), + {tonic::ToDart(image), tonic::ToDart(duration)}); } // Copied the source bitmap to the destination. If this cannot occur due to @@ -139,22 +138,24 @@ void MultiFrameCodec::State::GetNextFrameAndInvokeCallback( fml::WeakPtr resourceContext, fml::RefPtr unref_queue, size_t trace_id) { - fml::RefPtr frameInfo = NULL; + fml::RefPtr image = nullptr; + int duration = 0; sk_sp skImage = GetNextFrameImage(resourceContext); if (skImage) { - fml::RefPtr image = CanvasImage::Create(); + image = CanvasImage::Create(); image->set_image({skImage, std::move(unref_queue)}); SkCodec::FrameInfo skFrameInfo{0}; generator_->getFrameInfo(nextFrameIndex_, &skFrameInfo); - frameInfo = - fml::MakeRefCounted(std::move(image), skFrameInfo.fDuration); + duration = skFrameInfo.fDuration; } nextFrameIndex_ = (nextFrameIndex_ + 1) % frameCount_; - ui_task_runner->PostTask(fml::MakeCopyable( - [callback = std::move(callback), frameInfo, trace_id]() mutable { - InvokeNextFrameCallback(frameInfo, std::move(callback), trace_id); - })); + ui_task_runner->PostTask(fml::MakeCopyable([callback = std::move(callback), + image = std::move(image), + duration, trace_id]() mutable { + InvokeNextFrameCallback(std::move(image), duration, std::move(callback), + trace_id); + })); } Dart_Handle MultiFrameCodec::getNextFrame(Dart_Handle callback_handle) { diff --git a/lib/ui/painting/single_frame_codec.cc b/lib/ui/painting/single_frame_codec.cc index dbe603a5f043c..3d9d51f02a2dd 100644 --- a/lib/ui/painting/single_frame_codec.cc +++ b/lib/ui/painting/single_frame_codec.cc @@ -4,7 +4,6 @@ #include "flutter/lib/ui/painting/single_frame_codec.h" -#include "flutter/lib/ui/painting/frame_info.h" #include "flutter/lib/ui/ui_dart_state.h" #include "third_party/tonic/logging/dart_invoke.h" @@ -34,7 +33,8 @@ Dart_Handle SingleFrameCodec::getNextFrame(Dart_Handle callback_handle) { } if (status_ == Status::kComplete) { - tonic::DartInvoke(callback_handle, {tonic::ToDart(cached_frame_)}); + tonic::DartInvoke(callback_handle, + {tonic::ToDart(cached_image_), tonic::ToDart(0)}); return Dart_Null(); } @@ -82,8 +82,7 @@ Dart_Handle SingleFrameCodec::getNextFrame(Dart_Handle callback_handle) { auto canvas_image = fml::MakeRefCounted(); canvas_image->set_image(std::move(image)); - codec->cached_frame_ = fml::MakeRefCounted( - std::move(canvas_image), 0 /* duration */); + codec->cached_image_ = std::move(canvas_image); } // The cached frame is now available and should be returned to any @@ -91,9 +90,10 @@ Dart_Handle SingleFrameCodec::getNextFrame(Dart_Handle callback_handle) { codec->status_ = Status::kComplete; // Invoke any callbacks that were provided before the frame was decoded. - Dart_Handle frame = tonic::ToDart(codec->cached_frame_); for (const DartPersistentValue& callback : codec->pending_callbacks_) { - tonic::DartInvoke(callback.value(), {frame}); + tonic::DartInvoke( + callback.value(), + {tonic::ToDart(codec->cached_image_), tonic::ToDart(0)}); } codec->pending_callbacks_.clear(); }); @@ -109,9 +109,8 @@ Dart_Handle SingleFrameCodec::getNextFrame(Dart_Handle callback_handle) { size_t SingleFrameCodec::GetAllocationSize() const { const auto& data_size = descriptor_->GetAllocationSize(); - const auto frame_byte_size = (cached_frame_ && cached_frame_->image()) - ? cached_frame_->image()->GetAllocationSize() - : 0; + const auto frame_byte_size = + cached_image_ ? cached_image_->GetAllocationSize() : 0; return data_size + frame_byte_size + sizeof(this); } diff --git a/lib/ui/painting/single_frame_codec.h b/lib/ui/painting/single_frame_codec.h index 587bea6fed588..9685c047e3efd 100644 --- a/lib/ui/painting/single_frame_codec.h +++ b/lib/ui/painting/single_frame_codec.h @@ -7,7 +7,7 @@ #include "flutter/fml/macros.h" #include "flutter/lib/ui/painting/codec.h" -#include "flutter/lib/ui/painting/frame_info.h" +#include "flutter/lib/ui/painting/image.h" #include "flutter/lib/ui/painting/image_decoder.h" #include "flutter/lib/ui/painting/image_descriptor.h" @@ -39,7 +39,7 @@ class SingleFrameCodec : public Codec { fml::RefPtr descriptor_; uint32_t target_width_; uint32_t target_height_; - fml::RefPtr cached_frame_; + fml::RefPtr cached_image_; std::vector pending_callbacks_; FML_FRIEND_MAKE_REF_COUNTED(SingleFrameCodec); diff --git a/lib/web_ui/lib/src/engine/canvaskit/image.dart b/lib/web_ui/lib/src/engine/canvaskit/image.dart index 200152b0e0e90..d6474e0c08f51 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/image.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/image.dart @@ -53,7 +53,7 @@ class CkAnimatedImage implements ui.Image { } @override - ui.Image createHandle() => this; + ui.Image clone() => this; @override List? debugGetOpenHandleStackTraces() => null; @@ -120,7 +120,7 @@ class CkImage implements ui.Image { } @override - ui.Image createHandle() => this; + ui.Image clone() => this; @override List? debugGetOpenHandleStackTraces() => null; diff --git a/lib/web_ui/lib/src/engine/html_image_codec.dart b/lib/web_ui/lib/src/engine/html_image_codec.dart index afbb8cc42a32c..111fff8d5e2de 100644 --- a/lib/web_ui/lib/src/engine/html_image_codec.dart +++ b/lib/web_ui/lib/src/engine/html_image_codec.dart @@ -123,7 +123,7 @@ class HtmlImage implements ui.Image { } @override - ui.Image createHandle() => this; + ui.Image clone() => this; @override List? debugGetOpenHandleStackTraces() => null; diff --git a/lib/web_ui/lib/src/ui/painting.dart b/lib/web_ui/lib/src/ui/painting.dart index b55ea8ff895e8..647daf5c0490e 100644 --- a/lib/web_ui/lib/src/ui/painting.dart +++ b/lib/web_ui/lib/src/ui/painting.dart @@ -330,7 +330,7 @@ abstract class Image { Future toByteData({ImageByteFormat format = ImageByteFormat.rawRgba}); void dispose(); - Image createHandle() => this; + Image clone() => this; List? debugGetOpenHandleStackTraces() => null; diff --git a/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart b/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart index cee7059fd6035..e90f07f0eb17e 100644 --- a/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart +++ b/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart @@ -720,7 +720,7 @@ class TestImage implements Image { void dispose() {} @override - Image createHandle() => this; + Image clone() => this; List? debugGetOpenHandleStackTraces() => []; } diff --git a/testing/dart/image_dispose_test.dart b/testing/dart/image_dispose_test.dart index ca7495ad7e9ed..3b7b55d5af617 100644 --- a/testing/dart/image_dispose_test.dart +++ b/testing/dart/image_dispose_test.dart @@ -16,7 +16,6 @@ void main() { assertsEnabled = true; return true; }()); - final Matcher throwsAssertionError = throwsA(const TypeMatcher()); test('Handles are distinct', () async { final Uint8List bytes = await readFile('2x2.png'); @@ -25,11 +24,11 @@ void main() { expect(frame.image.width, 2); expect(frame.image.height, 2); - final Image handle1 = frame.image.createHandle(); + final Image handle1 = frame.image.clone(); expect(handle1.width, frame.image.width); expect(handle1.height, frame.image.height); - final Image handle2 = handle1.createHandle(); + final Image handle2 = handle1.clone(); expect(handle1 != handle2, true); expect(handle1 != frame.image, true); expect(frame.image == frame.image, true); @@ -44,7 +43,7 @@ void main() { expect(frame.image.width, 2); expect(frame.image.height, 2); - final Image handle1 = frame.image.createHandle(); + final Image handle1 = frame.image.clone(); final PictureRecorder recorder = PictureRecorder(); final Canvas canvas = Canvas(recorder); @@ -72,8 +71,8 @@ void main() { final Codec codec = await instantiateImageCodec(bytes); final FrameInfo frame = await codec.getNextFrame(); - final Image handle1 = frame.image.createHandle(); - final Image handle2 = handle1.createHandle(); + final Image handle1 = frame.image.clone(); + final Image handle2 = handle1.clone(); List stackTraces = frame.image.debugGetOpenHandleStackTraces(); expect(stackTraces.length, 3); @@ -90,7 +89,7 @@ void main() { expect(stackTraces, equals(frame.image.debugGetOpenHandleStackTraces())); frame.image.dispose(); - expect(() => frame.image.debugGetOpenHandleStackTraces(), throwsAssertionError); + expect(() => frame.image.debugGetOpenHandleStackTraces(), isEmpty); }, skip: !assertsEnabled); } From 4f1560079c23b62155afdb477e0ad0e46747ab45 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 11 Sep 2020 10:57:40 -0700 Subject: [PATCH 07/14] more docs --- lib/ui/painting.dart | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index deeda7bbae31c..7996c9ecf7200 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1737,6 +1737,9 @@ typedef ImageDecoderCallback = void Function(Image result); /// /// To obtain an instance of the [FrameInfo] interface, see /// [Codec.getNextFrame]. +/// +/// The recipient of this class is responsible for calling [Image.dispose] on +/// [image]. class FrameInfo { /// This class is created by the engine, and should not be instantiated /// or extended directly. @@ -1795,6 +1798,9 @@ class Codec extends NativeFieldWrapperClass2 { /// Wraps back to the first frame after returning the last frame. /// /// The returned future can complete with an error if the decoding has failed. + /// + /// The caller of this method is responsible for disposing the + /// [FrameInfo.image] on the returned object. Future getNextFrame() async { if (_cachedFrame == null || frameCount != 1) { final Completer completer = Completer.sync(); From 9eef9074ee7045592c07ddfe47a5c3a36ca673e6 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 11 Sep 2020 16:49:57 -0700 Subject: [PATCH 08/14] review --- lib/ui/painting.dart | 112 +++++++++++++++++++++++++++++++++---------- 1 file changed, 87 insertions(+), 25 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 7996c9ecf7200..a3490b9384894 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1574,9 +1574,8 @@ enum PixelFormat { /// disposed. /// /// If `dart:ui` passes an `Image` object and the recipient wishes to share -/// that handle with other callers, it is critical that [clone] is called -/// _before_ [dispose]. A handle that has been disposed cannot create new -/// handles anymore. +/// that handle with other callers, [clone] must be called _before_ [dispose]. +/// A handle that has been disposed cannot create new handles anymore. /// /// See also: /// @@ -1609,6 +1608,7 @@ class Image { return _image.height; } + bool _disposed = false; /// Release this handle's claim on the underlying Image. This handle is no /// longer usable after this method is called. /// @@ -1620,12 +1620,9 @@ class Image { /// 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; - }()); + assert(!_disposed && !_image._disposed); + assert(_image._handles.contains(this)); + _disposed = true; final bool removed = _image._handles.remove(this); assert(removed); if (_image._handles.isEmpty) { @@ -1645,8 +1642,6 @@ class Image { return _image.toByteData(format: format); } - bool _disposed = false; - /// If asserts are enabled, returns the [StackTrace]s of each open handle from /// [clone], in creation order. /// @@ -1662,6 +1657,71 @@ class Image { /// Creates a disposable handle to this image. /// + /// Holders of an [Image] must dispose of the image when they no longer need + /// to access it or draw it. However, once the underlying image is disposed, + /// it is no longer possible to use it. If a holder of an image needs to share + /// access to that image with another object or method, [clone] creates a + /// duplicate handle. The underlying image will only be disposed once all + /// outstanding handles are disposed. This allows for safe sharing of image + /// references while still disposing of the underlying resources when all + /// consumers are finished. + /// + /// It is safe to pass an [Image] handle to another object or method if the + /// current holder no longer needs it. + /// + /// The following example demonstrates valid usage. + /// + /// ```dart + /// import 'dart:async'; + /// + /// Future _loadImage(int width, int height) { + /// final Completer completer = Completer(); + /// decodeImageFromPixels( + /// Uint8List.fromList(List.filled(width * height * 4, 0xFF)), + /// width, + /// height, + /// PixelFormat.rgba8888, + /// // Don't worry about disposing or cloning this image - responsibility + /// // is transferred to the caller, and that is safe since this method + /// // will not touch it again. + /// (Image image) => completer.complete(image), + /// ); + /// return completer.future; + /// } + /// + /// Future main() async { + /// final Image image = await _loadImage(5, 5); + /// // Make sure to clone the image, because MyHolder might dispose it + /// // and we need to access it again. + /// final MyImageHolder holder = MyImageHolder(image.clone()); + /// final MyImageHolder holder2 = MyImageHolder(image.clone()); + /// // Now we dispose it because we won't need it again. + /// image.dispose(); + /// + /// final PictureRecorder recorder = PictureRecorder(); + /// final Canvas canvas = Canvas(recorder); + /// + /// holder.draw(canvas); + /// holder.dispose(); + /// + /// canvas.translate(50, 50); + /// holder2.draw(canvas); + /// holder2.dispose(); + /// } + /// + /// class MyImageHolder { + /// MyImageLoader(this.image); + /// + /// final Image image; + /// + /// void draw(Canvas canvas) { + /// canvas.drawImage(image, Offset.zero, Paint()); + /// } + /// + /// void dispose() => image.dispose(); + /// } + /// ``` + /// /// 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. @@ -1674,6 +1734,7 @@ class Image { 'handles, as the underlying data may have been released.' ); } + assert(!_image._disposed); return Image._(_image); } @@ -1686,7 +1747,8 @@ 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]. + // _Images are always handed out wrapped in [Image]s. To create an [Image], + // use the ImageDescriptor API. @pragma('vm:entry-point') _Image._(); @@ -1707,18 +1769,15 @@ class _Image extends NativeFieldWrapperClass2 { bool _disposed = false; 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; - }()); + 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; _dispose(); } @@ -1739,7 +1798,8 @@ typedef ImageDecoderCallback = void Function(Image result); /// [Codec.getNextFrame]. /// /// The recipient of this class is responsible for calling [Image.dispose] on -/// [image]. +/// [image]. To share the image with other interested parties, use +/// [Image.clone]. class FrameInfo { /// This class is created by the engine, and should not be instantiated /// or extended directly. @@ -1757,6 +1817,8 @@ class FrameInfo { /// The [Image] object for this frame. /// /// This object must be disposed by the recipient of this frame info. + /// + /// To share this image with other interested parties, use [Image.clone]. final Image image; } From 17254e31f48e194be6486b88d4cd6135d1429169 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Mon, 14 Sep 2020 10:32:37 -0700 Subject: [PATCH 09/14] docs --- lib/ui/compositing.dart | 4 ++++ lib/ui/painting.dart | 41 ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/lib/ui/compositing.dart b/lib/ui/compositing.dart index 3eccc80edf6e0..a6bd03b894906 100644 --- a/lib/ui/compositing.dart +++ b/lib/ui/compositing.dart @@ -23,6 +23,10 @@ class Scene extends NativeFieldWrapperClass2 { /// Creates a raster image representation of the current state of the scene. /// This is a slow operation that is performed on a background thread. + /// + /// Callers must dispose the [Image] when they are done with it. If the result + /// will be shared with other methods or classes, [Image.clone] should be used + /// and each handle created must be disposed. Future toImage(int width, int height) { if (width <= 0 || height <= 0) { throw Exception('Invalid image dimensions.'); diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index a3490b9384894..08bc39ebf2b7d 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1797,9 +1797,44 @@ typedef ImageDecoderCallback = void Function(Image result); /// To obtain an instance of the [FrameInfo] interface, see /// [Codec.getNextFrame]. /// -/// The recipient of this class is responsible for calling [Image.dispose] on -/// [image]. To share the image with other interested parties, use -/// [Image.clone]. +/// The recipient of an instance of this class is responsible for calling +/// [Image.dispose] on [image]. To share the image with other interested +/// parties, use [Image.clone]. If the [FrameInfo] object itself is passed to +/// another method or object, that method or object must assume it is +/// responsible for disposing the image when done, and the passer must not +/// access the [image] after that point. +/// +/// For example, the following code sample is incorrect: +/// +/// ```dart +/// /// BAD +/// Future nextFrameRoutine(Codec codec) async { +/// final FrameInfo frameInfo = await codec.getNextFrame(); +/// _cacheImage(frameInfo); +/// // ERROR - _cacheImage is now responsible for disposing the image, and +/// // the image may not be available any more for this drawing routine. +/// _drawImage(frameInfo); +/// // ERROR again - the previous methods might or might not have created +/// // handles to the image. +/// frameInfo.image.dispose(); +/// } +/// ``` +/// +/// Correct usage is: +/// +/// ```dart +/// /// GOOD +/// Future nextFrameRoutine(Codec codec) async { +/// final FrameInfo frameInfo = await codec.getNextFrame(); +/// _cacheImage(frameInfo.image.clone(), frameInfo.duration); +/// _drawImage(frameInfo.image.clone(), frameInfo.duration); +/// // This method is done with its handle, and has passed handles to its +/// // clients already. +/// // The image will live until those clients dispose of their handles, and +/// // this one must not be disposed since it will not be used again. +/// frameInfo.image.dispose(); +/// } +/// ``` class FrameInfo { /// This class is created by the engine, and should not be instantiated /// or extended directly. From f00dd18cb7c77556ded79d5d675bcd7db326073d Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 17 Sep 2020 17:15:21 -0700 Subject: [PATCH 10/14] merge and fix test --- testing/dart/image_dispose_test.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/testing/dart/image_dispose_test.dart b/testing/dart/image_dispose_test.dart index 3b7b55d5af617..dc89643906360 100644 --- a/testing/dart/image_dispose_test.dart +++ b/testing/dart/image_dispose_test.dart @@ -89,7 +89,7 @@ void main() { expect(stackTraces, equals(frame.image.debugGetOpenHandleStackTraces())); frame.image.dispose(); - expect(() => frame.image.debugGetOpenHandleStackTraces(), isEmpty); + expect(frame.image.debugGetOpenHandleStackTraces(), isEmpty); }, skip: !assertsEnabled); } From 31d47de73bcc92ae2c82e782a1fa4b9200b982dd Mon Sep 17 00:00:00 2001 From: Dan Field Date: Thu, 17 Sep 2020 21:28:31 -0700 Subject: [PATCH 11/14] make tests work in AOT --- lib/ui/painting.dart | 2 ++ lib/ui/painting/image_dispose_unittests.cc | 3 ++- lib/ui/painting/image_encoding_unittests.cc | 2 +- 3 files changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index b63c9f1477266..0fe31587442f1 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1592,6 +1592,8 @@ class Image { _image._handles.add(this); } + // C++ unit tests access this. + @pragma('vm:entry-point', !bool.fromEnvironment('dart.vm.product')) final _Image _image; StackTrace? _debugStack; diff --git a/lib/ui/painting/image_dispose_unittests.cc b/lib/ui/painting/image_dispose_unittests.cc index 34dfd3b89f621..63737cc3d0fec 100644 --- a/lib/ui/painting/image_dispose_unittests.cc +++ b/lib/ui/painting/image_dispose_unittests.cc @@ -45,7 +45,8 @@ TEST_F(ImageDisposeTest, ImageReleasedAfterFrame) { auto image_handle = Dart_GetNativeArgument(args, 0); auto native_image_handle = Dart_GetField(image_handle, Dart_NewStringFromCString("_image")); - ASSERT_FALSE(Dart_IsError(native_image_handle)); + ASSERT_FALSE(Dart_IsError(native_image_handle)) + << Dart_GetError(native_image_handle); ASSERT_FALSE(Dart_IsNull(native_image_handle)); CanvasImage* image = GetNativePeer(native_image_handle); Picture* picture = GetNativePeer(Dart_GetNativeArgument(args, 1)); diff --git a/lib/ui/painting/image_encoding_unittests.cc b/lib/ui/painting/image_encoding_unittests.cc index 5d0165274cf61..452c92ba9aa09 100644 --- a/lib/ui/painting/image_encoding_unittests.cc +++ b/lib/ui/painting/image_encoding_unittests.cc @@ -23,7 +23,7 @@ TEST_F(ShellTest, EncodeImageGivesExternalTypedData) { auto image_handle = Dart_GetNativeArgument(args, 0); image_handle = Dart_GetField(image_handle, Dart_NewStringFromCString("_image")); - ASSERT_FALSE(Dart_IsError(image_handle)); + ASSERT_FALSE(Dart_IsError(image_handle)) << Dart_GetError(image_handle); ASSERT_FALSE(Dart_IsNull(image_handle)); auto format_handle = Dart_GetNativeArgument(args, 1); auto callback_handle = Dart_GetNativeArgument(args, 2); From 03324dc673c18bcd0a89199bac85a94a7b0bcb55 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 22 Sep 2020 12:16:03 -0700 Subject: [PATCH 12/14] Fix nullability --- .../test/golden_tests/engine/recording_canvas_golden_test.dart | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart b/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart index 8e2eeba390cf7..e2cb9fcc6aa27 100644 --- a/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart +++ b/lib/web_ui/test/golden_tests/engine/recording_canvas_golden_test.dart @@ -726,7 +726,8 @@ class TestImage implements Image { @override Image clone() => this; - List? debugGetOpenHandleStackTraces() => []; + @override + List/*?*/ debugGetOpenHandleStackTraces() => []; } Paragraph createTestParagraph() { From 084796f29d876d380c0d22dbeca4dda460724302 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 22 Sep 2020 14:32:41 -0700 Subject: [PATCH 13/14] Make host release unit test work --- lib/ui/painting.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/ui/painting.dart b/lib/ui/painting.dart index 0fe31587442f1..993029a1e6998 100644 --- a/lib/ui/painting.dart +++ b/lib/ui/painting.dart @@ -1593,7 +1593,7 @@ class Image { } // C++ unit tests access this. - @pragma('vm:entry-point', !bool.fromEnvironment('dart.vm.product')) + @pragma('vm:entry-point') final _Image _image; StackTrace? _debugStack; From e38a09dcd959b2f0ac876c87dcfac3f104dcc706 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Tue, 22 Sep 2020 22:22:38 -0700 Subject: [PATCH 14/14] CI