Skip to content

Commit

Permalink
Dispose AnimationSheetRecorder to avoid leaks (#133365)
Browse files Browse the repository at this point in the history 
This PR adds `AnimationSheetRecorder.dispose`, which disposes all the images generated by the recorder, eliminating leaks.

Fixes flutter/flutter#133071.
  • Loading branch information
@dkwingsmt
dkwingsmt authored Sep 5, 2023
1 parent a7dbec3 commit e66ec8e
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 48 deletions.
22 changes: 5 additions & 17 deletions packages/flutter/test/animation/animation_sheet_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
@Tags(<String>['reduced-test-set'])
library;

import 'dart:ui' as ui;

import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';
Expand All @@ -23,6 +21,7 @@ void main() {
testWidgetsWithLeakTracking('recording disposes images',
(WidgetTester tester) async {
final AnimationSheetBuilder builder = AnimationSheetBuilder(frameSize: _DecuplePixels.size);
addTearDown(builder.dispose);

await tester.pumpFrames(
builder.record(
Expand All @@ -33,13 +32,12 @@ void main() {
);
},
skip: isBrowser, // [intended] https://github.com/flutter/flutter/issues/56001
// TODO(polina-c): remove after fixing https://github.com/flutter/flutter/issues/133071
leakTrackingTestConfig: const LeakTrackingTestConfig(allowAllNotDisposed: true),
);

testWidgetsWithLeakTracking('correctly records frames using collate',
(WidgetTester tester) async {
final AnimationSheetBuilder builder = AnimationSheetBuilder(frameSize: _DecuplePixels.size);
addTearDown(builder.dispose);

await tester.pumpFrames(
builder.record(
Expand All @@ -66,25 +64,20 @@ void main() {
const Duration(milliseconds: 100),
);

final ui.Image image = await builder.collate(5);

await expectLater(
image,
builder.collate(5),
matchesGoldenFile('test.animation_sheet_builder.collate.png'),
);

image.dispose();
},
skip: isBrowser, // [intended] https://github.com/flutter/flutter/issues/56001
// TODO(polina-c): remove after fixing https://github.com/flutter/flutter/issues/133071
leakTrackingTestConfig: const LeakTrackingTestConfig(allowAllNotDisposed: true),
); // https://github.com/flutter/flutter/issues/56001

testWidgetsWithLeakTracking('use allLayers to record out-of-subtree contents', (WidgetTester tester) async {
final AnimationSheetBuilder builder = AnimationSheetBuilder(
frameSize: const Size(8, 2),
allLayers: true,
);
addTearDown(builder.dispose);

// The `record` (sized 8, 2) is placed on top of `_DecuplePixels`
// (sized 12, 3), aligned at its top left.
Expand All @@ -105,17 +98,12 @@ void main() {
const Duration(milliseconds: 100),
);

final ui.Image image = await builder.collate(5);

await expectLater(
image,
builder.collate(5),
matchesGoldenFile('test.animation_sheet_builder.out_of_tree.png'),
);
image.dispose();
},
skip: isBrowser, // [intended] https://github.com/flutter/flutter/issues/56001
// TODO(polina-c): remove after fixing https://github.com/flutter/flutter/issues/133071
leakTrackingTestConfig: const LeakTrackingTestConfig(allowAllNotDisposed: true),
);
}

Expand Down
9 changes: 3 additions & 6 deletions packages/flutter/test/animation/live_binding_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
@Tags(<String>['reduced-test-set'])
library;

import 'dart:ui' as ui;

import 'package:flutter/material.dart';
import 'package:flutter_test/flutter_test.dart';
import 'package:leak_tracker_flutter_testing/leak_tracker_flutter_testing.dart';
Expand All @@ -23,6 +21,7 @@ void main() {

testWidgetsWithLeakTracking('Should show event indicator for pointer events', (WidgetTester tester) async {
final AnimationSheetBuilder animationSheet = AnimationSheetBuilder(frameSize: const Size(200, 200), allLayers: true);
addTearDown(animationSheet.dispose);
final List<Offset> taps = <Offset>[];
Widget target({bool recording = true}) => Container(
padding: const EdgeInsets.fromLTRB(20, 10, 25, 20),
Expand Down Expand Up @@ -81,6 +80,7 @@ void main() {

testWidgetsWithLeakTracking('Should show event indicator for pointer events with setSurfaceSize', (WidgetTester tester) async {
final AnimationSheetBuilder animationSheet = AnimationSheetBuilder(frameSize: const Size(200, 200), allLayers: true);
addTearDown(animationSheet.dispose);
final List<Offset> taps = <Offset>[];
Widget target({bool recording = true}) => Container(
padding: const EdgeInsets.fromLTRB(20, 10, 25, 20),
Expand Down Expand Up @@ -131,13 +131,10 @@ void main() {
await tester.pumpFrames(target(), const Duration(milliseconds: 50));
expect(taps, isEmpty);

final ui.Image image = await animationSheet.collate(6);

await expectLater(
image,
animationSheet.collate(6),
matchesGoldenFile('LiveBinding.press.animation.2.png'),
);
image.dispose();
},
skip: isBrowser, // [intended] https://github.com/flutter/flutter/issues/56001
// TODO(polina-c): remove after fixing https://github.com/flutter/flutter/issues/133071
Expand Down
12 changes: 8 additions & 4 deletions packages/flutter/test/material/progress_indicator_test.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,7 @@ void main() {

testWidgets('Material2 - RefreshProgressIndicator uses expected animation', (WidgetTester tester) async {
final AnimationSheetBuilder animationSheet = AnimationSheetBuilder(frameSize: const Size(50, 50));
addTearDown(animationSheet.dispose);

await tester.pumpFrames(animationSheet.record(
Theme(
Expand All @@ -729,13 +730,14 @@ void main() {
), const Duration(seconds: 3));

await expectLater(
await animationSheet.collate(20),
animationSheet.collate(20),
matchesGoldenFile('m2_material.refresh_progress_indicator.png'),
);
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001

testWidgets('Material3 - RefreshProgressIndicator uses expected animation', (WidgetTester tester) async {
final AnimationSheetBuilder animationSheet = AnimationSheetBuilder(frameSize: const Size(50, 50));
addTearDown(animationSheet.dispose);

await tester.pumpFrames(animationSheet.record(
Theme(
Expand All @@ -745,7 +747,7 @@ void main() {
), const Duration(seconds: 3));

await expectLater(
await animationSheet.collate(20),
animationSheet.collate(20),
matchesGoldenFile('m3_material.refresh_progress_indicator.png'),
);
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001
Expand Down Expand Up @@ -1017,6 +1019,7 @@ void main() {

testWidgets('Material2 - Indeterminate CircularProgressIndicator uses expected animation', (WidgetTester tester) async {
final AnimationSheetBuilder animationSheet = AnimationSheetBuilder(frameSize: const Size(40, 40));
addTearDown(animationSheet.dispose);

await tester.pumpFrames(animationSheet.record(
Theme(
Expand All @@ -1032,13 +1035,14 @@ void main() {
), const Duration(seconds: 2));

await expectLater(
await animationSheet.collate(20),
animationSheet.collate(20),
matchesGoldenFile('m2_material.circular_progress_indicator.indeterminate.png'),
);
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001

testWidgets('Material3 - Indeterminate CircularProgressIndicator uses expected animation', (WidgetTester tester) async {
final AnimationSheetBuilder animationSheet = AnimationSheetBuilder(frameSize: const Size(40, 40));
addTearDown(animationSheet.dispose);

await tester.pumpFrames(animationSheet.record(
Theme(
Expand All @@ -1054,7 +1058,7 @@ void main() {
), const Duration(seconds: 2));

await expectLater(
await animationSheet.collate(20),
animationSheet.collate(20),
matchesGoldenFile('m3_material.circular_progress_indicator.indeterminate.png'),
);
}, skip: isBrowser); // https://github.com/flutter/flutter/issues/56001
Expand Down
99 changes: 78 additions & 21 deletions packages/flutter_test/lib/src/animation_sheet.dart
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,36 @@ import 'package:flutter/rendering.dart';
import 'package:flutter/scheduler.dart';
import 'package:flutter/widgets.dart';

// A Future<ui.Image> that stores the resolved result.
class _AsyncImage {
_AsyncImage(Future<ui.Image> task) {
_task = task.then((ui.Image image) {
_result = image;
});
}

// Returns the resolved image.
Future<ui.Image> result() async {
if (_result != null) {
return _result!;
}
await _task;
assert(_result != null);
return _result!;
}

late final Future<void> _task;
ui.Image? _result;

// Wait for a list of `_AsyncImage` and returns the list of its resolved
// images.
static Future<List<ui.Image>> resolveList(List<_AsyncImage> targets) {
final Iterable<Future<ui.Image>> images = targets.map<Future<ui.Image>>(
(_AsyncImage target) => target.result());
return Future.wait<ui.Image>(images);
}
}

/// Records the frames of an animating widget, and later displays the frames as a
/// grid in an animation sheet.
///
Expand All @@ -20,6 +50,7 @@ import 'package:flutter/widgets.dart';
/// Using this class includes the following steps:
///
/// * Create an instance of this class.
/// * Register [dispose] to the test's tear down callbacks.
/// * Pump frames that render the target widget wrapped in [record]. Every frame
/// that has `recording` being true will be recorded.
/// * Acquire the output image with [collate] and compare against the golden
Expand All @@ -33,6 +64,7 @@ import 'package:flutter/widgets.dart';
/// testWidgets('Inkwell animation sheet', (WidgetTester tester) async {
/// // Create instance
/// final AnimationSheetBuilder animationSheet = AnimationSheetBuilder(frameSize: const Size(48, 24));
/// addTearDown(animationSheet.dispose);
///
/// final Widget target = Material(
/// child: Directionality(
Expand Down Expand Up @@ -90,6 +122,24 @@ class AnimationSheetBuilder {
this.allLayers = false,
}) : assert(!kIsWeb);

/// Dispose all recorded frames and result images.
///
/// This method must be called before the test case ends (usually as a tear
/// down callback) to properly deallocate the images.
///
/// After this method is called, there will be no frames to [collate].
Future<void> dispose() async {
final List<_AsyncImage> targets = <_AsyncImage>[
..._recordedFrames,
..._results,
];
_recordedFrames.clear();
_results.clear();
for (final ui.Image image in await _AsyncImage.resolveList(targets)) {
image.dispose();
}
}

/// The size of the child to be recorded.
///
/// This size is applied as a tight layout constraint for the child, and is
Expand All @@ -112,20 +162,7 @@ class AnimationSheetBuilder {
/// Defaults to false.
final bool allLayers;

final List<Future<ui.Image>> _recordedFrames = <Future<ui.Image>>[];
Future<List<ui.Image>> get _frames async {
final List<ui.Image> frames = await Future.wait<ui.Image>(_recordedFrames, eagerError: true);
assert(() {
for (final ui.Image frame in frames) {
assert(frame.width == frameSize.width && frame.height == frameSize.height,
'Unexpected size mismatch: frame has (${frame.width}, ${frame.height}) '
'while `frameSize` is $frameSize.'
);
}
return true;
}());
return frames;
}
final List<_AsyncImage> _recordedFrames = <_AsyncImage>[];

/// Returns a widget that renders a widget in a box that can be recorded.
///
Expand All @@ -152,22 +189,41 @@ class AnimationSheetBuilder {
key: key,
size: frameSize,
allLayers: allLayers,
handleRecorded: recording ? _recordedFrames.add : null,
handleRecorded: !recording ? null : (Future<ui.Image> futureImage) {
_recordedFrames.add(_AsyncImage(() async {
final ui.Image image = await futureImage;
assert(image.width == frameSize.width && image.height == frameSize.height,
'Unexpected size mismatch: frame has (${image.width}, ${image.height}) '
'while `frameSize` is $frameSize.'
);
return image;
}()));
},
child: child,
);
}

// The result images generated by `collate`.
//
// They're stored here to be disposed by [dispose].
final List<_AsyncImage> _results = <_AsyncImage>[];

/// Returns an result image by putting all frames together in a table.
///
/// This method returns a table of captured frames, `cellsPerRow` images
/// per row, from left to right, top to bottom.
/// This method returns an image that arranges the captured frames in a table,
/// which has `cellsPerRow` images per row with the order from left to right,
/// top to bottom.
///
/// The result image of this method is managed by [AnimationSheetBuilder],
/// and should not be disposed by the caller.
///
/// An example of using this method can be found at [AnimationSheetBuilder].
Future<ui.Image> collate(int cellsPerRow) async {
final List<ui.Image> frames = await _frames;
assert(frames.isNotEmpty,
assert(_recordedFrames.isNotEmpty,
'No frames are collected. Have you forgot to set `recording` to true?');
return _collateFrames(frames, frameSize, cellsPerRow);
final _AsyncImage result = _AsyncImage(_collateFrames(_recordedFrames, frameSize, cellsPerRow));
_results.add(result);
return result.result();
}
}

Expand Down Expand Up @@ -281,7 +337,8 @@ class _RenderPostFrameCallbacker extends RenderProxyBox {
}
}

Future<ui.Image> _collateFrames(List<ui.Image> frames, Size frameSize, int cellsPerRow) async {
Future<ui.Image> _collateFrames(List<_AsyncImage> futureFrames, Size frameSize, int cellsPerRow) async {
final List<ui.Image> frames = await _AsyncImage.resolveList(futureFrames);
final int rowNum = (frames.length / cellsPerRow).ceil();

final ui.PictureRecorder recorder = ui.PictureRecorder();
Expand Down

0 comments on commit e66ec8e

Please sign in to comment.