Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Instrument Image and Picture for leak tracking. #35274

Merged
merged 50 commits into from
Sep 9, 2022
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
50 commits
Select commit Hold shift + click to select a range
eb5cc0e
Update painting.dart
polina-c Aug 9, 2022
401a169
Update painting.dart
polina-c Aug 9, 2022
d6df6b8
-
polina-c Aug 10, 2022
eaf2ec8
-
polina-c Aug 10, 2022
3e17cd1
Update image_test.dart
polina-c Aug 10, 2022
b47c31e
-
polina-c Aug 10, 2022
427592e
Update painting.dart
polina-c Aug 10, 2022
dbe4eaf
Merge branch 'flutter:main' into painting
polina-c Aug 11, 2022
804d542
Merge branch 'main' of github.com:flutter/engine into painting
polina-c Aug 11, 2022
1434af9
Update painting.dart
polina-c Aug 11, 2022
60a95a0
Update image_test.dart
polina-c Aug 11, 2022
e2a1f76
-
polina-c Aug 11, 2022
ca99635
Update image_dispose_test.dart
polina-c Aug 11, 2022
1f85ecf
Update image_test.dart
polina-c Aug 11, 2022
4a3d810
Merge branch 'painting' of github.com:polina-c/engine into painting
polina-c Aug 11, 2022
b577107
-
polina-c Aug 11, 2022
1e902a1
Update picture_test.dart
polina-c Aug 11, 2022
b6ff393
-
polina-c Aug 11, 2022
f03e228
Update image_test.dart
polina-c Aug 11, 2022
6832d11
Update picture_test.dart
polina-c Aug 11, 2022
2ef20d0
Update image_test.dart
polina-c Aug 11, 2022
932932f
Update image_test.dart
polina-c Aug 11, 2022
4ea889e
-
polina-c Aug 13, 2022
0f53a5b
-
polina-c Aug 13, 2022
c9df53e
Update image_test.dart
polina-c Aug 13, 2022
3f258cd
Update image_test.dart
polina-c Aug 24, 2022
f951116
Update picture_test.dart
polina-c Aug 24, 2022
7940b1e
Update image_test.dart
polina-c Aug 25, 2022
cc9912f
-
polina-c Aug 25, 2022
5ae80a6
Update painting.dart
polina-c Aug 25, 2022
ba7d0a5
-
polina-c Aug 25, 2022
7fec23d
-
polina-c Aug 25, 2022
9a8de06
-
polina-c Aug 29, 2022
22babb6
Create image_test.dart
polina-c Aug 29, 2022
fca82fe
Merge branch 'master' of github.com:flutter/engine into painting
polina-c Aug 30, 2022
41754ed
-
polina-c Aug 30, 2022
4b24628
-
polina-c Aug 30, 2022
59cb250
Update image_test.dart
polina-c Aug 31, 2022
0efa9c8
Update BUILD.gn
polina-c Aug 31, 2022
62431c1
-
polina-c Aug 31, 2022
a958c1d
Update painting.dart
polina-c Aug 31, 2022
20cd6d9
Update api_conform_test.dart
polina-c Aug 31, 2022
76f096e
Update painting.dart
polina-c Aug 31, 2022
6f949a2
Update painting.dart
polina-c Aug 31, 2022
f87653e
Update canvas.dart
polina-c Aug 31, 2022
99ae37d
addressed comments
polina-c Sep 7, 2022
360ff2a
Update painting.dart
polina-c Sep 8, 2022
0da0e31
Merge branch 'flutter:main' into painting
polina-c Sep 8, 2022
e972fd8
Merge branch 'flutter:main' into painting
polina-c Sep 8, 2022
bf75813
Merge branch 'flutter:main' into painting
polina-c Sep 9, 2022
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
41 changes: 41 additions & 0 deletions lib/ui/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -1624,6 +1624,10 @@ enum PixelFormat {
bgra8888,
}


/// Signature for [Image] lifecycle events.
typedef ImageEventCallback = void Function(Image image);

/// Opaque handle to raw decoded image data (pixels).
///
/// To obtain an [Image] object, use the [ImageDescriptor] API.
Expand Down Expand Up @@ -1655,12 +1659,27 @@ class Image {
return true;
}());
_image._handles.add(this);
onCreate?.call(this);
}

// C++ unit tests access this.
@pragma('vm:entry-point')
final _Image _image;

/// A callback that is invoked to report an object creation.
polina-c marked this conversation as resolved.
Show resolved Hide resolved
///
/// It's preferred to use [MemoryAllocations] in flutter/foundation.dart
/// than to use [onCreate] directly because [MemoryAllocations]
/// allows multiple callbacks.
static ImageEventCallback? onCreate;

/// A callback that is invoked to report the object disposal.
polina-c marked this conversation as resolved.
Show resolved Hide resolved
///
/// It's preferred to use [MemoryAllocations] in flutter/foundation.dart
/// than to use [onDispose] directly because [MemoryAllocations]
/// allows multiple callbacks.
static ImageEventCallback? onDispose;

StackTrace? _debugStack;

/// The number of image pixels along the image's horizontal axis.
Expand All @@ -1681,6 +1700,7 @@ class Image {
/// useful when trying to determine what parts of the program are keeping an
/// image resident in memory.
void dispose() {
onDispose?.call(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here and elsewhere: can we guard calling these so it's compiled out of release mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I know there is no compile time flags (like bool.fromEnvironment('flutter.memory_allocations')) in engine.
And 'normal' flags will not be more efficient both from performance perspective and code size perspective, than comparison to null. So there is no point in such guarding.

Copy link
Contributor

Choose a reason for hiding this comment

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

Discussed offline - any flag we put in here would be at best a runtime check that's probably no faster than the null check here.

assert(!_disposed && !_image._disposed);
assert(_image._handles.contains(this));
_disposed = true;
Expand Down Expand Up @@ -5661,6 +5681,9 @@ class Canvas extends NativeFieldWrapperClass1 {
external void _drawShadow(Path path, int color, double elevation, bool transparentOccluder);
}

/// Signature for [Picture] lifecycle events.
typedef PictureEventCallback = void Function(Picture picture);

/// An object representing a sequence of recorded graphical operations.
///
/// To create a [Picture], use a [PictureRecorder].
Expand All @@ -5677,6 +5700,20 @@ class Picture extends NativeFieldWrapperClass1 {
@pragma('vm:entry-point')
Picture._();

/// A callback that is invoked to report an object creation.
polina-c marked this conversation as resolved.
Show resolved Hide resolved
///
/// It's preferred to use [MemoryAllocations] in flutter/foundation.dart
/// than to use [onCreate] directly because [MemoryAllocations]
/// allows multiple callbacks.
static PictureEventCallback? onCreate;

/// A callback that is invoked to report the object disposal.
polina-c marked this conversation as resolved.
Show resolved Hide resolved
///
/// It's preferred to use [MemoryAllocations] in flutter/foundation.dart
/// than to use [onDispose] directly because [MemoryAllocations]
/// allows multiple callbacks.
static PictureEventCallback? onDispose;

/// Creates an image from this picture.
///
/// The returned image will be `width` pixels wide and `height` pixels high.
Expand Down Expand Up @@ -5740,6 +5777,7 @@ class Picture extends NativeFieldWrapperClass1 {
_disposed = true;
return true;
}());
onDispose?.call(this);
_dispose();
}

Expand Down Expand Up @@ -5806,6 +5844,9 @@ class PictureRecorder extends NativeFieldWrapperClass1 {
_endRecording(picture);
_canvas!._recorder = null;
_canvas = null;
// We invoke the handler here, not in the Picture constructor, because we want
// [picture.approximateBytesUsed] to be available for the handler.
Picture.onCreate?.call(picture);
return picture;
}

Expand Down
4 changes: 4 additions & 0 deletions lib/web_ui/lib/canvas.dart
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,11 @@ abstract class Canvas {
);
}

typedef PictureEventCallback = void Function(Picture picture);

abstract class Picture {
static PictureEventCallback? onCreate;
static PictureEventCallback? onDispose;
Future<Image> toImage(int width, int height);
Image toImageSync(int width, int height);
void dispose();
Expand Down
5 changes: 5 additions & 0 deletions lib/web_ui/lib/painting.dart
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,12 @@ abstract class Gradient extends Shader {
matrix4 != null ? engine.toMatrix32(matrix4) : null);
}

typedef ImageEventCallback = void Function(Image image);

abstract class Image {
static ImageEventCallback? onCreate;
static ImageEventCallback? onDispose;

int get width;
int get height;
Future<ByteData?> toByteData({ImageByteFormat format = ImageByteFormat.rawRgba});
Expand Down
12 changes: 8 additions & 4 deletions lib/web_ui/lib/src/engine/canvaskit/image.dart
Original file line number Diff line number Diff line change
Expand Up @@ -149,9 +149,7 @@ Future<Uint8List> fetchImage(
/// A [ui.Image] backed by an `SkImage` from Skia.
class CkImage implements ui.Image, StackTraceDebugger {
CkImage(SkImage skImage, { this.videoFrame }) {
if (assertionsEnabled) {
_debugStackTrace = StackTrace.current;
}
_init();
if (browserSupportsFinalizationRegistry) {
box = SkiaObjectBox<CkImage, SkImage>(this, skImage);
} else {
Expand Down Expand Up @@ -200,10 +198,15 @@ class CkImage implements ui.Image, StackTraceDebugger {
}

CkImage.cloneOf(this.box) {
_init();
box.ref(this);
}

void _init() {
if (assertionsEnabled) {
_debugStackTrace = StackTrace.current;
}
box.ref(this);
ui.Image.onCreate?.call(this);
}

@override
Expand Down Expand Up @@ -237,6 +240,7 @@ class CkImage implements ui.Image, StackTraceDebugger {

@override
void dispose() {
ui.Image.onDispose?.call(this);
polina-c marked this conversation as resolved.
Show resolved Hide resolved
assert(
!_disposed,
'Cannot dispose an image that has already been disposed.',
Expand Down
1 change: 1 addition & 0 deletions lib/web_ui/lib/src/engine/canvaskit/picture.dart
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ class CkPicture extends ManagedSkiaObject<SkPicture> implements ui.Picture {
_debugDisposalStackTrace = StackTrace.current;
return true;
}());
ui.Picture.onDispose?.call(this);
if (Instrumentation.enabled) {
Instrumentation.instance.incrementCounter('Picture disposed');
}
Expand Down
7 changes: 6 additions & 1 deletion lib/web_ui/lib/src/engine/canvaskit/picture_recorder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,12 @@ class CkPictureRecorder implements ui.PictureRecorder {
final SkPicture skPicture = recorder.finishRecordingAsPicture();
recorder.delete();
_skRecorder = null;
return CkPicture(skPicture, _cullRect, _recordingCanvas!.pictureSnapshot);
final CkPicture result =
CkPicture(skPicture, _cullRect, _recordingCanvas!.pictureSnapshot);
// We invoke the handler here, not in the picture constructor, because we want
// [result.approximateBytesUsed] to be available for the handler.
ui.Picture.onCreate?.call(result);
return result;
}

@override
Expand Down
4 changes: 1 addition & 3 deletions lib/web_ui/lib/src/engine/embedder.dart
Original file line number Diff line number Diff line change
Expand Up @@ -412,9 +412,7 @@ class FlutterViewEmbedder {
/// Called immediately after browser window language change.
void _languageDidChange(DomEvent event) {
EnginePlatformDispatcher.instance.updateLocales();
if (ui.window.onLocaleChanged != null) {
ui.window.onLocaleChanged!();
}
ui.window.onLocaleChanged?.call();
}

static const String orientationLockTypeAny = 'any';
Expand Down
5 changes: 4 additions & 1 deletion lib/web_ui/lib/src/engine/html_image_codec.dart
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,17 @@ class SingleFrameInfo implements ui.FrameInfo {
}

class HtmlImage implements ui.Image {
HtmlImage(this.imgElement, this.width, this.height);
HtmlImage(this.imgElement, this.width, this.height) {
ui.Image.onCreate?.call(this);
}

final DomHTMLImageElement imgElement;
bool _requiresClone = false;

bool _disposed = false;
@override
void dispose() {
ui.Image.onDispose?.call(this);
// Do nothing. The codec that owns this image should take care of
// releasing the object url.
if (assertionsEnabled) {
Expand Down
7 changes: 6 additions & 1 deletion lib/web_ui/lib/src/engine/picture.dart
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@ class EnginePictureRecorder implements ui.PictureRecorder {
}
_isRecording = false;
_canvas!.endRecording();
return EnginePicture(_canvas, cullRect);
final EnginePicture result = EnginePicture(_canvas, cullRect);
// We invoke the handler here, not in the Picture constructor, because we want
// [result.approximateBytesUsed] to be available for the handler.
ui.Picture.onCreate?.call(result);
return result;
}
}

Expand Down Expand Up @@ -97,6 +101,7 @@ class EnginePicture implements ui.Picture {

@override
void dispose() {
ui.Picture.onDispose?.call(this);
_disposed = true;
}

Expand Down
44 changes: 44 additions & 0 deletions lib/web_ui/test/canvaskit/image_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,50 @@ void testMain() {
image.dispose();
// TODO(hterkelsen): https://github.com/flutter/flutter/issues/109265
}, skip: isFirefox);

test('Image constructor invokes onCreate once', () async {
int onCreateInvokedCount = 0;
ui.Image? createdImage;
ui.Image.onCreate = (ui.Image image) {
onCreateInvokedCount++;
createdImage = image;
};

final ui.Image image1 = await _createImage();

expect(onCreateInvokedCount, 1);
expect(createdImage, image1);

final ui.Image image2 = await _createImage();

expect(onCreateInvokedCount, 2);
expect(createdImage, image2);

ui.Image.onCreate = null;
// TODO(hterkelsen): https://github.com/flutter/flutter/issues/109265
}, skip: isFirefox);

test('dispose() invokes onDispose once', () async {
int onDisposeInvokedCount = 0;
ui.Image? disposedImage;
ui.Image.onDispose = (ui.Image image) {
onDisposeInvokedCount++;
disposedImage = image;
};

final ui.Image image1 = await _createImage()..dispose();

expect(onDisposeInvokedCount, 1);
expect(disposedImage, image1);

final ui.Image image2 = await _createImage()..dispose();

expect(onDisposeInvokedCount, 2);
expect(disposedImage, image2);

ui.Image.onDispose = null;
// TODO(hterkelsen): https://github.com/flutter/flutter/issues/109265
}, skip: isFirefox);
}

Future<ui.Image> _createImage() => _createPicture().toImage(10, 10);
Expand Down
46 changes: 46 additions & 0 deletions lib/web_ui/test/engine/image/image_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,52 @@ Future<void> testMain() async {
// TODO(polina-c): unskip the test when bug is fixed:
// https://github.com/flutter/engine/pull/35791
}, skip: true);

test('Image constructor invokes onCreate once', () async {
int onCreateInvokedCount = 0;
ui.Image? createdImage;
ui.Image.onCreate = (ui.Image image) {
onCreateInvokedCount++;
createdImage = image;
};

final ui.Image image1 = await _createImage();

expect(onCreateInvokedCount, 1);
expect(createdImage, image1);

final ui.Image image2 = await _createImage();

expect(onCreateInvokedCount, 2);
expect(createdImage, image2);

ui.Image.onCreate = null;
// TODO(polina-c): unskip the test when bug is fixed:
// https://github.com/flutter/engine/pull/35791
Copy link
Contributor

Choose a reason for hiding this comment

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

#35791 is now merged

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was wrong URL. Replaced with right one. The issue is not fixed yet.

}, skip: true);

test('dispose() invokes onDispose once', () async {
int onDisposeInvokedCount = 0;
ui.Image? disposedImage;
ui.Image.onDispose = (ui.Image image) {
onDisposeInvokedCount++;
disposedImage = image;
};

final ui.Image image1 = await _createImage()..dispose();

expect(onDisposeInvokedCount, 1);
expect(disposedImage, image1);

final ui.Image image2 = await _createImage()..dispose();

expect(onDisposeInvokedCount, 2);
expect(disposedImage, image2);

ui.Image.onDispose = null;
// TODO(polina-c): unskip the test when bug is fixed:
// https://github.com/flutter/engine/pull/35791
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

}, skip: true);
}

Future<ui.Image> _createImage() => _createPicture().toImage(10, 10);
Expand Down
Loading