Skip to content

fix(live recorder): answer caps queries at each track input so a late link cannot fail the encoder - #1075

Open
wagenet wants to merge 2 commits into
Eyevinn:mainfrom
wagenet:wagenet/inrec-caps-change
Open

wagenet wants to merge 2 commits into
Eyevinn:mainfrom
wagenet:wagenet/inrec-caps-change

Conversation

@wagenet

@wagenet wagenet commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Once isofmp4mux has caps, it answers a fixed caps query with exactly those caps. A Live Recorder track input passed queries through to the muxer, so that answer reached the encoder, including fields the encoder wrote itself. vtenc writes chroma-site=jpeg; WebRTC-decoded video has none. GstVideoEncoder's caps proxy copies the field, so vtenc then refuses its own input caps. The RECONFIGURE from the recorder's late parser link makes the converter in front re-check, it fails not-negotiated, and through the tee that stops the WHIP guest's whole input. Two guests lost video this way on a live call. builtin.recorder (splitmuxsink) does not pin its caps, so it did not show this.

Change

Each track input's sink pad answers CAPS queries itself (with the filter, or ANY), from a query probe next to the existing input probes in keepalive.rs. This is the answer the input gave before its parser was linked, so the answer no longer changes when the parser links or when the muxer gets caps. What the recorder cannot take is still refused with an element error when its caps arrive (chain_for / refuse_input), as the block guidelines require. The parser converts stream-format and alignment, so upstream loses nothing by not seeing the muxer's preferences.

Rejected:

  • Dropping the late link's RECONFIGURE: removes the trigger, not the cause.
  • Answering with the track's codec set: raw video would then fail as a silent not-negotiated instead of the recorder's error naming builtin.videoenc.
  • Changing the encoder or converter: the pin is the consumer's, and the consumer adapts.

Evidence

  • After the muxer has caps, a one-structure caps query at the recorder input no longer narrows to the muxer's current caps. the_input_answers_caps_queries_without_the_muxers_current_caps fails with the probe disabled:
    the recorder input narrowed a caps query to the muxer's current caps
    asked:    video/x-h264, stream-format=(string)byte-stream, alignment=(string)au, profile=(string)high-4:4:4, width=(int)320, ... chroma-site=(string)jpeg, ...
    answered: video/x-h264, profile=(string)high-4:4:4, ... chroma-site=(string)jpeg, ..., level=(string)1.3, ..., codec_data=(buffer)..., stream-format=(string)byte-stream, alignment=(string)au
    
  • Guests no longer die on join: replay on macOS arm64, GStreamer 1.28.6 (Homebrew). A copy of the production flow ran with 4 per-guest videoenc(vtenc_h264_hw) → Live Recorder branches, and 5 headless WHIP publishers (whipclientsink, H.264) joined at once, 7 trials per build. The flow's 8 return-audio blocks from an unmerged PR were removed; the guest-to-recorder path is unchanged.
    • main (d2ae537): 9 of 35 joins died with streaming stopped, reason not-negotiated, at least one in every trial. During trial 6 the server aborted on a panic in the WHIP signaller (gst-plugin-webrtc 0.15.2, whip_signaller/imp.rs:685, unrelated to this change); trials 6 and 7 were re-run after a restart.
    • This branch (18452df): 0 of 35 died, no not-negotiated warnings. All 28 per-guest video files decode, 8.9 to 9.4 s each for ~10 s connected.
  • No end-to-end guard test: a single-encoder test (vtenc behind videoconvert, a forced RECONFIGURE after the muxer has caps) passes with and without this change, because the converter alone renegotiates to chroma-site=jpeg. The fatal case needs the full flow (tee, leaky queue, WebRTC caps, concurrent joins).

Tests

Ran (macOS arm64):

  • cargo test --test live_recorder_test at 18452df: 19 passed, including the new test.
  • The new test with the probe disabled: fails, output quoted above.
  • cargo clippy --all-targets -- -D warnings, cargo fmt: clean.

Not run: the Linux and Windows CI jobs locally. The new test needs x264enc and isofmp4mux, which the existing Live Recorder tests already require, so it runs wherever they run.

For the reviewer

  • Every Live Recorder input now accepts any caps at query time, including after the muxer has caps. Mid-stream resolution changes still record into one file (checked with x264enc into mp4, mkv and mpegts, and vtenc into mp4 and mkv).
  • A mid-stream codec change (H.264 then H.265 on one input) still fails with a silent not-negotiated from the recorder's parser. On main it fails earlier, when the H.265 source starts. Tracked in Recorder: a video track that changes codec fails the flow #931.

Out of scope

  • A failing tee branch can still stop a shared source: tee and queue pass fatal flow returns upstream. That needs its own design.
  • isofmp4mux pins fields (chroma-site, colorimetry) in its caps answer that its own caps-change check ignores. Worth reporting to gst-plugins-rs separately.

Written by Claude (Claude Code) for @wagenet.

🤖 Generated with Claude Code

@srperens

Copy link
Copy Markdown
Collaborator

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 srperens mentioned this pull request Oct 14, 2026
@wagenet
wagenet marked this pull request as ready for review October 14, 2026 19:10

@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: Comment — right fix at the right layer, and the guard test calls the real block and executed in CI. Only Check (Linux) is red, on stinger tests that are not in this diff. It needs a run on a base that has #1076 before it can be approved.

Requested changes

  1. Merge or rebase onto main at 8defeac or later and let CI run again. A plain re-run keeps the old merge base, which is d2ae537. Check (Linux) fails in the_stinger_source_recovers_after_a_clip_a_consumer_refused and a_prores_clip_after_another_format_still_airs (backend/tests/common/stinger.rs). main failed the same family at d2ae537 (run 38369031375) and is green on Linux at 8defeac (run 38695443269).

Claims

Claim Verdict Evidence
The input now answers caps queries the way it did before its parser was linked CONFIRMED backend/src/blocks/builtin/live_recorder/keepalive.rs:227 — let caps = q.filter_owned().unwrap_or_else(gst::Caps::new_any);. Before the link, identity's unlinked src returns the filter or ANY (EXTERNAL: gst_pad_peer_query_caps)
What cannot be recorded is still refused with an element error CONFIRMED The caps-event path is unchanged: backend/src/blocks/builtin/live_recorder/mod.rs:455 — refuse_input(&input, &reason);
The test fails if the probe is removed EXTERNAL It builds the block through add_live_recorder and queries through the real identity. It fails on revert only if isofmp4mux answers with its current caps. The author's revert output shows it does on 1.28.6
The new test runs in CI CONFIRMED Check (Linux) at 18452df: the_input_answers_caps_queries_without_the_muxers_current_caps ... ok. init() asserts its elements are present, so it cannot skip

Diagnosis — The late link is real and happens once. backend/src/blocks/builtin/live_recorder/mod.rs:412 — if parser_inserted.swap(true, Ordering::SeqCst) { links the parser on the first caps event. After that, queries reach a muxer that has pinned its caps. Answering at the consumer's own input is the Block Input Contract's answer, which is better than editing the encoder or the converter. Coverage is BOUNDED: it fixes renegotiation through the caps query. A codec change mid-stream (#931) and a fatal flow return through a shared tee (#962's class) remain, and the PR body says so.

One side effect: identity's accept-caps checks its own caps query, so it now accepts everything too. A refusal lands at the parser instead of the input. That is no worse than before, and it is already the behaviour before the link.

Radius — LOCAL: every Live Recorder track input in all three containers, and nothing else. The probe fires per query, not per buffer, and the closure captures nothing. It follows the existing QUERY_DOWNSTREAM probe on the queue (keepalive.rs:245).

Overlaps — #1057 appends to the same test file and edits live_recorder/mod.rs; the overlap is textual and either can land first. #931 and #962 own the remaining classes. This PR settles neither.

Tests & CI — Linux builds, WASM and API contract are green. The 54 gst_mini_object_unref CRITICALs in live_recorder_test also appear on main (d2ae537, same count), so they are not from this diff. macOS and Windows were skipped. The change is platform-neutral, so Linux green is enough.

Confidence: HIGH

wagenet and others added 2 commits October 15, 2026 15:22
Once isofmp4mux has caps, it answers a caps query with exactly those,
including fields the encoder added itself (vtenc's chroma-site). Passed
through the input, that answer reached the encoder, which then refused its
own input caps when the parser link's reconfigure made it check them. On a
WHIP input feeding a per-guest recording, the resulting not-negotiated
stopped the guest's whole input.

Each track input now answers caps queries the way it did before its parser
was linked. What cannot be recorded is still refused, with a message, when
its caps arrive.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The input's identity already accepts any caps, so answering ACCEPT_CAPS
in the probe changed nothing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@wagenet
wagenet force-pushed the wagenet/inrec-caps-change branch from 18452df to a9c823b Compare October 15, 2026 22:22

@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.

Supersedes my review at 18452df. The verdict stands at Comment. The new head is a plain rebase onto main@8fce2ee, as I asked: the patch to live_recorder/ and live_recorder_test.rs is byte-identical to the one I reviewed. Only CI is new, and it is still red, in a test this diff does not reach.

Verdict: Comment — right fix at the right layer. I cannot approve while Check (Linux) is red, even though the failure is not from this diff.

For the maintainer (nothing for the author)

  1. Re-run the failed job: gh run rerun 38816640192 --failed. If stinger_prores_test goes red again on this branch but stays green on main, it is worth a closer look. If it flips, the test is flaky and needs its own issue.

Claims

Claim Verdict Evidence
The input answers caps queries as it did before its parser was linked CONFIRMED Same as in my review of 2026-10-15. backend/src/blocks/builtin/live_recorder/keepalive.rs:227 — let caps = q.filter_owned().unwrap_or_else(gst::Caps::new_any);
What cannot be recorded is still refused with an element error CONFIRMED Same as in my review of 2026-10-15. backend/src/blocks/builtin/live_recorder/mod.rs:455 — refuse_input(&input, &reason);
The new guard test runs in CI CONFIRMED Check (Linux) at a9c823b: the_input_answers_caps_queries_without_the_muxers_current_caps ... ok, live_recorder_test 19 passed
The red check is not from this diff CONFIRMED It fails in stinger_prores_test (3 of 4 tests), at backend/tests/common/stinger.rs:784 — assert_eq!(report.frames_arrived, n, "{report:?}"); with left: 29 right: 30. Neither file references the Live Recorder. main passed all four at the same base (run 38716278615)

Diagnosis: unchanged from my review of 2026-10-15. The parser is linked late, once, at backend/src/blocks/builtin/live_recorder/mod.rs:412 — if parser_inserted.swap(true, Ordering::SeqCst) {. Answering the caps query at the consumer's own input follows the Block Input Contract. Coverage is BOUNDED: a codec change mid-stream (#931) and a fatal flow return through a shared tee (#962) are not fixed here, and the PR body says so.

Radius: LOCAL. The probe covers every Live Recorder track input, in all three containers. It fires per query, not per buffer, and its closure captures nothing.

Overlaps: none that this PR settles. #1057 edits live_recorder/mod.rs and appends to the same test file. That overlap is textual only, and either PR can land first. #931 and #962 own the remaining failure classes.

Tests & CI: Builds (x86_64, ARM64), WASM and API Contract are green. The gst_mini_object_unref CRITICALs in this log appear on main at the same base too (187 there, 190 here), so they are not from this diff. macOS and Windows were skipped. The change is platform-neutral, so Linux green is enough once the re-run passes.

Confidence: HIGH

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