Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Renditions advertise delay, how far their minimum flush lateness trails the broadcast's earliest rendition, measured against one baseline shared by the catalog and never lowered. js/watch holds the largest delay + jitter over the renditions it subscribes to, and the video decoder now reads its floor from the full catalog entry. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 1 minute. 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 (35)
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: 9d47840096
ℹ️ 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".
| #[serde_as(as = "MillisCeil")] | ||
| #[serde(default)] | ||
| pub delay: Option<std::time::Duration>, |
There was a problem hiding this comment.
When a consumer selects CatalogFormat::Msf, this new field is lost: to_msf_media in rs/moq-mux/src/catalog/producer.rs copies only config.jitter into moq_msf::Track, whose schema has no delay, and catalog/msf/consumer.rs consequently restores only jitter. Thus both audio and video using the supported MSF catalog path still omit the measured cross-rendition offset, so a slower encoder remains late despite this change. Add the corresponding MSF extension and conversion in both directions.
Useful? React with 👍 / 👎.
| const renditions = effect.get(this.source.out.catalog)?.renditions; | ||
| const jitter = (track: string | undefined) => { | ||
| const config = track === undefined ? undefined : renditions?.[track]; | ||
| return config && renditionJitter(config); |
There was a problem hiding this comment.
During a broadcast or source switch, #active continues rendering the old subscription until the pending track catches up, but source.out.catalog already describes the new source. Looking up the active track name in this new map therefore makes its floor disappear when names differ, or substitutes the new rendition's floor when names match. If the old rendition has a larger delay or jitter, Sync shrinks prematurely and its remaining frames can arrive late during the handoff. Preserve the active rendition's catalog association or last required floor while still observing rises from its own catalog.
Useful? React with 👍 / 👎.
This branch has not been deployed
1 participant
Problem
A rendition's catalog
jitteris its spread above its own minimum flush lateness, so a constant offset between encoders (video flushing 200 ms behind audio) is advertised nowhere.js/watchplays every track from one clock anchored on the earliest track, so a slower rendition's frames arrive late for it. Separately, thejs/watchvideo decoder computed its sync floor from the decoder-only config, which carries neitherjitternorframerate, so the video rendition's catalog jitter never reachedSync.Approach
moq-mux).catalog::Producerowns one shared flush baseline: a common epoch plus a 10 s sliding minimum fed by every rendition that callsflush. Each rendition'sdelayis its own sliding minimum minus the broadcast's, reported as a lifetime maximum. Renditions that never flush (imports, gateways) advertise none. A discontinuity clears the broadcast window as well as the rendition's, so a paused-and-resumed broadcast does not read its pause as delay.max(delay + jitter)over the renditions it subscribes to, and the draft says a consumer MUST NOT subtractdelayacross renditions. This never under-buffers even when the earliest rendition changes; it over-buffers bymin(delay)when the broadcast's earliest rendition is not subscribed.Rendition::set/replacerefuse a lowereddelaywith the newError::DelayDecreased; measurements hold the published value.js/publish'sCatalogProducerrefuses a lowered or zerodelay.js/watch). Each rendition's registered floor is nowdelay + jitter(video: read live from the full catalog entry by track name, fixing the bug above; audio:playbackJitter).Syncalready recomputes on register, unregister, and a rising entry, so dropping a slow rendition lowers latency.delayfield section and changelog,doc/concept/audio-jitter.md"Across renditions" (the normative rule),doc/concept/hang.md, moq-mux/OBS/GStreamer pages.Tests inject the clock: a constant offset reports
delayon the slower track only, a common drift stays bounded, the earliest rendition drifting behind still covers the real spread, a discontinuity does not become delay, a decrease is refused in Rust and JS, andSyncresizes when a delayed rendition is dropped.Impact
hang: additivedelay: Option<Duration>onVideoConfig,AudioConfig,TextConfig(MillisCeil, zero-as-absent).moq-mux: additiveEstimate::delay+Estimate::with_delay,VideoHint::delay,Error::DelayDecreased.Estimator::new()standalone never measuresdelay.@moq/hang: additive optionaldelayon video, audio, and text rendition schemas.@moq/publish:CatalogProducer.mutatealso throws on a lowered or zerodelay.@moq/watch: no API change; videoDecoder.out.jitternow reflects the catalog (it was always undefined), and both decoders includedelay.delay, indraft-lcurley-moq-hang(moq-hang-03 changelog). Additive: older consumers ignore it.Alternatives
delayfield (receiver-measured): see the options in the removed quest. Rejected for unbounded growth, giving up never-lower, or a glitch on each new subscription respectively.Sync.out.delay(quest plan): skipped.Syncnever reads the catalog field by name, sincedelayis folded into each registered floor, and the rename would churn every consumer that quest(audio-jitter-target): Audio jitter target #3952 is also rewriting.Follow-ups
delayon top oftarget(...)in the audio decoder and on top ofmax(spread, jitter)in the video decoder;doc/concept/audio-jitter.mdalready statesplayout = max(delay + target).quest/m1/publish-delay.md: the browser publisher advertisesdelaylike moq-mux. Completesquest/m1/jitter-flush-clock.mdotherwise.Sync.receivedonly ever lowers its reference, so after the earliest subscribed track leaves, playback stays anchored to it. Receiver-side; belongs with the arrival minimum the audio jitter target expires.maindoes not compilemoq-ffi(E0283 from feat(ffi): advertise JSON tracks in the catalog, add binary data tracks #4137), so the Rust CI here fails until fix(ffi): name the binary config conversion so moq-ffi compiles #4157 lands. Localjust checkpasses with that fix applied.just dev); the video decoder change is covered only by unit tests of the helpers it calls.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code