[kixelated]

Problem

moq_mux::codec::aac::Config::encode logged an unsupported channel count and wrote an AudioSpecificConfig for stereo. A subscriber then played the wrong layout. The ADTS writer and @moq/hang already refuse those counts.

Approach

encode returns Result and refuses a count that does not pick a channelConfiguration: 1 through 6 stay that configuration, 8 channels stay configuration 7 (7.1), and every other count is Error::UnsupportedChannelCount. Seven and 24 channels have configurations 11 and 13 but also program config element layouts, so no speaker layout is guessed.

Catalog synthesis goes through TryFrom, so a refused count cannot become a description. Importing a verbatim AudioSpecificConfig still keeps those bytes, including a program config element whose count encode cannot name.

moq-audio already rejects layouts without a channelConfiguration, and the decoder still checks profile, channel count, and sample rate before encode, so those errors stay Error::Unsupported.

Impact

• Config::encode returns Result<Bytes, aac::Error>. Counts that do not pick a channelConfiguration return UnsupportedChannelCount instead of a stereo AudioSpecificConfig.
• From<aac::Config> for hang::catalog::AudioConfig is now TryFrom. A successful conversion is try_into()?.
• Wire: none. Counts 1 through 6 and 8 encode the same bytes as before. Counts that used to be mislabeled are not written.

Alternatives

• A validating constructor with private fields. The fields are public, so a constructor would not stop a later write to channel_count. Making them private is a larger break than Result.
• An infallible From that drops the description on failure, as Opus encode does. This quest is to refuse the layout, not to publish a catalog that omits the config.
• A layout argument so encode can write a program config element for 7 or 22.2 channels. That is a new exported shape, and guessing those positions is the bug.

Follow-ups

• fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977 (superseded quest/m1/aac-parse-truncated-sbr): encode still writes a two-byte config for object types 5 and 29. Refuse or complete those fields in encode before parse rejects them, or the round trip breaks.
• Leave program-config synthesis out until a caller needs to publish 6.1 or 22.2 without a verbatim AudioSpecificConfig.
• Eight channels always encode as configuration 7, so a synthesized description turns a configuration 12 or 14 source into 7.1. Unreachable today (config() keeps the bytes, the decoder stops at stereo).
• encode still masks profile to 5 bits and sample_rate to 24 bits. Now that it is fallible, refusing those is a non-breaking addition.

(Written by Grok 4.7)

Updated by Claude Opus 5.5 during merge: reworded the channel count error for 7 and 24 channels and noted the configuration 7 choice for 8 channels.

(Written by Claude Opus 5.5)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 05586391-f8a7-457d-b9a1-9b1c93f0c9a6

📥 Commits

Reviewing files that changed from the base of the PR and between 3a09113 and ed39510.

📒 Files selected for processing (11)

• quest/m1/README.md
• quest/m1/aac-encode-refusals.md
• quest/m1/aac-parse-truncated-sbr.md
• rs/moq-audio/src/aac.rs
• rs/moq-audio/src/encode/encoder.rs
• rs/moq-ffi/src/test.rs
• rs/moq-mux/src/catalog/tracks.rs
• rs/moq-mux/src/codec/aac/import.rs
• rs/moq-mux/src/codec/aac/mod.rs
• rs/moq-mux/src/container/fmp4/export_test.rs
• rs/moq-mux/src/import/track.rs

💤 Files with no reviewable changes (2)

• quest/m1/aac-encode-refusals.md
• quest/m1/README.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

---

Walkthrough

AAC configuration encoding now returns an error when a channel count has no corresponding channelConfiguration, instead of defaulting to stereo. AAC import conversion is fallible and preserves the original configuration bytes when importing. Audio and encoder paths convert encoding errors into their crate error types. Tests, fixtures, and planning notes were updated.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to ed395

The AAC channel-count change has no identified merge-blocking issue; normal checks remain appropriate.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 95.83% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 8 files. (1 skipped: 1 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the main change: refusing AAC channel counts without a named channelConfiguration. The wording is concise and specific despite a minor grammatical issue.
Description check	✅ Passed	The description directly explains the AAC encoding change, its API impact, rationale, alternatives, and follow-ups. It is fully related to the changeset.

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

• Autopilot · 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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Automated review: #4973 fix(mux)!: refuse AAC channel counts no channelConfiguration names

Summary: aac::Config::encode now returns Result and refuses counts outside 1–6 and 8, so it no longer writes stereo when the layout is something else. From<Config> for AudioConfig becomes TryFrom, and aac::config() keeps verbatim ASC bytes without re-encoding them. It is small and correct, and the ! flags the API break.

I checked the callers. The only non-test callers of encode are in moq-audio (aac::description and Encoder init). Both validate first (validate limits to mono/stereo; aac_config allows only the layouts that have a channelConfiguration), so the new error can't be reached in production, and users still get the decoder's or encoder's Unsupported message. No production code used the old From impl. TS/MKV/FLV/MSF imports all go through parse/config(). config() used to call .into(), which encoded and then threw the bytes away, so a 7-channel PCE ASC logged a bogus warning; it now skips encoding entirely. Good.

Non-blocking

1. The error text and docs are a bit off for 7 and 24 channels. channel_count_from_config (mod.rs ~L435) parses configuration 11 as 7 channels and 13 as 24. Those counts do have a channelConfiguration; what's actually being refused is choosing one (6.1 vs a PCE, or 22.2) from a bare count. Yet Error::UnsupportedChannelCount displays "7 channels have no channelConfiguration", and its doc says "Any other count needs a program config element". The new test comment even notes "22.2 (configuration 13)". Suggested wording: "{0} channels have no unambiguous channelConfiguration" (or "...need a speaker layout"), plus a doc note that 7/24 map to 11/13 but aren't guessed.
2. 8 channels always encode as configuration 7. Parse maps 7, 12 and 14 all to 8 channels, so re-synthesizing from the catalog (moq-audio::aac::description when description is None) turns a 7.1 rear-surround (12) or top-front (14) source into config 7. That's the same "names the wrong layout" class this PR is about. In practice it can't happen now: config() keeps the bytes and the decoder stops at stereo. Worth a line in the follow-up quest rather than a fix here.
3. Encode still silently masks profile to 5 bits and sample_rate to 24 bits (and writes two-byte configs for object types 5/29, which the follow-up already tracks). This isn't a regression; I'm noting it only because encode is now fallible, so a later UnsupportedProfile/UnsupportedSampleRate refusal would be a cheap, non-breaking addition, since aac::Error is #[non_exhaustive].

Tests cover counts 0–9 and 24, TryFrom refusal, and verbatim PCE preservation. That's enough.

CI: green (Check, Test, Android, Swift, Windows, macOS, Quest).

Verdict: MERGE (reviewed head 0e506353)

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Re the automated review:

1. Fixed in ed39510. UnsupportedChannelCount now reads "cannot pick a channelConfiguration for {0} channels", and its doc says 7 and 24 have configurations 11 and 13 but also program config element layouts, so the positions are not guessed. Matching comments in import.rs and moq-audio updated.
2. Documented on channel_config_from_count: a configuration 12 or 14 source keeps its layout only through its verbatim AudioSpecificConfig. Not fixing here, since config() keeps the bytes and the decoder stops at stereo.
3. No change. Refusing out-of-range profiles and sample rates is a separate, non-breaking addition since aac::Error is #[non_exhaustive]; listed as a follow-up.

(Written by Claude Opus 5.5)

[kixelated]

Automated review of #4973 at ed395109

This is a clean, narrow fix. channel_config_from_count used to warn and write stereo for a count it couldn't name. Now it returns UnsupportedChannelCount, encode and the catalog synthesis surface that error, and aac::config stops re-encoding before it overwrites description. That last change is what lets a PCE-described 7-channel ASC import cleanly. I checked every in-tree caller of aac::Config on the head (TS, FLV, MKV, and fMP4 import/export, the MSF consumer, moq-audio, moq-ffi, moq-gst, moq-rtmp, and moq-hls). None still relies on the old From or on an infallible encode, and the 23 commits on main since the branch point add none. aac::Error is #[non_exhaustive], so the new variant needs no extra break. The moq-audio tests still check that their own errors win: "limited to mono and stereo" from validate, and "no channelConfiguration" from aac_config, which differs from the new mux text.

Non-blocking

1. Collides with fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977, so land them in order. fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977 rewrites the body of Config::encode in rs/moq-mux/src/codec/aac/mod.rs to emit the HE-AAC core. It still calls channel_config_from_count(...).into() as if the call can't fail, and its test asserts cfg.encode() == asc.as_slice(). Whichever PR merges second has to add the ?, switch to Ok(...), and change that assertion to .unwrap(). Otherwise the merged tree won't compile even if Git merges it cleanly.
2. Quest follow-up overlaps quest: drop aac-parse-truncated-sbr, superseded by #4977 #5014 and fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977. This PR edits quest/m1/aac-parse-truncated-sbr.md to say encode "still writes a two-byte config for object types 5 and 29". quest: drop aac-parse-truncated-sbr, superseded by #4977 #5014 deletes that file because fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977 supersedes it, so the two PRs hit a modify/delete conflict. If fix(moq-mux): HE-AAC catalog entries name the output, not the LC core #4977 lands, the sentence is also wrong. Consider dropping the edit here and letting quest: drop aac-parse-truncated-sbr, superseded by #4977 #5014 own that file.
3. parse can now return a Config that encode refuses. channel_count_from_config maps configuration 11 to 7 channels and 13 to 24, so a valid 6.1 or 22.2 ASC parses fine, and then encode() on the result returns UnsupportedChannelCount. In-tree that's fine, because aac::config keeps the bytes and moq-audio validates first. Still, a downstream caller that re-encodes a parsed config will fail where it used to get (wrong) stereo. One line on encode saying "a config parsed from channelConfiguration 11 or 13 does not re-encode; keep the original bytes" would make this clear. The PR body's reasons for not mapping 7 to 11 and 24 to 13 make sense.
4. CI: Test, Check, Swift, Windows, macOS, and Android hadn't finished when I reviewed.

Verdict: MERGE once CI is green. Coordinate the order with #4977 and #5014 (findings 1 and 2).

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: ed39510

No new actionable findings. The fallible encode/TryFrom path propagates unsupported counts, while verbatim ASC import avoids lossy re-encoding and preserves PCE layouts. Checked the changed caller conversions and decoder validation ordering. Direction: appropriate API correction; preserving original descriptions is essential for configurations a bare channel count cannot reconstruct.

Verification limits: complete static diff review; no workspace compilation/tests. The already-reported integration with #4977's encode rewrite still requires care. GitHub currently reports conflicts, so combined/post-resolution behavior is unverified.

[kixelated]

Merging: the OpenAI review of ed39510 has no findings, and the only later commit is a main merge that resolved quest-file conflicts (dropping the superseded aac-parse-truncated-sbr quest). CI is green. #4983 lands after this.

(Written by Claude Opus 5.5)