Skip to content

feat(audio): carry surround layouts through decode, remix, and playback - #4119

Merged
kixelated merged 2 commits into
quest/m1/audio-codecs/READMEfrom
quest/m1/audio-codecs/layout
Sep 25, 2026
Merged

kixelated merged 2 commits into
quest/m1/audio-codecs/READMEfrom
quest/m1/audio-codecs/layout

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Problem

moq-audio stopped at stereo. A catalog count past 2 decoded as Layout::Discrete(n), which refuses every remix, so a 5.1 PCM broadcast could not be read at stereo or mono. playback::Sink accepted only mono or stereo, and the mixer ran a fixed stereo bus that left every speaker past the front pair silent on a surround device.

Approach

  • Layout gains the well-known surround layouts: TwoPointOne, ThreePointZero, Quad, FourPointZero, FivePointZero, FivePointOne, SixPointOne, and SevenPointOne. They interleave in the SMPTE/WAVE order, backed by a crate-private speaker list.
  • Layout::from_channels applies the WAVE default for each count: 3 is 2.1, 4 quad, 5 5.0, 6 5.1, 7 6.1, 8 7.1. Counts past 8 stay Discrete. The catalog and wire don't change.
  • resample::remix becomes a precomputed Remix matrix between any two named layouts. Downmix uses ITU-R BS.775: center and surrounds fold into the front pair at -3 dB, and the LFE is dropped. Upmix leaves the extra speakers silent. Mono keeps its old behavior: duplicated at full level when there is no center, and a mono output averages the stereo downmix. Discrete passes through and can drop to a same-count Discrete, but it never gets speaker positions. Producer, Consumer, and capture build the matrix once instead of once per write.
  • Capture keeps treating a device count past 2 as Discrete: a microphone array is not a speaker layout.
  • Playback: the mix bus runs in the device's layout, and each Sink remixes into it on write. The old fan-out is gone. The echo reference downmixes the bus to stereo on the audio thread without allocating. The device chooser keeps only counts 1 to 8. It prefers the device's own default count, then the widest layout.
  • moq-ffi and libmoq keep channels as a count. The Rust doc comments and every binding's doc (py, kt, swift, go, c) state the count-to-layout mapping. Dart has no audio yet.
  • js/hang Aac.audioSpecificConfig throws for a count with no channelConfiguration instead of describing it as stereo.
  • Regressions:
    • A 5.1 PCM broadcast goes through encode::Producer and decode::Consumer and reads back at 5.1, stereo, and mono with BS.775 values.
    • A 5.1 sink plays into a stereo bus, and a stereo sink into a 5.1 bus.
    • A surround bus mixes every speaker, and the echo reference sees its downmix.
    • The chooser opens six channels when offered, keeps a headset's native two, and refuses counts past 8.
  • The quest file and its references are removed.

Impact

  • moq_audio::Layout: eight new variants. The enum is #[non_exhaustive], so this is additive and stays on main.
  • Layout::from_channels(3..=8) returns a named layout instead of Discrete(n). So do decode::Decoder::layout() and Consumer::layout() for such a catalog.
  • playback::Input accepts any named layout (previously only mono or stereo) and still refuses Discrete.
  • Playback opens a surround device at its native layout. A device that offers only counts past 8 is refused, where it used to get stereo on its first two channels.
  • moq-ffi and libmoq: no record, repr(C), or binding changes. A count of 3 to 8 now names a layout, so asking for 2 channels from a 6-channel track downmixes instead of failing.
  • @moq/hang Aac.audioSpecificConfig throws for 0, 7, or more than 8 channels.
  • No wire or catalog change.

Alternatives

  • Variant names: I followed symphonia's FivePointOne style. Other options are Surround5_1, or a public speaker-mask struct. The struct is more extensible, but replacing the enum would be a dev break.
  • Device chooser: the quest says "widest well-known layout". ALSA's plug, PulseAudio, and PipeWire devices list every channel count up to their limit, so taking the widest would open a headset at 7.1. The device's default count goes first, and the widest is the fallback.
  • A device with only counts past 8: refused here. Keeping stereo on its first pair is the other option, but that guesses speaker positions.
  • AAC channel-order mapping (C L R Ls Rs LFE): not added. Every AAC path in moq-audio still refuses more than two channels, so the mapping would be dead code. AudioToolbox maps from its own channel layout tag. The Vorbis mapping belongs to opus-surround.

Follow-ups

  • moq_mux::codec::aac::Config::encode still writes stereo for a count it can't name, so JS and Rust now disagree. Making it fail needs a fallible signature, which is a dev break (see fix(mux): parse the AAC program config element instead of guessing stereo #4093).
  • The OBS source maps decoded counts through FFmpeg, where 4 is 4.0 and 5 is 4.1. That differs from the WAVE defaults. It doesn't use libmoq decode yet.
  • Platform code is compiled on Linux only. just rs macos and just rs windows were not run.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

@kixelated
kixelated marked this pull request as ready for review September 25, 2026 07:27
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 25, 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-09-25T14:27:21.512384Z f8a1cc0 New commits
ℹ️ 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 chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e8c33db929

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

3 => Ok(Self::TwoPointOne),
4 => Ok(Self::Quad),
5 => Ok(Self::FivePointZero),
6 => Ok(Self::FivePointOne),

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Preserve discrete PCM instead of inferring 5.1

When a PCM producer uses Layout::Discrete(6), which Encoder::new_pcm still accepts, the catalog serializes only its channel count and Decoder::new_pcm resolves that count through this arm. The stream therefore comes back as FivePointOne, so requesting stereo silently remixes arbitrary source-order channels and even drops the fourth channel as LFE instead of refusing the unsupported conversion. Preserve the discrete marker on the wire or reject layouts the PCM catalog cannot represent.

AGENTS.md reference: AGENTS.md:L75-L77

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not changing this. The catalog stores only a channel count, and Layout::from_channels is the WAVE default for that count on purpose. pcm_passes_discrete_multichannel_samples_through already records that Discrete(3) comes back as TwoPointOne with the samples untouched. A discrete marker, or refusing Discrete(3..=8) at encode, is a catalog or API change this PR does not make. Capture still keeps a microphone array past stereo as Discrete, so this process does not remix it as speakers.

(Written by Grok 4.7)

Comment on lines +414 to +418
if has(other) {
feed(other, 1.0);
} else {
feed(front, FRAC_1_SQRT_2);
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Route paired surrounds to the back-center output

When the output has BackCenter but not the input's side/back pair, this branch never feeds that available surround speaker. For example, FivePointZero -> FourPointZero leaves the back-center channel silent and folds both surrounds into the front pair, while SevenPointOne -> SixPointOne folds the back pair into the sides. Detect BackCenter before falling back to the equivalent pair or fronts and combine the pair into it with the appropriate coefficients.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Not changing the matrix. A side or back speaker folds onto the other of that pair when the output has it (remix_folds_back_into_side_surrounds is 7.1 into 5.1 at full level), and onto the front pair otherwise. That is the downmix this PR specified. Sending the pair to BackCenter first (5.0 to 4.0, 7.1 to 6.1) needs a coefficient the quest did not pick, so I am leaving it.

(Written by Grok 4.7)

@kixelated

Copy link
Copy Markdown
Collaborator Author

MERGE

Positive improvement, complexity justified, approach looks right.

Stopping at stereo left 5.1 (and other WAVE counts) stuck as Discrete(n), which refused remix, so surround catalogs could not play on stereo/mono devices and the mixer could not drive surround hardware. Naming the WAVE layouts, mapping from_channels(3..=8), and replacing ad-hoc mono↔stereo remix with a precomputed Remix matrix is the right shape.

Concrete strengths:

  • SMPTE/WAVE interleave + crate-private Speaker list; BS.775 downmix (center/surrounds at −3 dB, LFE dropped); upmix leaves missing speakers silent; mono special-cases match prior behavior.
  • Capture still treats mic arrays past 2 as Discrete—does not invent speaker positions.
  • Playback bus runs in the device layout; sinks remix on write; echo reference downmixes to stereo without allocating on the audio thread.
  • Device chooser prefers the device default count (avoids opening a headset at 7.1 just because ALSA lists every count), then widest, and refuses >8 instead of silently truncating.
  • Bindings keep channels as a count with docs for the mapping—no wire/catalog break.
  • Roundtrip + remix unit tests encode the BS.775 numbers explicitly; AAC config now fails closed on unrepresentable channel counts.

Semantic change for from_channels(3..=8) is intentional and additive on a #[non_exhaustive] enum. Complexity is justified by real playback correctness, not speculative API surface.

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

kixelated and others added 2 commits September 25, 2026 07:12
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated force-pushed the quest/m1/audio-codecs/layout branch from e8c33db to f8a1cc0 Compare September 25, 2026 14:22
@kixelated
kixelated merged commit c0c7856 into quest/m1/audio-codecs/README Sep 25, 2026
7 checks passed
@kixelated
kixelated deleted the quest/m1/audio-codecs/layout branch September 25, 2026 14:22

Copy link
Copy Markdown
Collaborator Author

Rebased onto quest/m1/audio-codecs/README (after #4093 and #4090) and pushed f8a1cc00d.

Conflict resolution only:

  • Quest list keeps the base's TS export PCE entry and drops the completed Layout quest. HE-AAC refusal and AAC PCE stay deleted; the base already finished them.
  • layout.md stays deleted. The PCE-to-layout sentence the base added there is still the AAC channel-order follow-up in the PR body, not part of this change.
  • aac-pce.md stays deleted.
  • decode-backend.md and doc/lib/rs/moq-audio.md kept both sides: the base's implicit-SBR note and this PR's layout text.

Left the Codex notes. A count of 3 to 8 coming back as the WAVE layout is the catalog contract, and folding a surround pair into BackCenter needs a coefficient this quest did not pick.

Local just check passed Rust (456), JS, and quest. The combined run hit a Dart analyzer "too many open files" error; just dart check alone passed. This base has no required checks, so the squash-merge ran immediately while Check, Test, OBS, and Swift were still queued.

(Written by Grok 4.7)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant