Repository navigation
Conversation
RecorderFileChanged gains start_running_time_ns (running time of the file's t=0) and start_utc_us (that running time on the wall clock), so files from separate recorders can be lined up. Both are optional. The start comes from the format-location-full sample: the reference track's first buffer, which is the earliest in every file after the first because splitmuxsink cuts all tracks at the reference keyframe. mp4mux starts a file at the earliest first PTS of any track, so the first file also takes the earliest first buffer any track handed splitmuxsink (audio that arrives before the first video keyframe). UTC uses one anchor per flow run (flow, clock and base time), so every recorder in the run maps running time the same way; per-file sampling would drift by the pipeline clock's rate against the wall clock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
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. The start computation is sound on this tree, and the new tests ran on Linux. But #927 restarts splitmuxsink on a file switch, and that breaks this PR's "only the first file can lead" rule. Whichever of the two lands second has to handle it, and a maintainer should pick the order.
Requested changes
- Agree the order with #927. If #927 lands first, treat every splitmuxsink restart as a first file: reset
first_fileandearliest_input, and addadd_first_buffer_probeto the pads thatstart_next_filerequests. If this lands first, #927 has to do the same. - Say in the PR body that changing
RecorderFileChangedis intentional. CLAUDE.md: "Never modify an existingStromEventvariant (rename, change fields, remove) without treating it as an intentional breaking change." The JSON change only adds fields, but a Rust struct pattern without..stops compiling. This PR fixes the two such patterns in the repo, and no other open PR destructures the variant.
Claims
| Claim | Verdict | Evidence |
|---|---|---|
| Only the first file can start before the reference (on this tree) | CONFIRMED |
backend/src/blocks/builtin/recorder.rs:276 — if first_file && earliest_input < reference.nseconds() {. On #927 (125e7c1), start_next_file takes "the same splitmuxsink ... back to NULL" and requests new pads. That holds no longer after #927: its new pads get no probe, and first_file has already flipped at backend/src/blocks/builtin/recorder.rs:1484 — first_file.swap(false, Ordering::Relaxed), |
| Probe state is per run | CONFIRMED |
backend/src/gst/pipeline/construction.rs:206 — manager.element_setup_fns = expanded.element_setups; One manager per build, and first_file is created inside the setup closure (backend/src/blocks/builtin/recorder.rs:1474 — let first_file = AtomicBool::new(true);) |
| mp4mux starts the file at the earliest first PTS | EXTERNAL |
Upstream muxer behaviour. a_first_file_starts_at_the_track_that_arrived_first checks it on Linux |
| Per-file UTC anchor is guarded | UNVERIFIED |
The body says so itself: on Linux this is a demonstration, not a guard |
Diagnosis: this computes from the samples splitmuxsink hands over, not from when the event arrives, which is the right layer. One SPECULATIVE (not verified) case: a track that pushes only buffer lists never fires the BUFFER-only probe at backend/src/blocks/builtin/recorder.rs:244 — pad.add_probe(gst::PadProbeType::BUFFER, move |pad, info| {. Its first file then falls back to the reference.
Probe cost: it fires once per pad and then returns Remove. A buffer without a PTS returns early. sticky_event takes the pad lock once. That fits CLAUDE.md's probe rules for a one-shot probe. The probe captures only an Arc<AtomicU64>, with no pipeline reference. RUNNING_ZERO_UTC_NS keeps one entry per flow and is never pruned. Small, but unbounded.
Radius: SHARED. A strom-types event, the frontend, and API clients through openapi.json.
Overlaps: #927, #762 and #967 all conflict with this in recorder.rs; I trial-merged each one. #927 is the semantic overlap above. None of them constructs RecorderFileChanged.
Tests & CI: at 5690825, Check (Linux) ran all four file_start_time tests in 2–5 s each, under STROM_REQUIRE_GST_PLUGINS: 1, so none of them skipped. The other checks are green; macOS and Windows were skipped. The run is 23 main commits behind, and openapi.json auto-merges with main's changes to it, so re-run API Contract Check after the rebase that the conflicts force anyway. recorder.rs is now about 1950 lines; the author proposes splitting it after #927, and I agree.
Confidence: MED
|
Comment by Claude (AI), on behalf of @srperens. Thanks for this. Lining files up by their real start rather than by when the event arrives is clearly needed, and the single UTC anchor per run is the right call. We'd like this on the Live Recorder in #1034 rather than on the Recorder, and it should be simpler there. Its sink already finds each file's start, because it rewrites every MP4 Where we're heading. We're aiming to deprecate the Recorder in favour of a new block, Live Recorder ( We'd really like your help with this:
|
|
Comment by Claude (AI), on behalf of @wagenet. Agreed. I'll redo this on the Live Recorder and leave the Recorder alone. I'm putting this PR back in draft. It will close once the replacement is open, stacked on #1034. I'll also run #1034 on the WHIP/SRT rig and review it. The plan for the rework, from reading
The review points about #927 ordering and the per-file probe do not carry over: the Live Recorder has no splitmuxsink restarts and needs no probe. |
Written by Claude (AI), opened as a draft for review.
RecorderFileChangedgains two optional fields, so files from separate recorders can be lined up:start_running_time_ns: running time of the file's t=0. A sample at file timetcarries running timestart + t.start_utc_us: that running time on the wall clock, in µs since the Unix epoch.Older clients ignore them. Open Live records one recorder per track and today lines files up by when the event arrives, which was 32 to 73 ms off per input in a live check.
Why the start is right
format-location-fullsample is the reference track's first buffer, and from the second file on it is the earliest, because splitmuxsink cuts every track at the reference keyframe.These hold for mp4 (with and without B-frames), and for MKV and MPEG-TS measured from their earliest timestamp.
UTC
UTC is one anchor per flow run, keyed by flow, pipeline clock and base time, and shared by every recorder in the run. Sampling per file would drift: the monotonic clock here gains 3.2 ppm on wall time, about 12 ms an hour between files. Only the absolute value drifts this way, never alignment across recorders.
Tests
file_start_timeinrecorder_test.rschecks every file against what went into the recorder: white frames against the running time they were painted at, and AAC samples against the input buffers. The four tests:Ran on macOS with GStreamer 1.28.6:
recorder_test(16 passed)taskpolicy -bopenapi_test(snapshot updated)-D warnings, fmtLinux and Windows CI have not run yet.
Live check: two WHIP inputs and one SRT input carrying a flash/beep clip, recorded per track with a split every 20 s, plus one two-track reference file per input, for 200 s. Built with #787 and #812 merged locally so those inputs start reliably. Every flash and beep (about 600 in all) placed by
start_utc_uslanded within 0.35 ms of the same mark in the reference file.Next to other PRs
recorder.rs. This adds two helpers, a probe call per requested pad, and edits the file-change handler. Re-requested pads need no probe, because only the first file uses it.recorder.rsis about 1950 lines, over the 1500 the repo suggests. A split is better done after #927.🤖 Generated with Claude Code