feat(cli): publish stdin imports on the broadcast clock - #4122
Conversation
`moq import ts|fmp4|flv` published source timestamps verbatim against a wall sampled at startup, so a late first frame or a large starting PTS advertised the wrong wall time, and a restarted source rewound or was refused. The fMP4, TS, and FLV importers gain `live()`, which translates onto the catalog's broadcast clock through one shared mapping per source: the first frame is live on arrival, every track keeps its source offset, and a restart continues forward after the real idle gap. fMP4 passthrough rewrites `tfdt`. `SourceMap` now runs on the same per-source anchor, and `Clock::new` puts PTS zero ten seconds back so earlier-muxed frames still map. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
21d9105 to
6ddfab7
Compare
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ddfab7eed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let extended = Timestamp::from_scale(unwrap.unwrap(raw), 90_000)?; | ||
| Ok(Some(match anchor { | ||
| Some(anchor) => anchor.translate(&mut unwrap.lane, extended)?, |
There was a problem hiding this comment.
When an encoder resets without a PCR discontinuity after its raw PTS has passed half of the 33-bit range (about 13.25 hours), PtsUnwrap::unwrap interprets the jump to zero as a normal wrap and adds 2^33 before Anchor sees it. The translated source time therefore moves forward by hours, so the lane never detects a restart and moq import ts publishes frames far in the future instead of resuming after the idle gap. Restart detection needs to consider the raw PTS or arrival progression before wrap extension.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
A signalled PCR discontinuity_indicator already calls PtsUnwrap::discontinuity, which drops the wrap baseline and marks the lane to restart, so that reset re-anchors after the idle gap even past the halfway point of the 33-bit field.
An unsignalled drop of more than half the field is the case PtsUnwrap exists for: the ~26.5h wrap. Treating it as a restart before the unwrap would also re-anchor a real wrap. Unsignalled forward jumps stay as they are; that is the follow-up already noted in the PR.
(Written by Grok 4.7)
| let stepped_back = lane | ||
| .last_source | ||
| .is_some_and(|last| last > src + SourceMap::MAX_REORDER.as_micros()); | ||
| // A restart another lane already applied is adopted, not applied twice. | ||
| if (lane.restart || stepped_back) && lane.offset.is_some() && lane.generation == self.generation { | ||
| self.reanchor(src, now); |
There was a problem hiding this comment.
For live TS input, this classifies any per-stream PTS step backward by more than 500 ms as a restart, although the TS importer explicitly accepts reorder delays up to two seconds in reorder_delay. A valid low-frame-rate or deeply reordered GOP can therefore move the shared anchor instead of retaining its PTS ordering, shifting that video lane away from the audio until the latter independently detects a restart. Use DTS or the TS discontinuity signal for restart detection, or make this bound cover the reorder range already accepted by the importer. rs/moq-mux/CLAUDE.mdL6-L6
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
reorder_delay's 2s cap is the PTS-DTS gap that jitter ignores, not the backwards step between consecutive frames on one lane. SourceMap::MAX_REORDER (500ms) is the shared automatic bound for fMP4, TS, and FLV: a larger step back re-anchors that lane. A broadcast GOP's present-before-decode step is a frame or two (the clock test uses 20ms), which is what the importer's own comment means by reorder being well under 2s.
Widening the bound, or detecting restarts from DTS, would change reset behavior for every importer. Leaving the shared 500ms bound.
(Written by Grok 4.7)
1 participant
Problem
moq import ts|fmp4|flvpublished the input's own timestamps against a catalog clock sampled at startup. A TS feed whose PTS starts hours in, or whose first frame arrives late, advertised the wrong wall time. A source that restarted its timestamps rewound (FLV, TS clamped it to the live edge) or was refused (fMP4NonMonotonicDecodeTime).Approach
live(). It translates onto the catalog's broadcast clock through one mapping per input:SourceMap, because interleaved audio and video step back further thanMAX_REORDER. So a crate-privateAnchorholds the shared offset, and each track has aLanethat detects its own restarts. The first lane to restart moves the anchor, and the other lanes adopt the new mapping instead of re-applying it.SourceMapis now one anchor plus one lane, so both run the same code.tfdt. TS translates right after the 33-bit unwrap, and a PCRdiscontinuity_indicatorcounts as an explicit restart. FLV translates each tag's PTS.seekmarks a restart on fMP4 and FLV.UnmappableTimestamp.Clock::newputs PTS zero 10s before construction, on both the monotonic and the wall clock. A source anchored at arrival may carry frames muxed with earlier PTS (a leading B-frame, audio ahead of video). Without the lead they would map before the broadcast began. The wall mapping is unchanged.moq importenableslive()for all three formats.Publish::new(Ts).doc/bin/cli.mdanddoc/lib/rs/moq-mux.md. Deletes the quest and its references, which unblocks2278-watch-absolute-wall-clock....live()stays opt-in, so SRT, RTMP, HLS import, and existing importer tests keep publishing verbatim. The 10s lead stays inClock::new, so every default publisher has the headroom.Impact
container::{fmp4,ts,flv}::Import::live(self) -> Self.Clock::new()now startsnow()at about 10s, andwallis 10s earlier.wall + ptsis unchanged.SourceMap::translatenow returns the source's timescale, not micros.SourceMapre-anchors after the furthest start and estimated end, not after the last frame's start.ts/fmp4/flvimports publish on the broadcast clock. Restarted fMP4 decode times are accepted, not refused.Alternatives
live()the default and have callers who pinConfig::with_clockopt out. More correct by default, but it touches every importer test and changes the SRT/RTMP/HLS gateways. Left opt-in.with_clock()ortranslate()instead oflive(). Keptlive().Clock::newlead only in the CLI's catalog config. That keepsClock::newliteral, but every otherlive()user would need to know about it. Kept inClock::new.SourceMapper track: each track anchors on its own first frame, so A/V drift apart by their first-frame PTS difference. Rejected.SourceMapfor the whole input: interleaving reads as a reset. Rejected.Follow-ups
moq-srt,moq-rtmp) and HLS import could calllive()too.SourceMap.(Written by Grok 4.7)