feat(replay): make screenshot optimizations opt-in - #761
Conversation
| bitmap = bitmapLease.bitmap | ||
| } else { | ||
| bitmapLease = null | ||
| bitmap = Bitmap.createBitmap(view.width, view.height, Bitmap.Config.ARGB_8888) |
There was a problem hiding this comment.
Low: Unbounded bitmap retention after PixelCopy timeouts
When optimizations are disabled, every capture allocates a new full-resolution bitmap. A timed-out request retains that bitmap until its callback arrives, but unlike the lease-backed path, it does not prevent subsequent captures from allocating more; repeated UI redraws during stalled callbacks can therefore exhaust the host app's memory. Keep full-resolution ARGB_8888 capture while applying equivalent single-request backpressure or another explicit bound to outstanding bitmaps.
There was a problem hiding this comment.
Agreed that this can create memory pressure if PixelCopy requests remain pending: their destination bitmaps stay live while later captures allocate more.
That allocation/capture behavior already exists on main. This PR restores it as the default; adding single-request backpressure would instead make later captures skip while an earlier timed-out request remains pending. That behavior is intentionally opt-in through optimizeScreenshots = true.
We're not changing the default outstanding-request policy in this PR. Leaving this thread unresolved to keep the existing memory-pressure risk visible.
PR overviewThis PR makes screenshot optimizations opt-in for Android session replay, using full-resolution bitmap capture when optimizations are disabled. One issue remains: stalled PixelCopy callbacks can allow full-resolution bitmaps to accumulate during repeated UI redraws, potentially exhausting the host app’s memory. The impact depends on timeout conditions and sustained redraw activity, and no reported issues have yet been addressed. Open issues (1)
Fixed/addressed: 0 · PR risk: 4/10 |
Prompt To Fix All With AI### Issue 1
posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt:1715
**Pending bitmaps can leak**
If a default-mode PixelCopy times out, its fresh full-resolution bitmap is left for the callback to recycle. If `uninstall()` quits the callback thread before that callback is delivered, the bitmap is not recycled and is not tracked by `pixelCopyBitmapBuffer.close()`. Multiple delayed captures can therefore retain large ARGB_8888 bitmaps without deterministic cleanup. Please retain ownership of pending default-mode bitmaps so uninstall can release them explicitly.
### Issue 2
posthog-android/src/test/java/com/posthog/android/replay/PostHogReplayIntegrationTest.kt:1785
**Cases are not parameterised**
These new tests loop over multiple cases inside ordinary tests, which violates the repository directive to prefer parameterised tests. A failure in one iteration prevents later cases from running and reports all inputs under one result. Please parameterise the optimization modes and dimension pairs before merging. The same pattern appears in the late-callback, transparency, mask-scaling, and invalid-dimension tests.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "feat(replay): make screenshot optimizati..." | Re-trigger Greptile |
| bitmap = bitmapLease.bitmap | ||
| } else { | ||
| bitmapLease = null | ||
| bitmap = Bitmap.createBitmap(view.width, view.height, Bitmap.Config.ARGB_8888) |
There was a problem hiding this comment.
If a default-mode PixelCopy times out, its fresh full-resolution bitmap is left for the callback to recycle. If uninstall() quits the callback thread before that callback is delivered, the bitmap is not recycled and is not tracked by pixelCopyBitmapBuffer.close(). Multiple delayed captures can therefore retain large ARGB_8888 bitmaps without deterministic cleanup. Please retain ownership of pending default-mode bitmaps so uninstall can release them explicitly.
Knowledge Base Used: Android session replay
Prompt To Fix With AI
This is a comment left during a code review.
Path: posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt
Line: 1715
Comment:
**Pending bitmaps can leak**
If a default-mode PixelCopy times out, its fresh full-resolution bitmap is left for the callback to recycle. If `uninstall()` quits the callback thread before that callback is delivered, the bitmap is not recycled and is not tracked by `pixelCopyBitmapBuffer.close()`. Multiple delayed captures can therefore retain large ARGB_8888 bitmaps without deterministic cleanup. Please retain ownership of pending default-mode bitmaps so uninstall can release them explicitly.
**Knowledge Base Used:** [Android session replay](https://app.greptile.com/posthog-org-19734/-/custom-context/knowledge-base/posthog/posthog-android/-/docs/android-session-replay.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
You're right that quitSafely() can reject a callback posted after shutdown, so explicit recycling is not guaranteed in that case. However, missing recycle() does not itself establish a leak: Android normally reclaims bitmap memory through GC once references are gone, and the default path does not add an SDK collection retaining these destinations.
Tracking pending destinations would not make it safe to recycle them during uninstall: PixelCopy could still be using them. The reusable buffer's close() likewise recycles only idle bitmaps and detaches an in-flight lease rather than recycling its bitmap immediately.
The shutdown behavior also exists on main. We're not adding forced recycling here. Leaving this thread unresolved for visibility; the late-callback test verifies cleanup when completion is delivered, not guaranteed callback delivery after real handler shutdown.
There was a problem hiding this comment.
That’s a fair clarification. I agree this does not establish an actionable leak: the SDK does not retain the default-mode bitmap, and recycling it during uninstall() would be unsafe while PixelCopy may still be using it. If the callback is never delivered, the bitmap can still be reclaimed by GC once the platform releases its references. I’ll leave this thread unresolved for visibility rather than requiring pending-bitmap tracking.
|
whats about this instead: Proposed usage—not available today sessionReplayConfig.apply {
screenshotQuality = PostHogScreenshotQuality.BALANCED
// Preserve transparency and full color precision.
screenshotColorMode = PostHogScreenshotColorMode.ARGB_8888
}Proposed APIAdd these properties inside the existing config class body, preserving its constructor signatures: public enum class PostHogScreenshotQuality {
ORIGINAL,
BALANCED,
LOW,
}
public enum class PostHogScreenshotColorMode {
ARGB_8888,
RGB_565,
}
// Inside PostHogSessionReplayConfig:
@PostHogExperimental
public var screenshotQuality = PostHogScreenshotQuality.ORIGINAL
@PostHogExperimental
public var screenshotColorMode = PostHogScreenshotColorMode.ARGB_8888Suggested initial behaviorThese preset values are starting points to benchmark, not measured recommendations: | Setting | Resolution | WebP quality |
|---|---|---|
| `ORIGINAL` — default | Full physical resolution | 30, preserving current behavior |
| `BALANCED` | Half width and height | 30 |
| `LOW` | Quarter width and height | 20 |(webp quality could be configurable eg screenshotCompressionQuality = 30 - 0 to 100 but not mandatory) Separate behavior:
This allows useful combinations: // Lower overhead without losing transparency:
screenshotQuality = PostHogScreenshotQuality.BALANCED
screenshotColorMode = PostHogScreenshotColorMode.ARGB_8888 |
|
@ioannisj wdyt as well? #761 (comment) |
suggestion: Consider density-aware screenshot sizing as a follow-upBuilding on the quality/color-mode proposal above, another possible improvement is to make the reduced-resolution presets density-aware, rather than always dividing physical width and height by a fixed factor. This is a non-blocking design suggestion, not a defect in this PR. Currently, the optimized capture uses approximately Possible configurationNo additional public setting is necessary: this could refine the proposed sessionReplayConfig.apply {
screenshot = true
screenshotQuality = PostHogScreenshotQuality.BALANCED
screenshotColorMode = PostHogScreenshotColorMode.ARGB_8888
throttleDelayMs = 1_000
}These are proposed APIs, not currently available. One possible preset mapping, instead of the fixed half/quarter-resolution mapping above, is:
The reduced preset values are starting points for benchmarks, not measured recommendations. Illustrative sizing logicAssuming a valid, positive source size and display density, sample these values once per capture: val density = view.resources.displayMetrics.density
val scale = when (quality) {
PostHogScreenshotQuality.ORIGINAL -> 1f
PostHogScreenshotQuality.BALANCED -> minOf(1f, 1f / density)
PostHogScreenshotQuality.LOW -> minOf(1f, 0.8f / density)
}
val captureWidth = maxOf(1, ceil(sourceWidth * scale).toInt())
val captureHeight = maxOf(1, ceil(sourceHeight * scale).toInt())Clamping the scale to 1 avoids upscaling low-density displays. This applies to the PixelCopy destination bitmap, not just replay layout coordinates. Tradeoffs / validation
|
|
a follow up that would spare some memory and cpu usage under heavy redrawns or slow captures |
Expose scale, WebP quality, and color-mode settings while preserving default capture fidelity and constructor compatibility. Reuse compatible destinations without recycling pending copies, and keep recording state and buffer lifecycle atomic across stop/start.
defdd17 to
89644b6
Compare
💡 Motivation and Context
Stacked on #756. This follow-up must land before the screenshot optimization is released so existing users retain the current capture defaults.
Adds experimental
sessionReplayConfig.optimizeScreenshots, defaulting tofalse:The option is sampled once per capture, including masking and bitmap release. Both modes safely reclaim late bitmaps and discard captures with non-positive source dimensions. The existing
screenshotdefault and all constructor signatures are unchanged.The follow-up also removes redundant casts and updates the changeset to describe the opt-in behavior and minor API addition.
💚 How did you test it?
make test: 355 Android tests and 9 Compose tests passed; 3 existing skips.make testJava: 942 core tests passed.make checkFormat,make api,./gradlew apiCheck, andgit diff --checkpassed.📝 Checklist
🤖 Agent context
Autonomy: Human-driven (agent-assisted).
Implemented and self-reviewed with Pi, Git/GitHub CLI, Gradle, Robolectric, and Java consumer checks. Kept one shared capture/completion pipeline with capture-local mode selection rather than duplicating the screenshot implementation. Human review is required; native/OEM PixelCopy behavior remains outside the local test coverage.