Skip to content

fix(whep_output): strip the offered profile from every viewer's video chain - #896

Merged
srperens merged 2 commits into
Eyevinn:mainfrom
wagenet:wagenet/whep-colorimetry-not-negotiated
Oct 19, 2026
Merged

srperens merged 2 commits into
Eyevinn:mainfrom
wagenet:wagenet/whep-colorimetry-not-negotiated

Conversation

@wagenet

@wagenet wagenet commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

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 ... and not-negotiated (-4). Reconnecting usually works. The colorimetry=sRGB in 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 an element-added watcher installed at payloader-setup. webrtcsink adds a stream's whole chain before emitting payloader-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 a HashMap<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-setup fires: the capsfilter directly upstream and the capsfilters downstream. Strip each one now and again on every notify::caps. The fields stripped are unchanged. The hook moves out of whep.rs (2850 lines) into whep/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:

build video set up first viewer got no video
upstream b28be77b 4/15 4/15 (exactly the video-first ones)
this branch 7/15 0/15

On 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 level list, taken from the offered profile-level-id (Chrome: 3.1). This PR does not strip level, 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 on b28be77b. 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.rs is the whole change. Check the position assumption against PayloadChainBuilder::build in 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 before configure_payloader emits payloader-setup. The webrtcsink behaviour behind the fix is setup_session_payloading_chain (build, then configure_payloader) and connect_input_stream (sets enc_filter caps later).

Tests.

  • New 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, same codec-parser-caps-video_0 warning as the rig). With the fix it passed 9/9. It needs x264enc (plugins-ugly), nicesrc (gstreamer1.0-nice) and plugins-bad, all in the CI package list.
  • cargo test --workspace passes locally (macOS, GStreamer 1.28). cargo clippy --all-targets -- -D warnings and cargo fmt --check are clean. In Linux CI (GStreamer 1.24.2, STROM_REQUIRE_GST_PLUGINS=1) the new test runs and passes.

🤖 Generated with Claude Code

@wagenet
wagenet marked this pull request as ready for review September 28, 2026 23:17
wagenet added a commit to wagenet/strom that referenced this pull request Sep 29, 2026
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>
wagenet added a commit to wagenet/strom that referenced this pull request Sep 29, 2026
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>
wagenet added a commit to wagenet/strom that referenced this pull request Sep 29, 2026
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
srperens previously approved these changes Oct 1, 2026

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

wagenet added a commit to wagenet/strom that referenced this pull request Oct 1, 2026
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 srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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

wagenet and others added 2 commits October 9, 2026 09:13
… 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>
@wagenet
wagenet force-pushed the wagenet/whep-colorimetry-not-negotiated branch from 8287229 to 5e737eb Compare October 9, 2026 16:14
wagenet added a commit to wagenet/strom that referenced this pull request Oct 9, 2026

@srperens srperens left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.rs moved from whep/ to whep/output/ and is byte-identical. So is whep_output_profile_filter_test.rs.
  • The deleted workaround block in whep/output/whepserversink.rs matches, line for line, the one deleted from the old whep.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 at backend/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 to whep/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

@srperens
srperens merged commit ee95255 into Eyevinn:main Oct 19, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants