feat(audio): carry surround layouts through decode, remix, and playback - #4119
Conversation
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. |
There was a problem hiding this comment.
💡 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), |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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)
| if has(other) { | ||
| feed(other, 1.0); | ||
| } else { | ||
| feed(front, FRAC_1_SQRT_2); | ||
| } |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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)
|
MERGE Positive improvement, complexity justified, approach looks right. Stopping at stereo left 5.1 (and other WAVE counts) stuck as Concrete strengths:
Semantic change for This is an automated review, not the maintainer's decision |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
e8c33db to
f8a1cc0
Compare
|
Rebased onto Conflict resolution only:
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 Local (Written by Grok 4.7) |
Problem
moq-audiostopped at stereo. A catalog count past 2 decoded asLayout::Discrete(n), which refuses every remix, so a 5.1 PCM broadcast could not be read at stereo or mono.playback::Sinkaccepted 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
Layoutgains the well-known surround layouts:TwoPointOne,ThreePointZero,Quad,FourPointZero,FivePointZero,FivePointOne,SixPointOne, andSevenPointOne. They interleave in the SMPTE/WAVE order, backed by a crate-private speaker list.Layout::from_channelsapplies 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 stayDiscrete. The catalog and wire don't change.resample::remixbecomes a precomputedRemixmatrix 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.Discretepasses through and can drop to a same-countDiscrete, but it never gets speaker positions.Producer,Consumer, and capture build the matrix once instead of once per write.Discrete: a microphone array is not a speaker layout.Sinkremixes 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.channelsas 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/hangAac.audioSpecificConfigthrows for a count with no channelConfiguration instead of describing it as stereo.encode::Produceranddecode::Consumerand reads back at 5.1, stereo, and mono with BS.775 values.Impact
moq_audio::Layout: eight new variants. The enum is#[non_exhaustive], so this is additive and stays onmain.Layout::from_channels(3..=8)returns a named layout instead ofDiscrete(n). So dodecode::Decoder::layout()andConsumer::layout()for such a catalog.playback::Inputaccepts any named layout (previously only mono or stereo) and still refusesDiscrete.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/hangAac.audioSpecificConfigthrows for 0, 7, or more than 8 channels.Alternatives
FivePointOnestyle. Other options areSurround5_1, or a public speaker-mask struct. The struct is more extensible, but replacing the enum would be adevbreak.C L R Ls Rs LFE): not added. Every AAC path inmoq-audiostill 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::encodestill writes stereo for a count it can't name, so JS and Rust now disagree. Making it fail needs a fallible signature, which is adevbreak (see fix(mux): parse the AAC program config element instead of guessing stereo #4093).just rs macosandjust rs windowswere not run.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code