Repository navigation
fix(whep_output): strip the offered profile from every viewer's video chain - #896
Conversation
Integration fix-up: Eyevinn#799 makes the WHEP Output block read video_convert_mode() at build time, which panics until detection has run. The block tests from Eyevinn#896 and Eyevinn#899 build it without that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Integration fix-up: Eyevinn#799 makes the WHEP Output block read video_convert_mode() at build time, which panics until detection has run. The block tests from Eyevinn#896 and Eyevinn#899 build it without that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Integration fix-up: Eyevinn#799 makes the WHEP Output block read video_convert_mode() at build time, which panics until detection has run. The block tests from Eyevinn#896 and Eyevinn#899 build it without that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
srperens
left a comment
There was a problem hiding this comment.
Verdict: Approve. The fix is at the right layer: it finds the filters from the payloader instead of racing webrtcsink's chain construction. The guard test runs in this repo's CI, and nothing here settles a question another open item is asking. One observation for the body, below; it does not block.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The old hook could never reach the encoder filter of a video-first chain | CONFIRMED |
The removed code stripped only what was downstream of the payloader at payloader-setup, plus capsfilters added afterwards through bin.connect_element_added. A filter upstream that already existed was untouched. The new code walks to it directly: backend/src/blocks/builtin/whep/profile_filter.rs:49 — let upstream = payloader |
| A filter whose caps webrtcsink sets later is still stripped | CONFIRMED |
backend/src/blocks/builtin/whep/profile_filter.rs:83 — ` filter.connect_notify(Some("caps"), move |
| No strong element reference in a signal closure | CONFIRMED |
The payloader-setup handler captures nothing. The notify closure captures only the consumer_id String and gets the filter as its argument |
webrtcsink builds the whole chain before payloader-setup, and the stream order is random per session |
EXTERNAL |
Assumes gst-plugin-webrtc 0.15.x PayloadChainBuilder::build → configure_payloader, as the body describes. The 4/15 vs 0/15 rig table is consistent with it |
| The guard runs in CI | CONFIRMED |
Check (Linux) job 109169377408: whep_output_profile_filter_test starts webrtcsink (its rtpgccbwe warning is printed) and reports baseline_offer_gets_high_profile_video ... ok. plugins_available() asserts rather than skips under STROM_REQUIRE_GST_PLUGINS |
Diagnosis. The cause explains both the 1-in-5 rate and the fact that reconnecting fixes it. Coverage is BOUNDED, and the body names the bound: level is still constrained. #899 handles that, and it is stacked on this PR.
Observation (not blocking). The upstream strip now applies on every session, including the raw-input path where webrtcsink encodes (backend/src/blocks/builtin/whep.rs:1359 — // 2. Feed raw video and let webrtcsink encode internally). Before, the strip happened there only when another stream was set up first. SPECULATIVE (not verified): if that filter is what constrains webrtcsink's own H.264 encoder, raw-input viewers now always get the encoder's default profile rather than Baseline. That is harmless for Chrome, but a viewer with a Baseline-only decoder would differ. Worth one line in the body. The test covers pre-encoded input only.
Radius. LOCAL. The hook lives in one block. It runs for every viewer of a WHEP Output, but it strips the same six fields as before.
Overlaps.
- #899 is stacked on this PR, and its level decision is its own; land this first.
- #900 (level rising mid-stream) is separate.
- #814 merges clean with this PR.
- #799 conflicts with main already, not because of this PR.
Tests & CI. All eight checks are green at 3b13d13. The branch is 17 commits behind main, and git merge-tree against main is clean. macOS and Windows show skipping, and nothing here is platform-specific. I did not run the author's revert-fails-5/5 check. The static reading in row 1 is why I expect it.
Confidence: HIGH
Integration fix-up: Eyevinn#799 makes the WHEP Output block read video_convert_mode() at build time, which panics until detection has run. The block tests from Eyevinn#896 and Eyevinn#899 build it without that. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
srperens
left a comment
There was a problem hiding this comment.
This supersedes my Approve of 2026-10-01 at 3b13d13, which was dismissed by the push. The verdict stands.
Verdict: Approve. The new head adds one commit, 8287229, a single test line. I posted this as a COMMENT review because GitHub refuses APPROVE from the account this stage runs under.
What moved. gh api compare/3b13d13...8287229 reports ahead=1 behind=0, so there was no rebase, and the compare patch is the whole delta:
backend/tests/whep_output_profile_filter_test.rs:82 — strom::gpu::detect_gpu_capabilities();
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The fix rows of my 2026-10-01 review still hold | CONFIRMED |
No product line changed since 3b13d13. The rows are carried by citation (hook location below) |
| Detecting first keeps the test working in either merge order | CONFIRMED |
video_convert_mode() panics until detection has run: backend/src/gpu.rs:48 — .expect("GPU capabilities not detected yet - call detect_gpu_capabilities() first"). #799, which the commit message names, was closed unmerged. Its successor #949 is on main, and on main no WHEP code calls video_convert_mode() (grepped). So the line is harmless today and protects against a later caller |
| The PR still merges into current main | CONFIRMED |
The branch is 64 commits behind. I trial-merged it onto origin/main with no conflict, including main's whep.rs changes from #949, #945, #985 and #910. The merged whep.rs has exactly one profile hook, the profile_filter::install call |
The hook is still installed from the payloader: backend/src/blocks/builtin/whep/profile_filter.rs:34 — whepserversink.connect("payloader-setup", false, |values| {
Diagnosis. It is unchanged from my 2026-10-01 review. Coverage is BOUNDED to profile, and #899 owns level. The raw-input observation in that review (every raw-input session now has its profile stripped) still applies and still does not block.
Radius. LOCAL. The new line is test-only.
Overlaps
- #899 (draft) is stacked on this PR; land this first.
- #814 also touches WHEP Output. I trial-merged it on top of the main+#896 merge, and it applies cleanly.
- No open issue owns the profile decision.
Tests & CI. At 8287229 these passed: Check (Linux), Build (Linux x86_64/ARM64), Check & Build (WASM), API Contract Check and Static JS Tests. macOS and Windows were skipping, and nothing here is platform-specific. In Check (Linux) job 110540927144, whep_output_profile_filter_test started webrtcsink (its rtpgccbwe warning is printed) and reported baseline_offer_gets_high_profile_video ... ok. That run predates main's 64 commits. My only evidence for the merged tree is the trial merge above, which is static and not a build.
Confidence: HIGH
… chain A WHEP viewer that offers H.264 Baseline gets a capsfilter in front of its payloader listing only Baseline-compatible profiles. The block strips that field so a High profile stream can pass, but it found the filter through an element-added watcher installed at payloader-setup, after webrtcsink had already added the chain. The strip only happened when another stream of the same viewer had been set up first, and webrtcsink sets streams up in HashMap order, so a viewer whose video came first failed with not-negotiated at the first keyframe. The filters are now found from the payloader itself: the one directly upstream and the ones downstream, each stripped now and on every later caps change. The hook moves to its own module. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…r test block The WHEP Output block will read video_convert_mode() at build time once Eyevinn#799 lands, and that panics until detection has run. Detecting here is harmless today and keeps the test working in either merge order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
8287229 to
5e737eb
Compare
srperens
left a comment
There was a problem hiding this comment.
Verdict: Approve. This supersedes my Approve at 8287229 (2026-10-06), and the verdict stands. The new head 5e737eb is a rebase onto dcccdd7 that follows #1053's split of the WHEP block into whep/input and whep/output. The logic is unchanged, and the run at this head is green with the guard executed.
What moved since 8287229
git range-diff: the test commit is=. The fix commit differs only in where it lands.profile_filter.rsmoved fromwhep/towhep/output/and is byte-identical. So iswhep_output_profile_filter_test.rs.- The deleted workaround block in
whep/output/whepserversink.rsmatches, line for line, the one deleted from the oldwhep.rs. The only differences are the module declaration and the call path:backend/src/blocks/builtin/whep/output/whepserversink.rs:381—super::profile_filter::install(&whepserversink);, declared atbackend/src/blocks/builtin/whep/output/mod.rs:6—mod profile_filter;.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The fix rows of my earlier reviews still hold | CONFIRMED |
The module is byte-identical to the one I approved, so I carry those rows by citation. The hook is still on the payloader: backend/src/blocks/builtin/whep/output/profile_filter.rs:34 — ` whepserversink.connect("payloader-setup", false, |
| The tree has exactly one profile hook after merging | CONFIRMED |
Trial merge onto b127864 is clean. payloader-setup is connected only in profile_filter.rs, and the old connect_element_added path is gone |
| The guard executed at this head | CONFIRMED |
Check (Linux) job 113912432066: webrtcsink starts (its rtpgccbwe warning is printed), then test baseline_offer_gets_high_profile_video ... ok |
Diagnosis: Unchanged. Coverage is BOUNDED to profile, and #899 owns level. The closures capture only a String, never an element, so the WeakRef rule is met.
Radius: LOCAL.
Overlaps
- #899 (draft) is stacked on the pre-split head and still edits
whep/profile_filter.rs. Land this first. #899's rebase must then move towhep/output/profile_filter.rs. - #814 also edits
whep/output/whepserversink.rs. It trial-merges cleanly on top of main plus this PR. - No open issue owns the profile decision.
Tests & CI: run 37957724150 at 5e737eb: Check (Linux), both Linux builds, WASM, API Contract and Static JS pass. macOS and Windows are skipping, and nothing here is platform-specific. Main has moved six commits since the merge base, none in whep/. My evidence for the merged tree is the trial merge, not a build. I ran nothing locally.
Confidence: HIGH
Written by Claude.
Current behaviour. About one WHEP viewer in five gets no video from a WHEP Output fed pre-encoded High profile H.264, as the Video Encoder block produces by default. The viewer's session fails at its first keyframe with
codec-parser-caps-video_0 ... transform could not transform video/x-h264, ... profile=(string)high ...andnot-negotiated (-4). Reconnecting usually works. Thecolorimetry=sRGBin that log line is incidental: the caps never change.Cause. Chrome offers H.264 Baseline first, and the block answers with that payload type. webrtcsink then sets a capsfilter directly in front of the viewer's payloader to
profile={baseline, constrained-baseline}. The block's WORKAROUND #2 strips that field, but it found the filter through anelement-addedwatcher installed atpayloader-setup. webrtcsink adds a stream's whole chain before emittingpayloader-setup, so the watcher only saw the video filter when another stream of the same viewer (an audio track) had been set up first. webrtcsink sets a viewer's streams up by iterating aHashMap<u32, WebRTCPad>, so the order is random per session.Fix. Find the filters from the payloader itself, since its chain is already linked when
payloader-setupfires: the capsfilter directly upstream and the capsfilters downstream. Strip each one now and again on everynotify::caps. The fields stripped are unchanged. The hook moves out ofwhep.rs(2850 lines) intowhep/profile_filter.rs.Evidence
Real Chrome viewers (Playwright) against the rig-shaped flow (WHIP input → GPU vision mixer 1280x720 → Video Encoder (vtenc, High 3.1) → WHEP Output with 2 audio tracks). One fresh server per trial, fix and upstream interleaved:
b28be77bOn the fixed build, every trial logs
Stripped profile from capsfilter webrtc-enc-filter-video_0. On upstream, that line appeared only in trials where an Opus payloader was set up before the H.264 one.Not addressed: streams above level 3.1 (1080p, 720p50/60)
The filter also carries webrtcsink's
levellist, taken from the offeredprofile-level-id(Chrome: 3.1). This PR does not striplevel, same as before, so an H.264 stream above level 3.1 still fails for Chrome viewers, whatever the order. That covers 1080p and 720p at 50 or 60 fps (level 3.2): the block fed either one negotiates no video with this fix, and fails the same way onb28be77b. Relaxing the level means sending above the level the viewer declared. That needs its own decision, so it is left for a follow-up.How to review.
profile_filter.rsis the whole change. Check the position assumption againstPayloadChainBuilder::buildin gst-plugin-webrtc 0.15.2:... codec-parser-caps, [encoded filter], webrtc-enc-filter, payloader, payload-chain-output-caps, webrtc-pay-filter, all added and linked beforeconfigure_payloaderemitspayloader-setup. The webrtcsink behaviour behind the fix issetup_session_payloading_chain(build, thenconfigure_payloader) andconnect_input_stream(setsenc_filtercaps later).Tests.
backend/tests/whep_output_profile_filter_test.rs: builds the real block, feeds x264 High profile, POSTs a Chrome-shaped video-only offer (H.264 PT 102,42001f) to the block's internal endpoint and requires the viewer's payloader to emit a buffer. A video-only offer makes video the only stream, so this is the failing order every time. With the old hook restored it failed 5/5 (payloaded 0 buffers, samecodec-parser-caps-video_0warning as the rig). With the fix it passed 9/9. It needsx264enc(plugins-ugly),nicesrc(gstreamer1.0-nice) and plugins-bad, all in the CI package list.cargo test --workspacepasses locally (macOS, GStreamer 1.28).cargo clippy --all-targets -- -D warningsandcargo fmt --checkare clean. In Linux CI (GStreamer 1.24.2,STROM_REQUIRE_GST_PLUGINS=1) the new test runs and passes.🤖 Generated with Claude Code