Address PR feedback

Author: markushi

CHANGELOG.md

@@ -5,9 +5,9 @@
 ### Features
 
 - Add experimental Sentry Android Distribution module for integrating with Sentry Build Distribution to check for and install updates ([#4804](https://github.com/getsentry/sentry-java/pull/4804))
-- Session Replay: Add new experimental Canvas Capture Strategy ([#4777](https://github.com/getsentry/sentry-java/pull/4777))
+- Session Replay: Add new _experimental_ Canvas Capture Strategy ([#4777](https://github.com/getsentry/sentry-java/pull/4777))
   - A new screenshot capture strategy that uses Android's Canvas API for more accurate text masking
-  - Any `.drawText()` calls are replaced with rectangles to ensure no text is not captured
+  - Any `.drawText()` or `.drawBitmap()` calls are replaced by rectangles, ensuring no text or images are present in the resulting output
   ```kotlin
   SentryAndroid.init(context) { options ->
     options.sessionReplay.screenshotStrategy = ScreenshotStrategyType.CANVAS

sentry-android-core/src/main/java/io/sentry/android/core/ManifestMetadataReader.java

@@ -6,6 +6,7 @@
 import io.sentry.ILogger;
 import io.sentry.InitPriority;
 import io.sentry.ProfileLifecycle;
+import io.sentry.ScreenshotStrategyType;
 import io.sentry.SentryFeedbackOptions;
 import io.sentry.SentryIntegrationPackageStorage;
 import io.sentry.SentryLevel;
@@ -111,6 +112,7 @@ final class ManifestMetadataReader {
   static final String REPLAYS_MASK_ALL_IMAGES = "io.sentry.session-replay.mask-all-images";
 
   static final String REPLAYS_DEBUG = "io.sentry.session-replay.debug";
+  static final String REPLAYS_SCREENSHOT_STRATEGY = "io.sentry.session-replay.screenshot-strategy";
 
   static final String FORCE_INIT = "io.sentry.force-init";
 
@@ -476,6 +478,16 @@ static void applyMetadata(
 
         options.getSessionReplay().setDebug(readBool(metadata, logger, REPLAYS_DEBUG, false));
 
+        final @Nullable String screenshotStrategyRaw =
+            readString(metadata, logger, REPLAYS_SCREENSHOT_STRATEGY, null);
+        if (screenshotStrategyRaw != null) {
+          if ("canvas".equals(screenshotStrategyRaw)) {
+            options.getSessionReplay().setScreenshotStrategy(ScreenshotStrategyType.CANVAS);
+          } else {
+            // always default to PIXEL_COPYq
+            options.getSessionReplay().setScreenshotStrategy(ScreenshotStrategyType.PIXEL_COPY);
+          }
+        }
         options.setIgnoredErrors(readList(metadata, logger, IGNORED_ERRORS));
 
         final @Nullable List<String> includes = readList(metadata, logger, IN_APP_INCLUDES);

sentry-android-core/src/test/java/io/sentry/android/core/ManifestMetadataReaderTest.kt

@@ -1835,4 +1835,51 @@ class ManifestMetadataReaderTest {
     // Assert
     assertFalse(fixture.options.feedbackOptions.isShowBranding)
   }
+
+  @Test
+  fun `applyMetadata reads screenshot strategy canvas to options`() {
+    // Arrange
+    val bundle = bundleOf(ManifestMetadataReader.REPLAYS_SCREENSHOT_STRATEGY to "canvas")
+    val context = fixture.getContext(metaData = bundle)
+
+    // Act
+    ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider)
+
+    // Assert
+    assertEquals(
+      io.sentry.ScreenshotStrategyType.CANVAS,
+      fixture.options.sessionReplay.screenshotStrategy,
+    )
+  }
+
+  @Test
+  fun `applyMetadata reads screenshot strategy and defaults to PIXEL_COPY for unknown value`() {
+    // Arrange
+    val bundle = bundleOf(ManifestMetadataReader.REPLAYS_SCREENSHOT_STRATEGY to "unknown")
+    val context = fixture.getContext(metaData = bundle)
+
+    // Act
+    ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider)
+
+    // Assert
+    assertEquals(
+      io.sentry.ScreenshotStrategyType.PIXEL_COPY,
+      fixture.options.sessionReplay.screenshotStrategy,
+    )
+  }
+
+  @Test
+  fun `applyMetadata reads screenshot strategy and keeps default if not found`() {
+    // Arrange
+    val context = fixture.getContext()
+
+    // Act
+    ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider)
+
+    // Assert
+    assertEquals(
+      io.sentry.ScreenshotStrategyType.PIXEL_COPY,
+      fixture.options.sessionReplay.screenshotStrategy,
+    )
+  }
 }

sentry-android-replay/src/main/java/io/sentry/android/replay/screenshot/CanvasStrategy.kt

@@ -29,12 +29,16 @@ import android.view.View
 import androidx.annotation.RequiresApi
 import io.sentry.SentryLevel
 import io.sentry.SentryOptions
+import io.sentry.SentryReplayOptions
+import io.sentry.SentryReplayOptions.IMAGE_VIEW_CLASS_NAME
+import io.sentry.SentryReplayOptions.TEXT_VIEW_CLASS_NAME
 import io.sentry.android.replay.ExecutorProvider
 import io.sentry.android.replay.ScreenshotRecorderCallback
 import io.sentry.android.replay.ScreenshotRecorderConfig
 import io.sentry.android.replay.util.submitSafely
 import io.sentry.util.AutoClosableReentrantLock
 import io.sentry.util.IntegrationUtils
+import java.io.Closeable
 import java.util.WeakHashMap
 import java.util.concurrent.atomic.AtomicBoolean
 import java.util.concurrent.atomic.AtomicReference
@@ -55,13 +59,14 @@ internal class CanvasStrategy(
   private val prescaledMatrix by
     lazy(NONE) { Matrix().apply { preScale(config.scaleFactorX, config.scaleFactorY) } }
   private val lastCaptureSuccessful = AtomicBoolean(false)
-  private val textIgnoringCanvas = TextIgnoringDelegateCanvas()
+  private val textIgnoringCanvas = TextIgnoringDelegateCanvas(options.sessionReplay)
 
   private val isClosed = AtomicBoolean(false)
 
   private val onImageAvailableListener: (holder: PictureReaderHolder) -> Unit = { holder ->
     if (isClosed.get()) {
       options.logger.log(SentryLevel.ERROR, "CanvasStrategy already closed, skipping image")
+      holder.close()
     } else {
       try {
         val image = holder.reader.acquireLatestImage()
@@ -86,12 +91,20 @@ internal class CanvasStrategy(
             }
           }
         } finally {
-          image.close()
+          try {
+            image.close()
+          } catch (_: Throwable) {
+            // ignored
+          }
         }
       } catch (e: Throwable) {
         options.logger.log(SentryLevel.ERROR, "CanvasStrategy: image processing failed", e)
       } finally {
-        freePictureRef.set(holder)
+        if (isClosed.get()) {
+          holder.close()
+        } else {
+          freePictureRef.set(holder)
+        }
       }
     }
   }
@@ -116,10 +129,7 @@ internal class CanvasStrategy(
       )
       return@Runnable
     }
-    val holder = unprocessedPictureRef.getAndSet(null)
-    if (holder == null) {
-      return@Runnable
-    }
+    val holder = unprocessedPictureRef.getAndSet(null) ?: return@Runnable
 
     try {
       if (!holder.setup.getAndSet(true)) {
@@ -135,14 +145,20 @@ internal class CanvasStrategy(
         surface.unlockCanvasAndPost(canvas)
       }
     } catch (t: Throwable) {
-      freePictureRef.set(holder)
+      if (isClosed.get()) {
+        holder.close()
+      } else {
+        freePictureRef.set(holder)
+      }
       options.logger.log(SentryLevel.ERROR, "Canvas Strategy: picture render failed", t)
     }
   }
 
   @SuppressLint("UnclosedTrace")
   override fun capture(root: View) {
-
+    if (isClosed.get()) {
+      return
+    }
     val holder = freePictureRef.getAndSet(null)
     if (holder == null) {
       options.logger.log(SentryLevel.DEBUG, "No free Picture available, skipping capture")
@@ -156,9 +172,12 @@ internal class CanvasStrategy(
     root.draw(textIgnoringCanvas)
     holder.picture.endRecording()
 
-    unprocessedPictureRef.set(holder)
-
-    executor.getExecutor().submitSafely(options, "screenshot_recorder.canvas", pictureRenderTask)
+    if (isClosed.get()) {
+      holder.close()
+    } else {
+      unprocessedPictureRef.set(holder)
+      executor.getExecutor().submitSafely(options, "screenshot_recorder.canvas", pictureRenderTask)
+    }
   }
 
   override fun onContentChanged() {
@@ -173,7 +192,11 @@ internal class CanvasStrategy(
           recycle()
         }
       }
+      screenshot = null
     }
+    // the image can be free, unprocessed or in transit
+    freePictureRef.getAndSet(null)?.reader?.close()
+    unprocessedPictureRef.getAndSet(null)?.reader?.close()
   }
 
   override fun lastCaptureSuccessful(): Boolean {
@@ -191,7 +214,7 @@ internal class CanvasStrategy(
 }
 
 @SuppressLint("UseKtx")
-private class TextIgnoringDelegateCanvas : Canvas() {
+private class TextIgnoringDelegateCanvas(sessionReplay: SentryReplayOptions) : Canvas() {
 
   lateinit var delegate: Canvas
   private val solidPaint = Paint()
@@ -205,6 +228,13 @@ private class TextIgnoringDelegateCanvas : Canvas() {
 
   private val bitmapColorCache = WeakHashMap<Bitmap, Pair<Int, Int>>()
 
+  private val maskAllText =
+    sessionReplay.maskViewClasses.contains(TEXT_VIEW_CLASS_NAME) ||
+      sessionReplay.maskViewClasses.size > 1
+  private val maskAllImages =
+    sessionReplay.maskViewClasses.contains(IMAGE_VIEW_CLASS_NAME) ||
+      sessionReplay.maskViewClasses.size > 1
+
   override fun isHardwareAccelerated(): Boolean {
     return false
   }
@@ -992,7 +1022,7 @@ private class PictureReaderHolder(
   val width: Int,
   val height: Int,
   val listener: (holder: PictureReaderHolder) -> Unit,
-) : ImageReader.OnImageAvailableListener {
+) : ImageReader.OnImageAvailableListener, Closeable {
   val picture = Picture()
 
   @SuppressLint("InlinedApi")
@@ -1005,4 +1035,12 @@ private class PictureReaderHolder(
       listener(this)
     }
   }
+
+  override fun close() {
+    try {
+      reader.close()
+    } catch (_: Throwable) {
+      // ignored
+    }
+  }
 }