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.