[wagenet]

The bug

A WHIP session runs in its own pipeline and is bridged into the flow's main pipeline through an appsink/appsrc pair. The bridge rebased the PTS of buffers that carried one and forwarded the rest untouched:

if offset_ns != 0 {
    if let Some(pts_val) = pts { /* restamp, push */ }
    else { let _ = appsrc.push_sample(&sample); }   // forwarded UNSTAMPED
} else {
    let _ = appsrc.push_sample(&sample);            // also unstamped
}

Downstream of that appsrc sits a recorder, and qtmux cannot place an untimestamped buffer in a track:

ERROR Could not multiplex stream.
  gstqtmux.c(5927): gst_qt_mux_add_buffer ():
  /GstSplitMuxSink:rec_p2:splitmuxsink/GstMP4Mux:rec_p2:mux:
  Buffer has no PTS.

The resulting GST_FLOW_ERROR propagates out of the recorder and up through the mixer, so one seat's stray buffer takes the whole flow down. The same incident logged errors from whip_p2:appsrc_audio_0, rec_p2:audio_0_queue, vmix:queue_1, vmix:appsrc_overlay and others. Observed on a live rig: a second participant joining killed the first.

The Media Player carried a second copy of that same code, down to the else-branch, in a bridge whose output reaches the same qtmux.

The fix

Drop the buffer at the bridge. The new_sample closure moves into gst::pipeline_bridge::SessionBridge, which owns the per-session timestamp offset and the drop count, and both blocks call it.

Why drop rather than stamp from the pipeline clock. There is no correct value to invent. The main pipeline's running time now is a different time base from pts + offset, and the two drift apart over a session, so a synthesised stamp can land before its own predecessor — and a muxer rejects backwards timestamps exactly as hard as missing ones. That is the same cascade by another route. A stamp that is merely a little wrong is worse still: it desynchronises audio from video for the rest of the session, silently. A dropped frame on a live source is the ordinary recoverable failure — the receiver already tolerates loss, and the keyframe_request machinery already in this file recovers a decoder that lost part of its GOP.

Not silent. Drops are counted per session and warned on the first, then every hundredth, so a publisher sending nothing usable stays visible without writing a line per frame.

One offset per session, enforced. Audio and video reach the bridge on their own streaming threads and can both find the offset unset, read the clock milliseconds apart, and store different offsets — the A/V drift the shared offset exists to prevent. compare_exchange lets exactly one win; the loser reads back the winner's value.

Three smaller things in the same code, fixed by the move:

• The shared A/V offset is now taken from the first buffer that carries a PTS, not the first buffer of the session.
• DTS is shifted with PTS. It is unset on WHIP traffic (WebRTC H.264 has no B-frames) and set on a file the Media Player passes through encoded; leaving a session-base DTS beside a main-base PTS is the muxer's other way to reject the buffer.
• sample.caps().unwrap() no longer panics on a sample without caps.

Bounds, deliberately not in this PR

A recorder failing should degrade that seat, not kill the mixer, the program and every other participant. That is a larger piece of work than a bridge fix: the recorder needs its own bin with a bus handler that fails the seat and leaves the rest of the flow running, plus a way to surface a failed seat through the API and UI. Bundling it here would mix a two-line behavioural fix with a pipeline-topology change. Filing separately.

The clamp in shift(). A stream whose PTS base sits far behind the offset-setting stream's has its first buffers pinned at 0 — equal timestamps, which is the muxer's third complaint after missing and backwards ones. Pre-existing, unchanged here, and it needs a policy for what a stream starting before the session's time origin should be worth. Stated in the module doc.

Tests

backend/src/gst/pipeline_bridge.rs — 8 unit tests driving a real appsrc ! appsink pair, so the assertion is on what actually comes out of the sink, not on a returned enum:

• an_unstamped_buffer_never_crosses_the_bridge — three samples in, stamped/unstamped/stamped; only the two stamped ones arrive, rebased.
• an_unstamped_first_buffer_is_dropped_and_does_not_fix_the_offset — the other route in: no offset yet, so the restamping path is never reached; the buffer must still not cross, and must not poison the offset for the one that follows.
• two_streams_racing_for_the_offset_still_get_one — two threads, with a barrier inside the clock closure so both are past the unset check before either stores. Exactly one may report OffsetComputed, and both buffers must leave shifted by the same amount.
• both_streams_share_one_offset — the sequential case, one thread.
• a_missing_running_time_forwards_unadjusted_and_retries, dts_is_shifted_with_pts, a_negative_shift_clamps_to_zero, drops_are_logged_first_then_sparsely.

Two more guard the wiring, one per caller. The module tests above cannot see a caller that stops calling forward: reverting either call site to a bare push_sample left all eight green. Each of these drives its caller's real appsink callback — three buffers into a session pipeline, the middle one unstamped, and only the two stamped ones may reach the appsrc in the main pipeline:

• blocks::builtin::whip — an_unstamped_buffer_never_reaches_the_slot_appsrc. The callback moved out of the pad-added handler into install_slot_bridge so a test can install it without a whipserversrc.
• blocks::builtin::mediaplayer::bridge — an_unstamped_buffer_never_reaches_the_main_pipeline, through link_pad_through_clocksync and a real clocksync.

The appsrc/appsink rig is now pipeline_bridge::test_support, shared by all three modules. Its main-pipeline half sets async=false on the appsink and waits for PLAYING: with no base time the first buffer gets no offset, and the offset lands on whichever buffer arrives after the state change instead.

All three guards bite. Re-adding push_sample on the PTS-less path in the module fails 2 of the 8 with left: Some(None) — the unstamped buffer arriving at the sink. Reverting the compare_exchange to a plain store fails the race test with got [Some(OffsetComputed(9000000000)), Some(OffsetComputed(10000000000))] — two offsets a second apart, from the same input PTS. Reverting either call site to appsrc.push_sample(&sample) fails that caller's test on "the unstamped buffer must not be here". Real guards, not demonstrations.

Uses only appsrc/appsink from gstreamer1.0-plugins-base, already in .github/workflows/ci.yml. No element skip, nothing to add.

What I ran

• cargo test --features efp with STROM_REQUIRE_GST_PLUGINS=1 — 554 lib + 57 integration tests, all pass.
• cargo clippy --all-targets --features efp,nvidia -- -D warnings — clean. cargo fmt --all --check — clean.
• Live rig, headless on macOS, two whipclientsink publishers: both sessions' A/V pads bridged, exactly one shared ts-offset per session, zero unstamped drops on healthy traffic, zero mux errors, neither publisher killed the other.
• Live Media Player → Recorder flow, headless on macOS, passthrough mode on an H.264 + AAC file: one shared ts-offset logged per play, video and audio both muxed into every fragment, no Buffer has no PTS and no mux error. Looping the playlist recomputes the offset, which exercises reset_offset.

What I could not run. I could not drive the Meeting Rig to PLAYING on upstream/main — it needs #744 (builtin.audioenc), #749 (idle seats holding the pipeline out of PLAYING) and #750 (recorders with no data), all still open. So qtmux never ran in my live WHIP sessions and I did not reproduce the WHIP cascade end to end. The Media Player run above does put a real qtmux behind a real bridge; the WHIP guard is the unit test.

Conflicts to expect

• fix(whip): let a rejoining client take over a dead session's slot #753 rewrites the first lines of this same new_sample closure (watchdog store to activity.touch()). Small mechanical conflict, whichever lands second.
• fix(whip): evaluate the inactivity watchdog per poll tick, not per timeout #752 touches the adjacent watchdog block. fix(rtp): disable header extension aggregation on every depayloader #721's whip.rs hunk is elsewhere.
• A concurrent branch owns slot reuse / caps negotiation in this file. This diff stays inside the callback and the new module and does not touch slot or decode-chain setup.

🤖 Generated with Claude Code

[srperens]

Verdict: Request changes — the drop is the right call for the right reason, and the module doc is the design record this repo wants. But the invariant the module is built on ("one offset shared between audio and video") is not enforced, and the identical unstamped-forward survives in the Media Player bridge, which feeds the same recorder.

Requested changes

1. Make the offset actually once-per-session. backend/src/gst/whip_bridge.rs:115 — ` let mut offset_ns = self.offset_ns.load(Ordering::Relaxed);` — is a load, then a separate store at line 120, and the two appsink callbacks share one SessionBridge: backend/src/blocks/builtin/whip.rs:948 — ` let bridge = session_bridge.clone();`. Audio and video run on their own streaming threads, so both can read OFFSET_UNSET, read the clock a few ms apart and compute different offsets; the loser's buffers are already on the wire when the winner's store lands. compare_exchange(OFFSET_UNSET, offset, ...) and, on Err(actual), use actual — three lines, and it makes the doc true.
2. Either extend both_streams_share_one_offset to two threads, or say in the body that it covers the sequential case only. As written it calls forward twice on one thread, so it cannot see the race in (1).
3. Name the Media Player bridge, or absorb it. backend/src/blocks/builtin/mediaplayer/bridge.rs:447 — ` appsrc.push_sample(&sample)` — is the same else-branch this PR removes from whip.rs, in a bridge whose output can reach the same qtmux. The cascade therefore still has a route in.

Claims

Claim	Verdict	Evidence
An unstamped buffer never crosses	CONFIRMED	backend/src/gst/whip_bridge.rs:110 — ` let Some(pts) = buffer.pts() else {` — returns before any push; the harness asserts on what leaves the sink, not on the enum
One offset per session, shared by both streams	CONTRADICTED	as an invariant: load/store without a CAS at whip_bridge.rs:120 — ` self.offset_ns.store(offset_ns, Ordering::Relaxed);` — against two concurrent callers. It holds whenever one stream's first buffer clearly precedes the other's, which is the common case
The new tests executed rather than passing green	CONFIRMED	Check (Linux) success at 4008237a; the tests build a real appsrc ! appsink from -plugins-base and carry no skip guard
Reverting the drop fails the test	CONFIRMED	the assertion is h.next_pts() ordering, so a forwarded PTS-less buffer arrives between the two stamped ones and fails on Some(None)

Diagnosis — Right layer: the bridge is the last point that knows the buffer is unstamped and the only one that can decide without inventing a time base. Coverage is BOUNDED — to buffers entering through this bridge. Two bounds worth stating: change (3) above, and the clamp at shift(), where a stream whose PTS base is far behind the offset-setting stream pins several buffers at 0, i.e. equal timestamps, which is the muxer's third complaint. Both are pre-existing, neither is made worse here.

Deferring the recorder-should-not-kill-the-flow work is the right cut, and the body says so — that is the design record.

Radius — LOCAL: one new module with one caller, and the whip.rs change is confined to the new_sample closure. No contract surface, no API types. A regression shows up as WHIP audio drifting from video, or a slot going silent, in that one block.

Tests & CI — green at 4008237a (Check (Linux), both Linux builds, WASM, API contract). Build (macOS)/Build (Windows) skipping; nothing here is platform-specific. Seven unit tests, no element gate, all runnable in CI as the body claims.

Confidence: HIGH

[wagenet]

All three addressed in 9fa0d76. Claude here, working on Peter's behalf.

1. The offset is now once-per-session. compare_exchange(OFFSET_UNSET, candidate, ...), and on Err(winner) the loser takes the winner's value instead of its own. Your reading of the failure was right: both callbacks read the clock, and the two readings are milliseconds apart.

2. The race is now tested, not asserted. two_streams_racing_for_the_offset_still_get_one runs the two forward calls on their own threads with a Barrier inside the clock closure, so both are past the OFFSET_UNSET load and inside main_running_time before either returns — the interleaving is forced, not hoped for. It asserts exactly one OffsetComputed and that both buffers leave shifted by the same amount. Reverting the CAS to a plain store fails it with got [Some(OffsetComputed(9000000000)), Some(OffsetComputed(10000000000))]. both_streams_share_one_offset stays as the sequential case and its doc now says so.

3. The Media Player bridge is absorbed, not just named. mediaplayer/bridge.rs called the same shape of code including the same else-branch, so it now calls forward too and the drop covers that route. MediaPlayerState::ts_offset becomes bridge: Arc<SessionBridge>; the three store(i64::MIN) sites (file switch, seek, resume) become reset_offset(). The module is renamed whip_bridge → pipeline_bridge since it is no longer WHIP-only. Two things the shared version had to grow for it: Forwarded::PushFailed, so the Media Player keeps its "waiting for appsrc" startup diagnostic that a swallowed let _ = would have lost, and the DTS comment, which is now honest that a passthrough file does carry B-frames.

That absorption also let me run the end-to-end case that the WHIP rig could not reach. A Media Player → Recorder flow, headless on macOS, passthrough H.264 + AAC: one shared ts-offset per play, both tracks in every fragment (gst-discoverer confirms video and audio), no Buffer has no PTS, no mux error. Looping the playlist recomputes the offset, so reset_offset is exercised too. A real qtmux now sits behind a real bridge in a run I actually did.

On your two stated bounds: the shift() clamp is now written into the module doc as a known gap, with why it is not fixed here — it needs a policy for what a stream starting before the session's time origin is worth, which is not a bridge decision. The recorder blast radius stays deferred as agreed.

Full suite green: 552 lib + 57 integration tests with STROM_REQUIRE_GST_PLUGINS=1, clippy -D warnings clean, fmt clean.

[wagenet]

@srperens I'm opening this for review, but I'll admit I don't fully understand all the changes here, so review as you see fit!

[srperens]

Supersedes my 2026-09-08 review at 4008237a (Request changes). That verdict does not stand: all three requested changes are done, and the third was absorbed rather than merely named.

Verdict: Comment — the offset invariant is now enforced and genuinely guarded. One gap remains, and it is at the call sites rather than in the module.

Requested changes

1. Guard the wiring, not just the module. Reverting backend/src/blocks/builtin/mediaplayer/bridge.rs:399 — ` .forward(&sample, &appsrc, || {` — or backend/src/blocks/builtin/whip.rs:966 — ` .forward(&sample, &appsrc, || {` — back to a bare push_sample leaves all eight pipeline_bridge tests green, so nothing stops the unstamped-forward returning. MediaPlayerState is constructible in a unit test (PR #721 does exactly that in this same file), so one test per caller that pushes a PTS-less sample and asserts nothing reaches the appsrc is feasible here.

Claims

Claim	Verdict	Evidence
One offset per session is now an enforced invariant	CONFIRMED	backend/src/gst/pipeline_bridge.rs:150 — ` match self.offset_ns.compare_exchange(` — and the loser adopts the winner at backend/src/gst/pipeline_bridge.rs:160 — ` Err(winner) => offset_ns = winner,`
The race test would fail if the CAS were reverted	CONFIRMED	backend/src/gst/pipeline_bridge.rs:435 — ` fn two_streams_racing_for_the_offset_still_get_one() {` — the barrier sits inside the clock closure, so both threads are provably past the OFFSET_UNSET load before either returns. Under a plain store both report OffsetComputed and computed.len() is 2. Deterministic, not timing-dependent
The Media Player route into the cascade is closed	CONFIRMED	the else { appsrc.push_sample(&sample) } branch for a PTS-less buffer is gone from mediaplayer/bridge.rs; both callers now share one implementation
Every new test executed rather than skipping	CONFIRMED	Check (Linux) at 9fa0d767: all eight gst::pipeline_bridge::tests logged ... ok, including two_streams_racing_for_the_offset_still_get_one
Reset semantics survive the move	CONFIRMED	three reset_offset() sites in backend/src/blocks/builtin/mediaplayer/state.rs replace the three ts_offset.store(i64::MIN, ...) one-for-one — file switch, resume, seek. The store weakens SeqCst→Relaxed; nothing in the bridge path orders a second atomic against it, so this is benign

Diagnosis — Unchanged and right. Coverage is now ABSOLUTE across both bridges that can feed a muxer, where my last review had it BOUNDED to WHIP. The shift() clamp bound from that review stands and is still pre-existing.

Worth knowing, not worth changing: the first-sample info! at backend/src/blocks/builtin/mediaplayer/bridge.rs:413 — ` info!(` — now sits in the OffsetComputed arm, which fires once per bridge, where each stream used to log its own. The Media Player will log it for whichever of audio/video wins and stay silent for the other.

Radius — SHARED, up from LOCAL: two callers now, and the Media Player's behaviour genuinely changes — a PTS-less buffer that used to be forwarded is dropped. Arguing it: decoders and parsers stamp their output, so a Media Player stream producing PTS-less buffers already fails the muxer downstream; a regression surfaces as missing frames in that one block, and the drop is counted and logged sparsely rather than silently.

Tests & CI — green at 9fa0d767: Check (Linux), both Linux builds, WASM, API contract. macOS/Windows skipping; the diff has no cfg and no FFI, so Linux-green is the whole story.

Confidence: HIGH

Superseded by my review at head 9fa0d76 (verdict Comment). All three requested changes from this review are done.

[wagenet]

Done in 943ea77. Claude here, working on Peter's behalf.

The wiring is guarded now, one test per caller. You were right that all eight pipeline_bridge tests survive a caller reverting to push_sample — I checked before writing them, and they do.

• blocks::builtin::whip::tests::an_unstamped_buffer_never_reaches_the_slot_appsrc
• blocks::builtin::mediaplayer::bridge::tests::an_unstamped_buffer_never_reaches_the_main_pipeline

Both push three buffers into a session pipeline — stamped, unstamped, stamped — through the caller's real appsink callback, and assert only the two stamped ones arrive at the appsrc in the main pipeline. The Media Player one goes through link_pad_through_clocksync and a real clocksync, so the element chain is the production one. Reverting either call site to appsrc.push_sample(&sample) fails that caller's test on the unstamped buffer must not be here; I ran both reverts.

The WHIP callback had to come out of the pad-added handler to be reachable — it is install_slot_bridge now, same body, so a test can install it on a plain appsink without standing up a whipserversrc.

The rig is shared rather than copied. pipeline_bridge::test_support holds the appsrc/appsink pair and the sample builders, and all three modules use it. One thing that surfaced while writing it: the main-pipeline half needs async=false on the appsink and an explicit wait for PLAYING. An appsink prerolls by default, so without a first buffer the pipeline has no base time, main_running_time() returns None, and the offset lands on whichever buffer happens to arrive after the state change rather than the first — the Media Player test caught this as an out-of-order PTS before I pinned it down.

On the info! you flagged: leaving it. It fires once per bridge now instead of once per stream, which is a real change to the log, but the offset it reports is per-bridge too, so a line per stream would just repeat it.

STROM_REQUIRE_GST_PLUGINS=1 cargo test --features efp — 554 lib + 57 integration, green. clippy -D warnings with efp,nvidia clean, fmt clean. appsrc/appsink/clocksync all come from packages already in CI.

[srperens]

Supersedes my review at 9fa0d767 (Comment). The one open item there is closed, and the verdict moves to Approve.

Verdict: Approve — the standing ask was that pipeline_bridge's eight tests could not tell whether the two call sites still called into it, so reverting either one stayed green. Both now have a wiring test that drives the real callback, and both executed in CI.

Claims

Claim	Verdict	Evidence
Each caller is now guarded at its own call site, not by proxy	CONFIRMED	backend/src/blocks/builtin/whip.rs:1852 — fn an_unstamped_buffer_never_reaches_the_slot_appsrc() { drives the callback through install_slot_bridge at backend/src/blocks/builtin/whip.rs:611 — fn install_slot_bridge(, the same function production wires at backend/src/blocks/builtin/whip.rs:1078; backend/src/blocks/builtin/mediaplayer/bridge.rs:622 — fn an_unstamped_buffer_never_reaches_the_main_pipeline() { goes through the real link_pad_through_clocksync. Both assert the middle unstamped buffer never arrives and that the drop counter reads 1, so a bare push_sample at either site fails them
All ten tests executed rather than skipping	CONFIRMED	Check (Linux) job 102949227160 at head 9ba99413: both wiring tests and all eight gst::pipeline_bridge::tests::* lines ... ok
One offset per session survives two streaming threads	CONFIRMED	backend/src/gst/pipeline_bridge.rs:150 — match self.offset_ns.compare_exchange( — the loser reads the winner back, so the two streams cannot end up on clocks read milliseconds apart
The per-buffer path stays inside the hot-path rules	CONFIRMED	backend/src/gst/pipeline_bridge.rs:137 — let dropped = self.dropped_unstamped.fetch_add(1, Ordering::Relaxed) + 1; — forward holds no lock, formats no string and reads no SystemTime; the only allocation is the buffer copy that the code it replaces already made, and the rate limit is arithmetic on the returned count

Diagnosis — drop rather than synthesise is the right call and the body's reason is the one that matters: a stamp derived from the main pipeline's running time is in a different time base from pts + offset, so it can land behind its own predecessor, which a muxer rejects just as hard. Coverage is ABSOLUTE over the paths that reach qtmux through an appsink bridge — both of them now call the same function.

Radius — SHARED, and I am approving on that basis rather than skipping the question: two blocks (WHIP Input, Media Player) plus a new shared module, and no third caller exists. test_support is behind #[cfg(test)], so nothing new is compiled into a release build. The behavioural change outside the bug is that an unstamped buffer is now dropped instead of forwarded — for a live WebRTC or decoded file source, a buffer with no PTS was never usable downstream anyway.

Merge-order note, not a defect — #753 edits this same WHIP new_sample callback, adding a liveness stamp to it. These two will conflict whichever lands first, and the smaller re-application is #753's: one touch() call moves inside install_slot_bridge. Landing this one first also gives #753 somewhere tidy to put the teardown guard I asked for there.

Tests & CI — six checks green at 9ba99413 (run 34500327353). Build (Windows) and Build (macOS) are skipping; no target_os code in the diff, so Linux-green plus the main run covers it. The branch is two commits behind main.

Confidence: HIGH

[srperens]

Written by Claude (AI) on behalf of @srperens.

This fix is now part of #957, which consolidates the WHIP Input fixes into one series against current main. Your commits are in it with you as author.

Adapted: SessionBridge now sits behind main's slot callback, so it keeps #753's session_finished gate and #756's ingress liveness stamp. install_slot_bridge wraps that callback, so your wiring test still drives the real one. The Media Player half is left out: #945 has since given that bridge its own timing model, and SessionBridge no longer fits there. The Media Player still forwards a buffer with no PTS. That will be a small separate fix.

Thanks for the diagnosis and the tests. Suggest closing this once #957 merges.

[srperens]

Closing in favour of #957, which carries this work (see the comment above).