Repository navigation
fix(aac): honor catalog-selected ADTS framing - #7023
Conversation
Quest-Claim: 71a5401c7b6d1f4c
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
Co-Authored-By: Codex <codex@openai.com>
…c-description-less
The shared stream write budget can yield a host task right after a SETUP write. Both moql SETUP refusal tests held the handshake promise unobserved across that write, so the refusal could land first and Bun reported it as an unhandled rejection. Observe each promise when it is created; the refusal assertions are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> (cherry picked from commit f336e62)
|
@codex review Current published head: d470d03681. No completed review or earlier request exists for this head, more than 30 minutes after its push. (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. |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
# Conflicts: # drafts/draft-lcurley-moq-hang.md # js/net/src/connection/connect.test.ts
…ion-less # Conflicts: # drafts/draft-lcurley-moq-hang.md # js/net/src/connection/connect.test.ts
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 5 minutes. View limit details
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. |
|
Took over this draft at the maintainer's request: its starter session had been quiet since 2026-10-29, and the draft had no (Written by Claude Opus 5.5) |
There was a problem hiding this comment.
ℹ️ No critical issues — one cleanup inline plus a stale comment below.
Reviewed changes
- ADTS parser relocated and typed:
codec::aac::adtsnow hostsHeader/Error, addsHeader::payloadand theblocksfield, and tightens the syncword check to also require zero layer bits. - Native AAC decode: symphonia strips the ADTS header for description-less Legacy/LOC packets and validates it against the catalog (LC profile, rate, channels, single block).
- TS export: a description-less AAC rendition becomes
Kind::Adtsand passes whole ADTS frames through, refusing a raw payload before it can be mislabeled. - Publication guard:
aac::Import::newrefuses a description-less config withError::MissingDescriptionbefore advertising. - GStreamer: described AAC caps now advertise
stream-format=raw. - Spec and tests: the hang draft states the framing and gains a changelog bullet; tests cover ADTS vs raw, CRC and multi-block headers, publication metadata, and CMAF.
The core parsing is correct. I re-derived the ADTS header layout against the FDK-AAC reference decoder (adtsRead_DecodeHeader): for a protected frame with N extra raw data blocks the header is 7 + 2*N positions bytes + 2 CRC bytes = 7 + 2*blocks, which is exactly what Header::parse computes, so the multi-block test reflects the spec. I also confirmed the new Import::new refusal does not affect any first-party publisher (TS importer, RTSP, the import::Track front door, and the GStreamer sink all synthesize a description), and that empty audio trim markers are turned into FrameEnd by container::consumer before they can reach es_payload.
ℹ️ A comment this change left stale
rs/moq-mux/src/container/ts/import.rs:7281 still explains the AAC give-up path as "adding context to the anyhow one adts::Header::parse returns, where the legacy path wraps a thiserror value". After this PR Header::parse returns the new thiserror aac::adts::Error and the call site converts it via err.into(), so both paths now wrap a thiserror value and the contrast no longer holds.
Technical details
# Stale anyhow-vs-thiserror comment
## Affected sites
- `rs/moq-mux/src/container/ts/import.rs:7279` — the comment above `aac_gives_up_when_nothing_ever_parses`.
## Required outcome
- The comment must not claim `adts::Header::parse` returns an `anyhow` error; it now returns `moq_mux::codec::aac::adts::Error`, converted with `.into()` at the call site.deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Landing summary at 806b7a54ea:
(Written by Claude Opus 5.5) |

Problem
Description-less AAC is ADTS in WebCodecs and moqsrc, but native audio decoded it as raw AAC and TS export added a second ADTS header. Both mismatches reproduced before the fix; native decoding could silently emit different PCM. The same framing tests exposed moqsrc spelling raw AAC caps as
aacinstead of GStreamer'sraw.Approach
Decode description-less legacy/LOC packets through the existing ADTS parser and pass complete ADTS frames through TS unchanged. An AudioSpecificConfig selects raw AAC; demuxed CMAF samples remain raw. Share the parser between moq-mux and moq-audio, and refuse raw AAC publication without its description before advertising a rendition. State the same contract in the hang draft.
First-party AAC publishers retain or synthesize an AudioSpecificConfig. Tests cover both framing modes, CRC and partial packets, header/config disagreement, publication metadata, and CMAF samples.
Impact
moq_mux::codec::aac::adts::{Header, Error}for the current moq-audio consumer.aac::Error::MissingDescriptionrefuses raw AAC importer configs without an AudioSpecificConfig; useaac::configoraac::Config::try_into()to construct one.descriptioncontain one ADTS frame; an AudioSpecificConfig selects raw AAC. No MoQ message framing changes.stream-format=raw; description-less AAC keepsadts.Alternatives
Duplicating the parser or adding an internal shared crate would keep moq-mux's public API unchanged but add another implementation or package boundary. The approved public parser reuses the existing code.
Validation dependency
Includes the exact test-only fix from moq-dev/moq#7083, f336e625ea, cherry-picked with its original author and commit credit. Full CI first reproduced that base SETUP test's unhandled rejection. Observing both handshake promises immediately fixes the rejection owner while preserving their refusal assertions and production behavior.
Validation
At 162feb4c2f, after merging current main (c04f7771dc):
nix develop -c just ci origin/mainpassed, including all 4,278 Rust tests (64 existing skips), JS checks/tests, drafts, Markdown, workflows, shell, and quest validation for 666 documents. Interop was not rerun after this merge.connect.test.ts, main had already landed the same client-sideconnecting.catchfrom moq-dev/moq#7083, so that hunk takes main's version and the cherry-pick no longer adds a diff.just ci origin/mainpassed; at 57b2bf2516, gatedjust test interop --allpassed all 169 matrix cases. The framing reproductions and the Chromium PCM comparison remain covered by this PR's fixtures: the native ADTS and raw paths produced the same samples, and TS emitted one ADTS header. No assertion, tolerance, or production behavior was weakened to get these results.rs/moq-mux/src/container/ts/import.rsandquest/m1/README.md; whichever lands second may need a base merge.Decisions
/quest-complete: exposemoq_mux::codec::aac::adts::{Header, Error}, with only the metadata and payload access moq-audio needs public, as implemented. The alternative was to keep moq-mux's public API unchanged and choose an internal sharing boundary.Choices made without asking
quest/m1/cut-audit/aac-description-less.mdwithquest rm: the public-API choice it left open was approved, and the CI and interop evidence it asked for is recorded above.Review
Codex,
gpt-6.1-sol, medium effort, round 1 on head806b7a54eaa86d5098512d11c667934942163ac4againstorigin/main(c04f7771dc): 0 findings.Online review (pullfrog, on
162feb4c2f):Kind::Adtsfields: refuted.Export::rebindcompares kinds (rs/moq-mux/src/container/ts/export.rs:2523), so the fields refuse a moved rendition announced differently; per-frame cross-checks would be wrong under SBR. The variant now documents this.rs/moq-mux/src/container/ts/import.rs: fixed.806b7a54ea changes only comments; gated
cargo fmt --check -p moq-muxandcargo clippy -p moq-mux --all-targets -D warningspassed.Follow-ups
No new quests. The earlier Opus gap remains owned by moq-dev/moq#6990; a passing matrix here does not close that investigation. Consider a release backport after landing, preserving the published API.
(Written by GPT-6; updated by Claude Opus 5.5)