Repository navigation
fix(live recorder): answer caps queries at each track input so a late link cannot fail the encoder - #1075
fix(live recorder): answer caps queries at each track input so a late link cannot fail the encoder#1075wagenet wants to merge 2 commits into
Conversation
cdc56ec to
010a72a
Compare
|
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
left a comment
There was a problem hiding this comment.
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
- Merge or rebase onto
mainat8defeacor later and let CI run again. A plain re-run keeps the old merge base, which isd2ae537.Check (Linux)fails inthe_stinger_source_recovers_after_a_clip_a_consumer_refusedanda_prores_clip_after_another_format_still_airs(backend/tests/common/stinger.rs).mainfailed the same family atd2ae537(run 38369031375) and is green on Linux at8defeac(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
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>
18452df to
a9c823b
Compare
srperens
left a comment
There was a problem hiding this comment.
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)
- Re-run the failed job:
gh run rerun 38816640192 --failed. Ifstinger_prores_testgoes red again on this branch but stays green onmain, 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
Problem
Once
isofmp4muxhas 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.vtencwriteschroma-site=jpeg; WebRTC-decoded video has none.GstVideoEncoder's caps proxy copies the field, sovtencthen 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:
builtin.videoenc.Evidence
the_input_answers_caps_queries_without_the_muxers_current_capsfails with the probe disabled: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.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-webrtc0.15.2,whip_signaller/imp.rs:685, unrelated to this change); trials 6 and 7 were re-run after a restart.vtencbehindvideoconvert, a forced RECONFIGURE after the muxer has caps) passes with and without this change, because the converter alone renegotiates tochroma-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_testat 18452df: 19 passed, including the new test.cargo clippy --all-targets -- -D warnings,cargo fmt: clean.Not run: the Linux and Windows CI jobs locally. The new test needs
x264encandisofmp4mux, which the existing Live Recorder tests already require, so it runs wherever they run.For the reviewer
x264encinto mp4, mkv and mpegts, andvtencinto mp4 and mkv).Out of scope
teeandqueuepass fatal flow returns upstream. That needs its own design.isofmp4muxpins 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