Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Commit 52664aa

Browse files
authored
[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 flutter/flutter#160199 Fixes flutter/flutter#158093
1 parent 0dfbd04 commit 52664aa

File tree

3 files changed

+61
-7
lines changed

3 files changed

+61
-7
lines changed

lib/web_ui/lib/src/engine/canvaskit/image.dart

+28-4
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import 'dart:js_interop';
77
import 'dart:math' as math;
88
import 'dart:typed_data';
99

10+
import 'package:meta/meta.dart';
1011
import 'package:ui/src/engine.dart';
1112
import 'package:ui/ui.dart' as ui;
1213
import 'package:ui/ui_web/src/ui_web.dart' as ui_web;
@@ -403,11 +404,13 @@ class CkImage implements ui.Image, StackTraceDebugger {
403404
CkImage(SkImage skImage, {this.imageSource}) {
404405
box = CountedRef<CkImage, SkImage>(skImage, this, 'SkImage');
405406
_init();
407+
imageSource?.refCount++;
406408
}
407409

408410
CkImage.cloneOf(this.box, {this.imageSource}) {
409411
_init();
410412
box.ref(this);
413+
imageSource?.refCount++;
411414
}
412415

413416
void _init() {
@@ -454,6 +457,8 @@ class CkImage implements ui.Image, StackTraceDebugger {
454457
ui.Image.onDispose?.call(this);
455458
_disposed = true;
456459
box.unref(this);
460+
461+
imageSource?.refCount--;
457462
imageSource?.close();
458463
}
459464

@@ -645,7 +650,26 @@ sealed class ImageSource {
645650
DomCanvasImageSource get canvasImageSource;
646651
int get width;
647652
int get height;
648-
void close();
653+
654+
/// The number of references to this image source.
655+
///
656+
/// Calling [close] is a no-op if [refCount] is greater than 0.
657+
///
658+
/// Only when [refCount] is 0 will the [close] method actually close the
659+
/// image source.
660+
int refCount = 0;
661+
662+
@visibleForTesting
663+
bool debugIsClosed = false;
664+
665+
void close() {
666+
if (refCount == 0) {
667+
_doClose();
668+
debugIsClosed = true;
669+
}
670+
}
671+
672+
void _doClose();
649673
}
650674

651675
class VideoFrameImageSource extends ImageSource {
@@ -654,7 +678,7 @@ class VideoFrameImageSource extends ImageSource {
654678
final VideoFrame videoFrame;
655679

656680
@override
657-
void close() {
681+
void _doClose() {
658682
// Do nothing. Skia will close the VideoFrame when the SkImage is disposed.
659683
}
660684

@@ -674,7 +698,7 @@ class ImageElementImageSource extends ImageSource {
674698
final DomHTMLImageElement imageElement;
675699

676700
@override
677-
void close() {
701+
void _doClose() {
678702
// There's no way to immediately close the <img> element. Just let the
679703
// browser garbage collect it.
680704
}
@@ -695,7 +719,7 @@ class ImageBitmapImageSource extends ImageSource {
695719
final DomImageBitmap imageBitmap;
696720

697721
@override
698-
void close() {
722+
void _doClose() {
699723
imageBitmap.close();
700724
}
701725

lib/web_ui/lib/src/engine/dom.dart

+6
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,12 @@ extension DomWindowExtension on DomWindow {
169169
/// The Trusted Types API (when available).
170170
/// See: https://developer.mozilla.org/en-US/docs/Web/API/Trusted_Types_API
171171
external DomTrustedTypePolicyFactory? get trustedTypes;
172+
173+
@JS('createImageBitmap')
174+
external JSPromise<JSAny?> _createImageBitmap(DomImageData source);
175+
Future<DomImageBitmap> createImageBitmap(DomImageData source) {
176+
return js_util.promiseToFuture<DomImageBitmap>(_createImageBitmap(source));
177+
}
172178
}
173179

174180
typedef DomRequestAnimationFrameCallback = void Function(JSNumber highResTime);

lib/web_ui/test/canvaskit/image_test.dart

+27-3
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,11 @@ import 'dart:typed_data';
77

88
import 'package:test/bootstrap/browser.dart';
99
import 'package:test/test.dart';
10-
import 'package:ui/src/engine/canvaskit/image.dart';
11-
import 'package:ui/src/engine/image_decoder.dart';
12-
import 'package:ui/src/engine/util.dart';
10+
import 'package:ui/src/engine.dart';
1311
import 'package:ui/ui.dart' as ui;
1412

1513
import 'common.dart';
14+
import 'test_data.dart';
1615

1716
void main() {
1817
internalBootstrapBrowserTest(() => testMain);
@@ -131,6 +130,31 @@ void testMain() {
131130

132131
expect(activeImages.length, 0);
133132
});
133+
134+
test('CkImage does not close image source too early', () async {
135+
final ImageSource imageSource = ImageBitmapImageSource(
136+
await domWindow.createImageBitmap(createBlankDomImageData(4, 4)),
137+
);
138+
139+
final SkImage skImage1 = canvasKit.MakeAnimatedImageFromEncoded(k4x4PngImage)!.makeImageAtCurrentFrame();
140+
final CkImage image1 = CkImage(skImage1, imageSource: imageSource);
141+
142+
final SkImage skImage2 = canvasKit.MakeAnimatedImageFromEncoded(k4x4PngImage)!.makeImageAtCurrentFrame();
143+
final CkImage image2 = CkImage(skImage2, imageSource: imageSource);
144+
145+
final CkImage image3 = image1.clone();
146+
147+
expect(imageSource.debugIsClosed, isFalse);
148+
149+
image1.dispose();
150+
expect(imageSource.debugIsClosed, isFalse);
151+
152+
image2.dispose();
153+
expect(imageSource.debugIsClosed, isFalse);
154+
155+
image3.dispose();
156+
expect(imageSource.debugIsClosed, isTrue);
157+
});
134158
}
135159

136160
Future<ui.Image> _createImage() => _createPicture().toImage(10, 10);

0 commit comments

Comments
 (0)