fix(whip): zero the processing deadline on whipclientsink's input appsinks

[wagenet]

Opened by Claude (AI) on behalf of @wagenet.

The WHIP Output counterpart of #814, for implementation = whipclientsink.

whipclientsink extends BaseWebRTCSink, the same base class as whepserversink, and ends each input in clocksync ! appsink. That appsink only hands buffers to the session pipeline, but its default 20 ms processing-deadline is added to the latency the flow reports. When the flow distributes that latency, the 20 ms is waited out twice before a packet is sent: by the appsink, and by webrtcbin's clocksync in the session. A flow adopts the largest latency any of its sinks reports, so a flow that publishes over WHIP while serving WHEP viewers gets nothing from #814: this sink puts the 20 ms back for every sink in the flow.

The fix is the same deep-element-added handler as #814: zero processing-deadline on appsinks added as direct children of the sink. The closure captures nothing. It is connected in the builder because webrtcsink creates the appsinks in request_new_pad, which runs when the flow links the block.

The default whipsink implementation is left alone. Its only sinks, nicesink and a fakesink inside its webrtcbin, are both sync=false, so neither adds a deadline and its pipeline never reports a live latency.

Measurements

Real block from builtin::get_builder("builtin.whip_output"), live audiotestsrc at 23.2 ms. The latency the flow reports goes from 43.2 ms to 23.2 ms with the fix (endpoint unreachable, 192.0.2.1).

Whether the session gets that latency depends on start-up: Strom computes a flow's latency when it goes to PLAYING and recomputes it only on a time-offset change, and if the appsink has no caps at that moment nothing is distributed, with or without the fix. Latency the session's appsrc received, publishing to a local whipserversrc:

flow	without the fix	with the fix
WHIP Output plus another syncing sink	43.2 ms in 3 of 5 starts, 23.2 ms in 2	23.2 ms, 5 of 5
same, latency recomputed after start	43.2 ms	23.2 ms
WHIP Output as the only sink	0	0

These rows come from uncommitted scratch tests; they bind a local TCP port and failed to start in 1 of 12 runs.

Arrival time, also from an uncommitted scratch test: ticks sent through the real block to a local whipserversrc over loopback, sender and receiver sharing one clock, a second output in the flow. With the fix, ticks arrived 18.1 ms sooner in 16 of 16 paired trials (every pair 17.9 to 18.6 ms; 8 on Strom's start path, 8 with latency recomputed). That equals the session's latency drop there, 41.3 to 23.2 ms: once the deadline is gone, the other output's latency sets the flow's.

Trade-off

The deadline was also slack against upstream stalls, but only against a stall in a thread separated from the block by a queue; in the block's own streaming thread it just shifts the wait. In a scratch rig (same setup, 6 pairs per condition), CPU load and stalls up to 18 ms made no meaningful difference. Stalls of 10-40 ms on 1 buffer in 25, upstream of a queue, put about 2% of packets up to 15 ms off beat with the fix, but its p99 packet still left before the unfixed median. In Strom that shape is WHIP Input relayed straight into WHIP Output; mixers and routers pace their own output.

Tests

New backend/tests/whip_output_latency_test.rs, two tests:

• every_whipclientsink_input_appsink_has_no_processing_deadline: asserts every appsink inside whipclientsink is at 0, and that at least one exists.
• whip_output_latency_excludes_appsink_processing_deadline: runs the flow to PLAYING and asserts the block adds less than 20 ms to the latency the flow reports.

Both fail with the fix reverted (appsink0 kept processing-deadline 20000000; whip_output adds 0:00:00.020000000) and pass with it. Both ran and passed in the Linux Check job; no CI package change was needed.

Ran locally (macOS, GStreamer 1.28.6):

• cargo test --test whip_output_latency_test, with and without the fix
• cargo test --test pipeline_lifecycle_test (passes, but it does not build a WHIP Output, so it does not exercise this handler)
• cargo test --test whip_whep_ice_transport_policy_test
• cargo fmt --all --check, cargo clippy --workspace --all-targets -- -D warnings

Not run: the full test suite locally, a publish to a real remote WHIP endpoint, and a browser receiver.

🤖 Generated with Claude Code

[srperens]

Noted as a draft, so I am holding off on a full review until it is marked ready for
review. If you want one before then, say so here or request me as a reviewer.

[srperens]

Verdict: Comment — the mechanism is right, the closure is correctly reference-free, and both new tests really executed in CI. What stops an approval is the trade-off the body is candid about: the deadline was also slack against upstream jitter, and the numbers that size that cost come from scratch tests nothing in this repo can re-run.

Requested changes

1. Put the trade-off where it survives the PR body. Two sentences in the comment block at backend/src/blocks/builtin/whip.rs:1466 — // when the flow links the block, so the handler has to be in place now. — saying the deadline was slack against a stall upstream of a queue, and naming the shape that pays for its removal (WHIP Input relayed straight into WHIP Output). Otherwise the next reader reinstates it, or re-measures it from scratch.

Claims

Claim	Verdict	Evidence
The block no longer adds the 20 ms to the latency the flow reports	CONFIRMED	backend/tests/whip_output_latency_test.rs:153 — added < DEFAULT_PROCESSING_DEADLINE, — against backend/tests/whip_output_latency_test.rs:17 — const DEFAULT_PROCESSING_DEADLINE: gst::ClockTime = gst::ClockTime::from_mseconds(20);. Check (Linux) job 107209809099: whip_output_latency_test "2 passed", both named tests ... ok. Reverted, the appsink contributes exactly that 20 ms, so the assertion cannot hold — the guard is real
Removing the deadline costs ~2% of packets up to 15 ms off beat under an upstream stall	UNVERIFIED	Measured in uncommitted scratch rigs. Nothing in the tree re-runs it, and no issue records it, so the cost is unsettled here — not EXTERNAL, since it is this repo's own pipelines
A flow recomputes latency only on a time-offset change, so the gain depends on the appsink having caps at PLAYING	CONFIRMED	backend/src/gst/pipeline/properties.rs:210 — let _ = self.pipeline.recalculate_latency(); — the only such call in the tree, on the Time Offset path

Diagnosis — Right layer: the deadline is zeroed on the appsinks that own it, not compensated downstream. Coverage is BOUNDED by the row above — a start where the appsink has no caps distributes nothing either way. The default implementation is untouched and remains the default: backend/src/blocks/builtin/whip.rs:136 — .unwrap_or(false);, so only a flow that opted into whipclientsink changes.

Radius — SHARED, and this is the point of the change rather than a side effect: a flow adopts the largest latency any sink reports, so every other sink in a flow containing an opted-in WHIP Output moves too. Confined to opted-in flows by the line above. The deep-element-added closure captures nothing — no gst::Bin or Element is cloned into it — so CLAUDE.md's reference rule holds: backend/src/blocks/builtin/whip.rs:1468 — bin.connect("deep-element-added", false, |args| {. Per-event, not per-buffer.

Overlaps — #814 (draft) is the same fix in whep.rs and owns the shared design question; both answer it identically, so nothing is left for a merge to settle, but landing both leaves two copies of this closure and one helper is the obvious follow-up. #854 (draft) splits whip.rs into sub-modules and would move this code — textual only, and this PR is ready while that is not, so this lands first.

Tests & CI — Nine checks green at 0073955; macOS/Windows skipped and nothing here is platform-specific. No element skip guard in the new file: the plugin is registered statically, so a missing one fails rather than skips. whip.rs is 2431 lines, well past CLAUDE.md's 1500-line mark — which is what #854 is for.

Confidence: HIGH

[wagenet]

Posted by Claude (AI) on behalf of @wagenet.

Addressed in 737aea2: the comment above the deep-element-added handler in whip.rs now records the trade-off. The deadline is slack against a stall in a thread upstream of a queue, a stall in the block's own streaming thread only shifts the wait, and WHIP Input relayed straight into WHIP Output is the shape that pays. Comment-only change; cargo fmt and clippy pass locally.

[srperens]

Supersedes my review of 2026-09-24 at 0073955; the verdict stands at Comment. The one change since then (737aea2, ahead_by=1, one file) is the requested comment, and it is done.

Verdict: Comment — the mechanism is right, the closure holds no references, and both new tests ran in CI. The only open row is the cost of removing the deadline, which is measured in scratch rigs this repo cannot re-run. The trade-off is now written at the handler, so accepting it is a one-line call for a maintainer. Nothing further is asked of the author.

What moved

The trade-off now sits in the code, where the next reader will find it:
backend/src/blocks/builtin/whip.rs:1468 — // WHIP Input relayed straight into WHIP Output is the shape that pays;

The handler is unchanged apart from its line number. It still captures nothing:
backend/src/blocks/builtin/whip.rs:1474 — bin.connect("deep-element-added", false, |args| {

Claims

Claim	Verdict	Evidence
The block no longer adds the 20 ms to the latency the flow reports	CONFIRMED	Carried from my review of 2026-09-24. At 737aea2, Check (Linux) job 108329645715 logs every_whipclientsink_input_appsink_has_no_processing_deadline ... ok and whip_output_latency_excludes_appsink_processing_deadline ... ok
Removing the deadline costs ~2% of packets up to 15 ms off beat under an upstream stall	UNVERIFIED	Still measured only in uncommitted scratch rigs. It is now documented, not settled. The maintainer decides whether the WHIP Input → WHIP Output relay shape may pay it

The diagnosis (BOUNDED by caps being present at PLAYING), the default-implementation gate and the recompute-on-time-offset row are carried unchanged from the 2026-09-24 review.

Radius — SHARED, as before. Every sink in a flow that opts into whipclientsink moves, and no other flow changes.

Overlaps — #814 is the same fix for whep.rs. It has left draft since my last review, and my review of it follows in this run. The two PRs answer the design question the same way, so neither merge settles anything the other leaves open. #854 (draft) would move this code, but only as a textual move.

Tests & CI — Run 36215184689 at 737aea2: Check (Linux), both Linux builds, WASM and API Contract Check all green. macOS and Windows were skipped, and nothing in this diff is platform-specific.

Confidence: HIGH

[srperens]

Verdict: Comment. This replaces my review at 737aea2, and the verdict is still Comment. The new head, 73b9b8b, is the same change squashed into one commit and rebased onto the split from #1053. Nothing in the fix changed. One question is still open, and it is the maintainer's call, not the author's: whether the relay shape may pay the trade-off.

What moved

The handler and its comment moved word for word from whip.rs into the new module. The code is still the same, and the closure still captures nothing:
backend/src/blocks/builtin/whip/output/whipclientsink.rs:175 — bin.connect("deep-element-added", false, |args| {

backend/tests/whip_output_latency_test.rs is byte-identical between 737aea2 and 73b9b8b. This branch is 7 commits behind main, but none of those 7 changes whipclientsink.rs after the split. The file on main has the same blob as this PR's parent.

Claims

Claim	Verdict	Evidence
The block no longer adds the 20 ms to the latency the flow reports	CONFIRMED	At 73b9b8b, Check (Linux) job 113910556485 logs every_whipclientsink_input_appsink_has_no_processing_deadline ... ok and whip_output_latency_excludes_appsink_processing_deadline ... ok
Removing the deadline puts about 2% of packets up to 15 ms off beat under an upstream stall	UNVERIFIED	Measured only in uncommitted scratch rigs. It is now documented at backend/src/blocks/builtin/whip/output/whipclientsink.rs:169 — // WHIP Input relayed straight into WHIP Output is the shape that pays;

The diagnosis rows are carried unchanged from my reviews of 2026-09-24 and 2026-09-28: BOUNDED by caps being present at PLAYING, the default whipsink left alone, and the recompute on a time-offset change.

Radius. SHARED, as before. Every sink in a flow that uses whipclientsink moves. No other flow changes.

Overlaps. #814 makes the same fix in WHEP Output and answers the design question the same way, so landing either one first settles nothing the other leaves open. #854 is closed, so it drops out. #1072 also works on latency, but on the WHIP Input bridge appsrcs (whip/input/slots.rs), which is a different code path. That is not an overlap.

Tests & CI. Run 37957172837 at 73b9b8b: Check (Linux) is green, with backend, shared-types and frontend test steps all success. Both Linux builds, WASM, API Contract Check and Static JS Tests are green. macOS and Windows were skipped, and nothing here is platform-specific. pipeline_lifecycle_test does not build a WHIP Output, so the new closure's teardown is covered only by its capturing nothing. It holds no object, so nothing is left to guard.

For the maintainer. Accepting the trade-off takes one line. If you accept it, this PR and #814 can land in either order.

Confidence: HIGH

[srperens]

Accepting the trade-off: merging this together with #814. One correction for the record, verified against the sources Strom builds with (gst-plugin-webrtc 0.15.2, gstreamer-utils 0.25.4, GStreamer 1.26).

The 20 ms is waited out once, not twice. The session pipeline shares the flow's clock and base time (webrtcsink/imp.rs:3418-3420) and the consumer appsrc forwards segments (handle-segment-change=true), so running time is the same on both sides. The appsink releases at PTS + L; webrtcbin's clocksync in the session waits for PTS + L + E, where E is the encoder chain's declared latency (clocksync waits on its upstream latency). The second wait subsumes the first, so the deadline shifts the send time by 20 ms once. That matches the measurements in the body (43.2 → 23.2 ms, ticks 18.1 ms sooner). The handler comment's "where webrtcbin's clocksync waits for it again" reads as additive; it is the same deadline on the same timeline. The full breakdown is on #814.

Also worth knowing for the trade-off comment: with a zero deadline the session still keeps E − t_enc of slack against late arrivals (about 20 ms for Opus), so the cost is mostly a video concern, where webrtcsink's zerolatency/low-latency encoder settings keep E small. No packets are dropped by the change: appsink keeps max-lateness=-1 and clocksync never drops late buffers.