Skip to content

fix(aac): honor catalog-selected ADTS framing - #7023

Merged
moq-bot[bot] merged 11 commits into
mainfrom
quest/m1/cut-audit/aac-description-less
Nov 1, 2026
Merged

moq-bot[bot] merged 11 commits into
mainfrom
quest/m1/cut-audit/aac-description-less

Conversation

@kixelated

@kixelated kixelated commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

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 aac instead of GStreamer's raw.

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

  • Public API: additive moq_mux::codec::aac::adts::{Header, Error} for the current moq-audio consumer. aac::Error::MissingDescription refuses raw AAC importer configs without an AudioSpecificConfig; use aac::config or aac::Config::try_into() to construct one.
  • Wire: AAC legacy/LOC payloads without description contain one ADTS frame; an AudioSpecificConfig selects raw AAC. No MoQ message framing changes.
  • GStreamer: described AAC advertises stream-format=raw; description-less AAC keeps adts.

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):

  • Gated nix develop -c just ci origin/main passed, 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.
  • The merge resolved two conflicts. The hang changelog keeps this PR's AAC entry alongside main's path-spelling entry. In connect.test.ts, main had already landed the same client-side connecting.catch from moq-dev/moq#7083, so that hunk takes main's version and the cherry-pick no longer adds a diff.
  • Earlier, at 890abbdf98, gated just ci origin/main passed; at 57b2bf2516, gated just test interop --all passed 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.
  • moq-dev/moq#7437 (queued) also edits rs/moq-mux/src/container/ts/import.rs and quest/m1/README.md; whichever lands second may need a base merge.

Decisions

  • The maintainer approved option 1 during /quest-complete: expose moq_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

  • Took over the stale draft (starter quiet since 2026-10-29) at the user's request.
  • Deleted the finished quest quest/m1/cut-audit/aac-description-less.md with quest 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 head 806b7a54eaa86d5098512d11c667934942163ac4 against origin/main (c04f7771dc): 0 findings.

Online review (pullfrog, on 162feb4c2f):

  • Unused Kind::Adts fields: refuted. Export::rebind compares 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.
  • Stale anyhow/thiserror comment in rs/moq-mux/src/container/ts/import.rs: fixed.

806b7a54ea changes only comments; gated cargo fmt --check -p moq-mux and cargo clippy -p moq-mux --all-targets -D warnings passed.

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)

kixelated and others added 4 commits October 27, 2026 23:33
@kixelated kixelated changed the title A description-less AAC rendition means one thing everywhere fix(aac): honor catalog-selected ADTS framing Oct 28, 2026
kixelated and others added 2 commits October 28, 2026 09:37
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)

Copy link
Copy Markdown
Collaborator Author

@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)

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-28T16:51:40.967016Z d470d03 Manual request
ℹ️ 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" or "@codex security review".

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

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. More of your lovely PRs please.

Reviewed commit: d470d03681

ℹ️ 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".

# Conflicts:
#	drafts/draft-lcurley-moq-hang.md
#	js/net/src/connection/connect.test.ts
kixelated and others added 2 commits October 31, 2026 14:59
…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>
@kixelated
kixelated marked this pull request as ready for review October 31, 2026 22:12
@coderabbitai

coderabbitai Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You'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.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 4551bb3f-4484-42ed-88ca-287e12d9569e
Commits

Reviewing files that changed from the base of the PR and between 111b5ec and 82fcbe5.

Files selected for processing: 19
  • drafts/draft-lcurley-moq-hang.md
  • js/hang/src/util/libav.test.ts
  • js/watch/src/audio/decoder.test.ts
  • quest/m1/cut-audit/README.md
  • quest/m1/cut-audit/aac-description-less.md
  • rs/moq-audio/src/aac.rs
  • rs/moq-audio/src/decode/backend/symphonia.rs
  • rs/moq-audio/src/decode/decoder.rs
  • rs/moq-gst/src/source/imp.rs
  • rs/moq-mux/src/codec/aac/adts.rs
  • rs/moq-mux/src/codec/aac/import.rs
  • rs/moq-mux/src/codec/aac/mod.rs
  • rs/moq-mux/src/container/ts/export.rs
  • rs/moq-mux/src/container/ts/export_test.rs
  • rs/moq-mux/src/container/ts/export_timing_test.rs
  • rs/moq-mux/src/container/ts/import.rs
  • rs/moq-mux/src/container/ts/mod.rs
  • rs/moq-mux/src/container/ts/revision_test.rs
  • rs/moq-srt/src/server.rs
  • 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.

Share

@kixelated

Copy link
Copy Markdown
Collaborator Author

Took over this draft at the maintainer's request: its starter session had been quiet since 2026-10-29, and the draft had no ## Decisions needed section. I merged current main (two conflicts: one changelog entry and a test hunk that main already contains), deleted the finished quest, reran gated just ci origin/main, and ran a local Codex review at 162feb4c2f, which found nothing. Then I marked the PR ready.

(Written by Claude Opus 5.5)

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ No critical issues — one cleanup inline plus a stale comment below.

Reviewed changes

  • ADTS parser relocated and typed: codec::aac::adts now hosts Header/Error, adds Header::payload and the blocks field, 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::Adts and passes whole ADTS frames through, refusing a raw payload before it can be mislabeled.
  • Publication guard: aac::Import::new refuses a description-less config with Error::MissingDescription before 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.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Comment thread rs/moq-mux/src/container/ts/export.rs Resolved
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated enabled auto-merge (squash) October 31, 2026 23:01
@kixelated

Copy link
Copy Markdown
Collaborator Author

Landing summary at 806b7a54ea:

  • Changes: description-less AAC is now ADTS everywhere. Native decode strips and validates the ADTS header, TS export passes ADTS frames through, raw AAC publication requires an AudioSpecificConfig, and GStreamer advertises stream-format=raw for described AAC. They share the moq_mux::codec::aac::adts::{Header, Error} parser, which the maintainer approved.
  • Pullfrog findings: the Kind::Adts fields stay, since they guard rebind, and the variant now documents this. The stale import.rs comment is fixed.
  • Local Codex review of the new head: 0 findings. quest check passed.

(Written by Claude Opus 5.5)

@moq-bot
moq-bot Bot merged commit 4a3f7c0 into main Nov 1, 2026
1 check passed
@moq-bot
moq-bot Bot deleted the quest/m1/cut-audit/aac-description-less branch November 1, 2026 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant