From 52664aa344fc0880d198e8fe17cfaf41c2f25f61 Mon Sep 17 00:00:00 2001 From: Mouad Debbar Date: Mon, 16 Dec 2024 14:10:17 -0500 Subject: [PATCH] [web] Don't close image source too early (#57200) A `CkImage` instance holds a reference to `ImageSource?`. When that `CkImage` gets cloned, the `ImageSource` instance becomes shared between the original `CkImage` and its new clone. Then when one of the `CkImage`s gets disposed of, it closes the shared `ImageSource` leaving other live `CkImage`s holding on to a closed `ImageSource`. The quick solution to this is to have a ref count on the `ImageSource` to count how many `CkImage`s are referencing it. The `ImageSource` will only be closed if its ref count reaches 0. Fixes https://github.com/flutter/flutter/issues/160199 Fixes https://github.com/flutter/flutter/issues/158093 --- .../lib/src/engine/canvaskit/image.dart | 32 ++++++++++++++++--- lib/web_ui/lib/src/engine/dom.dart | 6 ++++ lib/web_ui/test/canvaskit/image_test.dart | 30 +++++++++++++++-- 3 files changed, 61 insertions(+), 7 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/image.dart b/lib/web_ui/lib/src/engine/canvaskit/image.dart index 812c98feb2099..5b4edb6a2939f 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/image.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/image.dart @@ -7,6 +7,7 @@ import 'dart:js_interop'; import 'dart:math' as math; import 'dart:typed_data'; +import 'package:meta/meta.dart'; import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart' as ui; import 'package:ui/ui_web/src/ui_web.dart' as ui_web; @@ -403,11 +404,13 @@ class CkImage implements ui.Image, StackTraceDebugger { CkImage(SkImage skImage, {this.imageSource}) { box = CountedRef(skImage, this, 'SkImage'); _init(); + imageSource?.refCount++; } CkImage.cloneOf(this.box, {this.imageSource}) { _init(); box.ref(this); + imageSource?.refCount++; } void _init() { @@ -454,6 +457,8 @@ class CkImage implements ui.Image, StackTraceDebugger { ui.Image.onDispose?.call(this); _disposed = true; box.unref(this); + + imageSource?.refCount--; imageSource?.close(); } @@ -645,7 +650,26 @@ sealed class ImageSource { DomCanvasImageSource get canvasImageSource; int get width; int get height; - void close(); + + /// The number of references to this image source. + /// + /// Calling [close] is a no-op if [refCount] is greater than 0. + /// + /// Only when [refCount] is 0 will the [close] method actually close the + /// image source. + int refCount = 0; + + @visibleForTesting + bool debugIsClosed = false; + + void close() { + if (refCount == 0) { + _doClose(); + debugIsClosed = true; + } + } + + void _doClose(); } class VideoFrameImageSource extends ImageSource { @@ -654,7 +678,7 @@ class VideoFrameImageSource extends ImageSource { final VideoFrame videoFrame; @override - void close() { + void _doClose() { // Do nothing. Skia will close the VideoFrame when the SkImage is disposed. } @@ -674,7 +698,7 @@ class ImageElementImageSource extends ImageSource { final DomHTMLImageElement imageElement; @override - void close() { + void _doClose() { // There's no way to immediately close the element. Just let the // browser garbage collect it. } @@ -695,7 +719,7 @@ class ImageBitmapImageSource extends ImageSource { final DomImageBitmap imageBitmap; @override - void close() { + void _doClose() { imageBitmap.close(); } diff --git a/lib/web_ui/lib/src/engine/dom.dart b/lib/web_ui/lib/src/engine/dom.dart index 5fb335f11b728..caf4888bbc05a 100644 --- a/lib/web_ui/lib/src/engine/dom.dart +++ b/lib/web_ui/lib/src/engine/dom.dart @@ -169,6 +169,12 @@ extension DomWindowExtension on DomWindow { /// The Trusted Types API (when available). /// See: https://developer.mozilla.org/en-US/docs/Web/API/Trusted_Types_API external DomTrustedTypePolicyFactory? get trustedTypes; + + @JS('createImageBitmap') + external JSPromise _createImageBitmap(DomImageData source); + Future createImageBitmap(DomImageData source) { + return js_util.promiseToFuture(_createImageBitmap(source)); + } } typedef DomRequestAnimationFrameCallback = void Function(JSNumber highResTime); diff --git a/lib/web_ui/test/canvaskit/image_test.dart b/lib/web_ui/test/canvaskit/image_test.dart index c23ae19fd196a..9a30849365011 100644 --- a/lib/web_ui/test/canvaskit/image_test.dart +++ b/lib/web_ui/test/canvaskit/image_test.dart @@ -7,12 +7,11 @@ import 'dart:typed_data'; import 'package:test/bootstrap/browser.dart'; import 'package:test/test.dart'; -import 'package:ui/src/engine/canvaskit/image.dart'; -import 'package:ui/src/engine/image_decoder.dart'; -import 'package:ui/src/engine/util.dart'; +import 'package:ui/src/engine.dart'; import 'package:ui/ui.dart' as ui; import 'common.dart'; +import 'test_data.dart'; void main() { internalBootstrapBrowserTest(() => testMain); @@ -131,6 +130,31 @@ void testMain() { expect(activeImages.length, 0); }); + + test('CkImage does not close image source too early', () async { + final ImageSource imageSource = ImageBitmapImageSource( + await domWindow.createImageBitmap(createBlankDomImageData(4, 4)), + ); + + final SkImage skImage1 = canvasKit.MakeAnimatedImageFromEncoded(k4x4PngImage)!.makeImageAtCurrentFrame(); + final CkImage image1 = CkImage(skImage1, imageSource: imageSource); + + final SkImage skImage2 = canvasKit.MakeAnimatedImageFromEncoded(k4x4PngImage)!.makeImageAtCurrentFrame(); + final CkImage image2 = CkImage(skImage2, imageSource: imageSource); + + final CkImage image3 = image1.clone(); + + expect(imageSource.debugIsClosed, isFalse); + + image1.dispose(); + expect(imageSource.debugIsClosed, isFalse); + + image2.dispose(); + expect(imageSource.debugIsClosed, isFalse); + + image3.dispose(); + expect(imageSource.debugIsClosed, isTrue); + }); } Future _createImage() => _createPicture().toImage(10, 10);