Conversation
Detecting implicit SBR needs a full Huffman walk of the channel elements, not a small sniff. Document the half-rate LC core on symphonia instead. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
MERGEPositive improvement: yes. The quest assumed a small first-packet sniff could refuse implicit-SBR HE-AAC. That was wrong — the SBR fill element sits after the channel elements, so finding it means a full Huffman walk (large, licensing-sensitive). Leaving the quest open would keep selling work that is not a small patch. Worth the complexity: yes, because this removes complexity. Quest text only (+10/−43). Cross-refs in the audio-codecs README, Different approach? An upstream symphonia “error on in-band SBR” patch or a home-grown element walker were considered and correctly deferred. Keeping the quest as “blocked / needs upstream” would still mislead contributors about size. Documenting the limitation is the right call. Ship it. This is an automated review, not the maintainer's decision |
![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: f83c8fe63b
ℹ️ 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".
| ## Related | ||
|
|
||
| - [HE-AAC refusal](/quest/m1/audio-codecs/he-aac-refusal.md) - reads the same first block | ||
| - [Layout](/quest/m1/audio-codecs/layout.md) - what the parsed PCE eventually maps to |
There was a problem hiding this comment.
The ADTS plan at lines 20-22 still says to parse the PCE “the same way the HE-AAC sniff reads” the block, even though this commit abandons that sniff as infeasible and deletes its quest. Removing only the Related link leaves the remaining quest dependent on nonexistent work and gives its implementer no valid parsing approach; rewrite that step to be self-contained or reassess its feasibility. quest/CLAUDE.mdL111-L113
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Moot now: #4093 completed the AAC PCE quest and deleted aac-pce.md; this PR merged that deletion.
(Written by Claude Opus 5.5)
…to land-4090 # Conflicts: # quest/m1/audio-codecs/README.md # quest/m1/audio-codecs/aac-pce.md # quest/m1/audio-codecs/he-aac-refusal.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2914143e27
ℹ️ 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".
| and rejected at construction. HE-AAC signaled only in band (implicit SBR, as | ||
| over MPEG-TS) plays as its half-rate LC core on symphonia; detecting it needs a | ||
| full element walk, so the docs state it instead of refusing it. A platform backend claims every catalog |
There was a problem hiding this comment.
For Symphonia users receiving implicit-SBR HE-AAC over MPEG-TS, this change abandons the only quest that would prevent silent half-rate decoding and makes documentation the mitigation, but only schedules that documentation inside the larger decode-seam quest. In the reviewed tree, doc/lib/rs/moq-audio.md and doc/bin/cli.md still merely advertise AAC-LC decoding, so affected users receive degraded audio with no warning until unrelated future work lands. Update the user-facing docs as part of this abandonment.
AGENTS.md reference: AGENTS.md:L26-L27
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed, fixed in 5c61a75: doc/lib/rs/moq-audio.md and doc/bin/cli.md now say implicit-SBR HE-AAC plays as its half-rate LC core.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Landed. Merged the updated line (#4093 had completed and deleted (Written by Claude Opus 5.5) |
1 participant
Problem
The quest planned a small sniff of the first AAC raw data block to refuse implicit-SBR HE-AAC (ADTS over MPEG-TS signals LC, so symphonia plays the half-rate LC core silently).
Reproduced: a 44.1 kHz HE-AAC v1 ADTS stream from
fdkaacencdecodes throughmoq_audio::decode::Decoderas 1024 samples/channel at 22.05 kHz with no error.But the sniff is not small. The SBR fill element follows the channel elements, so reaching it means Huffman-decoding every channel's scalefactor and spectral data (12 codebooks, per-rate band tables, roughly 500-600 lines, with tables most easily borrowed from MPL-2.0 symphonia). Scanning only the packet tail is unreliable. Symphonia detects in-band SBR internally but neither exposes nor refuses it.
Approach
Abandon the quest, per the maintainer:
quest/m1/audio-codecs/he-aac-refusal.mdand its references.doc/lib/rs/moq-audio.mdanddoc/bin/cli.mdstate the half-rate behavior now, since the refusal is not coming.aac-pce.md, which this PR had edited).Alternatives considered: an upstream symphonia patch to error on in-band SBR (needs an outside PR and a release), or our own element walker (large, licensing call).
Impact
None (quest and doc text only).
(Written by Claude Opus 5.5)
🤖 Generated with Claude Code