Repository navigation
Conversation
|
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. |
407e25b to
c8a8767
Compare
srperens
left a comment
There was a problem hiding this comment.
Verdict: Request changes. The mechanism is right and the per-buffer path is clean. CI only guards half of the fix, though. The caller-change reset (c8a8767) has no test that runs in CI, and main has since landed what the skip gate was waiting for.
Requested changes
- Rebase onto main, then re-test the version gate. The branch is 83 commits behind (merge-base
eae2467). The gate's reason is #856:backend/tests/mpegtssrt_input_stale_head_test.rs:401—(major, minor, micro) >= (1, 24, 3)— "tsdemux then replaces its source pad, which the block does not relink". #877 (b28be77) is now on main, and it relinks exactly that replacement (link_to_output, "tsdemuxadds the new pad first"). If both second-caller tests pass on 1.24.2 after the rebase, drop the gate.git merge-tree origin/main pr813merges clean. - If the gate has to stay, guard the wiring another way.
a_caller_change_drops_the_offsetcallsreset()directly (backend/src/blocks/builtin/tsdemux_anchor.rs:343—anchor.state.reset();). Deletingwatch_callerwould fail nothing that runs in CI. A unit test can build a realsrtsrc, apply a correction,emit_by_name("caller-added", ...), and assertcorrection == 0. That would coverbackend/src/blocks/builtin/tsdemux_anchor.rs:125—state.reset();.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Probes do atomics only, no lock or allocation on the per-buffer path | CONFIRMED |
input probe: backend/src/blocks/builtin/tsdemux_anchor.rs:111 — state.last_input.store(pts.nseconds(), Ordering::Relaxed);. In the output probe, only the correction branch leaves atomics: info! and backend/src/blocks/builtin/tsdemux_anchor.rs:281 — pad.set_offset(offset);, which takes the pad's object lock. That happens once per correction, not per buffer |
| No strong element reference in a signal closure | CONFIRMED |
backend/src/blocks/builtin/tsdemux_anchor.rs:135 — let anchor = self.clone(); clones an Arc<State>. Every handler captures that state, never an element |
| The stale-head correction is guarded in CI | CONFIRMED |
Check (Linux) job 107386718642: stale_head_is_not_stamped_ahead_{passthrough,decoded} ... ok over ~3.6 s, so they ran |
| The offset reaches the buffer that triggered it | EXTERNAL |
assumes gst_pad_push_data re-sends the sticky SEGMENT after buffer probes. The 25/49 ms residual is consistent with that |
Repo rules. CLAUDE.md: "A test that requires a GStreamer element must be able to run in CI. Tests that skip ... pass green and guard nothing". In the same job, both second_caller_* tests print ok within 1 ms of the binary starting. That is the gate's early return. The body says so; requested change 2 closes it.
Diagnosis. The body traces the root cause into tsdemux and reproduces it without Strom, so this is the right layer. Coverage is BOUNDED, and the body names the bounds: ignore_pcr=true only, listener mode only for the reset, and a first ~1 s still stamped ahead. CLOSED loop: on the healthy path, atomics per buffer and no set_offset.
Radius. LOCAL: one block. It is active on every mpegtssrt_input with the default ignore_pcr, in both decode and passthrough.
Overlaps.
- #812 changes the same file in different hunks. Pairwise merge is clean, and the body's table shows the two combined. Either can land first.
- #936 consolidates SRT test port helpers. This PR adds another
srt_port(), so whichever lands second should adopt the shared one.
Tests & CI. Eight checks green at 8bcd79b, on a base 83 commits old. Both tsdemux_anchor unit tests and both stale-head tests ran. macOS and Windows show skipping, and nothing here is platform-specific.
Confidence: HIGH
8bcd79b to
9e73d31
Compare
|
Claude (AI), on behalf of @wagenet. Addressed both requested changes:
#936 has not landed, so the local |
srperens
left a comment
There was a problem hiding this comment.
This supersedes my Request changes review of 8bcd79b. Both requested changes are done. The verdict moves to Comment, not Approve, because #997 was filed since that review, and it reports this mechanism firing on healthy streams.
Verdict: Comment. The rebase, the removed gate and CI coverage are all correct. Whether this lands as it is now, or after #997 settles the trigger rule, is a maintainer's call.
Options for a maintainer
- Land as is. #997 then becomes a follow-up against main.
- Wait for #997 and change nothing here. #997 owns the threshold, per-pad measurement and the "never step backwards mid-stream" rule, and all three are in this PR's code.
- Narrow it here first. For example, only correct during an initial window after
caller-added, which is when the stale head this PR targets occurs. A late re-anchor is what #997 sees going wrong.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| The reconnect tests now run on CI's 1.24.2 and guard the caller-change wiring | CONFIRMED |
9e73d31 removes the version gate. In Check (Linux) job 110867228063, second_caller_resuming_is_not_stamped_behind and second_caller_restarting_is_not_stamped_behind report ok about 3.2 s after the stale-head pair, so they ran rather than returned early. |
| The rest of the series is unchanged since my last review | CONFIRMED |
git range-diff shows commits 1 to 4 patch-identical to 557d444..8bcd79b. The probe-cost and WeakRef rows stay CONFIRMED as in my review of 2026-10-01. |
| A correction can fire at any point in a connection, across both pads, and step running time backwards | CONFIRMED |
backend/src/blocks/builtin/tsdemux_anchor.rs:259 — if min_lead <= MAX_LEAD.nseconds() as i64 { is the only gate, and nothing limits it to the start of a connection. The window's minimum is taken over every demuxer pad's samples together. The shift is backend/src/blocks/builtin/tsdemux_anchor.rs:266 — let correction = correction - mean_lead;. |
| Healthy callers are never re-anchored | UNVERIFIED |
The body tested 128 and 32 kbit/s AAC. #997 reports video holding 290–440 ms of lead from its first-PES burst, and audio PES of about 11 AAC frames (ffmpeg's default TS muxing) at 100–230 ms. That occasionally gives a window minimum of 201–205 ms against backend/src/blocks/builtin/tsdemux_anchor.rs:49 — pub const MAX_LEAD: gst::ClockTime = gst::ClockTime::from_mseconds(200);. The result is a mid-stream step of about −380 ms and decreasing DTS in mp4mux. That reproduction ran outside this repo. The PR's tests do not cover this stream shape. |
Diagnosis. The stale-head cause and the fix layer stand as in my earlier review. Coverage is BOUNDED, by the same bounds the body names. #997 adds a cost on the healthy path that the body does not list: the loop is CLOSED, but its trigger is a fixed 200 ms on the combined minimum, and the healthy lead range for some senders reaches it. #997 says it was seen on "Strom 68632494 (main)", but tsdemux_anchor.rs is not on this repo's main, so that build must have carried this branch.
Radius. LOCAL. It is one block, active on every mpegtssrt_input with the default ignore_pcr.
Overlaps.
- #997 owns the design decision on when this anchor may fire (see the options above).
- #812 changes the same file in different hunks, and either can land first.
- #936: whichever of the two lands second adopts the shared
srt_port().
Tests & CI. At 9e73d31, Check (Linux), both Linux builds, WASM, API Contract Check and Static JS Tests pass. macOS and Windows were skipped, and nothing here is platform-specific. The branch is 31 commits behind main, and git merge-tree against main is clean. Nothing was built or run here.
Confidence: HIGH
Superseded by my review of 9e73d31: the requested changes are addressed.
|
Posted by Claude (Anthropic's AI assistant) on behalf of @wagenet. #997 is fixed in this PR as of The fix follows option 3 from the last review:
|
With ignore-pcr, tsdemux maps PTS to output time from the first PES it parses and never takes a new reference. An SRT caller that encoded while it waited for the listener sends a few frames from the start of its encode, then live frames. The stale frame becomes the reference, so live audio and video are stamped as far into the future as the caller waited, and every downstream sink and mixer holds the source back by that much for the whole connection. TsDemuxAnchor compares each demuxed buffer's running time with the latest srtsrc timestamp (arrival less SRT transit). When the smallest lead over a 500 ms window exceeds 200 ms, it moves all of the demuxer's source pads back by the window's mean lead with one shared pad offset. The probes use buffer timestamps and atomics only. PCR mode is left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The replay dropped every packet between the head and the live part, which broke the table continuity counters as well as the media. GStreamer 1.24.2, which Ubuntu 24.04 and CI ship, then re-applies the PMT and, with ignore-pcr, mistakes it for a new program (fixed upstream in 1.24.3). tsdemux replaces its source pad, the new pad is unlinked, and srtsrc stops with not-linked after the head, so no buffers reached the correction window. Forward the table packets from the gap and drop only the media. The PTS jump the test guards is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
srtsrc keeps listening after a caller leaves and tsdemux takes a fresh time reference for the next one, but the pad offset stayed. A second caller with a clean stream left the block as far behind its arrival as the first caller's stale head had been ahead of it (-2041 ms measured), where every consumer that waits for running time drops it as late. Nothing brought it back, because the offset only ever moved further back. Drop the correction on srtsrc's caller-added and caller-removed. Output that sits behind arrival is still left alone: a sender that stalls and resumes looks the same from here, and giving the offset back would throw its stream forward by that much in one buffer. The replay test gains a second caller in both shapes, resuming and restarting, and the module gains unit tests for what a caller change and a window spent behind arrival each do to the offset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Before 1.24.3 a re-parsed PMT reads as a new program under ignore-pcr, and a caller change re-parses it. tsdemux replaces its source pad, the block does not relink it (Eyevinn#856), and the second caller delivers no audio at all -- so the reconnect tests cannot pass on Ubuntu 24.04 or CI, whatever the anchor does. Skip them below 1.24.3 with the version in the message. They run on the release image, which ships 1.26, and on development machines. CI keeps the two stale-head tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Eyevinn#877 relinks the pad tsdemux swaps in when 1.24.2 re-parses the PMT, so a second caller now delivers audio there too. Both reconnect tests pass on 1.24.2 (Ubuntu 24.04, as CI) and fail there when the caller-change reset is removed, so CI guards it. This reverts commit de6cd99. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A healthy stream runs ahead of arrival by what the sender packs together: multi-frame audio PES and the video burst at connect. Windows of such a stream reached a 205 ms minimum lead, crossed the 200 ms threshold mid-stream, and the shift stepped running time back about 380 ms, so recorders wrote decreasing DTS. A stale head only occurs at connect, so a correction may now be made only in the first 3 s of a caller's input, at most once, and only above 500 ms. A caller change re-arms the watcher. The integration tests keep their own 200 ms on-time bound instead of borrowing MAX_LEAD. Fixes Eyevinn#997 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Rust 1.98's clippy denies chunks_exact with a constant size. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5d248ad to
6f6648a
Compare
srperens
left a comment
There was a problem hiding this comment.
This supersedes my Comment review of 9e73d31. The verdict moves to Approve. The author implemented option 3 from that review, correcting only at connect, and #997, which owned the trigger rule, is now closed.
Verdict: Approve. A correction can now fire only within 3 s of a caller's first input, and only once per caller. The threshold of 500 ms sits above the healthy lead that #997 measured. Guard tests cover each part, and all of them ran green at this head.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| A correction is gated to the arm period and disarms after it fires | CONFIRMED |
The gate at line 267, quoted under the table, returns before any measurement. :275 disarms once input passes the deadline, and :311 disarms after a correction |
| The threshold clears #997's healthy lead | CONFIRMED |
backend/src/blocks/builtin/tsdemux_anchor.rs:57 — pub const MAX_LEAD: gst::ClockTime = gst::ClockTime::from_mseconds(500); against the 440 ms video lead at connect. a_healthy_stream_is_never_re_anchored replays that shape and would fail on the old 200 ms rule, whose minimum of 230 ms exceeds 200 |
| The probes stay within CLAUDE.md's BUFFER-probe rules | CONFIRMED |
The input probe adds one load and one store (backend/src/blocks/builtin/tsdemux_anchor.rs:253 — if state.arm_deadline.load(Ordering::Relaxed) == ARM_PENDING {). Once disarmed, the output probe returns after two loads. The only formatting is the info! on the single correction, as before |
| A caller change re-arms the watcher, and CI guards that wiring | CONFIRMED |
reset() stores ARM_PENDING. In Check (Linux) job 112511813527 at 6f6648a, second_caller_resuming_is_not_stamped_behind, second_caller_restarting_is_not_stamped_behind and a_caller_change_drops_the_offset_and_re_arms all report ok |
The arm gate: backend/src/blocks/builtin/tsdemux_anchor.rs:267 — if deadline == DISARMED || deadline == ARM_PENDING {
Diagnosis. The right layer, and the loop is now CLOSED with a bound: on the healthy path the probe pays two atomic loads per buffer after 3 s. Coverage is BOUNDED, and the body names all three bounds:
- a stale head under 500 ms goes uncorrected;
- nothing is corrected after the first 3 s;
mode=callernever re-arms, becausesrtsrcemits nocaller-addedthere.
The earlier rule shows why a narrower trigger is the better trade: a false correction writes backwards timestamps into every recording, while a missed one only leaves a source late. The rejected "give the offset back when behind" alternative is in the body with its measured failure.
Overlaps. #997 is closed, so no open item owns this decision. git merge-tree is clean against current main (35 commits ahead of this branch), against #812 and against #936.
Radius. LOCAL: one block, active on every mpegtssrt_input with the default ignore_pcr.
Tests & CI. At 6f6648a, Check (Linux), both Linux builds, WASM, API Contract and Static JS pass, and all four unit tests and four integration tests ran ok there. macOS and Windows were skipped, and nothing here is platform-specific. The CI base is 35 commits behind main. The merge is clean but has not been built.
Confidence: HIGH
Opened by Claude (AI) on behalf of @wagenet.
Problem
When an SRT caller has been encoding before it connects to
builtin.mpegtssrt_input(typically because it started before the flow and kept retrying), its media leaves the block stamped as far into the future as the caller waited. That lasts for the whole connection, and every downstream sink and mixer holds the source back by the same amount. On the Open Live rig, a caller that waited ~1.2 s reached program at ~1460 ms instead of ~525 ms.Cause
With
ignore-pcr=true(the block default), tsdemux maps PTS to output time from a single reference: the upstream timestamp of the buffer that carried the first PES it parsed, and it never takes a new one (mpegtspacketizer.c,mpegts_packetizer_pts_to_ts_internal: "Take the first base time we see as a reference"). Its ">5 s" catch-up moves both sides of the mapping together, so it keeps any error.A caller that waited sends a few frames from the start of its encode and then jumps to live.
GST_DEBUG=tsdemux:5, sender started 2.2 s before the listener:srtsrc's own timestamps stay within ~5 ms of arrival; the error is only in the demuxer's reference. The bug reproduces without Strom:srtsrc ! decodebinmeasured 2448 ms end to end against 215 ms, with ~200 ms arrival delay in both runs.Not a base_time change
The original report suspected srtsrc buffers produced before PLAYING carried a different base_time. Traced trials show a single base_time per pipeline run. The "450–900 ms" in that report was the multi-second shift above, aliased by pairing ticks sent every 1.5 s. With 4 s ticks, the penalty tracks how long the caller waited.
Fix
TsDemuxAnchor(blocks/builtin/tsdemux_anchor.rs) is wired in whenignore_pcr=true, in both decode and passthrough modes. A BUFFER probe onsrtsrc:srcrecords the latest input timestamp. A probe on each tsdemux source pad measures how far the buffer's running time is ahead of that timestamp (its lead).Healthy lead depends on how the sender packs its stream. All PES completed within one SRT message are placed from the same reference, which gives a sawtooth of about ±50 ms for our test sender. ffmpeg's default TS muxing, with ~11 AAC frames per PES and a video burst at connect, holds audio 100–230 ms and video 290–440 ms ahead, and the smallest lead in a window has reached 205 ms (#997). When the smallest lead over a 500 ms window exceeds 500 ms, all of the demuxer's source pads get one shared pad offset of minus the window's mean lead. Audio and video move together, and the sawtooth ends up centred where a clean connect leaves it.
A stale head only happens at connect, so a correction may only be made in the first 3 s of a caller's input, and only once. After that the output probe only keeps the offset applied. A shift made mid-stream steps running time backwards, which recorders write as decreasing DTS: #997 saw a healthy stream re-anchored by about −380 ms 1–22 s into a connection, when the threshold was 200 ms and the rule had no time limit.
The offset belongs to the caller it was measured on.
keep_listeningis on by default, so onesrtsrcserves caller after caller and tsdemux takes a fresh reference for each, which means an offset left over from the previous caller stamps the next one that far behind its arrival — where every consumer that waits for running time drops it. So the offset is dropped onsrtsrc'scaller-addedandcaller-removed, and the watcher re-arms for the next caller's first 3 s. Output that sits behind arrival is otherwise left alone: a sender that stalls and resumes looks identical from here, and giving the offset back would throw its stream forward by that much in one buffer.The probes use buffer timestamps and atomics only: no clock reads, locks or allocation.
set_offsetruns only when a correction is made. PCR mode is untouched. Withignore-pcr=false, tsdemux recovers from the same stream on its own, but it costs 90–130 ms steady state and would change a default.Evidence
The rig is the Open Live program flow: WHIP seats plus this SRT input ("C") into
builtin.mixer, out over WHEP, with mix-minus returns from a second flow. Release builds, a fresh backend per trial, builds interleaved. C sends a tick every 4 s. Early: the caller starts 1 s before the flow start call. Late: the caller starts 1 s after the call returns. Each cell gives per-trial medians (ms) of C's delay at the program output / seat A's return.eae2467An earlier, non-interleaved round agrees: upstream early/125 on program measured 1434–1488 ms over 8 trials. The correction logged a 935 ms shift; it fired once in every early trial and never in a late one. These trials ran on
557d444, before the 3 s arm period and the 500 ms threshold. A caller that waited ~1.2 s produced a 935 ms correction about 1 s in, which both still allow; the rig has not been re-run on the head.Tests
mpegtssrt_input_stale_head_testbuilds the real block in decode and passthrough mode and connects to it over SRT. It sends 150 ms from the start of a pre-encoded AAC transport stream, then the same stream from 2 s in, paced in real time. After two windows, no buffer leavingaudio_output_0may be 200 ms or more ahead of the latest SRT input (the test's ownON_TIMEbound, kept at the old threshold rather than followingMAX_LEADup).Two more tests in the same file run a second caller with a clean stream after the corrected one, with
keep_listening=true: one where the encoder resumes and one where it restarts from zero. 300 ms after the second caller's first buffer, nothing may be 200 ms off its arrival in either direction.All four run on every GStreamer version, CI's 1.24.2 included. On 1.24.2 the caller change re-parses the PMT and tsdemux replaces its source pad (the equality bug below); #877 relinks that pad, so the second caller now delivers audio there too.
Four unit tests in
tsdemux_anchor.rsdriveon_input_buffer/on_output_bufferdirectly and run everywhere:a_healthy_stream_is_never_re_anchored: tsdemux re-anchor fires on healthy SRT inputs and steps recordings ~0.38 s backwards #997's stream shape, 440 / 230 ms leads through the arm period, then a 205 ms window at 10 s. No offset.a_stale_head_is_re_anchored_once: a 2 s lead is corrected; a further 1 s lead in the next window is not.no_correction_after_the_arm_period: a 2 s lead first seen 4 s into the connection is left alone.a_caller_change_drops_the_offset_and_re_arms: the next caller starts unshifted, and a later caller's stale head is corrected.The replay drops the audio between the two parts but forwards the PAT/PMT packets (the tables describing the streams) from the gap. Without them, GStreamer 1.24.2 — what Ubuntu 24.04 and CI ship — re-applies the PMT after the continuity break and, with
ignore-pcr, treats it as a new program (fixed upstream in 1.24.3, "mpegtsbase: Fix Program equality check"): tsdemux replaces its source pad, the new pad was not linked before #877, andsrtsrcstoppednot-linkedafter the head. That was the first CI failure, not load — the test passed 8/8 under 14 and 40 busy loops on macOS.Each test fails when the behaviour it guards is reverted. Run on macOS, GStreamer 1.28.6, at the head:
resetnot clearing the offsetMAX_LEADback to 200 msa_healthy_stream_is_never_re_anchoredfailsno_correction_after_the_arm_periodfailsa_stale_head_is_re_anchored_oncefailsa_caller_change_drops_the_offset_and_re_armsfailsResidual after correction, at the head: 25 ms decoded and 49 ms passthrough against the 200 ms limit; second caller −4 to +84 ms.
On
fcd773f, in an Ubuntu 24.04 container matching CI's GStreamer 1.24.2 and libsrt 1.5.3 (arm64, not CI's x86_64): the two stale-head tests passed 3 runs idle and 5 under 32 busy loops on 16 cores, failed with the anchor disabled by 2085 / 2064 ms, and failed with the earlier replay (PSI dropped) with 0 buffers after the window — the CI failure. After rebasing onto3225792(which has #877), same container: all four tests passed 6/6 runs withSTROM_REQUIRE_GST_PLUGINS=1, and with thecaller-added/caller-removedreset removed both second-caller tests failed. That is the check that CI now guards thewatch_callerwiring, not onlyreset().At
6f6648a(rebased onto main, Rust 1.98.1), on macOS: all four integration tests (STROM_REQUIRE_GST_PLUGINS=1), the four unit tests, and CI's clippy command (--features efp -- -D warnings). The last four reverts in the table ran on the same change before the rebase (5d248ad); the rows above them ran on the earlier head and were not repeated. The #997 stream shape is covered only by the unit test; its ffmpeg repro is outside this repo and was not run against this commit.Also run on macOS at
9e73d31:pipeline_lifecycle_test(the closure-leak guard, 3 passed) andcargo test --lib tsdemux_anchor. On the earlier commit: merged with #812, together with #812'smpegtssrt_absent_caller_test;mpegtssrt_streamheader_test,cargo test --lib. Clippy and fmt ran through the pre-commit hook on every commit. Not run locally: the rest of the integration suite.The test needs plugins-bad (
srtsrc,srtsink,tsdemux,mpegtsmux) and gst-libav (AAC), both already in the CI package list.Two adversarial passes on the earlier head drove the real block through what the tests do not cover, and these held: healthy 128 kbit/s and 32 kbit/s callers are left alone (the 32 kbit/s sawtooth peaked at 85 ms); a 700 ms SRT stall and its catch-up is not mistaken for a stale head; an H.264 + AAC stale head is corrected on both pads at once, 12 ms apart; three callers in a row are each corrected independently; a second caller with its own stale head is corrected rather than inheriting anything; a stray second SRT connection disturbs nothing. They also found the two failures fixed above.
Risks for review
queue ! fakesink sync=true, the shape of every consuming block: delivery peaks 909 ms (decoded) and 974 ms (passthrough) of wall clock behind arrival, recovers to 16–102 ms once the correction lands, andsrtsrcdelivers nothing for about that second. Upstream, without the fix, the same measurement peaks at 956 ms and never recovers. So the transient is not removed, only bounded — under a 125 ms SRT receive buffer it is a window where loss is possible. A faster decision on a large jump would close it and is not in this PR.mode=calleris not covered. Dialling out to a remote listener produces nocaller-added, so if that peer restarts after a correction has been made, its stream stays shifted and runs late until the flow restarts. Undoing the shift whenever the output sits behind arrival was tried and removed: it cannot tell that case from a sender that stalled, and it made a 900 ms sender hiccup jump the output 852 ms forward in one buffer and left a recovering SRT stall parked ~200 ms early. A signal that distinguishes them is follow-up work.mpegtssrt_input.rsand merges cleanly. fix(mpegtssrt_input): keep an absent SRT peer from holding the flow #812 decides when the decodebin starts; this PR corrects timestamps once tsdemux exists. The merged build is in the table.srtsrc(keep-listening=true) hung once in ~10 test runs, ingst_pad_stop_taskwhile srtsrc waited for a new caller. The test stops the receiver first.Also fixes #997, which only occurred with this PR's earlier code and is closed.
🤖 Generated with Claude Code