diff --git a/CHANGELOG.md b/CHANGELOG.md index bf354f1c46..b85d79bc78 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Improvements + +- Session Replay: Use main thread looper to schedule replay capture ([#4542](https://github.com/getsentry/sentry-java/pull/4542)) + ## 8.16.1-alpha.2 ### Fixes diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt index 618f25fb78..2d866e6a6d 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt @@ -60,6 +60,9 @@ internal class ScreenshotRecorder( private val debugOverlayDrawable = DebugOverlayDrawable() fun capture() { + if (options.sessionReplay.isDebug) { + options.logger.log(DEBUG, "Capturing screenshot, isCapturing: %s", isCapturing.get()) + } if (!isCapturing.get()) { if (options.sessionReplay.isDebug) { options.logger.log(DEBUG, "ScreenshotRecorder is paused, not capturing screenshot") @@ -67,6 +70,15 @@ internal class ScreenshotRecorder( return } + if (options.sessionReplay.isDebug) { + options.logger.log( + DEBUG, + "Capturing screenshot, contentChanged: %s, lastCaptureSuccessful: %s", + contentChanged.get(), + lastCaptureSuccessful.get(), + ) + } + if (!contentChanged.get() && lastCaptureSuccessful.get()) { screenshotRecorderCallback?.onScreenshotRecorded(screenshot) return @@ -84,99 +96,95 @@ internal class ScreenshotRecorder( return } - // postAtFrontOfQueue to ensure the view hierarchy and bitmap are ase close in-sync as possible - mainLooperHandler.post { - try { - contentChanged.set(false) - PixelCopy.request( - window, - screenshot, - { copyResult: Int -> - if (copyResult != PixelCopy.SUCCESS) { - options.logger.log(INFO, "Failed to capture replay recording: %d", copyResult) - lastCaptureSuccessful.set(false) - return@request - } - - // TODO: handle animations with heuristics (e.g. if we fall under this condition 2 times - // in a row, we should capture) - if (contentChanged.get()) { - options.logger.log(INFO, "Failed to determine view hierarchy, not capturing") - lastCaptureSuccessful.set(false) - return@request - } + try { + contentChanged.set(false) + PixelCopy.request( + window, + screenshot, + { copyResult: Int -> + if (copyResult != PixelCopy.SUCCESS) { + options.logger.log(INFO, "Failed to capture replay recording: %d", copyResult) + lastCaptureSuccessful.set(false) + return@request + } + + // TODO: handle animations with heuristics (e.g. if we fall under this condition 2 times + // in a row, we should capture) + if (contentChanged.get()) { + options.logger.log(INFO, "Failed to determine view hierarchy, not capturing") + lastCaptureSuccessful.set(false) + return@request + } + + // TODO: disableAllMasking here and dont traverse? + val viewHierarchy = ViewHierarchyNode.fromView(root, null, 0, options) + root.traverse(viewHierarchy, options) + + recorder.submitSafely(options, "screenshot_recorder.mask") { + val debugMasks = mutableListOf() + + val canvas = Canvas(screenshot) + canvas.setMatrix(prescaledMatrix) + viewHierarchy.traverse { node -> + if (node.shouldMask && (node.width > 0 && node.height > 0)) { + node.visibleRect ?: return@traverse false + + // TODO: investigate why it returns true on RN when it shouldn't + // if (viewHierarchy.isObscured(node)) { + // return@traverse true + // } + + val (visibleRects, color) = + when (node) { + is ImageViewHierarchyNode -> { + listOf(node.visibleRect) to screenshot.dominantColorForRect(node.visibleRect) + } - // TODO: disableAllMasking here and dont traverse? - val viewHierarchy = ViewHierarchyNode.fromView(root, null, 0, options) - root.traverse(viewHierarchy, options) - - recorder.submitSafely(options, "screenshot_recorder.mask") { - val debugMasks = mutableListOf() - - val canvas = Canvas(screenshot) - canvas.setMatrix(prescaledMatrix) - viewHierarchy.traverse { node -> - if (node.shouldMask && (node.width > 0 && node.height > 0)) { - node.visibleRect ?: return@traverse false - - // TODO: investigate why it returns true on RN when it shouldn't - // if (viewHierarchy.isObscured(node)) { - // return@traverse true - // } - - val (visibleRects, color) = - when (node) { - is ImageViewHierarchyNode -> { - listOf(node.visibleRect) to - screenshot.dominantColorForRect(node.visibleRect) - } - - is TextViewHierarchyNode -> { - val textColor = - node.layout?.dominantTextColor ?: node.dominantColor ?: Color.BLACK - node.layout.getVisibleRects( - node.visibleRect, - node.paddingLeft, - node.paddingTop, - ) to textColor - } - - else -> { - listOf(node.visibleRect) to Color.BLACK - } + is TextViewHierarchyNode -> { + val textColor = + node.layout?.dominantTextColor ?: node.dominantColor ?: Color.BLACK + node.layout.getVisibleRects( + node.visibleRect, + node.paddingLeft, + node.paddingTop, + ) to textColor } - maskingPaint.setColor(color) - visibleRects.forEach { rect -> - canvas.drawRoundRect(RectF(rect), 10f, 10f, maskingPaint) - } - if (options.replayController.isDebugMaskingOverlayEnabled()) { - debugMasks.addAll(visibleRects) + else -> { + listOf(node.visibleRect) to Color.BLACK + } } + + maskingPaint.setColor(color) + visibleRects.forEach { rect -> + canvas.drawRoundRect(RectF(rect), 10f, 10f, maskingPaint) + } + if (options.replayController.isDebugMaskingOverlayEnabled()) { + debugMasks.addAll(visibleRects) } - return@traverse true } + return@traverse true + } - if (options.replayController.isDebugMaskingOverlayEnabled()) { - mainLooperHandler.post { - if (debugOverlayDrawable.callback == null) { - root.overlay.add(debugOverlayDrawable) - } - debugOverlayDrawable.updateMasks(debugMasks) - root.postInvalidate() + if (options.replayController.isDebugMaskingOverlayEnabled()) { + mainLooperHandler.post { + if (debugOverlayDrawable.callback == null) { + root.overlay.add(debugOverlayDrawable) } + debugOverlayDrawable.updateMasks(debugMasks) + root.postInvalidate() } - screenshotRecorderCallback?.onScreenshotRecorded(screenshot) - lastCaptureSuccessful.set(true) - contentChanged.set(false) } - }, - mainLooperHandler.handler, - ) - } catch (e: Throwable) { - options.logger.log(WARNING, "Failed to capture replay recording", e) - lastCaptureSuccessful.set(false) - } + screenshotRecorderCallback?.onScreenshotRecorded(screenshot) + lastCaptureSuccessful.set(true) + contentChanged.set(false) + } + }, + mainLooperHandler.handler, + ) + } catch (e: Throwable) { + options.logger.log(WARNING, "Failed to capture replay recording", e) + lastCaptureSuccessful.set(false) } } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt index 7039389427..197ec09966 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt @@ -4,20 +4,17 @@ import android.annotation.TargetApi import android.graphics.Point import android.view.View import android.view.ViewTreeObserver +import io.sentry.SentryLevel.DEBUG +import io.sentry.SentryLevel.ERROR +import io.sentry.SentryLevel.WARNING import io.sentry.SentryOptions import io.sentry.android.replay.util.MainLooperHandler import io.sentry.android.replay.util.addOnPreDrawListenerSafe -import io.sentry.android.replay.util.gracefullyShutdown import io.sentry.android.replay.util.hasSize import io.sentry.android.replay.util.removeOnPreDrawListenerSafe -import io.sentry.android.replay.util.scheduleAtFixedRateSafely import io.sentry.util.AutoClosableReentrantLock import java.lang.ref.WeakReference -import java.util.concurrent.Executors import java.util.concurrent.ScheduledExecutorService -import java.util.concurrent.ScheduledFuture -import java.util.concurrent.ThreadFactory -import java.util.concurrent.TimeUnit.MILLISECONDS import java.util.concurrent.atomic.AtomicBoolean @TargetApi(26) @@ -36,25 +33,90 @@ internal class WindowRecorder( private val rootViews = ArrayList>() private var lastKnownWindowSize: Point = Point() private val rootViewsLock = AutoClosableReentrantLock() - private var recorder: ScreenshotRecorder? = null - private var capturingTask: ScheduledFuture<*>? = null - private val capturer by lazy { - Executors.newSingleThreadScheduledExecutor(RecorderExecutorServiceThreadFactory()) + @Volatile private var capturer: Capturer? = null + + private class Capturer( + private val options: SentryOptions, + private val mainLooperHandler: MainLooperHandler, + ) : Runnable { + + var recorder: ScreenshotRecorder? = null + var config: ScreenshotRecorderConfig? = null + private val isRecording = AtomicBoolean(true) + + fun resume() { + if (options.sessionReplay.isDebug) { + options.logger.log(DEBUG, "Resuming the capture runnable.") + } + recorder?.resume() + isRecording.getAndSet(true) + val posted = mainLooperHandler.post(this) + if (!posted) { + options.logger.log( + WARNING, + "Failed to post the capture runnable, main looper is not ready.", + ) + } + } + + fun pause() { + recorder?.pause() + isRecording.getAndSet(false) + } + + fun stop() { + recorder?.close() + recorder = null + isRecording.getAndSet(false) + } + + override fun run() { + // protection against the case where the capture is executed after the recording has stopped + if (!isRecording.get()) { + if (options.sessionReplay.isDebug) { + options.logger.log(DEBUG, "Not capturing frames, recording is not running.") + } + return + } + + try { + if (options.sessionReplay.isDebug) { + options.logger.log(DEBUG, "Capturing a frame.") + } + recorder?.capture() + } catch (e: Throwable) { + options.logger.log(ERROR, "Failed to capture a frame", e) + } + + if (options.sessionReplay.isDebug) { + options.logger.log( + DEBUG, + "Posting the capture runnable again, frame rate is ${config?.frameRate ?: 1} fps.", + ) + } + val posted = mainLooperHandler.postDelayed(this, 1000L / (config?.frameRate ?: 1)) + if (!posted) { + options.logger.log( + WARNING, + "Failed to post the capture runnable, main looper is shutting down.", + ) + } + } } override fun onRootViewsChanged(root: View, added: Boolean) { rootViewsLock.acquire().use { if (added) { rootViews.add(WeakReference(root)) - recorder?.bind(root) + capturer?.recorder?.bind(root) determineWindowSize(root) } else { - recorder?.unbind(root) + capturer?.recorder?.unbind(root) rootViews.removeAll { it.get() == root } val newRoot = rootViews.lastOrNull()?.get() if (newRoot != null && root != newRoot) { - recorder?.bind(newRoot) + capturer?.recorder?.bind(newRoot) determineWindowSize(newRoot) } else { Unit // synchronized block wants us to return something lol @@ -102,7 +164,13 @@ internal class WindowRecorder( return } - recorder = + if (capturer == null) { + // don't recreate runnable for every config change, just update the config + capturer = Capturer(options, mainLooperHandler) + } + + capturer?.config = config + capturer?.recorder = ScreenshotRecorder( config, options, @@ -113,59 +181,47 @@ internal class WindowRecorder( val newRoot = rootViews.lastOrNull()?.get() if (newRoot != null) { - recorder?.bind(newRoot) + capturer?.recorder?.bind(newRoot) } - // TODO: change this to use MainThreadHandler and just post on the main thread with delay - // to avoid thread context switch every time - capturingTask = - capturer.scheduleAtFixedRateSafely( - options, - "$TAG.capture", + + val posted = + mainLooperHandler.postDelayed( + capturer, 100L, // delay the first run by a bit, to allow root view listener to register - 1000L / config.frameRate, - MILLISECONDS, - ) { - recorder?.capture() - } + ) + if (!posted) { + options.logger.log( + WARNING, + "Failed to post the capture runnable, main looper is shutting down.", + ) + } } override fun resume() { - recorder?.resume() + capturer?.resume() } override fun pause() { - recorder?.pause() + capturer?.pause() } override fun reset() { lastKnownWindowSize.set(0, 0) rootViewsLock.acquire().use { - rootViews.forEach { recorder?.unbind(it.get()) } + rootViews.forEach { capturer?.recorder?.unbind(it.get()) } rootViews.clear() } } override fun stop() { - recorder?.close() - recorder = null - capturingTask?.cancel(false) - capturingTask = null + capturer?.stop() + capturer = null isRecording.set(false) } override fun close() { reset() stop() - capturer.gracefullyShutdown(options) - } - - private class RecorderExecutorServiceThreadFactory : ThreadFactory { - private var cnt = 0 - - override fun newThread(r: Runnable): Thread { - val ret = Thread(r, "SentryWindowRecorder-" + cnt++) - ret.setDaemon(true) - return ret - } + mainLooperHandler.removeCallbacks(capturer) } } diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/MainLooperHandler.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/MainLooperHandler.kt index 7c067111e7..691cce03a7 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/MainLooperHandler.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/MainLooperHandler.kt @@ -6,7 +6,15 @@ import android.os.Looper internal class MainLooperHandler(looper: Looper = Looper.getMainLooper()) { val handler = Handler(looper) - fun post(runnable: Runnable) { - handler.post(runnable) + fun post(runnable: Runnable): Boolean { + return handler.post(runnable) + } + + fun postDelayed(runnable: Runnable?, delay: Long): Boolean { + return handler.postDelayed(runnable ?: return false, delay) + } + + fun removeCallbacks(runnable: Runnable?) { + handler.removeCallbacks(runnable ?: return) } } diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt index 0b3b8097d9..c26e6be9c4 100644 --- a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt +++ b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt @@ -24,6 +24,7 @@ import io.sentry.rrweb.RRWebVideoEvent import io.sentry.transport.CurrentDateProvider import io.sentry.transport.ICurrentDateProvider import java.time.Duration +import java.util.concurrent.Executors import java.util.concurrent.TimeUnit import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.BeforeTest @@ -35,6 +36,7 @@ import org.junit.Rule import org.junit.Test import org.junit.rules.TemporaryFolder import org.junit.runner.RunWith +import org.mockito.ArgumentMatchers.anyLong import org.mockito.kotlin.any import org.mockito.kotlin.anyOrNull import org.mockito.kotlin.check @@ -65,7 +67,6 @@ class ReplaySmokeTest { .whenever(it) .configureScope(any()) } - var count: Int = 0 private class ImmediateHandler : Handler( @@ -75,6 +76,8 @@ class ReplaySmokeTest { } ) + private val recordingThread = Executors.newSingleThreadScheduledExecutor() + fun getSut( context: Context, dateProvider: ICurrentDateProvider = CurrentDateProvider.getInstance(), @@ -88,9 +91,14 @@ class ReplaySmokeTest { mainLooperHandler = mock { whenever(mock.handler).thenReturn(ImmediateHandler()) - whenever(mock.post(any())).then { - (it.arguments[0] as Runnable).run() - count++ + whenever(mock.post(any())).then { (it.arguments[0] as Runnable).run() } + whenever(mock.postDelayed(any(), anyLong())).then { + // have to use another thread here otherwise it will block the test thread + recordingThread.schedule( + it.arguments[0] as Runnable, + it.arguments[1] as Long, + TimeUnit.MILLISECONDS, + ) } }, )