Repository navigation
feat(moq-mux)!: importers publish stream timestamps with monotonic group starts; the catalog clock maps them to wall - #4543
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…k maps them to wall Delete live() from the fMP4, MPEG-TS, and FLV importers. They publish the source's own timestamps and anchor the catalog clock on their first frame, unless Config::with_clock pinned it. fMP4 and FLV rewinds now end the import with an error. Removes SourceMap, Clock::source, and the crate-private Anchor/Lane. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A backward timestamp is an encoder restart and a new broadcast. MPEG-TS drops the Reanchor shift, so loop wraps, restarts, and joins below the edge end the import with TimestampRewind. MKV anchors the catalog clock on its first block and dedups drain replays by byte offset, so a rewind errors instead of being dropped. Deletes the ts-import-shared-shift quest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
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. |
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: 798ce057fb
ℹ️ 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".
| track.pending_sequence = Some(sequence); | ||
| track.last_decode_time = None; |
There was a problem hiding this comment.
When an HLS import skips a sequence or crosses an EXT-X-DISCONTINUITY, push_segment calls seek before decoding the next segment. Clearing last_decode_time here allows that segment's tfdt to rewind without triggering NonMonotonicDecodeTime, and the fMP4 passthrough then publishes timestamps below earlier groups under the unchanged catalog clock instead of ending the import as intended. Keep the prior decode time across this sequence-only seek, or otherwise compare the next tfdt against it.
AGENTS.md reference: rs/moq-mux/AGENTS.md:L9-L9
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this for the maintainer: it turns on the rewind rule, which is still being decided (the PR now uses monotonic group starts). Whether an HLS sequence skip or EXT-X-DISCONTINUITY keeps the prior decode time depends on whether a discontinuity may legitimately rewind tfdt. Recommendation: keep the prior decode time across a sequence-only skip (a skip is not a timebase break), and reset it only on a signalled discontinuity, which the plan treats as a new epoch.
(Written by Claude Opus 5.5)
container::Producer and Consumer refuse a group starting before the previous group's start, or a frame below the start of the group before its own, instead of any frame below the previous group's content. A keyframe overlapping the previous group's last frame now publishes. The hang draft states the rule, and quest/m1/legacy-end-overshoot.md carries it to the JS consumer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d4424da51f
ℹ️ 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 floor = match frame.keyframe { | ||
| true => self.start, | ||
| false => self.floor, | ||
| }; | ||
| if floor.is_some_and(|floor| timestamp_lt(frame.timestamp, floor)) { |
There was a problem hiding this comment.
When a new keyframe overlaps the previous group's tail but remains above its start, this Rust producer now emits it, while js/hang/src/container/consumer.ts lines 388-408 still compares later groups against the maximum delivered timestamp and aborts the track as being below the live edge; js/hang/src/container/legacy.ts lines 195-198 likewise refuses producing the same valid sequence. Browser consumers will therefore reject Rust broadcasts using the newly allowed overlap semantics, so update the JS producer, consumer, and tests with this container change.
AGENTS.md reference: rs/moq-mux/AGENTS.md:L12-L14
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Leaving this for the maintainer: it follows from the group-start rewind rule, which is still being decided. If monotonic group starts stand, the JS consumer (js/hang/src/container/consumer.ts) and legacy producer (legacy.ts) need the same group-start comparison so browsers accept overlapping keyframes, and that JS change should land in this PR. If the rule reverts to a live-edge floor, nothing changes in JS.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed head: d4424da
Direction: worth doing. Source-owned timestamps plus one catalog mapping remove substantial translation/restart machinery and fit redundant ingest. Prefer targeted completion over a new abstraction: cover every timestamp-emitting route, preserve sequence-only seek history, and ship the JS ordering semantics alongside Rust.
New P2 below: the clock-only MPEG-TS route can publish timestamped SCTE-35 sections without anchoring the catalog.
I also source-confirmed the existing browser overlap finding (r4139488387) and fMP4 seek/decode-history finding (r4139281525), so I am not duplicating those inline. The JS counterpart is explicitly deferred, but remains an integration/release blocker: groups [100,133,166] then [150,183] now pass Rust while the browser still rejects the second group. The seek reset itself predates this PR's non-live mode; it is an unclosed hole in the new no-rewind guarantee after translation is removed. HLS calls that path on sequence gaps/discontinuities.
Verification: static review of all production diffs, changed test boundaries, affected importers, producer/consumer, HLS caller, and JS checks. No build, runtime, or packet-level tests executed; the Rust/Bun toolchains are unavailable here.
(Written by OpenAI)
| // The first PES is live on arrival: its PTS needs no unwrap yet. | ||
| if let Some(pts) = pending.pts { | ||
| self.catalog.anchor(Timestamp::from_scale(pts, 90_000)?)?; | ||
| } |
There was a problem hiding this comment.
P2: Anchor the catalog for section-only MPEG-TS output
The catalog is now anchored only when flushing a PES, but MPEG-1/2 video uses Stream::Clock and returns from handle_pes_start before entering the pending/flush path. An input with MPEG-2 video plus SCTE-35 sections, without an audio or other published PES stream, therefore never anchors its catalog: SectionStream::emit publishes the video's source PTS while the catalog retains its construction-time clock. A first video PTS of 3600 seconds produces cues whose advertised wall time is approximately 3590 seconds in the future. The previous live path anchored during handle_pes_start, so this regresses CLI TS imports. Anchor before publishing timestamped sections, including the Stream::Clock path, and add a section-only clock-mapping regression test.
Automated finding by review (OpenAI). (Written by OpenAI)
There was a problem hiding this comment.
Agreed, and reproduced: with MPEG-2 video as the only PES, the catalog clock stayed at its construction-time reading. 6118ee0 anchors the catalog on a clock-only stream's PTS in handle_pes_start, and adds a_clock_only_stream_anchors_the_catalog, which fails without the fix.
(Written by Claude Opus 5.5)
…ions Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
A group starting at the previous group's start is a rewind too, in the producer, consumer, and hang draft. Untimed payloads and cues sharing the previous cue's instant land 1 us past the latest group start. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Reverts the strict group-start rule: only a start below the previous group's is a rewind. Drops Producer::next_start, so cues and untimed payloads land on the live edge again, and the draft says MUST NOT be lower than. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…y resets it An HLS media-sequence skip no longer lets the next segment's tfdt rewind unnoticed. fmp4::Import::discontinuity starts a new timeline, and the HLS importer calls it when the discontinuity sequence changes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Problem
live()on the fMP4, MPEG-TS, and FLV importers rewrote every timestamp onto an arrival-time anchor and re-anchored an encoder restart forward. That hid the source's own timeline from everything downstream and broke same-hop importers, which must derive every timestamp from the input alone (#4388). Withoutlive(), the importers published verbatim timestamps but advertised a construction-time catalog clock. That put the wall mapping off for any source not starting near zero, and the SRT, RTMP, and HLS gateways all took that path. MPEG-TS also shifted any rewind (loop wraps, restarts, joins) forward onto the live edge, and MKV silently dropped rewound blocks.Approach
tfdt.catalog::Producerkeeps the clock in its shared state. A crate-privateanchor(pts)sets an unset clock once soptsreads now. Every clone, track, and HLS rendition then shares that mapping, and later calls are no-ops.Config::with_clockpins the clock, and an importer never re-anchors it. This covers recordings whose zero names their real start.Clocknow stores one (instant, reading) pair, so it can read an arbitrary PTS now without subtracting fromInstant, which fails before boot on Windows.Clock::new()is that with a 10 s reading, so the boot-time fallback is gone.container::Producer::writereplaces its live-edge floor with this rule. A keyframe below the previous group's start, or any frame below the start of the group before its own, returnsTimestampRewind. Frames may still dip below the previous group's content: B-frames, open-GOP pictures, and a keyframe overlapping the last frame (moq import ts aborts on continuous content join (TimestampRewind) #3798).live_edge()stays as where untimed payloads (TS sections, PES without PTS) land, but it is no longer a floor.container::Consumerenforces the same rule instead of rejecting any frame below the previous group's content.drafts/draft-lcurley-moq-hang.mdstates the rule.quest/m1/legacy-end-overshoot.mdnow carries it, so that quest aligns the JS consumer, which still aborts below the live edge.Reanchorshift, along with the codecfloor/live_edgehelpers that only fed it. Loop wraps and restarts, flagged or not, now surface asTimestampRewind. A flagged discontinuity that jumps forward still publishes with its break markers.tfdtmust advance per track (NonMonotonicDecodeTime). FLV goes through the producer rule.TimestampRewind, and MKV anchors the clock on its first block like the others.live()(fMP4, TS, FLV),clock::Anchor/Lane,SourceMap, andClock::source.ts-import-shared-shiftquest. Rewinds are a new broadcast per the maintainer, so there is no shift left to share, and fix(moq-mux): one TS re-anchor shift per program #4513 is obsolete.Impact
moq_mux::container::{fmp4,ts,flv}::Import::liveremoved (breaking, targetsdev).moq_mux::SourceMapandmoq_mux::Clock::sourceremoved (breaking).moq_mux::container::fmp4::Import::discontinuityadded: an fMP4seeknow keeps each track's last decode time, so an HLS sequence skip cannot rewindtfdt, and only a signalledEXT-X-DISCONTINUITYresets it.moq_mux::catalog::Producer::clock()may change once, when an importer anchors it on its first frame, unlessConfig::with_clockset it.moq_mux::Error::UnmappableTimestampnow means a first timestamp too large to map (PTS zero before 2020).moq import {ts,fmp4,flv},import srt,import rtmp,import hls, and MKV import publish source timestamps verbatim with a catalog clock mapping the first frame to its arrival.moq export ts: a continuous timeline whose content restarts permanently stalls video and primary audio since #3375 #3533's shape) is still a rewind, because each audio frame is its own group.container::Producer::writeandcontainer::Consumerenforce non-decreasing group starts instead of a live-edge floor.TimestampRewind's message changes to match.clockshape is unchanged; only its value differs.Alternatives
Follow-ups
tfdtcounted from 1970) cannot be anchored and errors out. A signed or rebased wall would lift that.quest/m1/legacy-end-overshoot.mdnow owns aligning it.moq import srtshould republish automatically belongs to the broadcast epoch line.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code