fix(replay): Fix visual artifacts for the Canvas strategy on some devices

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Fixes
+
+- Fix visual artifacts for the Canvas strategy on some devices ([#4861](https://github.com/getsentry/sentry-java/pull/4861))
+
 ### Improvements
 
 - Fallback to distinct-id as user.id logging attribute when user is not set ([#4847](https://github.com/getsentry/sentry-java/pull/4847))

sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt

@@ -244,21 +244,6 @@ internal class WindowRecorder(
   override fun getExecutor(): ScheduledExecutorService = replayExecutor
 
   override fun getMainLooperHandler(): MainLooperHandler = mainLooperHandler
-
-  override fun getBackgroundHandler(): Handler {
-    // only start the background thread if it's actually needed, as it's only used by Canvas Capture
-    // Strategy
-    if (backgroundProcessingHandler == null) {
-      backgroundProcessingHandlerLock.acquire().use {
-        if (backgroundProcessingHandler == null) {
-          backgroundProcessingHandlerThread = HandlerThread("SentryReplayBackgroundProcessing")
-          backgroundProcessingHandlerThread?.start()
-          backgroundProcessingHandler = Handler(backgroundProcessingHandlerThread!!.looper)
-        }
-      }
-    }
-    return backgroundProcessingHandler!!
-  }
 }
 
 internal interface ExecutorProvider {
@@ -267,7 +252,4 @@ internal interface ExecutorProvider {
 
   /** Returns a handler associated with the main thread looper. */
   fun getMainLooperHandler(): MainLooperHandler
-
-  /** Returns a handler associated with a background thread looper. */
-  fun getBackgroundHandler(): Handler
 }

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

@@ -15,34 +15,34 @@ import android.graphics.NinePatch
 import android.graphics.Paint
 import android.graphics.Path
 import android.graphics.Picture
-import android.graphics.PixelFormat
 import android.graphics.PorterDuff
 import android.graphics.Rect
 import android.graphics.RectF
 import android.graphics.Region
 import android.graphics.RenderNode
+import android.graphics.SurfaceTexture
 import android.graphics.fonts.Font
 import android.graphics.text.MeasuredText
-import android.media.ImageReader
 import android.os.Build
+import android.view.PixelCopy
+import android.view.Surface
 import android.view.View
 import androidx.annotation.RequiresApi
 import io.sentry.SentryLevel
+import io.sentry.SentryLevel.DEBUG
 import io.sentry.SentryOptions
 import io.sentry.android.replay.ExecutorProvider
 import io.sentry.android.replay.ScreenshotRecorderCallback
 import io.sentry.android.replay.ScreenshotRecorderConfig
 import io.sentry.android.replay.util.ReplayRunnable
 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
 import kotlin.LazyThreadSafetyMode.NONE
-import kotlin.use
 
-@SuppressLint("UseKtx")
+@SuppressLint("NewApi", "UseKtx")
 internal class CanvasStrategy(
   private val executor: ExecutorProvider,
   private val screenshotRecorderCallback: ScreenshotRecorderCallback?,
@@ -51,73 +51,19 @@ internal class CanvasStrategy(
 ) : ScreenshotStrategy {
 
   @Volatile private var screenshot: Bitmap? = null
-
-  // Lock to synchronize screenshot creation
+  private var unprocessedPictureRef = AtomicReference<Picture>(null)
   private val screenshotLock = AutoClosableReentrantLock()
   private val prescaledMatrix by
     lazy(NONE) { Matrix().apply { preScale(config.scaleFactorX, config.scaleFactorY) } }
   private val lastCaptureSuccessful = AtomicBoolean(false)
   private val textIgnoringCanvas = TextIgnoringDelegateCanvas()
-
   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()
-        try {
-          if (image.planes.size > 0) {
-            val plane = image.planes[0]
-
-            if (screenshot == null) {
-              screenshotLock.acquire().use {
-                if (screenshot == null) {
-                  screenshot =
-                    Bitmap.createBitmap(holder.width, holder.height, Bitmap.Config.ARGB_8888)
-                }
-              }
-            }
-
-            val bitmap = screenshot
-            if (bitmap != null) {
-              val buffer = plane.buffer.rewind()
-              synchronized(bitmap) {
-                if (!bitmap.isRecycled) {
-                  bitmap.copyPixelsFromBuffer(buffer)
-                  lastCaptureSuccessful.set(true)
-                }
-              }
-              screenshotRecorderCallback?.onScreenshotRecorded(bitmap)
-            }
-          }
-        } finally {
-          try {
-            image.close()
-          } catch (_: Throwable) {
-            // ignored
-          }
-        }
-      } catch (e: Throwable) {
-        options.logger.log(SentryLevel.ERROR, "CanvasStrategy: image processing failed", e)
-      } finally {
-        if (isClosed.get()) {
-          holder.close()
-        } else {
-          freePictureRef.set(holder)
-        }
-      }
+  private val surfaceTexture =
+    SurfaceTexture(false).apply {
+      setDefaultBufferSize(config.recordingWidth, config.recordingHeight)
     }
-  }
-
-  private var freePictureRef =
-    AtomicReference(
-      PictureReaderHolder(config.recordingWidth, config.recordingHeight, onImageAvailableListener)
-    )
-
-  private var unprocessedPictureRef = AtomicReference<PictureReaderHolder>(null)
+  private val surface = Surface(surfaceTexture)
 
   init {
     IntegrationUtils.addIntegrationToSdkVersion("ReplayCanvasStrategy")
@@ -126,59 +72,88 @@ internal class CanvasStrategy(
   @SuppressLint("NewApi")
   private val pictureRenderTask = Runnable {
     if (isClosed.get()) {
-      options.logger.log(
-        SentryLevel.DEBUG,
-        "Canvas Strategy already closed, skipping picture render",
-      )
+      options.logger.log(DEBUG, "Canvas Strategy already closed, skipping picture render")
       return@Runnable
     }
-    val holder = unprocessedPictureRef.getAndSet(null) ?: return@Runnable
+    val picture = unprocessedPictureRef.getAndSet(null) ?: return@Runnable
 
     try {
-      if (!holder.setup.getAndSet(true)) {
-        holder.reader.setOnImageAvailableListener(holder, executor.getBackgroundHandler())
-      }
-
-      val surface = holder.reader.surface
-      val canvas = surface.lockHardwareCanvas()
+      // It's safe to access the surface because the render task,
+      // as well as surface release are executed on the same single threaded executor
+      // Draw picture to the Surface for PixelCopy
+      val surfaceCanvas = surface.lockHardwareCanvas()
       try {
-        canvas.drawColor(Color.BLACK, PorterDuff.Mode.CLEAR)
-        holder.picture.draw(canvas)
+        surfaceCanvas.drawColor(Color.BLACK, PorterDuff.Mode.CLEAR)
+        picture.draw(surfaceCanvas)
       } finally {
-        surface.unlockCanvasAndPost(canvas)
+        surface.unlockCanvasAndPost(surfaceCanvas)
       }
-    } catch (t: Throwable) {
-      if (isClosed.get()) {
-        holder.close()
-      } else {
-        freePictureRef.set(holder)
+
+      if (screenshot == null) {
+        screenshotLock.acquire().use {
+          if (screenshot == null) {
+            screenshot = Bitmap.createBitmap(picture.width, picture.height, Bitmap.Config.ARGB_8888)
+          }
+        }
       }
+      PixelCopy.request(
+        surface,
+        screenshot!!,
+        { result ->
+          if (isClosed.get()) {
+            options.logger.log(DEBUG, "CanvasStrategy is closed, ignoring capture result")
+            return@request
+          }
+          executor
+            .getExecutor()
+            .submit(
+              ReplayRunnable("screenshot_recorder.mask") {
+                if (isClosed.get()) {
+                  options.logger.log(DEBUG, "CanvasStrategy is closed, ignoring capture result")
+                  return@ReplayRunnable
+                }
+                if (result == PixelCopy.SUCCESS) {
+                  lastCaptureSuccessful.set(true)
+                  val bitmap = screenshot
+                  if (bitmap != null) {
+                    synchronized(bitmap) {
+                      if (!bitmap.isRecycled)
+                        screenshotRecorderCallback?.onScreenshotRecorded(bitmap)
+                    }
+                  }
+                } else {
+                  options.logger.log(
+                    SentryLevel.ERROR,
+                    "Canvas Strategy: PixelCopy failed with code $result",
+                  )
+                  lastCaptureSuccessful.set(false)
+                }
+              }
+            )
+        },
+        executor.getMainLooperHandler().handler,
+      )
+    } catch (t: Throwable) {
       options.logger.log(SentryLevel.ERROR, "Canvas Strategy: picture render failed", t)
+      lastCaptureSuccessful.set(false)
     }
   }
 
-  @SuppressLint("UnclosedTrace")
+  @SuppressLint("NewApi")
   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")
-      lastCaptureSuccessful.set(false)
-      return
-    }
 
-    val pictureCanvas = holder.picture.beginRecording(config.recordingWidth, config.recordingHeight)
-    textIgnoringCanvas.delegate = pictureCanvas
+    val picture = Picture()
+    val canvas = picture.beginRecording(config.recordingWidth, config.recordingHeight)
+    textIgnoringCanvas.delegate = canvas
     textIgnoringCanvas.setMatrix(prescaledMatrix)
     root.draw(textIgnoringCanvas)
-    holder.picture.endRecording()
+    picture.endRecording()
 
-    if (isClosed.get()) {
-      holder.close()
-    } else {
-      unprocessedPictureRef.set(holder)
+    if (!isClosed.get()) {
+      unprocessedPictureRef.set(picture)
       executor.getExecutor().submit(ReplayRunnable("screenshot_recorder.canvas", pictureRenderTask))
     }
   }
@@ -192,28 +167,15 @@ internal class CanvasStrategy(
     executor
       .getExecutor()
       .submit(
-        ReplayRunnable(
-          "CanvasStrategy.close",
-          {
-            screenshot?.let {
-              synchronized(it) {
-                if (!it.isRecycled) {
-                  it.recycle()
-                }
-              }
-            }
-          },
-        )
+        ReplayRunnable("CanvasStrategy.close") {
+          screenshot?.let { synchronized(it) { if (!it.isRecycled) it.recycle() } }
+          surface.release()
+          surfaceTexture.release()
+        }
       )
-
-    // the image can be free, unprocessed or in transit
-    freePictureRef.getAndSet(null)?.reader?.close()
-    unprocessedPictureRef.getAndSet(null)?.reader?.close()
   }
 
-  override fun lastCaptureSuccessful(): Boolean {
-    return lastCaptureSuccessful.get()
-  }
+  override fun lastCaptureSuccessful(): Boolean = lastCaptureSuccessful.get()
 
   override fun emitLastScreenshot() {
     if (lastCaptureSuccessful()) {
@@ -1031,30 +993,3 @@ private class TextIgnoringDelegateCanvas : Canvas() {
     }
   }
 }
-
-private class PictureReaderHolder(
-  val width: Int,
-  val height: Int,
-  val listener: (holder: PictureReaderHolder) -> Unit,
-) : ImageReader.OnImageAvailableListener, Closeable {
-  val picture = Picture()
-
-  @SuppressLint("InlinedApi")
-  val reader: ImageReader = ImageReader.newInstance(width, height, PixelFormat.RGBA_8888, 1)
-
-  var setup = AtomicBoolean(false)
-
-  override fun onImageAvailable(reader: ImageReader?) {
-    if (reader != null) {
-      listener(this)
-    }
-  }
-
-  override fun close() {
-    try {
-      reader.close()
-    } catch (_: Throwable) {
-      // ignored
-    }
-  }
-}

sentry-android-replay/src/test/java/io/sentry/android/replay/ScreenshotRecorderTest.kt

@@ -1,6 +1,5 @@
 package io.sentry.android.replay
 
-import android.os.Handler
 import androidx.test.ext.junit.runners.AndroidJUnit4
 import io.sentry.ScreenshotStrategyType
 import io.sentry.SentryOptions
@@ -30,8 +29,6 @@ class ScreenshotRecorderTest {
           override fun getExecutor(): ScheduledExecutorService = mock<ScheduledExecutorService>()
 
           override fun getMainLooperHandler(): MainLooperHandler = mock<MainLooperHandler>()
-
-          override fun getBackgroundHandler(): Handler = mock<Handler>()
         },
         null,
       )

sentry-android-replay/src/test/java/io/sentry/android/replay/screenshot/PixelCopyStrategyTest.kt

@@ -2,7 +2,6 @@ package io.sentry.android.replay.screenshot
 
 import android.app.Activity
 import android.os.Bundle
-import android.os.Handler
 import android.os.Looper
 import android.widget.LinearLayout
 import android.widget.LinearLayout.LayoutParams
@@ -45,8 +44,6 @@ class PixelCopyStrategyTest {
           override fun getExecutor(): ScheduledExecutorService = executor
 
           override fun getMainLooperHandler(): MainLooperHandler = MainLooperHandler()
-
-          override fun getBackgroundHandler(): Handler = mock()
         },
         callback,
         options,