Skip to content

Commit

Permalink
Revert "[web] remove obsolete object caches; simplify native object m…
Browse files Browse the repository at this point in the history
…anagement (#40894)"

This reverts commit 3e5f27d.
  • Loading branch information
CaseyHillers authored Apr 5, 2023
1 parent 4219c72 commit 2256882
Show file tree
Hide file tree
Showing 13 changed files with 772 additions and 559 deletions.
2 changes: 0 additions & 2 deletions ci/licenses_golden/licenses_flutter
Original file line number Diff line number Diff line change
Expand Up @@ -1876,7 +1876,6 @@ ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_tree.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/mask_filter.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/n_way_canvas.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/noto_font.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/painting.dart + ../../../flutter/LICENSE
ORIGIN: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart + ../../../flutter/LICENSE
Expand Down Expand Up @@ -4461,7 +4460,6 @@ FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_scene_builder.d
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/layer_tree.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/mask_filter.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/n_way_canvas.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/native_memory.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/noto_font.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/painting.dart
FILE: ../../../flutter/lib/web_ui/lib/src/engine/canvaskit/path.dart
Expand Down
1 change: 0 additions & 1 deletion lib/web_ui/lib/src/engine.dart
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,6 @@ export 'engine/canvaskit/layer_scene_builder.dart';
export 'engine/canvaskit/layer_tree.dart';
export 'engine/canvaskit/mask_filter.dart';
export 'engine/canvaskit/n_way_canvas.dart';
export 'engine/canvaskit/native_memory.dart';
export 'engine/canvaskit/noto_font.dart';
export 'engine/canvaskit/painting.dart';
export 'engine/canvaskit/path.dart';
Expand Down
2 changes: 2 additions & 0 deletions lib/web_ui/lib/src/engine/canvaskit/canvas.dart
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ class CkCanvas {
offset.dx,
offset.dy,
);
paragraph.markUsed();
}

void drawPath(CkPath path, CkPaint paint) {
Expand Down Expand Up @@ -1111,6 +1112,7 @@ class CkDrawParagraphCommand extends CkPaintCommand {
offset.dx,
offset.dy,
);
paragraph.markUsed();
}
}

Expand Down
13 changes: 5 additions & 8 deletions lib/web_ui/lib/src/engine/canvaskit/canvaskit_api.dart
Original file line number Diff line number Diff line change
Expand Up @@ -3392,7 +3392,7 @@ abstract class Collector {
class ProductionCollector implements Collector {
ProductionCollector() {
_skObjectFinalizationRegistry =
createSkObjectFinalizationRegistry((SkDeletable deletable) {
SkObjectFinalizationRegistry((SkDeletable deletable) {
// This is called when GC decides to collect the wrapper object and
// notify us, which may happen after the object is already deleted
// explicitly, e.g. when its ref count drops to zero. When that happens
Expand Down Expand Up @@ -3568,13 +3568,10 @@ extension JsConstructorExtension on JsConstructor {
/// 6. We call `delete` on SkPaint.
@JS('window.FinalizationRegistry')
@staticInterop
class SkObjectFinalizationRegistry {}

SkObjectFinalizationRegistry createSkObjectFinalizationRegistry(JSFunction cleanup) {
return js_util.callConstructor(
_finalizationRegistryConstructor!.toObjectShallow,
<Object>[cleanup],
);
class SkObjectFinalizationRegistry {
// TODO(hterkelsen): Add a type for the `cleanup` function when
// native constructors support type parameters.
external factory SkObjectFinalizationRegistry(JSFunction cleanup);
}

extension SkObjectFinalizationRegistryExtension on SkObjectFinalizationRegistry {
Expand Down
56 changes: 50 additions & 6 deletions lib/web_ui/lib/src/engine/canvaskit/image.dart
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ import 'canvas.dart';
import 'canvaskit_api.dart';
import 'image_wasm_codecs.dart';
import 'image_web_codecs.dart';
import 'native_memory.dart';
import 'painting.dart';
import 'picture.dart';
import 'picture_recorder.dart';
import 'skia_object_cache.dart';

/// Instantiates a [ui.Codec] backed by an `SkAnimatedImage` from Skia.
FutureOr<ui.Codec> skiaInstantiateImageCodec(Uint8List list,
Expand Down Expand Up @@ -227,8 +227,52 @@ Future<Uint8List> readChunked(HttpFetchPayload payload, int contentLength, WebOn
/// A [ui.Image] backed by an `SkImage` from Skia.
class CkImage implements ui.Image, StackTraceDebugger {
CkImage(SkImage skImage, { this.videoFrame }) {
box = CountedRef<CkImage, SkImage>(skImage, this, 'SkImage');
_init();
if (browserSupportsFinalizationRegistry) {
box = SkiaObjectBox<CkImage, SkImage>(this, skImage);
} else {
// If finalizers are not supported we need to be able to resurrect the
// image if it was temporarily deleted. To do that, we keep the original
// pixels and ask the SkiaObjectBox to make an image from them when
// resurrecting.
//
// IMPORTANT: the alphaType, colorType, and colorSpace passed to
// _encodeImage and to canvasKit.MakeImage must be the same. Otherwise
// Skia will misinterpret the pixels and corrupt the image.
final ByteData? originalBytes = _encodeImage(
skImage: skImage,
format: ui.ImageByteFormat.rawRgba,
alphaType: canvasKit.AlphaType.Premul,
colorType: canvasKit.ColorType.RGBA_8888,
colorSpace: SkColorSpaceSRGB,
);
if (originalBytes == null) {
printWarning('Unable to encode image to bytes. We will not '
'be able to resurrect it once it has been garbage collected.');
return;
}
final int originalWidth = skImage.width().toInt();
final int originalHeight = skImage.height().toInt();
box = SkiaObjectBox<CkImage, SkImage>.resurrectable(this, skImage, () {
final SkImage? skImage = canvasKit.MakeImage(
SkImageInfo(
alphaType: canvasKit.AlphaType.Premul,
colorType: canvasKit.ColorType.RGBA_8888,
colorSpace: SkColorSpaceSRGB,
width: originalWidth.toDouble(),
height: originalHeight.toDouble(),
),
originalBytes.buffer.asUint8List(),
(4 * originalWidth).toDouble(),
);
if (skImage == null) {
throw ImageCodecException(
'Failed to resurrect image from pixels.'
);
}
return skImage;
});
}
}

CkImage.cloneOf(this.box, {this.videoFrame}) {
Expand All @@ -247,9 +291,9 @@ class CkImage implements ui.Image, StackTraceDebugger {
StackTrace get debugStackTrace => _debugStackTrace;
late StackTrace _debugStackTrace;

// Use ref counting because `SkImage` may be deleted either due to this object
// 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 CountedRef<CkImage, SkImage> box;
late final SkiaObjectBox<CkImage, SkImage> box;

/// For browsers that support `ImageDecoder` this field holds the video frame
/// from which this image was created.
Expand All @@ -261,9 +305,9 @@ class CkImage implements ui.Image, StackTraceDebugger {

/// The underlying Skia image object.
///
/// Do not store the returned value. It is memory-managed by [CountedRef].
/// Do not store the returned value. It is memory-managed by [SkiaObjectBox].
/// Storing it may result in use-after-free bugs.
SkImage get skImage => box.nativeObject;
SkImage get skImage => box.skiaObject;

bool _disposed = false;

Expand Down
216 changes: 0 additions & 216 deletions lib/web_ui/lib/src/engine/canvaskit/native_memory.dart

This file was deleted.

3 changes: 2 additions & 1 deletion lib/web_ui/lib/src/engine/canvaskit/picture.dart
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ class CkPicture extends ManagedSkiaObject<SkPicture> implements ui.Picture {
/// false.
///
/// This extra flag is necessary on top of [rawSkiaObject] because
/// [rawSkiaObject] being null does not indicate permanent deletion.
/// [rawSkiaObject] being null does not indicate permanent deletion. See
/// similar flag [SkiaObjectBox.isDeletedPermanently].
bool _isDisposed = false;

/// The stack trace taken when [dispose] was called.
Expand Down
Loading

0 comments on commit 2256882

Please sign in to comment.