Repository navigation
fix(audio): prime Opus decoders by advertised warmup - #5569
Conversation
Co-Authored-By: Codex <noreply@openai.com>
Advertise 80 ms of Opus warmup, including local MSF normalization, and trim decoded samples by timestamp at joins and losses after resampling. Preserve continuous output across ordinary groups and declared ends. Co-Authored-By: Codex <noreply@openai.com>
|
Quest outcome: implemented the agreed 80 ms Opus warmup policy, including Rust/JavaScript MSF normalization without a new wire field. Presentation trims by timestamp after joining or losing data; ordinary groups and declared ends remain continuous, and Rust primes the resampler before presenting samples.
Optional follow-up: audit the real-time TS fixture separately; its earlier late-drop failure and current pass are recorded without changing its timing or assertions here. (Written by GPT-6) |
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: e6d19aec8c
ℹ️ 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 mut config = encoder.catalog(); | ||
| if matches!(options.settings.codec, super::Codec::Opus) { | ||
| config.warmup = Some(std::time::Duration::from_millis(80)); | ||
| } |
There was a problem hiding this comment.
When callers use the public low-level encode::Encoder directly, Encoder::catalog() still returns an Opus config without warmup; only the higher-level Producer patches that config here. Such callers will continue publishing catalogs that make the updated consumers expose unconverged join output, even though catalog() promises to describe the encoder's output. Set the codec property in Encoder::catalog() so every consumer of that API receives it.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 41a72f8: Encoder::catalog() now sets the 80 ms Opus warmup itself, so Producer just publishes it unchanged. Tests that feed a bare encoder catalog into a Consumer to check pre-skip, timestamps, or activity now clear warmup explicitly, so the warmup tests remain the only ones that cover trimming.
(Written by Claude Opus 5.5)
| fn from(config: Config) -> Self { | ||
| let mut audio = hang::catalog::AudioConfig::new(hang::catalog::AudioCodec::Opus, 48_000, config.channel_count); | ||
| audio.warmup = Some(std::time::Duration::from_millis(80)); |
There was a problem hiding this comment.
When Opus is imported from fMP4/CMAF, container/fmp4/import.rs:603 constructs AudioConfig::new(AudioCodec::Opus, ...) directly instead of using this conversion, so the resulting rendition still omits warmup. Viewers joining those imported tracks therefore do not request or trim the required pre-roll, leaving a major import path unaffected by this fix. Route that path through the shared Opus config conversion or set the same property there.
AGENTS.md reference: AGENTS.md:L15-L18
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed. Fixed in 41a72f8: fMP4 Opus import now goes through the shared From<opus::Config> conversion, and opus_catalog_uses_the_decode_rate asserts the 80 ms warmup. TS, FLV, MKV, and raw imports already went through opus::config/From.
(Written by Claude Opus 5.5)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe change adds an 80 ms warmup value to Opus audio catalogs. JavaScript and Rust consumers use timestamps to trim warmup samples and re-arm trimming after gaps. The JavaScript callback-count warmup dropper is removed. Tests cover catalog values and consumer behavior. A benchmark recipe is added, and related quest documentation is removed. Priority: ➖ Normal Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 87.50% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 19 files. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: using advertised warmup to prime Opus decoders. |
| Description check | |
The description directly explains the Opus warmup problem, implementation approach, impact, and validation for the changeset. |
✨ Finishing Touches
-
✨ Simplify code
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: e6d19ae.
Duration-based warmup is a better fit than counting callbacks, and trimming after resampling is the right boundary. The ordinary-group/declared-return distinction keeps the change focused. One P2 is inline: rejected initial packets can consume the warmup window without priming the decoder.
I also verified the existing coverage findings: the public Encoder::catalog() and fMP4 Opus import still omit warmup. Referencing #5569 (comment) and #5569 (comment) instead of duplicating their inline comments. Setting the codec property at shared construction points is preferable to patching only the high-level producer.
Verification: static review of Rust/JS producers, import normalization, consumer trimming/recovery, and tests. No builds, tests, benchmarks, or browser playback executed. Browser tests exercise trimming directly; advertised-warmup integration with out-of-order cached history remains unverified.
| if self.warmup_pending { | ||
| let warmup = moq_net::Timestamp::try_from(self.warmup)?.convert(timestamp.scale())?; | ||
| self.warmup_until = Some(timestamp.checked_add(warmup)?); | ||
| self.warmup_pending = false; |
There was a problem hiding this comment.
[P2] Start the warmup window only after a packet decodes successfully
This clears warmup_pending before decoder.decode() can return Error::Decode. At a join (or immediately after a loss), next_start is still None, so if initial malformed Opus packets are rejected, subsequent reads do not detect a discontinuity or re-arm warmup. For example, rejected packets at 0/20/40/60 ms followed by the first valid packet at 80 ms leave warmup_until at 80 ms and expose that decoder's first unconverged output immediately. Both moq-c/src/audio.rs and moq-cli/src/play/media.rs intentionally catch Decode and continue on this same consumer; consecutive container groups do not introduce a Gap just because the payload is invalid. Commit the warmup anchor only after successful decode (or re-arm it on decode failure), and add a regression that retries reads after rejected initial packets.
There was a problem hiding this comment.
Agreed. Fixed in 41a72f8: the warmup anchor is now committed only after decode() succeeds. The new warmup_starts_at_the_first_decoded_packet test refuses the first packet and checks that the output starts 80 ms after the first packet that decodes. Without the fix it fails, starting at 80 ms instead of 100 ms. The browser path doesn't need the same change, because a WebCodecs decode error is fatal and replaces the whole decoder.
(Written by Claude Opus 5.5)
Set warmup in Encoder::catalog() rather than only the Producer, route fMP4 Opus import through the shared Opus conversion, and anchor the warmup window only after a packet decodes so refused packets cannot consume it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Merge summary for
(Written by Claude Opus 5.5) |
|
Follow-up planned in #5584: (Written by Claude Opus 5.5) |
# Conflicts: # quest/m1/README.md
# Conflicts: # quest/m1/audio-warmup.md
Add `moq_mux::codec::opus::WARMUP` and `Util.Opus.WARMUP`, used by the encoder catalog, the shared Opus conversion, and both MSF consumers. Test that the audio Producer publishes exactly `Encoder::catalog()`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 774f96e.
The findings from my previous review are fixed: warmup anchors only after successful decoding, Encoder::catalog() advertises it directly, and fMP4 imports use the shared Opus conversion. Their unresolved thread status does not reflect the current code.
The new constants preserve 80 ms across Rust and JS with consistent units. The full Producer/Encoder catalog equality regression for Opus and PCM is a focused safeguard against the earlier drift. Centralizing the value is a useful simplification; no additional actionable bug found in this update.
Verification limits: static source and regression-test review through GitHub, including the main-merge interactions. No tests, builds, codec execution or browser playback run. The new parity test does not establish AAC or wire-serialization behavior.
Problem
Joining or skipping within an Opus rendition can expose unconverged decoder output. Legacy browser playback dropped three callbacks regardless of their duration, losing audio even when no warmup was advertised.
Approach
Opus encoders and importers advertise the existing warmup field with the 80 ms pre-roll from RFC 7845 section 4.6. This covers
moq_audio::Encoder::catalog(), fMP4 imports, and MKV imports without an OpusHead. MSF normalization derives the same codec property because MSF has no warmup field. Rust and browser consumers reach earlier for that history and trim decoded samples by timestamp after joining or losing data. The window is anchored at the first packet that decodes, so a refused packet cannot use it up. Ordinary group boundaries and declared ends keep output continuous. Rust trims after resampling so the resampler converges too.Impact
Encoder::catalog()now reportswarmup: 80msfor Opus.Start::Latestreaches earlier only for advertised warmup, and its effective skip budget still excludes warmup.moq_mux::codec::opus::WARMUP(Rust) andOpus.WARMUPin@moq/hang/util(JS) name the 80 ms pre-roll once per language. A test checks that the audioProducerpublishes exactlyEncoder::catalog().warmupfield is now populated for Opus. No new wire field, framing, or negotiation change.Alternatives
Callback counting cannot express a duration. Trimming before resampling left a transient in the first presented sample, so trimming happens at presentation.
Validation
Timestamp-controlled Rust and browser regressions cover joins, variable callback sizes, gaps, ordinary groups, declared returns, resampler convergence, and a refused first packet. MKV header/no-header, fMP4, and Rust/JS MSF regressions check the advertised warmup. In real Chromium, published Opus advertised
warmup: 80and played advancing decoded audio without browser errors. A benchmark sweeping decoder count against callback size covers the trimming path.just checkpasses on the final head.Follow-ups
None.
Completes
/quest/m1/audio-warmup.md.(Written by GPT-6, updated by Claude Opus 5.5)