Skip to content

feat(recorder): report when each file's timeline starts - #944

Closed
wagenet wants to merge 1 commit into
Eyevinn:mainfrom
wagenet:wagenet/recorder-file-start-time
Closed

wagenet wants to merge 1 commit into
Eyevinn:mainfrom
wagenet:wagenet/recorder-file-start-time

Conversation

@wagenet

@wagenet wagenet commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Written by Claude (AI), opened as a draft for review.

RecorderFileChanged gains 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 time t carries running time start + 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

  • mp4mux starts a file at the earliest first PTS of any track. The format-location-full sample 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.
  • The first file can start earlier: a non-reference track keeps whatever it received before the first keyframe. So for the first file only, the start also takes the earliest first buffer any track handed splitmuxsink. That buffer is recorded by a one-shot probe per pad.

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_time in recorder_test.rs checks 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:

  • two video recorders that start and split at different times
  • one-track audio and one-track video recorders
  • a two-track first file whose audio leads by 0.5 s
  • a direct-timing flow re-run on another clock
Revert Fails
first file ignores the earliest track two-track test, 533 ms off
raw PTS instead of running time all, 1 h off (x264enc's offset)
per-file UTC anchor anchors spread 21 µs in 4 s. macOS only: Linux disciplines both clocks together, so this guard likely passes there
anchor not keyed on clock re-run test, UTC 56 years off

Ran on macOS with GStreamer 1.28.6:

  • recorder_test (16 passed)
  • the new tests 5 times, plus 3 more under taskpolicy -b
  • openapi_test (snapshot updated)
  • workspace lib tests, clippy -D warnings, fmt

Linux 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_us landed within 0.35 ms of the same mark in the reference file.

Next to other PRs

recorder.rs is about 1950 lines, over the 1500 the repo suggests. A split is better done after #927.

🤖 Generated with Claude Code

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

srperens commented Oct 2, 2026

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

  1. Agree the order with #927. If #927 lands first, treat every splitmuxsink restart as a first file: reset first_file and earliest_input, and add add_first_buffer_probe to the pads that start_next_file requests. If this lands first, #927 has to do the same.
  2. Say in the PR body that changing RecorderFileChanged is intentional. CLAUDE.md: "Never modify an existing StromEvent variant (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

@srperens

srperens commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

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 tfdt and Matroska cluster timestamp so that each file starts at zero. Would you be up for reworking this onto #1034 once you've had a look at it?

Where we're heading. We're aiming to deprecate the Recorder in favour of a new block, Live Recorder (builtin.liverecorder), in #1034. It replaces splitmuxsink with a live aggregator muxer (isofmp4mux, matroskamux or mpegtsmux) and a small sink of our own that does the splitting. A track that stops then holds nothing up, and when it comes back it continues in the same file. There is no watchdog, no ended track, and no new file per stall. Both blocks will exist side by side until the new one has run in real productions. The Recorder stays as it is in the meantime.

We'd really like your help with this:

  • Test it on the rigs where you found these bugs (WHIP guests, SRT inputs, per-seat and program recordings). Swap builtin.recorder for builtin.liverecorder; the pads and properties are the same. It builds and runs on GStreamer 1.24 and later, like the rest of Strom.
  • Tell us what you think of the approach, and what it still lacks for your use.
  • Review feat(live recorder): a recorder where a quiet track holds nothing up #1034. You know the failure cases better than anyone, and its "Out of scope" section lists the gaps we know about.

@wagenet

wagenet commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

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 fragment_sink.rs:

  • Start of each file. Use the file_base the sink already takes for the tfdt/cluster shift. For MPEG-TS, use the first cut point's PTS, since TS times are not shifted. Then map that value to running time and UTC through the same single anchor per flow run as here.
  • When the event fires. For fMP4, the first file is opened by the init segment, before any moof, so file_base is still unknown when on_file_opened runs. I'll report the start when the base is first set (the first fragment of each file) rather than at open. Later files are fine: file_base is set before report_opened.
  • Needs a test, not an assumption. Whether isofmp4mux decode times and matroskamux cluster timestamps are running time or offset from it. If they are offset, the start needs that offset added back, and the test will show which.
  • Event fields. The rework carries the RecorderFileChanged field additions from here (start_running_time_ns, start_utc_us, both optional). The PR body will call them out as an intentional change to the variant.

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.

@wagenet

wagenet commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

Comment by Claude (AI), on behalf of @wagenet.

Replaced by #1042, on the Live Recorder (stacked on #1034).

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