feat: Refactor screenshot capture for improved performance and reliability #147

thisames · 2025-01-08T12:23:12Z

This commit refactors the captureScreenshot method in ScreenshotCapturer
to improve performance and reliability. The changes include:

Synchronous Setup: The method now performs synchronous operations first
(getting context, render object, calculating pixel ratio, and getting the
initial image) before executing the main process asynchronously. This
ensures that the initial setup is done on the main UI thread, as required.
Asynchronous Processing: The main image processing and masking logic
are now executed asynchronously using a Future and Completer.

💡 Motivation and Context

#140

💚 How did you test it?

📝 Checklist

I reviewed the submitted code.
I added tests to verify the changes.
I updated the docs if needed.
No breaking change or entry added to the changelog.

…ility This commit refactors the `captureScreenshot` method in `ScreenshotCapturer` to improve performance and reliability. The changes include: - **Synchronous Setup:** The method now performs synchronous operations first (getting context, render object, calculating pixel ratio, and getting the initial image) before executing the main process asynchronously. This ensures that the initial setup is done on the main UI thread, as required. - **Asynchronous Processing:** The main image processing and masking logic are now executed asynchronously using a `Future` and `Completer`, preventing UI jank. - **Snapshot Management:** The `_views` Expando is replaced with a `SnapshotManager` to manage the status of the view, improving code organization and maintainability.

thisames · 2025-01-08T12:26:36Z

lib/src/replay/screenshot/screenshot_capturer.dart

@@ -30,8 +34,8 @@ class ViewTreeSnapshotStatus {
 class ScreenshotCapturer {


what do you think about changing the class name? ScreenshotCapturer looks weird to me

find to rename it, its only used internally

marandaneto · 2025-01-10T07:34:43Z

lib/src/posthog_widget_widget.dart

-      return;
-    }
-
+    // Ensure no asynchronous calls occur before this function,


calling isSessionReplayActive is important even if its inside captureScreenshot because we don't want to pay the performance price of taking the screenshot if session replay is inactive

yes, I put this in

posthog-flutter/lib/src/replay/screenshot/screenshot_capturer.dart

Lines 111 to 117 in 25a3690

Future(() async {

final isSessionReplayActive =

await _nativeCommunicator.isSessionReplayActive();

if (!isSessionReplayActive) {

return;

}

what do you think?

marandaneto · 2025-01-10T07:35:26Z

lib/src/replay/element_parsers/element_data.dart

-    if (!isRoot) {
-      rects.add(rect);
-    }
+  List<ElementData> extractRects([bool isRoot = true]) {


Suggested change

List<ElementData> extractRects([bool isRoot = true]) {

List<ElementData> extractRects({bool isRoot = true}) {

I think optionals are better than positionals

marandaneto · 2025-01-10T07:36:05Z

lib/src/replay/mask/posthog_mask_controller.dart

+  ///   - `List<ElementData>`: A list of [ElementData] objects representing the
+  ///     renderable elements.
+  ///
+  List<ElementData>? getCurrentWidgetsElements() {


Suggested change

List<ElementData>? getCurrentWidgetsElements() {

List<ElementData>? _getCurrentWidgetsElements() {

can be private I assume

marandaneto · 2025-01-10T07:36:34Z

lib/src/replay/mask/posthog_mask_controller.dart

+      final ElementData? widgetElementsTree =
+          _widgetScraper.parseRenderTree(context);


Suggested change

final ElementData? widgetElementsTree =

_widgetScraper.parseRenderTree(context);

final widgetElementsTree =

_widgetScraper.parseRenderTree(context);

the type is inferred already

marandaneto · 2025-01-10T07:37:18Z

lib/src/replay/screenshot/screenshot_capturer.dart

+    try {
+      final ByteData? byteData =
+          await img.toByteData(format: ui.ImageByteFormat.png);
+      if (byteData == null) {


why removing the byteData.lengthInBytes == 0 check?

sorry, I had copy the function of old version mine

marandaneto · 2025-01-10T07:37:46Z

lib/src/replay/screenshot/screenshot_capturer.dart

  }

-  Future<ImageInfo?> captureScreenshot() async {
+  Future<ImageInfo?> captureScreenshot() {


the function has to be async if it returns a Future, right?

Not necessarily, a function does not need to be explicitly marked as async to return a Future.

In our case, we need to work synchronously with the function. Then we execute the main process asynchronously as a "task". Managing this with the Completer. When this is ready, we retrieve the Completer and return the Future.

This way, the frame image and the masks always move in the main thread.

true that, nice

marandaneto · 2025-01-10T08:38:50Z

lib/src/replay/screenshot/screenshot_capturer.dart

-      statusView.imageBytes = pngBytes;
+        final recorder = ui.PictureRecorder();
+        final canvas = Canvas(recorder);
+        final image = await syncImage;


Suggested change

final image = await syncImage;

final image = await renderObject.toImage(pixelRatio: pixelRatio);

would that work?

would that work?

it doesn't work, because we need to first get the renderObject reference at that moment.

we do this with

final renderObject = context.findRenderObject() as RenderRepaintBoundary?;

then from the reference in the main thread of the renderObject we get the image reference

final syncImage = renderObject.toImage(pixelRatio: pixelRatio);

with

final image = await renderObject.toImage(pixelRatio: pixelRatio);

the renderObject reference may have changed in the main thread, so it would be out of sync

can we do the whole process inside of the Future then? so its all done in sequence

can we do the whole process inside of the Future then? so its all done in sequence

I don't know if I understood what you meant, but if the entire process is done inside a single synchronous block, any computationally expensive operations (like rendering an image, processing masks, or manipulating large byte arrays) will block the main thread, this could lead to dropped frames or the image frames and masks got mixed up

But if you have a solution to the whole process in the Future block, I would like to see it, maybe I missed something or something is beyond my ability haha

everything right now is being done on the main thread, Dart is single-threaded, unless you use background workers which is not available in all Dart/Flutter versions.
Maybe you are misunderstanding the concept of async/await with multithreading?

Isolates can also do background work

marandaneto · 2025-01-10T08:39:15Z

lib/src/replay/screenshot/screenshot_capturer.dart

    try {
      final srcWidth = renderObject.size.width;
      final srcHeight = renderObject.size.height;
      final pixelRatio =
          _getPixelRatio(srcWidth: srcWidth, srcHeight: srcHeight);

-      final ui.Image image = await renderObject.toImage(pixelRatio: pixelRatio);
+      final syncImage = renderObject.toImage(pixelRatio: pixelRatio);


well this is misleading because its not sync, since returns a future and you don't await.

marandaneto · 2025-01-10T08:40:16Z

nice, almost there.

thisames commented Jan 8, 2025

View reviewed changes

marandaneto reviewed Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Refactor screenshot capture for improved performance and reliability #147

feat: Refactor screenshot capture for improved performance and reliability #147

thisames commented Jan 8, 2025

thisames Jan 8, 2025

marandaneto Jan 10, 2025

marandaneto Jan 10, 2025

thisames Jan 10, 2025

marandaneto Jan 10, 2025

marandaneto Jan 10, 2025

marandaneto Jan 10, 2025

marandaneto Jan 10, 2025

thisames Jan 10, 2025

marandaneto Jan 10, 2025

thisames Jan 10, 2025

marandaneto Jan 10, 2025

marandaneto Jan 10, 2025

thisames Jan 10, 2025

thisames Jan 10, 2025

marandaneto Jan 10, 2025

thisames Jan 10, 2025

thisames Jan 10, 2025

marandaneto Jan 10, 2025

marandaneto Jan 10, 2025

marandaneto Jan 10, 2025

marandaneto commented Jan 10, 2025

		@@ -30,8 +34,8 @@ class ViewTreeSnapshotStatus {
		class ScreenshotCapturer {

	Future(() async {
	final isSessionReplayActive =
	await _nativeCommunicator.isSessionReplayActive();
	if (!isSessionReplayActive) {
	return;
	}

	List<ElementData> extractRects([bool isRoot = true]) {
	List<ElementData> extractRects({bool isRoot = true}) {

	List<ElementData>? getCurrentWidgetsElements() {
	List<ElementData>? _getCurrentWidgetsElements() {

		final ElementData? widgetElementsTree =
		_widgetScraper.parseRenderTree(context);

	final image = await syncImage;
	final image = await renderObject.toImage(pixelRatio: pixelRatio);

feat: Refactor screenshot capture for improved performance and reliability #147

Are you sure you want to change the base?

feat: Refactor screenshot capture for improved performance and reliability #147

Conversation

thisames commented Jan 8, 2025

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marandaneto commented Jan 10, 2025