fix(replay): keep Compose screenshots by routing them onto the verified mask path - #713
Conversation
…ed mask path A Jetpack Compose window recomposes on almost every frame, and the legacy redraw guard can never classify a Compose redraw as animation-only, so it discarded every screenshot and the recording showed a blank gray screen. Since a discarded first frame emits nothing, there was also no full snapshot, so the player reported "this recording can't be played". Route Compose-rooted windows onto the mask-alignment verification path (added in b4e79ff), which compares real mask geometry and keeps frames through pixel-only redraws. Also log a warning after several consecutive discards so a blank recording stops being silent. Generated-By: PostHog Desktop Task-Id: 3ceeac8c-691c-446e-bf60-790794c09f5a
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 0 should fix, 1 consider. Published 1 finding (view the review). Resolved comments: 1 fixed |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
The consecutive-discard warning hardcoded "because the screen kept
changing during capture", but recordScreenshotDiscarded is invoked for
every null base64 — including PixelCopy timeouts (latch.await returns
false and logs nothing) and bitmap encoding failures (webpBase64 returns
null silently). On repeated timeouts the operator was told the screen
kept changing, sending support down the wrong path.
Reword the summary to list the possible causes (screen changing,
PixelCopy failing or timing out, bitmap encoding failing) instead of
asserting a single one. Pure log-string change; no capture, masking, or
frame behavior changes. The substring the covering test asserts on
("screenshots in a row") is preserved.
Generated-By: PostHog Desktop
Task-Id: e6bd6325-2e7a-4afa-9936-461f97ebaeea
turnipdabeets
left a comment
turnipdabeets
left a comment
There was a problem hiding this comment.
Looks close, just left some questions and nits.
I also think it's worth running this on a device/emulator before merging, since it moves the mask pre-walk onto the main thread on every capture.
| view: View, | ||
| drawState: WindowDrawState, | ||
| ): Boolean { | ||
| return config.sessionReplayConfig.verifyScreenshotMaskAlignment || |
There was a problem hiding this comment.
[suggestion] This makes the @PostHogExperimental verified path the default for Compose apps, but verifyScreenshotMaskAlignment's KDoc still reads "Defaults to false" with no mention of the Compose opt-in (PostHogSessionReplayConfig.kt:65-72) — so setting it false now looks like it disables verification when it doesn't. I think we need to update that doc alongside this, and it's worth deciding whether the flag stays @PostHogExperimental once its path ships to most new Android apps.
There was a problem hiding this comment.
Fixed in 4473a4b, the KDoc now says Compose-rooted windows always take the verified path regardless of the flag, so setting it false doesn't disable verification for them.
Left @PostHogExperimental on it for now. Dropping it is a public API change that'd want its own changeset bump, so I think it's a separate call rather than something to fold into a fix. Worth opening separately?
| view: View, | ||
| drawState: WindowDrawState, | ||
| ): ArmedMaskCapture? { | ||
| // The whole loop runs in ONE main-thread message so a draw can't land between |
There was a problem hiding this comment.
[question] Moving the arm loop into one main-thread message is a solid fix for the tearing. It does put the whole pre-walk on main for everyone on the verified path, up to 3 attempts, with the capture thread blocking up to 1s — was that measured on a device? The PR notes unit tests only, and ComposeReplayActivity looks like exactly the harness for a frame-timing check before release.
There was a problem hiding this comment.
Measured it. Pixel 8 emulator, ComposeReplayActivity with the infinite animation, 60s of capture, timing runArmMaskCaptureLoop directly rather than inferring it from frame stats.
Main-thread occupancy came out at 0.31ms p50, 1.86ms p95, 4.14ms worst across 57 arm cycles, so 0 of 57 crossed a 60Hz frame budget and the worst was about a quarter of one. The capture thread never got near the 1s ceiling either, worst block was 305ms, and the 3-attempt loop never re-ran once.
One caveat on that block number: it's dominated by waiting for the main thread to dequeue rather than by the walk itself, and this is a software-rendered emulator sitting at ~57% janky frames, so the queue wait is pessimistic. The occupancy figure is the one that decides whether frames drop. I tried the frame-stats A/B you'd expect first and it was useless, the emulator's jank floor swamped the signal entirely.
| // Compose can be mounted lazily, so a "not Compose" verdict must be re-checked after a | ||
| // layout. A "Compose" verdict never needs re-checking: the window simply stays on the | ||
| // verified path (the safe direction), avoiding a tree re-walk on every layout pass. | ||
| if (composeRooted == false) { |
There was a problem hiding this comment.
[suggestion] For a View-rooted window that merely has Compose on the classpath this never settles: every layout clears the verdict and the next draw re-walks the whole tree, and onDrawCallback is the unthrottled draw callback (PostHogReplayIntegration.kt:331). containsComposeView also allocates a fresh mutableSetOf<Int>() per walk, unlike MaskWalk.visitedViews which is reused. Could we bound the re-check — at most once every N ms while it's false — so lazy-mounted Compose still gets picked up without a per-frame walk?
There was a problem hiding this comment.
Fixed in e087bdc, the re-check is rate-limited to once a second while the verdict is false, and the walk reuses an IntHashSet instead of allocating a fresh mutableSetOf<Int>() per call, matching MaskWalk.visitedViews.
One correction on the premise though: recordLayout() only clears a false verdict, a Compose verdict is sticky, so it was never re-walking on every layout for Compose-rooted windows. The case you're describing is real for a View-rooted window with Compose on the classpath, which is what the bound fixes.
Worth flagging that my first cut of this broke compose detection failure is not cached and re-runs on the next draw, since an indefinite verdict was spending the re-check budget. That's handled with clearComposeRootCheck() now, and there's a new test that fails if the bound comes out.
| } | ||
|
|
||
| // Warns once after a run of discards so a silently blank recording stops being invisible. | ||
| private fun recordScreenshotDiscarded(drawState: WindowDrawState) { |
There was a problem hiding this comment.
[question] config.logger only writes when debug = true (PostHogAndroidLogger.kt:12-16), so this warning won't reach a production build — which is the "support ends up guessing" case the PR description opens with. Is a log the right channel, or should a discard streak surface as a property on the replay payload?
There was a problem hiding this comment.
Fair point, you're right that it doesn't reach a production build. PostHogAndroidLogger.isEnabled() returns config.debug, so the warning is invisible in exactly the case the PR description opens with.
I'd rather not add the payload property here though. .capture(postHog) is already used in this file for replay events so the channel exists, but nothing currently attaches diagnostics to the $snapshot payload, and inventing snapshot metadata wants a posthog-js parity check on the contract first. Happy to take it as a follow-up, or would you rather block this on it?
| // discards this tick and retries at the next scheduled snapshot. | ||
| private const val MAX_BASELINE_ARM_ATTEMPTS: Int = 3 | ||
|
|
||
| // Consecutive screenshot discards before warning that the recording may be blank. |
There was a problem hiding this comment.
[nit] This comment describes CONSECUTIVE_DISCARD_WARNING_THRESHOLD, which lives in ViewTreeSnapshotStatus.kt — sitting here it reads as documenting integrationInstalled.
There was a problem hiding this comment.
Fixed in 4473a4b, dropped the comment rather than moving it, CONSECUTIVE_DISCARD_WARNING_THRESHOLD reads clearly enough at its declaration in ViewTreeSnapshotStatus.kt.
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "posthog-android": patch | |||
There was a problem hiding this comment.
[question] Should this be minor? It's a fix, but it switches the default capture path for every Compose-rooted app and moves where arming runs for anyone already on verifyScreenshotMaskAlignment — a bit more than patch usually implies.
There was a problem hiding this comment.
Checked the convention and I think patch is right. Every fix(...) changeset in the last ~30 on this repo used patch regardless of behavioural scope, #722 (replay surviving session rotation), #717 (flush retry semantics) and #714 (date offset overflow) are all the same shape as this one.
Happy to bump it if you'd rather, but it'd be diverging from what the repo's been doing rather than correcting it.
| </activity> | ||
|
|
||
| <activity | ||
| android:name="com.posthog.android.sample.ComposeReplayActivity" |
There was a problem hiding this comment.
[nit] ComposeReplayActivity isn't reachable from the sample's UI, so it's adb-only. A button on the main screen would make it a usable repro for the next person who hits this.
There was a problem hiding this comment.
Fixed in d150e39, there's a Compose Replay button on the main sample screen now.
It was a bit worse than adb-only actually, am start refuses it outright with a SecurityException since the activity isn't exported, so there was no way to reach it without the button. Verified on the emulator that it launches.
2 participants
💡 Motivation and Context
PostHogReplayIntegration, the default screenshot path (maskLegacyScreenshot) discards a screenshot whenever a draw lands during capture unless the redraw classifier can prove it is animation-only. That classifier needs transient state or aSurfaceView/TextureViewin the tree — a Compose window has neither, so every recomposition invalidates the frame.59725fc, a discarded frame emits nothing (not a placeholder), so a dropped first capture leaves no full snapshot and the player cannot start playback.b4e79ffadded real mask-alignment verification that survives pixel-only redraws, but it sits behindverifyScreenshotMaskAlignment, an experimental flag defaulting tofalse.💚 How did you test it?
PostHogReplayIntegrationTest:verifyScreenshotMaskAlignmentis off (the legacy animation-redraw classifier does not run for it);:posthog-androidunit test suite, plusspotlessCheck,detekt, andapiCheck— all green.Changes
shouldVerifyMaskAlignment(view, drawState)returns true when the flag is set or the window is Compose-rooted, so Compose windows use the verified capture path automatically. The result is detected once from the view tree and cached onWindowDrawState, so the redraw handling does not re-walk the tree on every draw.3consecutive screenshot discards, so a blank recording becomes diagnosable.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Fully autonomous
PostHogReplayIntegration.ktand theb4e79ffverification work, then chose to auto-route Compose windows onto the verified path rather than weakening the legacy redraw classifier — the classifier cannot distinguish a pixel-only redraw from a geometry change on Compose, so keeping frames there risks a PII leak, whereas the verified path compares real mask geometry.Created with PostHog Desktop from this inbox report.