From 89a46af119a4f9b989a6af98da2b64af810e8cd5 Mon Sep 17 00:00:00 2001 From: Dan Field Date: Fri, 2 Oct 2020 12:57:01 -0700 Subject: [PATCH] Implement Image.clone for CanvasKit (#21555) --- .../src/engine/canvaskit/canvaskit_api.dart | 4 ++ .../lib/src/engine/canvaskit/image.dart | 41 +++++++++---- .../engine/canvaskit/skia_object_cache.dart | 58 ++++++++++++++++--- lib/web_ui/test/canvaskit/image_test.dart | 42 ++++++++++++++ .../canvaskit/skia_objects_cache_test.dart | 43 +++++++++++++- 5 files changed, 168 insertions(+), 20 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart b/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart index bca5a9b995..1252cba16e 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart @@ -683,6 +683,8 @@ class SkAnimatedImage { /// /// This object is no longer usable after calling this method. external void delete(); + external bool isAliasOf(SkAnimatedImage other); + external bool isDeleted(); } @JS() @@ -698,6 +700,8 @@ class SkImage { ); external Uint8List readPixels(SkImageInfo imageInfo, int srcX, int srcY); external SkData encodeToData(); + external bool isAliasOf(SkImage other); + external bool isDeleted(); } @JS() diff --git a/lib/web_ui/lib/src/engine/canvaskit/image.dart b/lib/web_ui/lib/src/engine/canvaskit/image.dart index badaa8e604..192e301c1b 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/image.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/image.dart @@ -43,8 +43,15 @@ class CkAnimatedImage implements ui.Image { // being garbage-collected, or by an explicit call to [delete]. late final SkiaObjectBox box; - CkAnimatedImage(this._skAnimatedImage) { - box = SkiaObjectBox(this, _skAnimatedImage as SkDeletable); + CkAnimatedImage(SkAnimatedImage skAnimatedImage) : this._(skAnimatedImage, null); + + CkAnimatedImage._(this._skAnimatedImage, SkiaObjectBox? boxToClone) { + if (boxToClone != null) { + assert(boxToClone.skObject == _skAnimatedImage); + box = boxToClone.clone(this); + } else { + box = SkiaObjectBox(this, _skAnimatedImage as SkDeletable); + } } @override @@ -53,13 +60,17 @@ class CkAnimatedImage implements ui.Image { } @override - ui.Image clone() => this; + ui.Image clone() => CkAnimatedImage._(_skAnimatedImage, box); @override - bool isCloneOf(ui.Image other) => other == this; + bool isCloneOf(ui.Image other) { + return other is CkAnimatedImage + && other._skAnimatedImage.isAliasOf(_skAnimatedImage); + } @override - List? debugGetOpenHandleStackTraces() => null; + List? debugGetOpenHandleStackTraces() => box.debugGetStackTraces(); + int get frameCount => _skAnimatedImage.getFrameCount(); /// Decodes the next frame and returns the frame duration. @@ -116,8 +127,15 @@ class CkImage implements ui.Image { // being garbage-collected, or by an explicit call to [delete]. late final SkiaObjectBox box; - CkImage(this.skImage) { - box = SkiaObjectBox(this, skImage as SkDeletable); + CkImage(SkImage skImage) : this._(skImage, null); + + CkImage._(this.skImage, SkiaObjectBox? boxToClone) { + if (boxToClone != null) { + assert(boxToClone.skObject == skImage); + box = boxToClone.clone(this); + } else { + box = SkiaObjectBox(this, skImage as SkDeletable); + } } @override @@ -126,13 +144,16 @@ class CkImage implements ui.Image { } @override - ui.Image clone() => this; + ui.Image clone() => CkImage._(skImage, box); @override - bool isCloneOf(ui.Image other) => other == this; + bool isCloneOf(ui.Image other) { + return other is CkImage + && other.skImage.isAliasOf(skImage); + } @override - List? debugGetOpenHandleStackTraces() => null; + List? debugGetOpenHandleStackTraces() => box.debugGetStackTraces(); @override int get width => skImage.width(); diff --git a/lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart b/lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart index 36cdf7e7c5..eaca5c43ca 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart @@ -239,23 +239,46 @@ abstract class OneShotSkiaObject extends SkiaObject { } } -/// Manages the lifecycle of a Skia object owned by a wrapper object. +/// Uses reference counting to manage the lifecycle of a Skia object owned by a +/// wrapper object. /// -/// When the wrapper is garbage collected, deletes the corresponding -/// [skObject] (only in browsers that support weak references). +/// When the wrapper is garbage collected, decrements the refcount (only in +/// browsers that support weak references). /// -/// The [delete] method can be used to eagerly delete the [skObject] -/// before the wrapper is garbage collected. +/// The [delete] method can be used to eagerly decrement the refcount before the +/// wrapper is garbage collected. /// /// The [delete] method may be called any number of times. The box /// will only delete the object once. class SkiaObjectBox { - SkiaObjectBox(Object wrapper, this.skObject) { + SkiaObjectBox(Object wrapper, SkDeletable skObject) + : this._(wrapper, skObject, {}); + + SkiaObjectBox._(Object wrapper, this.skObject, this._refs) { + if (assertionsEnabled) { + _debugStackTrace = StackTrace.current; + } + _refs.add(this); if (browserSupportsFinalizationRegistry) { boxRegistry.register(wrapper, this); } } + /// Reference handles to the same underlying [skObject]. + final Set _refs; + + late final StackTrace? _debugStackTrace; + /// If asserts are enabled, the [StackTrace]s representing when a reference + /// was created. + List? debugGetStackTraces() { + if (assertionsEnabled) { + return _refs + .map((SkiaObjectBox box) => box._debugStackTrace!) + .toList(); + } + return null; + } + /// The Skia object whose lifecycle is being managed. final SkDeletable skObject; @@ -269,16 +292,33 @@ class SkiaObjectBox { box.delete(); })); - /// Deletes the [skObject]. + /// Returns a clone of this object, which increases its reference count. + /// + /// Clones must be [dispose]d when finished. + SkiaObjectBox clone(Object wrapper) { + assert(!_isDeleted, 'Cannot clone from a deleted handle.'); + assert(_refs.isNotEmpty); + return SkiaObjectBox._(wrapper, skObject, _refs); + } + + /// Decrements the reference count for the [skObject]. /// /// Does nothing if the object has already been deleted. + /// + /// If this causes the reference count to drop to zero, deletes the + /// [skObject]. void delete() { if (_isDeleted) { + assert(!_refs.contains(this)); return; } + final bool removed = _refs.remove(this); + assert(removed); _isDeleted = true; - _skObjectDeleteQueue.add(skObject); - _skObjectCollector ??= _scheduleSkObjectCollection(); + if (_refs.isEmpty) { + _skObjectDeleteQueue.add(skObject); + _skObjectCollector ??= _scheduleSkObjectCollection(); + } } } diff --git a/lib/web_ui/test/canvaskit/image_test.dart b/lib/web_ui/test/canvaskit/image_test.dart index 11ed6e2a4b..08be25e093 100644 --- a/lib/web_ui/test/canvaskit/image_test.dart +++ b/lib/web_ui/test/canvaskit/image_test.dart @@ -38,6 +38,27 @@ void testMain() { expect(image.box.isDeleted, true); }); + test('CkAnimatedImage can be cloned and explicitly disposed of', () async { + final SkAnimatedImage skAnimatedImage = canvasKit.MakeAnimatedImageFromEncoded(kTransparentImage); + final CkAnimatedImage image = CkAnimatedImage(skAnimatedImage); + final CkAnimatedImage imageClone = image.clone(); + + expect(image.isCloneOf(imageClone), true); + expect(image.box.isDeleted, false); + await Future.delayed(Duration.zero); + expect(skAnimatedImage.isDeleted(), false); + image.dispose(); + expect(image.box.isDeleted, true); + expect(imageClone.box.isDeleted, false); + await Future.delayed(Duration.zero); + expect(skAnimatedImage.isDeleted(), false); + imageClone.dispose(); + expect(image.box.isDeleted, true); + expect(imageClone.box.isDeleted, true); + await Future.delayed(Duration.zero); + expect(skAnimatedImage.isDeleted(), true); + }); + test('CkImage toString', () { final SkImage skImage = canvasKit.MakeAnimatedImageFromEncoded(kTransparentImage).getCurrentFrame(); final CkImage image = CkImage(skImage); @@ -54,6 +75,27 @@ void testMain() { image.dispose(); expect(image.box.isDeleted, true); }); + + test('CkImage can be explicitly disposed of when cloned', () async { + final SkImage skImage = canvasKit.MakeAnimatedImageFromEncoded(kTransparentImage).getCurrentFrame(); + final CkImage image = CkImage(skImage); + final CkImage imageClone = image.clone(); + + expect(image.isCloneOf(imageClone), true); + expect(image.box.isDeleted, false); + await Future.delayed(Duration.zero); + expect(skImage.isDeleted(), false); + image.dispose(); + expect(image.box.isDeleted, true); + expect(imageClone.box.isDeleted, false); + await Future.delayed(Duration.zero); + expect(skImage.isDeleted(), false); + imageClone.dispose(); + expect(image.box.isDeleted, true); + expect(imageClone.box.isDeleted, true); + await Future.delayed(Duration.zero); + expect(skImage.isDeleted(), true); + }); // TODO: https://github.com/flutter/flutter/issues/60040 }, skip: isIosSafari); } diff --git a/lib/web_ui/test/canvaskit/skia_objects_cache_test.dart b/lib/web_ui/test/canvaskit/skia_objects_cache_test.dart index 08e0cb3d55..a47246c248 100644 --- a/lib/web_ui/test/canvaskit/skia_objects_cache_test.dart +++ b/lib/web_ui/test/canvaskit/skia_objects_cache_test.dart @@ -12,6 +12,7 @@ import 'package:ui/ui.dart' as ui; import 'package:ui/src/engine.dart'; import 'common.dart'; +import '../matchers.dart'; void main() { internalBootstrapBrowserTest(() => testMain); @@ -153,9 +154,49 @@ void _tests() { expect(SkiaObjects.oneShotCache.debugContains(object2), isFalse); }); }); + + + group(SkiaObjectBox, () { + test('Records stack traces and respects refcounts', () async { + TestOneShotSkiaObject.deleteCount = 0; + final TestOneShotSkiaObject skObject = TestOneShotSkiaObject(); + final Object wrapper = Object(); + final SkiaObjectBox box = SkiaObjectBox(wrapper, skObject); + + expect(box.debugGetStackTraces().length, 1); + + final SkiaObjectBox clone = box.clone(wrapper); + expect(clone, isNot(same(box))); + expect(clone.debugGetStackTraces().length, 2); + expect(box.debugGetStackTraces().length, 2); + + box.delete(); + + expect(() => box.clone(wrapper), throwsAssertionError); + + expect(box.isDeleted, true); + + // Let any timers elapse. + await Future.delayed(Duration.zero); + expect(TestOneShotSkiaObject.deleteCount, 0); + + expect(clone.debugGetStackTraces().length, 1); + expect(box.debugGetStackTraces().length, 1); + + clone.delete(); + expect(() => clone.clone(wrapper), throwsAssertionError); + + // Let any timers elapse. + await Future.delayed(Duration.zero); + expect(TestOneShotSkiaObject.deleteCount, 1); + + expect(clone.debugGetStackTraces().length, 0); + expect(box.debugGetStackTraces().length, 0); + }); + }); } -class TestOneShotSkiaObject extends OneShotSkiaObject { +class TestOneShotSkiaObject extends OneShotSkiaObject implements SkDeletable { static int deleteCount = 0; TestOneShotSkiaObject() : super(SkPaint()); -- GitLab