fix(session-replay): derive the replay meta viewport from the captured frame - #1002
fix(session-replay): derive the replay meta viewport from the captured frame#1002elliotrfeinberg wants to merge 3 commits into
Conversation
…d frame The meta event described Resources.getSystem().displayMetrics while the screenshot is captured from the window decorView. Those disagree on a non-edge-to-edge activity, where the system metrics exclude the navigation bar: the frame is taller than the viewport the player scales it to, so the bottom of the app UI is cropped out of the recording. Record the dimensions of each captured frame and build the meta event from them, falling back to the system metrics before the first capture. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Confidence Score: 4/5The PR needs the cross-replay in-flight capture race fixed before merging because a new replay can still open without its own viewport metadata. A screenshot started by the previous recording can complete after the new replay resets viewport tracking, repopulate the shared dimension state, and suppress metadata for the new replay’s first same-sized frame. Files Needing Attention: session-replay/src/main/java/com/mixpanel/android/sessionreplay/MPSessionReplayInstance.kt; session-replay/src/main/java/com/mixpanel/android/sessionreplay/tracking/EventHandler.kt
|
| Filename | Overview |
|---|---|
| session-replay/src/main/java/com/mixpanel/android/sessionreplay/MPSessionReplayInstance.kt | Propagates captured dimensions and resets viewport tracking, but does not prevent a prior replay's in-flight capture from repopulating the reset state. |
| session-replay/src/main/java/com/mixpanel/android/sessionreplay/tracking/EventHandler.kt | Emits dimension metadata alongside frames and deduplicates unchanged dimensions. |
| session-replay/src/main/java/com/mixpanel/android/sessionreplay/services/EventService.kt | Retains viewport metadata during bounded-queue eviction. |
| session-replay/src/main/java/com/mixpanel/android/sessionreplay/tracking/ScreenRecorder.kt | Returns compressed frame bytes together with the final bitmap dimensions. |
| session-replay/src/main/java/com/mixpanel/android/sessionreplay/utils/MPSessionReplayEncoder.kt | Serializes queued metadata rather than synthesizing system-display dimensions per batch. |
Reviews (3): Last reviewed commit: "Queue the meta event with the frame it d..." | Re-trigger Greptile
Test Results 37 files 37 suites 2m 54s ⏱️ Results for commit ce9cfde. ♻️ This comment has been updated with latest results. |
The screen recorder outlives any one replay, so a replay that flushed a batch before its own first capture described itself with the previous replay's dimensions instead of falling back to the system metrics. After an orientation change between replays those dimensions are wrong. Captures now carry the replay they belong to, and a payload reads them only when the ids match. Keying rather than clearing matters because stopRecording flushes the finished replay asynchronously: its final batch can be built after the next replay has started, and it still describes its own frames. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
What if we were to just keep the previous w/h of the last frame. When it comes time to send the next frame, check the w/h and if it differs send a new metadata event + new screenshot event. I'm surprised to see the code using This looks like the proper implementation. The screen can change at any time (ex: rotation). Flutter implementation checks each snapshot for the screen size, and if a change is detected, places a new metadata event in the queue. It looks like the Android implementation is flawed in that it tries to send a metadata with each batch, which would still contain different screen sizes within that path. https://github.com/mixpanel/mixpanel-flutter/blob/main/packages/mixpanel_flutter_session_replay/lib/src/internal/event_recorder.dart#L124 |
A batch carried one synthesized meta event built from the system display metrics, which exclude the navigation bar on non-edge-to-edge activities and so understate the captured frame. The player sizes its iframe to that viewport, so an over-tall frame was clipped at the bottom, taking fixed bottom UI such as a tab bar with it. Describing the batch cannot be right in general: a batch spans up to 50 events and can cross a rotation, so its frames have two sizes and one meta can only state one of them. The meta now goes into the event queue from the captured frame, immediately ahead of that frame, and only when the dimensions change or a replay begins. This matches the Flutter SDK, whose serializer likewise emits no per-batch meta. Queueing it in EventHandler, which already runs on a serial executor, removes CapturedViewport along with its cross-thread state and its replay-id tagging. It also decouples the viewport from which batch or replay id ships the frames. Eviction has to keep the viewport in front of the frames that survive it, so the most recent evicted meta is re-stated at the head of the queue. It does not count towards the eviction quota, or eviction would free no room and the queue would grow. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Agreed, and thanks — this is the right call. Rewritten on your design in ce9cfde. You were right about more than the simplification. A batch holds up to 50 events and can cross a rotation, so its frames can have two sizes and a single meta per batch can only state one of them. Describing the batch is not something that can be made correct, which makes the previous approach in this PR the wrong shape rather than just a heavier one. We followed the Flutter implementation you linked. Worth noting it goes one step further than your description: It lands in On Two things your comment could not have seen from the Flutter side: Eviction. Moving the meta into the queue exposes it to The wire format. Batches after the first now carry no meta unless the dimensions changed, which is only safe if the player retains the viewport across batches within a replay. Flutter shipping this way is good evidence, but could you confirm, or point us at whoever owns the player? Today's bug crops frames; getting this wrong would break sizing outright. It is the one thing we would want settled before this leaves draft. 344 unit tests pass, lint and ktlint clean. The viewport and eviction tests are mutation-checked — neutering the meta emission fails 7 of 8 viewport tests, and removing the eviction re-statement fails exactly one test. — Assisted by Claude |
|
|
||
| scheduleScreenshotCapture() | ||
| flushService.start() | ||
| eventService.resetViewportTracking() |
There was a problem hiding this comment.
Stale capture suppresses viewport metadata
If a screenshot from the previous recording remains in flight when the next recording starts, it can complete after resetViewportTracking() and restore lastMetaDimensions. When the new replay's first frame has the same dimensions, its opening meta event is suppressed, leaving that replay without its own viewport metadata.
Knowledge Base Used:
There was a problem hiding this comment.
resetViewportTracking just nulls out the dimensions. This means the next frame would set the proper dimensions again. Worst case is an extra metadata call right? This is already and edge case and no adverse impacts to this scenario here.
"Its opening meta event is suppressed, leaving that replay without its own viewport metadata." I don't think thats accurate? If we don't have a dimension, we immediately capture and queue a metadata event right before the frame event.
There was a problem hiding this comment.
You're right — I overstated the risk. resetViewportTracking() is serialized on the same executor as screenshot events, so when it runs before the new frame, lastMetaDimensions is null and that frame queues its own meta immediately before the frame.
In the stale-capture interleaving, the old capture may run after the reset and restore the dimensions, so the new frame can suppress a duplicate meta. But that stale capture has already queued a valid meta immediately before its own frame; if both remain in the queue, the new replay still has viewport metadata, and if the dimensions differ the new frame emits a second meta. The eviction logic also re-states a retained meta when needed.
So the specific claim that the replay can be left without viewport metadata is inaccurate. At most this changes whether an otherwise redundant meta is emitted. I’ll consider this comment resolved.
|
Open question blocking ready-for-review — does the replay player retain the viewport across batches within a replay? Pulling this out of the thread above so it is easier to answer on its own, since it is the one thing we cannot settle from the SDK side. Before this PR, every batch carried a meta event, synthesized in That is safe if and only if the player carries the viewport forward from the previous batch of the same replay. If it instead expects a meta at the head of every batch, this would break sizing outright, which is worse than the bug being fixed here (today's bug only crops the bottom of the frame). Evidence that it is safe, but not proof:
Two things would settle it:
Happy to keep this in draft until then. Everything else here is green — CI passes on — Assisted by Claude |
Confirming that if its the same replayId, were simply appending a stream of new events. If the dimensions haven't changed, we don't need to send a new metadata event. |
tylerjroach
left a comment
There was a problem hiding this comment.
Approving this from a PR perspective but don't merge yet. We need to manually test this on a few devices with and without frames because we don't have integration tests that can handle this.
TL;DR — On some Android devices the SDK told the replay player the screen was shorter than the screenshot it actually sent, so the player cropped the bottom slice off every frame, taking fixed bottom UI such as a tab bar with it. The dimensions now travel with the frame they describe, so nothing gets cut off.
The bug
Two dimension sources disagree:
DeviceInfo.screenWidth/Height→Resources.getSystem().displayMetricsScreenRecorder.calculateBitmapScale→view.width/height ÷ densityon the windowdecorViewSessionReplayEncoder.jsonPayloadbuilt the meta event from the first while the frame came from the second. The player fixes its iframe to the meta viewport and the snapshot template styles the framewidth: 100%; height: auto, so an over-tall frame overflows and is clipped at the bottom — exactly where a fixed bottom tab bar lives.This shows up on non-edge-to-edge activities, where
Resources.getSystem().displayMetrics.heightPixelsexcludes the navigation bar but the windowdecorViewspans it. How large the discrepancy is depends on navigation mode and OEM inset behaviour, which is why it reproduces on gesture navigation on some devices and not on others.The fix
The meta event is queued into the event stream from the captured frame, immediately ahead of that frame, and only when the dimensions change or a replay begins.
jsonPayloadno longer synthesizes one per batch.Per @tylerjroach's review — a batch holds up to 50 events and can cross a rotation, so its frames can have two sizes and a single meta per batch can only state one of them. Describing the batch is not a thing that can be made correct; describing the frame is. This is what mixpanel-flutter does:
_saveSnapshotToQueuecompares against_lastMetadataDimensionsand enqueues a metadata event sharing the screenshot's timestamp, andpayload_serializer.dartemits no per-batch meta at all.The work happens in
EventHandler.receivedScreenshotEvent, which already runs on a serial executor and already enqueues the frame. That placement deletesCapturedViewportoutright — the object, its cross-thread@Volatilestate and its replay-id tagging — and decouples the viewport from which batch or replay id ends up shipping the frames.ScreenRecorder.captureScreenshotnow returns aCapturedFrame(bytes plus dimensions) instead of a bareByteArray. The dimensions are read off the bitmap before it goes back to the pool, after sub-window compositing, so they describe the frame that actually ships.Eviction
Moving the meta into the queue exposes it to
EventService.evictEvents, which drops from the head under memory pressure. A cut landing between a meta and its frames would leave those frames with no dimensions, so the most recent evicted meta is re-stated at the head of the queue.That re-stated meta must not count towards the eviction quota. If it did, the next eviction would drop it and immediately re-add it, freeing no room, and the queue would grow without bound past its limit. It costs one slot, so the queue settles at
queueSizeLimit + 1.Testing
./gradlew :session-replay:ktlintCheck :session-replay:test :session-replay:lint— 344 tests, 0 failures, lint and ktlint clean.ViewportMetaEventTest(8 tests, Robolectric,@Config(sdk = [28])) covers the first frame of a replay, a frame taller than the system metrics report, timestamp sharing, an unchanged size not repeating the meta, a rotation emitting a second meta mid-batch, a new replay re-stating the viewport, a failed capture describing nothing, and incremental frames. Two tests inEventServiceTestscover eviction retaining the viewport and staying bounded while doing so.Both are mutation-checked rather than assumed:
a failed capture does not describe the viewport) passes vacuously, as it should.testEvictionKeepsTheViewportInFrontOfSurvivingFramesand nothing else.Not covered by tests
Nothing asserts that
ScreenRecorderpopulates the dimensions on a real capture — that path needs aViewattached to a window, so it belongs in the instrumented suite, and no emulator was available locally. The Session Replay CI workflow runs:session-replay:connectedDebugAndroidTeston API 21 / 30 / 34, which exercises the capture path.Worth a second opinion before merge
This changes the wire format: batches after the first carry no meta unless the dimensions changed. That is safe only if the player retains the viewport across batches within a replay. mixpanel-flutter already ships this way, which is good evidence, but someone who owns the player should confirm — today's bug crops frames, whereas getting this wrong would break sizing outright.
Rollout
Behaviour is unchanged wherever the two sources already agree, which is the overwhelming majority of sessions. Once released, affected recordings render at their true size.