From 622422e97e25a7cddf1189bdcedf39fdde4e2fe6 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Mon, 3 Feb 2025 09:17:12 +0100 Subject: [PATCH 1/5] refactor: remove ScreenshotStabilizer --- .../screenshot_event_processor.dart | 36 +---- .../native/cocoa/cocoa_replay_recorder.dart | 28 ++-- .../lib/src/replay/scheduled_recorder.dart | 20 ++- flutter/lib/src/screenshot/stabilizer.dart | 147 ------------------ .../screenshot_event_processor_test.dart | 21 --- 5 files changed, 24 insertions(+), 228 deletions(-) delete mode 100644 flutter/lib/src/screenshot/stabilizer.dart diff --git a/flutter/lib/src/event_processor/screenshot_event_processor.dart b/flutter/lib/src/event_processor/screenshot_event_processor.dart index 4d2446c2a4..23ee438948 100644 --- a/flutter/lib/src/event_processor/screenshot_event_processor.dart +++ b/flutter/lib/src/event_processor/screenshot_event_processor.dart @@ -9,7 +9,6 @@ import '../screenshot/recorder.dart'; import '../screenshot/recorder_config.dart'; import 'package:flutter/widgets.dart' as widget; -import '../screenshot/stabilizer.dart'; import '../utils/debouncer.dart'; class ScreenshotEventProcessor implements EventProcessor { @@ -126,37 +125,6 @@ class ScreenshotEventProcessor implements EventProcessor { } @internal - Future createScreenshot() async { - if (_options.experimental.privacyForScreenshots == null) { - return _recorder.capture((screenshot) => - screenshot.pngData.then((v) => v.buffer.asUint8List())); - } else { - // If masking is enabled, we need to use [ScreenshotStabilizer]. - final completer = Completer(); - final stabilizer = ScreenshotStabilizer( - _recorder, _options, - (screenshot) async { - final pngData = await screenshot.pngData; - completer.complete(pngData.buffer.asUint8List()); - }, - // This limits the amount of time to take a stable masked screenshot. - maxTries: 5, - // We need to force the frame the frame or this could hang indefinitely. - frameSchedulingMode: FrameSchedulingMode.forced, - ); - try { - unawaited( - stabilizer.capture(Duration.zero).onError(completer.completeError)); - // DO NOT return completer.future directly - we need to dispose first. - return await completer.future.timeout(const Duration(seconds: 1), - onTimeout: () { - _options.logger( - SentryLevel.warning, 'Timed out taking a stable screenshot.'); - return null; - }); - } finally { - stabilizer.dispose(); - } - } - } + Future createScreenshot() => _recorder.capture( + (screenshot) => screenshot.pngData.then((v) => v.buffer.asUint8List())); } diff --git a/flutter/lib/src/native/cocoa/cocoa_replay_recorder.dart b/flutter/lib/src/native/cocoa/cocoa_replay_recorder.dart index a39a3cc6de..6393b2656e 100644 --- a/flutter/lib/src/native/cocoa/cocoa_replay_recorder.dart +++ b/flutter/lib/src/native/cocoa/cocoa_replay_recorder.dart @@ -6,24 +6,24 @@ import '../../../sentry_flutter.dart'; import '../../replay/replay_recorder.dart'; import '../../screenshot/recorder.dart'; import '../../screenshot/recorder_config.dart'; -import '../../screenshot/stabilizer.dart'; import '../native_memory.dart'; @internal class CocoaReplayRecorder { final SentryFlutterOptions _options; final ScreenshotRecorder _recorder; - late final ScreenshotStabilizer _stabilizer; - var _completer = Completer?>(); CocoaReplayRecorder(this._options) : _recorder = ReplayScreenshotRecorder( - ScreenshotRecorderConfig( - pixelRatio: - _options.experimental.replay.quality.resolutionScalingFactor, - ), - _options) { - _stabilizer = ScreenshotStabilizer(_recorder, _options, (screenshot) async { + ScreenshotRecorderConfig( + pixelRatio: + _options.experimental.replay.quality.resolutionScalingFactor, + ), + _options, + ); + + Future?> captureScreenshot() async { + return _recorder.capture((screenshot) async { final data = await screenshot.rawRgbaData; _options.logger( SentryLevel.debug, @@ -35,15 +35,7 @@ class CocoaReplayRecorder { final json = data.toNativeMemory().toJson(); json['width'] = screenshot.width; json['height'] = screenshot.height; - _completer.complete(json); - }); - } - - Future?> captureScreenshot() async { - _completer = Completer(); - _stabilizer.ensureFrameAndAddCallback((msSinceEpoch) { - _stabilizer.capture(msSinceEpoch).onError(_completer.completeError); + return json; }); - return _completer.future; } } diff --git a/flutter/lib/src/replay/scheduled_recorder.dart b/flutter/lib/src/replay/scheduled_recorder.dart index 06b75a2a60..4a79dd832d 100644 --- a/flutter/lib/src/replay/scheduled_recorder.dart +++ b/flutter/lib/src/replay/scheduled_recorder.dart @@ -1,9 +1,9 @@ import 'dart:async'; +import 'package:flutter/scheduler.dart'; import 'package:meta/meta.dart'; import '../../sentry_flutter.dart'; -import '../screenshot/stabilizer.dart'; import '../screenshot/screenshot.dart'; import 'replay_recorder.dart'; import 'scheduled_recorder_config.dart'; @@ -19,7 +19,6 @@ class ScheduledScreenshotRecorder extends ReplayScreenshotRecorder { late final ScheduledScreenshotRecorderCallback _callback; var _status = _Status.running; late final Duration _frameDuration; - late final ScreenshotStabilizer _stabilizer; // late final _idleFrameFiller = _IdleFrameFiller(_frameDuration, _onScreenshot); @override @@ -35,15 +34,23 @@ class ScheduledScreenshotRecorder extends ReplayScreenshotRecorder { _frameDuration = Duration(milliseconds: 1000 ~/ config.frameRate); assert(_frameDuration.inMicroseconds > 0); - _stabilizer = ScreenshotStabilizer(this, options, _onImageCaptured); - _scheduler = Scheduler(_frameDuration, _stabilizer.capture, - _stabilizer.ensureFrameAndAddCallback); + _scheduler = Scheduler( + _frameDuration, + (_) => capture(_onImageCaptured), + _addPostFrameCallback, + ); if (callback != null) { _callback = callback; } } + void _addPostFrameCallback(FrameCallback callback) { + options.bindingUtils.instance! + ..ensureVisualUpdate() + ..addPostFrameCallback(callback); + } + set callback(ScheduledScreenshotRecorderCallback callback) { _callback = callback; } @@ -63,12 +70,10 @@ class ScheduledScreenshotRecorder extends ReplayScreenshotRecorder { } Future _stopScheduler() { - _stabilizer.stopped = true; return _scheduler.stop(); } void _startScheduler() { - _stabilizer.stopped = false; _scheduler.start(); // We need to schedule a frame because if this happens in-between user @@ -82,7 +87,6 @@ class ScheduledScreenshotRecorder extends ReplayScreenshotRecorder { options.logger(SentryLevel.debug, "$logName: stopping capture."); _status = _Status.stopped; await _stopScheduler(); - _stabilizer.dispose(); // await Future.wait([_stopScheduler(), _idleFrameFiller.stop()]); options.logger(SentryLevel.debug, "$logName: capture stopped."); } diff --git a/flutter/lib/src/screenshot/stabilizer.dart b/flutter/lib/src/screenshot/stabilizer.dart deleted file mode 100644 index 72ee1df15a..0000000000 --- a/flutter/lib/src/screenshot/stabilizer.dart +++ /dev/null @@ -1,147 +0,0 @@ -import 'dart:async'; -import 'dart:math'; - -import 'package:flutter/scheduler.dart'; -import 'package:meta/meta.dart'; - -import '../../sentry_flutter.dart'; -import 'recorder.dart'; -import 'screenshot.dart'; - -/// We're facing an issue: the tree walked with visitChildElements() is out of -/// sync to what is currently rendered by RenderRepaintBoundary.toImage(), -/// even though there's no async gap between these two. This causes masks to -/// be off during repaints, e.g. when scrolling a view or when text is rendered -/// in different places between two screens. This is most easily reproducible -/// when there's no animation between the two screens. -/// For example, Spotube's Search vs Library (2nd and 3rd bottom bar buttons). -/// -/// To get around this issue, we're taking two subsequent screenshots -/// (after two frames) and only actually capture a screenshot if the -/// two are exactly the same. -@internal -class ScreenshotStabilizer { - final SentryFlutterOptions _options; - final ScreenshotRecorder _recorder; - final Future Function(Screenshot screenshot) _callback; - final int? maxTries; - final FrameSchedulingMode frameSchedulingMode; - Screenshot? _previousScreenshot; - int _tries = 0; - bool stopped = false; - - ScreenshotStabilizer(this._recorder, this._options, this._callback, - {this.maxTries, this.frameSchedulingMode = FrameSchedulingMode.normal}) { - assert(maxTries == null || maxTries! > 1, - "Cannot use ScreenshotStabilizer if we cannot retry at least once."); - } - - void dispose() { - _previousScreenshot?.dispose(); - _previousScreenshot = null; - } - - void ensureFrameAndAddCallback(FrameCallback callback) { - final binding = _options.bindingUtils.instance!; - switch (frameSchedulingMode) { - case FrameSchedulingMode.normal: - binding.scheduleFrame(); - break; - case FrameSchedulingMode.forced: - binding.scheduleForcedFrame(); - break; - } - binding.addPostFrameCallback(callback); - } - - Future capture(Duration _) { - _tries++; - return _recorder.capture(_onImageCaptured); - } - - Future _onImageCaptured(Screenshot screenshot) async { - if (stopped) { - _tries = 0; - return; - } - - var prevScreenshot = _previousScreenshot; - try { - _previousScreenshot = screenshot.clone(); - if (prevScreenshot != null && - await prevScreenshot.hasSameImageAs(screenshot)) { - // Sucessfully captured a stable screenshot (repeated at least twice). - _tries = 0; - - // If it's from the same (retry) flow, use the first screenshot - // timestamp. Otherwise this was called from a scheduler (in a new flow) - // so use the new timestamp. - await _callback((prevScreenshot.flow.id == screenshot.flow.id) - ? prevScreenshot - : screenshot); - - // Do not just return the Future resulting from callback(). - // We need to await here so that the dispose runs ASAP. - return; - } - } finally { - // [prevScreenshot] and [screenshot] are unnecessary after this line. - // Note: we need to dispose (free the memory) before recursion. - // Also, we need to reset the variable to null so that the whole object - // can be garbage collected. - prevScreenshot?.dispose(); - prevScreenshot = null; - // Note: while the caller will also do `screenshot.dispose()`, - // it would be a problem in a long recursion because we only return - // from this function when the screenshot is ultimately stable. - // At that point, the caller would have accumulated a lot of screenshots - // on stack. This would lead to OOM. - screenshot.dispose(); - } - - if (maxTries != null && _tries >= maxTries!) { - throw Exception('Failed to capture a stable screenshot. ' - 'Giving up after $_tries tries.'); - } else { - // Add a delay to give the UI a chance to stabilize. - // Only do this on every other frame so that there's a greater chance - // of two subsequent frames being the same. - final sleepMs = _tries % 2 == 1 ? min(100, 10 * (_tries - 1)) : 0; - - if (_tries > 1) { - _options.logger( - SentryLevel.debug, - '${_recorder.logName}: ' - 'Retrying screenshot capture due to UI changes. ' - 'Delay before next capture: $sleepMs ms.'); - } - - if (sleepMs > 0) { - await Future.delayed(Duration(milliseconds: sleepMs)); - } - - final completer = Completer(); - ensureFrameAndAddCallback((Duration sinceSchedulerEpoch) async { - _tries++; - try { - await _recorder.capture(_onImageCaptured, screenshot.flow); - completer.complete(); - } catch (e, stackTrace) { - completer.completeError(e, stackTrace); - } - }); - return completer.future; - } - } -} - -@internal -enum FrameSchedulingMode { - /// The frame is scheduled only if the UI is visible. - /// If you await for the callback, it may take indefinitely long if the - /// app is in the background. - normal, - - /// A forced frame is scheduled immediately regardless of the UI visibility. - forced, -} diff --git a/flutter/test/event_processor/screenshot_event_processor_test.dart b/flutter/test/event_processor/screenshot_event_processor_test.dart index dae0bf468d..095e398892 100644 --- a/flutter/test/event_processor/screenshot_event_processor_test.dart +++ b/flutter/test/event_processor/screenshot_event_processor_test.dart @@ -68,27 +68,6 @@ void main() { await _addScreenshotAttachment(tester, null, added: true, isWeb: false); }); - testWidgets('does not block if the screenshot fails to stabilize', - (tester) async { - fixture.options.automatedTestMode = false; - fixture.options.experimental.privacy.maskAllText = true; - // Run with real async https://stackoverflow.com/a/54021863 - await tester.runAsync(() async { - final sut = fixture.getSut(null, false); - - await tester.pumpWidget(SentryScreenshotWidget( - child: Text('Catching Pokémon is a snap!', - textDirection: TextDirection.ltr))); - - final throwable = Exception(); - event = SentryEvent(throwable: throwable); - hint = Hint(); - await sut.apply(event, hint); - - expect(hint.screenshot, isNull); - }); - }); - testWidgets('adds screenshot attachment with canvasKit renderer', (tester) async { await _addScreenshotAttachment(tester, FlutterRenderer.canvasKit, From f4af4717b9ecb9a93aaa5c7aef3cf9bd9449f508 Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Mon, 3 Feb 2025 09:18:11 +0100 Subject: [PATCH 2/5] test: fixup native test --- flutter/test/replay/replay_native_test.dart | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/flutter/test/replay/replay_native_test.dart b/flutter/test/replay/replay_native_test.dart index 7cfee2e574..d00ff0d0bd 100644 --- a/flutter/test/replay/replay_native_test.dart +++ b/flutter/test/replay/replay_native_test.dart @@ -36,6 +36,10 @@ void main() { late Map replayConfig; setUp(() { + hub = MockHub(); + fs = MemoryFileSystem.test(); + native = NativeChannelFixture(); + if (mockPlatform.isIOS) { replayConfig = { 'replayId': '123', @@ -48,14 +52,11 @@ void main() { 'height': 600, 'frameRate': 1000, }; + fs.directory(replayConfig['directory']).createSync(recursive: true); + when(native.handler('addReplayScreenshot', any)) + .thenAnswer((_) => Future.value()); } - hub = MockHub(); - - fs = MemoryFileSystem.test(); - - native = NativeChannelFixture(); - options = defaultTestOptions(MockPlatformChecker(mockPlatform: mockPlatform)) ..fileSystem = fs @@ -126,8 +127,7 @@ void main() { await pumpTestElement(tester); if (mockPlatform.isAndroid) { - final replayDir = fs.directory(replayConfig['directory']) - ..createSync(recursive: true); + final replayDir = fs.directory(replayConfig['directory']); var callbackFinished = Completer(); From 984e066aafce8522ca96ad79ac2475d08383914e Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Mon, 3 Feb 2025 09:22:58 +0100 Subject: [PATCH 3/5] workaround flutter image issue --- flutter/lib/src/screenshot/recorder.dart | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/flutter/lib/src/screenshot/recorder.dart b/flutter/lib/src/screenshot/recorder.dart index 497eb4ebfd..6c6b6dc680 100644 --- a/flutter/lib/src/screenshot/recorder.dart +++ b/flutter/lib/src/screenshot/recorder.dart @@ -190,6 +190,18 @@ class _Capture { final recorder = PictureRecorder(); final canvas = Canvas(recorder); final image = await futureImage; + + // Note: there's a weird bug when we write image to canvas directly. + // If the UI is updating quickly in some apps, the image could get + // out-of-sync with the UI and/or it can get completely mangled. + // This can be reproduced, for example, by switching between Spotube's + // Search vs Library (2nd and 3rd bottom bar buttons). + // Weirdly, dumping the image data seems to prevent this issue... + { + // we do so in a block so it can be GC'ed early. + final _ = image.toByteData(); + } + try { canvas.drawImage(image, Offset.zero, Paint()); } finally { From 9b96e825a531eefde9190527be383f05b135c45c Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Tue, 4 Feb 2025 15:45:46 +0100 Subject: [PATCH 4/5] fixup --- flutter/lib/src/screenshot/recorder.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flutter/lib/src/screenshot/recorder.dart b/flutter/lib/src/screenshot/recorder.dart index 6c6b6dc680..93d590a464 100644 --- a/flutter/lib/src/screenshot/recorder.dart +++ b/flutter/lib/src/screenshot/recorder.dart @@ -199,7 +199,7 @@ class _Capture { // Weirdly, dumping the image data seems to prevent this issue... { // we do so in a block so it can be GC'ed early. - final _ = image.toByteData(); + final _ = await image.toByteData(); } try { From d87317f4244754c99952abde239c7c2649209b6b Mon Sep 17 00:00:00 2001 From: Ivan Dlugos Date: Wed, 5 Feb 2025 09:28:29 +0100 Subject: [PATCH 5/5] try to fix ci --- flutter/test/replay/replay_native_test.dart | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/flutter/test/replay/replay_native_test.dart b/flutter/test/replay/replay_native_test.dart index d00ff0d0bd..90bfcbc2c1 100644 --- a/flutter/test/replay/replay_native_test.dart +++ b/flutter/test/replay/replay_native_test.dart @@ -100,6 +100,10 @@ void main() { expect(scope.replayId, isNull); await closure(scope); expect(scope.replayId.toString(), replayConfig['replayId']); + + if (mockPlatform.isAndroid) { + await native.invokeFromNative('ReplayRecorder.stop'); + } }); });