Skip to content

fix(session-replay): derive the replay meta viewport from the captured frame - #1002

Open
elliotrfeinberg wants to merge 3 commits into
masterfrom
session-replay-meta-viewport-from-captured-frame
Open

fix(session-replay): derive the replay meta viewport from the captured frame#1002
elliotrfeinberg wants to merge 3 commits into
masterfrom
session-replay-meta-viewport-from-captured-frame

Conversation

@elliotrfeinberg

@elliotrfeinberg elliotrfeinberg commented Aug 28, 2026

Copy link
Copy Markdown

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:

source includes nav bar?
Meta viewport DeviceInfo.screenWidth/HeightResources.getSystem().displayMetrics no — window-independent global metrics
Screenshot ScreenRecorder.calculateBitmapScaleview.width/height ÷ density on the window decorView yes — the full window

SessionReplayEncoder.jsonPayload built 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 frame width: 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.heightPixels excludes the navigation bar but the window decorView spans 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. jsonPayload no 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: _saveSnapshotToQueue compares against _lastMetadataDimensions and enqueues a metadata event sharing the screenshot's timestamp, and payload_serializer.dart emits 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 deletes CapturedViewport outright — the object, its cross-thread @Volatile state and its replay-id tagging — and decouples the viewport from which batch or replay id ends up shipping the frames.

ScreenRecorder.captureScreenshot now returns a CapturedFrame (bytes plus dimensions) instead of a bare ByteArray. 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:lint344 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 in EventServiceTests cover eviction retaining the viewport and staying bounded while doing so.

Both are mutation-checked rather than assumed:

  • Making the meta emission a no-op fails 7 of the 8 viewport tests. The 8th (a failed capture does not describe the viewport) passes vacuously, as it should.
  • Removing the eviction re-statement fails testEvictionKeepsTheViewportInFrontOfSurvivingFrames and nothing else.

Not covered by tests

Nothing asserts that ScreenRecorder populates the dimensions on a real capture — that path needs a View attached to a window, so it belongs in the instrumented suite, and no emulator was available locally. The Session Replay CI workflow runs :session-replay:connectedDebugAndroidTest on 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.

…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>
@greptile-apps

greptile-apps Bot commented Aug 28, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

The 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

Important Files Changed

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

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Test Results

 37 files   37 suites   2m 54s ⏱️
187 tests 187 ✅ 0 💤 0 ❌
389 runs  389 ✅ 0 💤 0 ❌

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>
@tylerjroach

tylerjroach commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

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 SessionEventData.DimensionData(DeviceInfo.screenWidth, DeviceInfo.screenHeight). Overall this is a good find, but I think the implementation of this could be simplified.

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>
@elliotrfeinberg

Copy link
Copy Markdown
Author

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: payload_serializer.dart emits no per-batch meta at all, so the queued metadata events are the whole story. Android now matches — the meta is queued from the captured frame, immediately ahead of it, only when the dimensions change or a replay begins, and jsonPayload no longer synthesizes one.

It lands in EventHandler.receivedScreenshotEvent, which already runs on a serial executor and already enqueues the frame. That deletes CapturedViewport entirely, cross-thread state and all.

On DeviceInfo — the frame is captured from the window decorView while DeviceInfo reads Resources.getSystem().displayMetrics, which is window-independent and excludes the navigation bar. The two only agree on edge-to-edge activities, which is why this reproduces on some devices and not others.

Two things your comment could not have seen from the Flutter side:

Eviction. Moving the meta into the queue exposes it to EventService.evictEvents, which drops from the head under memory pressure. A cut between a meta and its frames leaves those frames with no dimensions, so the most recent evicted meta is re-stated at the head. It also cannot count towards the eviction quota — otherwise the next eviction drops and re-adds it, frees no room, and the queue grows past its limit. Flutter never hits this, having a SQLite-backed queue.

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()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@elliotrfeinberg

elliotrfeinberg commented Aug 28, 2026

Copy link
Copy Markdown
Author

Open question blocking ready-for-review — does the replay player retain the viewport across batches within a replay?

@tylerjroach

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 SessionReplayEncoder.jsonPayload. After it, meta events are queued alongside the frames they describe and emitted only when the dimensions change or a replay begins — so a replay's second and later batches usually carry no meta event at all.

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:

  1. Confirmation that the player treats a replay's events as one continuous stream across batches, so a batch with no meta inherits the previous viewport.
  2. A pointer to whoever owns the player, if that is not someone already on this PR.

Happy to keep this in draft until then. Everything else here is green — CI passes on ce9cfdec including the API 21/30/34 emulator matrices, and the unit suite is 344 passing.

— Assisted by Claude

@elliotrfeinberg
elliotrfeinberg marked this pull request as ready for review August 31, 2026 19:27
@elliotrfeinberg
elliotrfeinberg requested a review from a team August 31, 2026 19:27
@tylerjroach

Copy link
Copy Markdown
Contributor

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).

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 tylerjroach left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants