fix(replay): verify mask alignment instead of discarding frames on any redraw - #676
fix(replay): verify mask alignment instead of discarding frames on any redraw#676arnohillen wants to merge 4 commits into
Conversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
@arnohillen, this requires manual testing; otherwise, we risk leaking PII. Have you tested this, or are you purely relying on the unit tests? |
🦔 ReviewHog reviewed this pull requestFound 1 must fix, 1 should fix, 0 consider. Published 2 findings (view the review). |
|
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}
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 6 issues
Files (3)
.changeset/replay-animated-screens-capture.mdposthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.ktposthog-android/src/main/java/com/posthog/android/replay/internal/ViewTreeSnapshotStatus.kt
|
Verified: I ran the Android sample app in screenshot-based session replay mode with a continuously animated loading spinner visible. Before the patch, every capture attempt during the animation was discarded and no replay frames were sent; after the patch, frames were captured continuously with no discard errors, and the animation was represented by changing screenshot payloads. |
awesome, @dustinbyrne @arnohillen worth testing this on react native as well since react native relies on that and sometimes there are some incompatibilities |
…y redraw Screenshot captures were discarded whenever the window redrew during PixelCopy unless an animation-type heuristic matched (hasTransientState, surface/texture views). Most animations (indeterminate spinners such as ProgressDialog, animated GIFs, Lottie, Material progress indicators, Compose infinite animations) match neither signal, so screens showing them produced no replay frames at all. The draw-dirty flags were also shared across all tracked windows, so an animating dialog blanked the static activity behind it. Scope draw-dirty tracking per window and replace the heuristics with direct verification: sample mask rects before and after the pixel copy and keep the frame only when they are identical, no layout pass ran, and the walks saw nothing untrustworthy. Fail closed when a walk meets a rendered view with unknowable geometry (legacy view animation, transient state), when the Compose semantics pass times out, and when PixelCopy times out before masks are painted. Closes #596 Generated-By: PostHog Code Task-Id: 3e1a675f-53c2-4356-9a2f-079d36be9790
…esets Two review findings on the discard guard: Check walk poison before the clean-frame path: a poisoned walk's rect set may be silently incomplete (pruned unstable view, timed-out Compose semantics pass), so keeping a clean frame and painting the incomplete post-walk rects would ship the unmasked content. Drop the drawState.reset() from the PixelCopy callback's finally block: after a latch timeout the callback can fire while a newer capture for the same window is in flight, and the stale reset erased draw/layout flags that capture depended on. The reset at capture start (and in the executor's finally) already provides per-capture hygiene. Generated-By: PostHog Code Task-Id: 3e1a675f-53c2-4356-9a2f-079d36be9790
Trim multi-line comments to single-line WHYs and let the code carry the WHAT: the pruned-but-rendered poison condition moves into a named helper (isRenderedButUnplaceable), and the post-walk skip condition into an alreadyDoomed val. Generated-By: PostHog Code Task-Id: 3e1a675f-53c2-4356-9a2f-079d36be9790
7abe3a3 to
999a082
Compare
|
working as expected on RN |
There was a problem hiding this comment.
ReviewHog Report
Changes
Issues: 2 issues
Files (5)
.changeset/replay-animated-screens-capture.mdposthog-android/build.gradle.ktsposthog-android/gradle.lockfileposthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.ktposthog-android/src/main/java/com/posthog/android/replay/internal/ViewTreeSnapshotStatus.kt
| internal fun onDrawCallback( | ||
| view: View, | ||
| drawState: WindowDrawState, | ||
| ) { | ||
| onDrawCallback(drawState) | ||
| val captureToken = drawState.beginDrawSample() ?: return | ||
|
|
||
| /** | ||
| * Walks the view tree looking for a visible surface/texture-backed view (e.g. Rive, ExoPlayer) | ||
| * that is actively rendering. Unlike ValueAnimator-driven libraries, these render on their own | ||
| * worker thread and never set [View.hasTransientState], but their view geometry stays stable | ||
| * while they animate, so — like the Lottie case — masks remain aligned and the frame is safe | ||
| * to keep. Returns on the first match, and prunes hidden subtrees, to stay cheap on the hot | ||
| * per-draw path. | ||
| */ | ||
| private fun View.hasActiveSurfaceRendering(): Boolean { | ||
| return try { | ||
| when { | ||
| // Descends from the being-drawn decor view, so per-node visibility + alpha pruning | ||
| // skips hidden/transparent subtrees (mirroring isVisible()): a surface mid | ||
| // fade-transition, or under a faded-out ancestor, isn't rendering to the user and | ||
| // must not relax the discard guard. | ||
| visibility != View.VISIBLE || | ||
| alpha <= 0f || | ||
| (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q && transitionAlpha <= 0f) -> false | ||
| this is TextureView -> isAvailable && width > 0 && height > 0 | ||
| this is SurfaceView -> width > 0 && height > 0 | ||
| this is ViewGroup -> { | ||
| for (i in 0 until childCount) { | ||
| if (getChildAt(i).hasActiveSurfaceRendering()) { | ||
| return true | ||
| } | ||
| } | ||
| false | ||
| } | ||
| else -> false | ||
| } | ||
| val walk = MaskWalk() | ||
| try { | ||
| findMaskableWidgets(view, walk) | ||
| } catch (e: Throwable) { | ||
| config.logger.log("Session Replay surface rendering check failed: $e.") | ||
| false | ||
| config.logger.log("Session Replay draw-time mask walk failed: $e.") | ||
| walk.poisoned = true | ||
| } | ||
| drawState.recordMaskWalk(captureToken, walk.rects, walk.poisoned) |
There was a problem hiding this comment.
Draw-time mask verification runs a full recursive tree walk (with inline Compose semantics traversal) on every onDraw() call while a capture is in flight
Why we think it's a valid issue
- Checked:
NextDrawListener.onDraw()(NextDrawListener.kt:20-24), the registration wiring (PostHogReplayIntegration.kt:272), the walking overload (:230-244),beginDrawSamplegating (ViewTreeSnapshotStatus.kt:77-83), the capture-window boundaries (beginMaskCaptureat:1243-1247beforePixelCopy.request;finish/cancelat:1282,:1254,:1335; 1s latch at:1329), and the inline Compose path (:1126-1146). - Found:
onDraw()invokes the walking overload unthrottled on every frame (only the separateonDrawThrottlerCallbackis throttled). Whenever a capture is active,beginDrawSample()returns non-null and the overload runs a full recursivefindMaskableWidgets(view, walk)synchronously on the main/UI thread; the active window spansbeginMaskCapture→finish/cancel, bounded only by PixelCopy completion or the 1s timeout. For Compose views the walk runsgetAllSemanticsNodes(true)inline on that same main thread by design (:1128-1131). - Found: This is a newly introduced cost. The prior draw callback only set a flag —
recordDraw()togglesisOnDrawnCalledand bumps a generation counter (ViewTreeSnapshotStatus.kt:48-53), with no tree walk. So the PR adds synchronous full-tree + Compose-semantics walks onto the main-thread draw path. - Impact: During the continuously-animating screens this PR exists to keep capturing (spinners, GIFs, Lottie, Compose infinite animations), frames draw at display rate, so any draw that lands inside a capture window triggers a full view-tree walk plus an inline Compose semantics merge on the host app's UI thread, unthrottled — a plausible jank/dropped-frame regression on the customer's main thread, worst on deep hierarchies and slow-PixelCopy devices. Concrete trigger + concrete consequence, and a real regression versus the previous flag-only callback → meets the performance / SDK resource-discipline keep bar.
Issue description
NextDrawListener.onDraw() (internal/NextDrawListener.kt) invokes the registered draw callback on every single frame the decor view draws — unthrottled: override fun onDraw() { onDrawCallback(); throttler.throttle { onDrawThrottlerCallback() } }. This PR wires the new onDrawCallback(view, drawState) into that per-draw hook (replacing the old cheap onDrawCallback(decorView)). Inside it, drawState.beginDrawSample() returns a non-null token for the whole window a screenshot capture is active — from beginMaskCapture() right before PixelCopy.request() until finishMaskCapture()/cancelMaskCapture() fires in the callback or the outer timeout finally (which can stretch to the full 1s PixelCopy latch timeout on a busy device). Every draw that lands inside that window triggers a full findMaskableWidgets(view, walk) recursive tree walk synchronously on the calling thread — which is the main UI thread, since onDraw() is a ViewTreeObserver.OnDrawListener callback. Worse, the new Compose branch explicitly runs the semantics traversal (getAllSemanticsNodes(true)) inline on that same thread when already on main ("Draw-time verification already runs on main, where posting and waiting would deadlock, so execute inline in that case", lines ~1126-1131), instead of the previous post-and-await pattern. Because this PR's entire purpose is to keep capturing screens that redraw many times per second (spinners, GIFs, Lottie, Compose animations), that is exactly the population of screens where onDraw() fires repeatedly during the capture window — so a single screenshot attempt can spend dozens of extra full-tree (and full Compose-semantics) walks on the host app's main thread, purely to catch the rare case of geometry moving and reverting between the pre- and post-copy walks. On a moderately deep hierarchy this is a real jank/dropped-frame risk on the customer's UI thread, imposed precisely during the animations this fix exists to support — a resource-discipline regression an SDK embedded in someone else's app should not introduce.
Suggested fix
Bound the cost of draw-time verification instead of re-running the full walk on every intervening frame: e.g. sample at most once (or a small fixed number of times) per capture window, reuse the existing throttler pattern to rate-limit draw-time samples, or track a cheaper proxy signal (e.g. only recheck if a layout pass or a hasTransientState-style flag changed) and only fall back to a full re-walk when that cheap signal fires. At minimum, avoid running the Compose semantics pass on every draw sample — it is the most expensive part of the walk and is being invoked synchronously on the main thread now.
Prompt to fix with AI (copy-paste)
## Context
@posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt#L230-244
@posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt#L1126-1146
<issue_description>
NextDrawListener.onDraw() (internal/NextDrawListener.kt) invokes the registered draw callback on every single frame the decor view draws — unthrottled: `override fun onDraw() { onDrawCallback(); throttler.throttle { onDrawThrottlerCallback() } }`. This PR wires the new `onDrawCallback(view, drawState)` into that per-draw hook (replacing the old cheap `onDrawCallback(decorView)`). Inside it, `drawState.beginDrawSample()` returns a non-null token for the whole window a screenshot capture is active — from `beginMaskCapture()` right before `PixelCopy.request()` until `finishMaskCapture()`/`cancelMaskCapture()` fires in the callback or the outer timeout `finally` (which can stretch to the full 1s PixelCopy latch timeout on a busy device). Every draw that lands inside that window triggers a full `findMaskableWidgets(view, walk)` recursive tree walk synchronously on the calling thread — which is the **main UI thread**, since `onDraw()` is a `ViewTreeObserver.OnDrawListener` callback. Worse, the new Compose branch explicitly runs the semantics traversal (`getAllSemanticsNodes(true)`) **inline** on that same thread when already on main ("Draw-time verification already runs on main, where posting and waiting would deadlock, so execute inline in that case", lines ~1126-1131), instead of the previous post-and-await pattern. Because this PR's entire purpose is to keep capturing screens that redraw many times per second (spinners, GIFs, Lottie, Compose animations), that is exactly the population of screens where `onDraw()` fires repeatedly during the capture window — so a single screenshot attempt can spend dozens of extra full-tree (and full Compose-semantics) walks on the host app's main thread, purely to catch the rare case of geometry moving and reverting between the pre- and post-copy walks. On a moderately deep hierarchy this is a real jank/dropped-frame risk on the customer's UI thread, imposed precisely during the animations this fix exists to support — a resource-discipline regression an SDK embedded in someone else's app should not introduce.
</issue_description>
<issue_validation>
- **Checked:** `NextDrawListener.onDraw()` (`NextDrawListener.kt:20-24`), the registration wiring (`PostHogReplayIntegration.kt:272`), the walking overload (`:230-244`), `beginDrawSample` gating (`ViewTreeSnapshotStatus.kt:77-83`), the capture-window boundaries (`beginMaskCapture` at `:1243-1247` before `PixelCopy.request`; `finish`/`cancel` at `:1282`, `:1254`, `:1335`; 1s latch at `:1329`), and the inline Compose path (`:1126-1146`).
- **Found:** `onDraw()` invokes the walking overload unthrottled on every frame (only the separate `onDrawThrottlerCallback` is throttled). Whenever a capture is active, `beginDrawSample()` returns non-null and the overload runs a full recursive `findMaskableWidgets(view, walk)` synchronously on the main/UI thread; the active window spans `beginMaskCapture` → `finish/cancel`, bounded only by PixelCopy completion or the 1s timeout. For Compose views the walk runs `getAllSemanticsNodes(true)` inline on that same main thread by design (`:1128-1131`).
- **Found:** This is a newly introduced cost. The prior draw callback only set a flag — `recordDraw()` toggles `isOnDrawnCalled` and bumps a generation counter (`ViewTreeSnapshotStatus.kt:48-53`), with no tree walk. So the PR adds synchronous full-tree + Compose-semantics walks onto the main-thread draw path.
- **Impact:** During the continuously-animating screens this PR exists to keep capturing (spinners, GIFs, Lottie, Compose infinite animations), frames draw at display rate, so any draw that lands inside a capture window triggers a full view-tree walk plus an inline Compose semantics merge on the host app's UI thread, unthrottled — a plausible jank/dropped-frame regression on the customer's main thread, worst on deep hierarchies and slow-PixelCopy devices. Concrete trigger + concrete consequence, and a real regression versus the previous flag-only callback → meets the performance / SDK resource-discipline keep bar.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Bound the cost of draw-time verification instead of re-running the full walk on every intervening frame: e.g. sample at most once (or a small fixed number of times) per capture window, reuse the existing throttler pattern to rate-limit draw-time samples, or track a cheaper proxy signal (e.g. only recheck if a layout pass or a `hasTransientState`-style flag changed) and only fall back to a full re-walk when that cheap signal fires. At minimum, avoid running the Compose semantics pass on every draw sample — it is the most expensive part of the walk and is being invoked synchronously on the main thread now.
</potential_solution>
There was a problem hiding this comment.
this is somewhat existential in that this is the entire point of this change
There was a problem hiding this comment.
that's not good, I'm letting Claude do a performance review.
| drawState.reset() | ||
| val drawGenerationBeforePreWalk = drawState.currentDrawGeneration() | ||
|
|
||
| // Sampled before the pixels freeze. Draw-time walks remain active until the post-copy | ||
| // walk, so geometry that changes and returns between these endpoints also invalidates it. | ||
| val preWalk = MaskWalk() | ||
| try { | ||
| findMaskableWidgets(view, preWalk) | ||
| } catch (e: Throwable) { | ||
| config.logger.log("Session Replay mask walk failed: $e.") | ||
| preWalk.poisoned = true | ||
| } | ||
|
|
||
| if (preWalk.poisoned) { | ||
| config.logger.log("Session Replay screenshot discarded due to screen changes.") | ||
| return null | ||
| } | ||
|
|
||
| val captureToken = | ||
| drawState.beginMaskCapture( | ||
| preWalk.rects, | ||
| drawGenerationBeforePreWalk, | ||
| ) |
There was a problem hiding this comment.
beginMaskCapture's drawGeneration check invalidates virtually every capture on continuously-animating screens, silently reintroducing the discard bug this PR exists to fix
Why we think it's a valid issue
- Checked: The generation sampling/arming sequence in
toScreenshotWireframe(PostHogReplayIntegration.kt:1226,:1232,:1243-1247),beginMaskCapture/recordDraw/finishMaskCapture(ViewTreeSnapshotStatus.kt:48-53,:61-75,:109-126), the draw-listener wiring (:227,:234,:272), the keep gate (:1287), and the threading (executor is background —:137-138;generateSnapshotsubmitted at:278; comment at:112confirms 'Main-thread writes race the capture executor's reads'). - Found:
drawGenerationBeforePreWalkis sampled BEFORE the preWalk, andbeginMaskCapturesetsinvalid = drawGeneration != expectedDrawGeneration(ViewTreeSnapshotStatus.kt:71).recordDraw()bumpsdrawGenerationon everyonDraw, unconditionally (:48-53). Since the preWalk runs on the background executor while the main thread keeps animating, any redraw during the preWalk advances the counter, so the capture is marked invalid at birth — andfinishMaskCapturereturns false wheneverinvalid(:120), which forcescaptureAligned=falseand discards the frame regardless ofpreWalk.rects == postWalk.rects. - Found: Layout-changing draws are already caught by
didLayoutSinceReset(:102-107, checked inshouldKeepFrame/finishMaskCapture), so the generation check's marginal effect is to discard pixel-only redraws during the preWalk — precisely the frames the PR exists to keep. For Compose the preWalk includes a main-thread round trip (:1126-1140); on an animating Compose screen a draw during that window is near-certain, so nearly every capture is invalidated. - Found: No test exercises a draw landing during the preWalk (before
beginMaskCapture); the 'kept when redraws leave geometry untouched' test (PostHogReplayIntegrationTest.kt:1864-1882) injects the draw inside the PixelCopy callback — after arming — so this path is uncovered. - Impact: On continuously-animating screens (the PR's headline target, spinners/GIF/Lottie/Compose), a stable-geometry pixel-only redraw during the preWalk discards the frame with 'Session Replay screenshot discarded due to screen changes' — partially reintroducing the exact bug the PR claims to fix, severely so for Compose. Real correctness/efficacy defect on the changed code with a concrete trigger and consequence → keep.
Issue description
toScreenshotWireframe samples drawGenerationBeforePreWalk = drawState.currentDrawGeneration() before running the full recursive preWalk (findMaskableWidgets, which can include an up-to-1s Compose semantics round trip), then calls drawState.beginMaskCapture(preWalk.rects, drawGenerationBeforePreWalk). Inside WindowDrawState.beginMaskCapture (ViewTreeSnapshotStatus.kt, new WindowDrawState class), the new ActiveMaskCapture is constructed with invalid = drawGeneration != expectedDrawGeneration — i.e. if ANY draw occurred anywhere between the generation sample and this call, the capture is marked invalid from birth, unconditionally. drawGeneration is bumped by recordDraw() on every single onDraw() pass of the window (unthrottled), regardless of whether a capture is active. On the PR's own primary target scenario — a screen with a continuously-animating spinner/GIF/Lottie/Compose animation redrawing at ~60fps — the window keeps drawing throughout the entire preWalk. Because the preWalk is a full tree walk (and can itself invoke findMaskableComposeWidgets with a real main-thread round trip), it will very commonly take longer than one frame interval (~16ms), especially on non-trivial view hierarchies or busy devices. That means at least one draw will almost always land between the generation sample and beginMaskCapture on exactly the animating screens this PR is meant to rescue, so invalid becomes true immediately — and finishMaskCapture() unconditionally returns false whenever capture.invalid is true, regardless of whether preWalk.rects and postWalk.rects actually match. Since the call site is if (captureAligned && shouldKeepFrame(...)), a false captureAligned discards the frame no matter what shouldKeepFrame would have concluded from the (still-accurate) rect comparison. The net effect: on real devices, continuously-animating screens can still hit "Session Replay screenshot discarded due to screen changes." on effectively every capture attempt — the exact behavior this PR's title and changeset claim to have fixed — via a new code path that none of the described unit tests exercise, because the PR's own E2E tests only inject a state change "between the pre- and post-copy walks" (i.e. after beginMaskCapture already armed), not a real draw firing during the preWalk itself, which Robolectric's synchronous test execution model wouldn't produce spontaneously anyway.
Suggested fix
Don't equate "a draw happened during the preWalk" with "the mask geometry is now misaligned." Either (a) sample drawGenerationBeforePreWalk immediately before calling beginMaskCapture (i.e. right after the preWalk completes) so the check only catches a genuine race at the arm point rather than the entire preWalk duration, or (b) route draws that land during the preWalk through the same rect-comparison verification used for the post-arm window (re-walk and compare against preWalk.rects, only marking invalid on an actual rect mismatch) instead of the coarse generation-counter check. Add a Robolectric/E2E regression test that calls drawState.recordDraw() (simulating a pixel-only animation frame) while findMaskableWidgets is conceptually 'in progress' for the preWalk — e.g. via a hook view whose isVisible()/mask-rect accessor triggers a recordDraw() call mid-walk — and assert the frame is still kept when the geometry is unchanged, mirroring the existing 'dirty-but-stable frame kept' test but targeting the pre-walk window specifically instead of only the pre/post-walk gap.
Prompt to fix with AI (copy-paste)
## Context
@posthog-android/src/main/java/com/posthog/android/replay/PostHogReplayIntegration.kt#L1225-1247
<issue_description>
toScreenshotWireframe samples drawGenerationBeforePreWalk = drawState.currentDrawGeneration() before running the full recursive preWalk (findMaskableWidgets, which can include an up-to-1s Compose semantics round trip), then calls drawState.beginMaskCapture(preWalk.rects, drawGenerationBeforePreWalk). Inside WindowDrawState.beginMaskCapture (ViewTreeSnapshotStatus.kt, new WindowDrawState class), the new ActiveMaskCapture is constructed with `invalid = drawGeneration != expectedDrawGeneration` — i.e. if ANY draw occurred anywhere between the generation sample and this call, the capture is marked invalid from birth, unconditionally. drawGeneration is bumped by recordDraw() on every single onDraw() pass of the window (unthrottled), regardless of whether a capture is active. On the PR's own primary target scenario — a screen with a continuously-animating spinner/GIF/Lottie/Compose animation redrawing at ~60fps — the window keeps drawing throughout the entire preWalk. Because the preWalk is a full tree walk (and can itself invoke findMaskableComposeWidgets with a real main-thread round trip), it will very commonly take longer than one frame interval (~16ms), especially on non-trivial view hierarchies or busy devices. That means at least one draw will almost always land between the generation sample and beginMaskCapture on exactly the animating screens this PR is meant to rescue, so `invalid` becomes true immediately — and finishMaskCapture() unconditionally returns false whenever `capture.invalid` is true, regardless of whether preWalk.rects and postWalk.rects actually match. Since the call site is `if (captureAligned && shouldKeepFrame(...))`, a false captureAligned discards the frame no matter what shouldKeepFrame would have concluded from the (still-accurate) rect comparison. The net effect: on real devices, continuously-animating screens can still hit "Session Replay screenshot discarded due to screen changes." on effectively every capture attempt — the exact behavior this PR's title and changeset claim to have fixed — via a new code path that none of the described unit tests exercise, because the PR's own E2E tests only inject a state change "between the pre- and post-copy walks" (i.e. after beginMaskCapture already armed), not a real draw firing *during* the preWalk itself, which Robolectric's synchronous test execution model wouldn't produce spontaneously anyway.
</issue_description>
<issue_validation>
- **Checked:** The generation sampling/arming sequence in `toScreenshotWireframe` (`PostHogReplayIntegration.kt:1226`, `:1232`, `:1243-1247`), `beginMaskCapture`/`recordDraw`/`finishMaskCapture` (`ViewTreeSnapshotStatus.kt:48-53`, `:61-75`, `:109-126`), the draw-listener wiring (`:227`, `:234`, `:272`), the keep gate (`:1287`), and the threading (executor is background — `:137-138`; `generateSnapshot` submitted at `:278`; comment at `:112` confirms 'Main-thread writes race the capture executor's reads').
- **Found:** `drawGenerationBeforePreWalk` is sampled BEFORE the preWalk, and `beginMaskCapture` sets `invalid = drawGeneration != expectedDrawGeneration` (`ViewTreeSnapshotStatus.kt:71`). `recordDraw()` bumps `drawGeneration` on every `onDraw`, unconditionally (`:48-53`). Since the preWalk runs on the background executor while the main thread keeps animating, any redraw during the preWalk advances the counter, so the capture is marked invalid at birth — and `finishMaskCapture` returns false whenever `invalid` (`:120`), which forces `captureAligned=false` and discards the frame regardless of `preWalk.rects == postWalk.rects`.
- **Found:** Layout-changing draws are already caught by `didLayoutSinceReset` (`:102-107`, checked in `shouldKeepFrame`/`finishMaskCapture`), so the generation check's *marginal* effect is to discard pixel-only redraws during the preWalk — precisely the frames the PR exists to keep. For Compose the preWalk includes a main-thread round trip (`:1126-1140`); on an animating Compose screen a draw during that window is near-certain, so nearly every capture is invalidated.
- **Found:** No test exercises a draw landing during the preWalk (before `beginMaskCapture`); the 'kept when redraws leave geometry untouched' test (`PostHogReplayIntegrationTest.kt:1864-1882`) injects the draw inside the PixelCopy callback — after arming — so this path is uncovered.
- **Impact:** On continuously-animating screens (the PR's headline target, spinners/GIF/Lottie/Compose), a stable-geometry pixel-only redraw during the preWalk discards the frame with 'Session Replay screenshot discarded due to screen changes' — partially reintroducing the exact bug the PR claims to fix, severely so for Compose. Real correctness/efficacy defect on the changed code with a concrete trigger and consequence → keep.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Don't equate "a draw happened during the preWalk" with "the mask geometry is now misaligned." Either (a) sample drawGenerationBeforePreWalk immediately before calling beginMaskCapture (i.e. right after the preWalk completes) so the check only catches a genuine race at the arm point rather than the entire preWalk duration, or (b) route draws that land during the preWalk through the same rect-comparison verification used for the post-arm window (re-walk and compare against preWalk.rects, only marking invalid on an actual rect mismatch) instead of the coarse generation-counter check. Add a Robolectric/E2E regression test that calls drawState.recordDraw() (simulating a pixel-only animation frame) while findMaskableWidgets is conceptually 'in progress' for the preWalk — e.g. via a hook view whose isVisible()/mask-rect accessor triggers a recordDraw() call mid-walk — and assert the frame is still kept when the geometry is unchanged, mirroring the existing 'dirty-but-stable frame kept' test but targeting the pre-walk window specifically instead of only the pre/post-walk gap.
</potential_solution>
3 participants
💡 Motivation and Context
Closes #596.
In screenshot mode, a frame was discarded whenever its window redrew during the PixelCopy capture, unless an animation-type heuristic matched (
hasTransientStatefrom #529, surface/texture views from #649). Most animated content matches neither signal: indeterminate spinners (ProgressDialog), animated GIFs (Glide/Coil), Lottie (which never sets transient state, so the #529 exemption structurally could not fire for it), Material progress indicators, and Compose infinite animations all redraw per frame on the UI thread. On screens showing any of them, essentially every capture logged "Session Replay screenshot discarded due to screen changes" and the replay showed nothing. On top of that, the draw-dirty flags were single fields shared across all tracked windows, so an animating loader dialog also blanked captures of the static activity behind it.The guard exists for a real reason (#254 / #234): mask rects are computed from live views after the pixels are frozen, so a structural change mid-capture can drift masks off sensitive content. This PR keeps that protection but stops proxying it with "did anything redraw":
isOnDrawnCalled/didLayoutSinceResetmove into aWindowDrawStateonViewTreeSnapshotStatus. PixelCopy copies a single window's surface and masks come from that window's own tree, so one window's draws say nothing about another window's mask alignment.PixelCopy.requestand again in the callback. A dirty frame is kept only when both walks agree, no layout pass ran, and neither walk was poisoned. Pixel-only animation redraws pass this check no matter which library drives them; structural changes still discard. ThehasTransientState/surface-view exemptions are deleted: frames they legitimately kept have stable geometry and pass rect equality anyway, and frames they kept with moving masked geometry were unsafe to keep at all.view.animation, transient state mid-animation), and when the Compose semantics pass times out. Previously such views were silently pruned from the walk, which would have shipped them unmasked. A timed-out PixelCopy latch also no longer ships the bitmap before masks are painted.Behavior is monotone for safety: no frame that was previously discarded for a genuine structural change is now kept, and the discard log line is unchanged for support diagnostics. The default wireframe mode is untouched, and the Flutter/RN forced-screenshot bridge inherits the fix through the same path.
💚 How did you test it?
generateSnapshot+ShadowPixelCopy, using a hook view that injects state changes exactly between the pre- and post-copy walks: dirty-but-stable frame kept (the fix path), masked widget moved mid-capture discarded, layout mid-capture discarded, masked view mid legacy animation fails closed (regression test for the walk-pruning hole), and a redraw+layout in another window no longer discards this window's capture (fails under the old shared-flag code).posthog-androidunit test suite,apiCheck, andspotlessCheckpass locally.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Autonomy: Human-driven (agent-assisted)
ProgressBar/AnimatedVectorDrawable/ViewRootImpl), and the fix history of chore: do not capture screenshot during screen changes #254/fix: stop screenshot frames being dropped during animations #529/fix(replay): keep frames on screens with continuous surface rendering (e.g. Rive) #649, then adversarially verified claim by claim.setHasTransientStateis only called byViewPropertyAnimator,Editor,Transition, and view-translation in the framework, so the fix: stop screenshot frames being dropped during animations #529 exemption never fired for Lottie or any drawable-level animation; sibling SDKs (posthog-ios, Sentry Android) avoid this bug class by sampling masks synchronously with the frame, which is what the rect pre/post verification approximates while keeping PixelCopy off the main thread.Created with PostHog Code