Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 31 additions & 12 deletions lib/web_ui/lib/src/engine/canvaskit/image.dart
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,16 @@ class CkAnimatedImage implements ui.Image {

// Use a box because `SkImage` may be deleted either due to this object
// being garbage-collected, or by an explicit call to [delete].
late final SkiaObjectBox box;
late final RefCountedSkiaObjectBox box;

CkAnimatedImage(this._skAnimatedImage) {
box = SkiaObjectBox(this, _skAnimatedImage as SkDeletable);
CkAnimatedImage(SkAnimatedImage skAnimatedImage) : this._(skAnimatedImage, null);

CkAnimatedImage._(this._skAnimatedImage, RefCountedSkiaObjectBox? boxToClone) {
if (boxToClone != null) {
box = boxToClone.clone(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we assert that the image in boxToClone and _skAnimatedImage are the same?

Alternatively, we could split this into two specialized constructors.

} else {
box = RefCountedSkiaObjectBox(this, _skAnimatedImage as SkDeletable);
}
}

@override
Expand All @@ -53,13 +59,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 == _skAnimatedImage;
}

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

int get frameCount => _skAnimatedImage.getFrameCount();

/// Decodes the next frame and returns the frame duration.
Expand Down Expand Up @@ -114,10 +124,16 @@ class CkImage implements ui.Image {

// Use a box because `SkImage` may be deleted either due to this object
// being garbage-collected, or by an explicit call to [delete].
late final SkiaObjectBox box;
late final RefCountedSkiaObjectBox box;

CkImage(this.skImage) {
box = SkiaObjectBox(this, skImage as SkDeletable);
CkImage(SkImage skImage) : this._(skImage, null);

CkImage._(this.skImage, RefCountedSkiaObjectBox? boxToClone) {
if (boxToClone != null) {
box = boxToClone.clone(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as in CkAnimatedImage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added these asserts

} else {
box = RefCountedSkiaObjectBox(this, skImage as SkDeletable);
}
}

@override
Expand All @@ -126,13 +142,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 == skImage;
}

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

@override
int get width => skImage.width();
Expand Down
66 changes: 66 additions & 0 deletions lib/web_ui/lib/src/engine/canvaskit/skia_object_cache.dart
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,72 @@ class SkiaObjectBox {
}
}

/// Uses reference counting to manage the lifecycle of a Skia object owned by a
/// wrapper object.
///
/// When the wrapper is garbage collected, decrements the refcount (only in
/// browsers that support weak references).
///
/// 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 RefCountedSkiaObjectBox extends SkiaObjectBox {
Copy link
Contributor

Choose a reason for hiding this comment

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

The image classes are the only users of SkiaObjectBox. Can we just make SkiaObjectBox ref-counted and not have two classes?

RefCountedSkiaObjectBox(Object wrapper, SkDeletable skObject)
: this._(wrapper, skObject, <RefCountedSkiaObjectBox>{});

RefCountedSkiaObjectBox._(Object wrapper, SkDeletable skObject, this._refs)
: super(wrapper, skObject) {
_refs.add(this);
if (assertionsEnabled) {
_debugStackTrace = StackTrace.current;
}
}

/// Reference handles to the same underlying [skObject].
final Set<RefCountedSkiaObjectBox> _refs;

late final StackTrace? _debugStackTrace;
/// If asserts are enabled, the [StackTrace]s representing when a reference
/// was created.
List<StackTrace>? debugGetStackTraces() {
if (assertionsEnabled) {
return _refs
.map<StackTrace>((RefCountedSkiaObjectBox box) => box._debugStackTrace!)
.toList();
}
return null;
}

RefCountedSkiaObjectBox clone(Object wrapper) {
assert(!_isDeleted, 'Cannot clone from a deleted handle.');
assert(_refs.isNotEmpty);
return RefCountedSkiaObjectBox._(wrapper, skObject, _refs);
}

/// Removes the reference count for the [skObject].
Copy link
Contributor

Choose a reason for hiding this comment

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

"Removes the reference count" doesn't sound right. Maybe "Decrement the reference count" or "Remove this box from reference list"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done and done.

///
/// Does nothing if the object has already been deleted.
///
/// If this causes the reference count to drop to zero, deletes the
/// [skObject].
@override
void delete() {
if (_isDeleted) {
assert(!_refs.contains(this));
return;
}
final bool removed = _refs.remove(this);
assert(removed);
_isDeleted = true;
if (_refs.isEmpty) {
_skObjectDeleteQueue.add(skObject);
_skObjectCollector ??= _scheduleSkObjectCollection();
}
}
}

/// Singleton that manages the lifecycles of [SkiaObject] instances.
class SkiaObjects {
@visibleForTesting
Expand Down
30 changes: 30 additions & 0 deletions lib/web_ui/test/canvaskit/image_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,21 @@ void testMain() {
expect(image.box.isDeleted, true);
});

test('CkAnimatedImage can be cloned and explicitly disposed of', () {
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);
image.dispose();
expect(image.box.isDeleted, true);
expect(imageClone.box.isDeleted, false);
imageClone.dispose();
expect(image.box.isDeleted, true);
expect(imageClone.box.isDeleted, true);
});

test('CkImage toString', () {
final SkImage skImage = canvasKit.MakeAnimatedImageFromEncoded(kTransparentImage).getCurrentFrame();
final CkImage image = CkImage(skImage);
Expand All @@ -54,6 +69,21 @@ void testMain() {
image.dispose();
expect(image.box.isDeleted, true);
});

test('CkImage can be explicitly disposed of', () {
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);
image.dispose();
expect(image.box.isDeleted, true);
expect(imageClone.box.isDeleted, false);
imageClone.dispose();
expect(image.box.isDeleted, true);
expect(imageClone.box.isDeleted, true);
});
// TODO: https://github.com/flutter/flutter/issues/60040
}, skip: isIosSafari);
}
33 changes: 32 additions & 1 deletion lib/web_ui/test/canvaskit/skia_objects_cache_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -153,6 +154,36 @@ void _tests() {
expect(SkiaObjects.oneShotCache.debugContains(object2), isFalse);
});
});

group(RefCountedSkiaObjectBox, () {
test('Records stack traces and respects refcounts', () {
final TestSkiaObject skObject = TestSkiaObject();
final Object wrapper = Object();
final RefCountedSkiaObjectBox box = RefCountedSkiaObjectBox(wrapper, skObject);

expect(box.debugGetStackTraces().length, 1);

final RefCountedSkiaObjectBox 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(skObject.deleteCount, 0);
expect(clone.debugGetStackTraces().length, 1);
expect(box.debugGetStackTraces().length, 1);

clone.delete();
expect(() => clone.clone(wrapper), throwsAssertionError);

expect(skObject.deleteCount, 1);
expect(clone.debugGetStackTraces().length, 0);
expect(box.debugGetStackTraces().length, 0);
});
});
}

class TestOneShotSkiaObject extends OneShotSkiaObject<SkPaint> {
Expand All @@ -167,7 +198,7 @@ class TestOneShotSkiaObject extends OneShotSkiaObject<SkPaint> {
}
}

class TestSkiaObject extends ManagedSkiaObject<SkPaint> {
class TestSkiaObject extends ManagedSkiaObject<SkPaint> implements SkDeletable {
int createDefaultCount = 0;
int resurrectCount = 0;
int deleteCount = 0;
Expand Down