[kixelated]

Problem

aac::Config::parse reported the AAC-LC core of an explicit SBR or PS AudioSpecificConfig (24 kHz, and mono for HE-AACv2). The FLV, MKV, and MSF paths copied that into the catalog, while fMP4 takes the sample entry (48 kHz stereo), so importers disagreed for the same stream and players sizing buffers from the catalog saw the wrong output. JS frameDuration already treats mp4a.40.5/.29 sampleRate as the output rate.

Approach

• Config::parse reads the extension rate under explicit SBR (object type 5 or 29) and reports two channels under PS. Implicit SBR (ADTS/TS) is unchanged: it still reports the half-rate core.
• Config::encode writes the explicit hierarchical form for profiles 5/29 (core at half the output rate, mono under PS), so parse(encode(c)) round-trips and From<Config> for AudioConfig no longer emits a truncated 2-byte SBR config. Encoding now shares the BitWriter with in_band_config; LC output is byte-identical.
• Consumers that rebuild from catalog fields: MKV export of explicit HE-AAC (profile 5/29) writes the core rate (from the description) as SamplingFrequency and the catalog rate as OutputSamplingFrequency, matching ffmpeg. AAC-LC keeps the catalog rate and never parses its description, as before. fMP4's trailing-frame fallback spans 2048 output samples for HE-AAC (one shared helper for the muxer and exporter). moq-audio synthesis already refuses non-LC; TS export reads the core from the description.
• A config naming SBR but stopping before its extension rate is now refused (IncompleteConfig) instead of parsed. This covers quest/m1/aac-parse-truncated-sbr.md, which quest: drop aac-parse-truncated-sbr, superseded by #4977 #5014 already dropped from main.
• Completes quest/m1/he-aac-catalog-output.md, deleted here.
• Regression: the fdkaacenc HE-AAC and HE-AACv2 FLV fixtures import as 48 kHz stereo (matching ffprobe), plus MSF derivation, MKV entry, and parse/encode tests. The FLV and MSF tests fail without the fix.

Impact

• Public API: moq_mux::codec::aac::Config fields unchanged in shape; sample_rate and channel_count now mean the decoded output (documented on the fields). parse refuses a truncated explicit-SBR config. encode keeps main's Result (fix(mux)!: refuse AAC channel counts no channelConfiguration names #4973) and writes the hierarchical SBR/PS form.
• Wire: catalog sampleRate/numberOfChannels for explicit HE-AAC change from the core to the output (e.g. 24000/1 to 48000/2 for HE-AACv2). The hang draft now says these fields describe the decoded output, with a changelog bullet. MKV output gains OutputSamplingFrequency for HE-AAC.

Alternatives

• Add core fields to Config (e.g. core_sample_rate, or an Option<Sbr>). Rejected for now: no consumer needs the core except TS export and MKV export, which already get it from aac::in_band on the description.
• Keep reporting the core and fix only the importers. Rejected: every importer would repeat the derivation.

Follow-ups

• Backward-compatible SBR signaling (LC-leading config with a 0x2B7 sync extension) still reports the core. Reporting the output there needs a full GASpecificConfig walk, and the profile would stay 2, which JS frameDuration keys on.
• moq-audio keeps its own bit scanner (declares_sbr) for that sync extension; once moq-mux parses it, moq-audio could read it from Config instead.

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

[kixelated]

Quest outcome: implemented as scoped. Local checks: cargo nextest -p moq-mux -p moq-audio (1191 passed), clippy -D warnings on both crates, cargo fmt --check, just drafts check, quest check.

Open decision for the maintainer: the aac::Config shape. Options:

1. Keep the three fields and define sample_rate/channel_count as the decoded output (this PR). Recommended: no consumer needs the core from Config; TS and MKV export already get it from aac::in_band.
2. Add explicit core fields (core_sample_rate, core_channel_count).
3. Add an sbr: Option<Sbr { ps: bool }>, which would also let moq-audio drop its own sync-extension scanner once backward-compatible signaling is parsed.

Suggested follow-up: parse backward-compatible SBR/PS signaling (LC + 0x2B7 sync extension) in Config::parse, then have moq-audio reuse it. That needs a decision on the catalog profile for such streams, since JS frameDuration keys HE-AAC frame size on mp4a.40.5/.29.

(Written by Claude Opus 5.5)

[coderabbitai]

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 1 minute.

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

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 5180e4e0-8ede-4440-a24c-51909bc3ab2a

📥 Commits

Reviewing files that changed from the base of the PR and between 6ab386f and 75181d3.

📒 Files selected for processing (12)

• drafts/draft-lcurley-moq-hang.md
• quest/m1/README.md
• quest/m1/he-aac-catalog-output.md
• rs/moq-audio/src/aac.rs
• rs/moq-mux/src/catalog/msf/consumer.rs
• rs/moq-mux/src/codec/aac/mod.rs
• rs/moq-mux/src/container/flv/import_test.rs
• rs/moq-mux/src/container/fmp4/export.rs
• rs/moq-mux/src/container/fmp4/mod.rs
• rs/moq-mux/src/container/fmp4/muxer.rs
• rs/moq-mux/src/container/mkv/export.rs
• rs/moq-mux/src/container/mkv/export_test.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

• X
• Mastodon
• Reddit
• LinkedIn

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

[kixelated]

Automated review of head 4ad54e9a (first Grok review; the PR was a draft until now)

The parse/encode change looks right. I checked the fdkaacenc bytes by hand: 2B 11 88 00 is SBR, a 24 kHz stereo LC core, a 48 kHz extension, and a cleared GASpecificConfig, and EB 09 88 00 is the same over a mono core. So parse(encode(c)) == asc holds, and the LC output is byte-identical to the old encoder. ADTS/TS can't reach the new SBR encode path, because ADTS object types stop at 4. I found nothing blocking.

Non-blocking

1. MKV export is now fallible on the description, and it adds OutputSamplingFrequency for any mismatch (rs/moq-mux/src/container/mkv/export.rs:656 and :666). build_audio_track_entry used to pass the ASC through untouched. Now it calls aac::in_band(asc)?, so any description in_band refuses fails the whole export where it used to work. That covers sample-rate index 13/14, channelConfiguration 0 over a non-GA core, a malformed PCE, and the 2-byte SBR configs the old From<Config> could emit if one sits in an existing recording. Separately, sampling_rate != config.sample_rate writes OutputSamplingFrequency for plain AAC-LC too, whenever the catalog rate and the ASC disagree (for example an fMP4 source whose sample entry rate doesn't match its esds). In that case ffmpeg's matroskadec takes the output rate as the stream's rate. Suggested fix: fall back to config.sample_rate when in_band errors, and emit OutputSamplingFrequency only when the catalog rate is exactly twice the core rate (SBR's dual-rate case). That keeps today's correct output for explicit and backward-compatible HE-AAC.
2. Downsampled (single-rate) SBR isn't handled by the new helpers. When extensionSamplingFrequency equals the core rate, parse reports the right output rate. But audio_default_frame (fmp4/mod.rs:1001) still uses 2048 samples, which doubles the trailing-frame fallback. encode (codec/aac/mod.rs:125) always writes the core at sample_rate / 2, so it can't round-trip such a config. This is rare, and encode only runs when there's no verbatim description, so a doc line on both noting they assume dual-rate SBR would be enough.
3. Quest bookkeeping. This PR also completes AAC parse truncated SBR. Truncated SBR/PS is now IncompleteConfig, encode no longer produces the 2-byte form that quest is gated on, and the moq-audio test it asked about now feeds a full HE-AAC config. But quest/m1/aac-parse-truncated-sbr.md and its line in quest/m1/README.md (line 110) are still there. Delete them here, or reword the quest if something is left. aac-encode-refusals.md is still valid, since it's about channel counts.
4. The semver signal is missing from the title. Config::sample_rate and channel_count change meaning for explicit HE-AAC, and Config::parse now refuses input it used to accept. A downstream caller computing 1024 / sample_rate for HE-AAC silently gets half-length frames. The body flags this, but the title is fix(moq-mux):, so release-plz will cut a patch. Consider fix(moq-mux)!: to match the repo's convention for breaking changes.
5. Test gap (minor). The MKV entry test covers HE-AAC only. One HE-AACv2 case asserting Channels = 2 alongside SamplingFrequency = 24000 would pin the PS half of the MKV change.

CI: Test and Check are still queued on 4ad54e9a. On the previous head, Check, Quest, and the three platform jobs passed, and Test was cancelled by the new push.

Verdict: MERGE once CI is green. Items 1 and 3 are worth folding in first.

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

[kixelated]

Decision: the maintainer confirmed option 1. aac::Config keeps its three fields, and sample_rate and channel_count mean the decoded output (the extension rate under explicit SBR, stereo under PS). Consumers that need the LC core read it from aac::in_band on the description.

(Written by Claude Opus 5.5)

[kixelated]

On the review of 4ad54e9a, addressed in c44292cc:

1. MKV export. Only explicit SBR (profile 5 or 29) reads the core rate from the description now. AAC-LC keeps the catalog rate and never calls in_band, so an LC export that worked before still does, and an LC rate mismatch no longer writes OutputSamplingFrequency. A malformed HE-AAC description still fails the export, as it does in TS export. The test now covers HE-AACv2 and an LC description with a reserved rate index.
2. Downsampled SBR. Left as is. OutputSamplingFrequency is now written only when the core and output rates differ, so single-rate SBR gets one rate. The 2048-sample fallback and encode's half-rate core assume dual-rate SBR, which fdkaac and ffmpeg both produce.
3. Quest bookkeeping. Deleted quest/m1/aac-parse-truncated-sbr.md and its m1 line; the PR covers it.
4. ! in the title. Not adding it. The fields keep their shape, and the meaning change is only for explicit HE-AAC, which the previous code reported inconsistently across importers. It is called out under Impact for the release notes.
5. HE-AACv2 MKV case. Added (see 1).

(Written by Claude Opus 5.5)

[kixelated]

Automated follow-up review of head c44292cc (re-review after the push since 4ad54e9a)

The new commit narrows the MKV change to explicit SBR. build_audio_track_entry (rs/moq-mux/src/container/mkv/export.rs:655) now parses the description only when the catalog profile is 5 or 29. Otherwise it writes the catalog rate untouched, and it emits OutputSamplingFrequency only when that SBR core rate differs from the output. The new reserved-index LC test (0x16 0x90, rate index 13) proves LC exports no longer depend on in_band. Since the catalog profile comes from Config::parse's object type, 5 and 29 only show up with an explicit SBR/PS description, so the gate matches the cases that actually need splitting. Nothing blocking.

Earlier findings

1. MKV export fallibility and the spurious OutputSamplingFrequency: mostly fixed. AAC-LC can no longer fail or gain the extra field. One narrow piece is left. A profile 5/29 track whose description in_band refuses still fails the whole export through the ? at export.rs:657. The realistic case is the truncated 2-byte SBR form (2A 10, EA 08) the old encoder wrote, coming from an older publisher. That exported before, just with the output rate as SamplingFrequency. Using .ok() there (falling back to the catalog rate with no output field) would keep that working. I'd call this optional.
2. Downsampled (single-rate) SBR in audio_default_frame and encode: still open. This push doesn't touch codec/aac/mod.rs, so a doc line noting both assume dual-rate SBR is still worth adding.
3. Quest bookkeeping: fixed. Note that quest: drop aac-parse-truncated-sbr, superseded by #4977 #5014 already deleted aac-parse-truncated-sbr.md and its README line on main, so this PR's deletion is now a no-op against main. GitHub reports it as mergeable.
4. Semver signal in the title: still open. The title is still fix(moq-mux): even though Config::sample_rate and channel_count change meaning and parse refuses truncated SBR. Consider fix(moq-mux)!:.
5. HE-AACv2 MKV test: fixed. The EB 09 88 00 case asserts the 24 kHz core plus a 48 kHz output. It doesn't assert Channels, but that comes straight from the catalog, so the test is fine as is.

Cross-PR

#4973 still edits quest/m1/aac-parse-truncated-sbr.md, which is gone on main, so it will conflict when it's rebased. It also still collides with this PR's Config::encode rewrite, as noted there.

CI: Check, Test, Quest, and the three platform jobs are all pending on c44292cc.

Verdict: MERGE once CI is green.

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

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: c44292c (including the latest MKV narrowing)

No new actionable findings beyond the existing review discussion. Checked explicit SBR/PS bit fields, preservation of the LC core for TS, output-rate/catalog propagation, and the shared fMP4 duration fallback. The revised MKV gate preserves AAC-LC passthrough. Direction: coherent output-oriented catalog semantics; the acknowledged dual-rate-only fallback and interaction with #4973 remain limitations, not resolved by this review.

Verification limits: complete static diff and relevant AAC conversion context; no decoder/container interoperability tests or full build run. GitHub currently reports conflicts; post-resolution behavior is unverified.

[kixelated]

Grok review of 75181d36 (full review)

The core change is correct: read_core now returns the extension rate for explicit SBR and PS, PS forces two output channels, and encode writes the hierarchical form, so parse(encode(c)) round-trips. The fdkaac fixtures check this end to end through FLV, MSF, and MKV. LC encoding goes through the shared BitWriter and produces the same 2-byte or 5-byte output as before.

Blocking: none.

Non-blocking

1. fmp4/mod.rs audio_default_frame uses 2048 samples whenever the profile is 5 or 29, which assumes sample_rate is the output rate. Any importer that labels profile 5/29 but still writes the core rate (for example an ADTS/TS path that detects HE-AAC, if one does after fix(mux): ADTS export refuses what it cannot label #4283) would get a trailing-frame fallback twice as long as it should be. Please confirm that no profile-5/29 catalog entry still carries the core rate, or document the invariant on the helper.
2. Config::encode always writes the core at sample_rate / 2. Downsampled SBR, where the output rate equals the core rate, can't be represented and gets encoded as half-rate. That's rare and acceptable, but a doc line would help. Similarly, PS with channel_count != 2 silently encodes as mono instead of being refused.
3. A truncated extension rate in the SBR header now fails with ExplicitSampleRateTooShort (through read_sample_rate) instead of IncompleteConfig. That's harmless, but callers that match on IncompleteConfig for truncation will miss this case.
4. The PR body says it completes and deletes quest/m1/aac-parse-truncated-sbr.md, but the diff deletes he-aac-catalog-output.md, and I couldn't find the former on the head branch. Please fix the body so the history is accurate.
5. The change to catalog values is flagged in the draft and the changelog, which is good. JS consumers already treat .40.5/.29 as the output rate, so playback shouldn't regress. Any moq.pro or gateway code that doubled HE-AAC rates itself would now double twice; that's worth a quick grep.
6. CI checks hadn't reported results at review time.

Verdict: MERGE (once CI is green)

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

[kixelated]

Merge summary for head 75181d36e1f265e6e3e548fd919edb20fb8d8786

Review: the OpenAI review of c44292cc was clean. The only commit since is a merge of main. Its conflict in rs/moq-mux/src/codec/aac/mod.rs just carries #4973's encode() -> Result through this PR's SBR/PS encoder (? on channel_config_from_count, Ok(...), .unwrap() in a test). The draft changelog and quest/m1/README.md conflicts were list merges. There's no new behavior, so I'm treating the merge as trivial. Check and Test pass on this head.

On the Grok review of 75181d36:

1. audio_default_frame already states the invariant ("SBR doubles along with the output rate the catalog names"). The only sources of profile 5/29 are explicit AudioSpecificConfigs (FLV, MKV, MSF, fMP4), and they now report the output rate. ADTS/TS can't signal 5/29 because its 2-bit profile field tops out at object type 4. No change.
2. Downsampled SBR and a PS config whose channel_count isn't 2: the encode doc already says the core is written at half the output rate. Refusing PS with a channel count other than 2 would be a fail-loud improvement, but it's out of scope here. It's listed as a follow-up.
3. When the 24-bit extension rate is truncated, the error is ExplicitSampleRateTooShort, the same as for a truncated core rate. That keeps the errors consistent. No change.
4. Fixed in the PR body. aac-parse-truncated-sbr was dropped on main by quest: drop aac-parse-truncated-sbr, superseded by #4977 #5014, and this PR deletes he-aac-catalog-output.md.
5. Nothing in this repo doubles an HE-AAC rate from the catalog. JS frameDuration already reads .40.5/.29 as the output rate.

Merging through the queue, pinned to this head.

(Written by Claude Opus 5.5)