Repository navigation
Downmix: Fit surround to devices that are neither stereo nor wider - #538
Open
devinprater wants to merge 3 commits into
Open
devinprater wants to merge 3 commits into
devinprater wants to merge 3 commits into
Conversation
DownmixProcessor.process: matches an input with more channels than the output against none of its branches when the device is neither stereo nor mono, and falls off the end leaving the caller's buffer untouched. The samples reach the device as they were, silence, so FreeSurround upmixing a stereo track to 5.1 over a 4-channel device plays nothing at all. Add the missing case: downmix to stereo first, then fit that to the device's layout, both of which the file already does. The final branch loses its inConfig == outConfig test, since with the counts equal the samples are already in the order the device takes them, and now covers every remaining pair rather than silently matching none. Verified over DownmixProcessor for 6, 5, 4 and 3-channel outputs, all of which wrote nothing before and carry audio after.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Equal-width layouts are copied without semantic channel remapping, potentially routing audio to incorrect speakers.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Fixes silent playback when surround audio is sent to narrower, non-stereo devices.
Changes:
- Downmixes surround input to stereo before fitting the device layout.
- Adds fallback handling for equal channel counts.
| File | Description |
|---|---|
Audio/Shared/Downmix.m |
Extends channel fitting for previously unhandled layouts. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
302
to
306
| } else { | ||
| /* Same channel count: the samples are already in the order the device | ||
| * wants them, whether or not the two layouts are named the same. */ | ||
| memcpy(outBuffer, inBuffer, frames * outputFormat.mBytesPerPacket); | ||
| } |
The last branch of process: was a bare else that copied the bytes positionally whenever the channel counts matched. Counts match but layouts differ (a 2.1 source, a 3.0 device) and a positional copy sends the LFE sample to the centre speaker. Route that case through upmix, which maps each channel to the position its flag names and clears positions that have no input to fill, and keep the raw copy only for identical layouts (inConfig == outConfig). Addresses the Copilot review on PR losnoco#538.
For a flag whose bit is not in channelConfig (e.g. LFE queried against a 3.0 FL|FR|C layout), the old code walked past the set bit and returned the running ordinal (3) rather than ~0. That made the '!= ~0' guard in Upmix's generic branch a no-op and routed that channel's samples to an out-of-bounds slot. Return ~0 when the flag's bit is set (as expected) but the config lacks it. All other code paths (present flags, ordinal counting) are unchanged. Fixes: PR losnoco#538 Copilot note on 2.1→3.0 LFE→centre misroute — the root cause behind the copy that d1faf99 replaced with upmix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #537.
DownmixProcessor.process:has a case it does not handle: an input with more channels than the output, where the device is neither stereo nor mono. None of its branches match, and since the chain ends without anelse, the method returns having written nothing to the caller's buffer. The samples reach the device as they were — silence.The usual way to meet it is FreeSurround. It upmixes a stereo track to 5.1, and if the output device takes 4 channels, that is 6 in and 4 out: the two stereo branches fail on
outConfig, the upmix branch fails on the channel count comparison, and theinConfig == outConfigbranch fails because the two layouts differ. 3, 5 and 4-channel devices are all affected; 2, 6 and 8-channel ones are not, which is why this goes unnoticed until someone plugs in a device of the wrong width.The fix adds the missing case and leaves the existing ones alone. Downmix to stereo first, then fit that stereo to the device's layout — both steps the file already performs elsewhere, so no new behaviour is introduced.
The last branch also changes.
inConfig == outConfigis dropped, because with the channel counts equal the samples are already in the order the device takes them whether or not the two layouts are named the same, and because the branch now has to cover the remaining cases rather than matching none. That is the part I am least certain about, and the one I would most like a second opinion on: it is a fall-through that silently did nothing, and I would rather it copied than returned nothing at all.Verified by building a harness over the framework's own
DownmixProcessorwith the output buffer prefilled, so "wrote nothing" can be told apart from "wrote zeros". Input channels against device channels, FreeSurround's 5.1 output in, unchanged buffer out:The 6 -> 4 figure is identical to the 6 -> 2 figure, which is the expected result: 5.1 folds to stereo, and stereo takes the device's front pair.
On the device that prompted this: a USB dock exposing two stereo jacks, a headphone jack and a line out, presented as one 4-channel device. The front pair is the headphone jack, so the audio lands where the listener is. Deactivating the unused stream does not reduce the device's channel count (
kAudioStreamPropertyIsActiveaccepts the write and the device still reports four), and an aggregate withchannels-outset to 2 still reports four, so the fix has to be here.The one thing this does not do is send surround material to a genuine quad device's rear pair: 6 -> 4 folds the back channels into front like the 6 -> 2 case does, rather than mapping them to channels 2 and 3. That seemed better than silence, and better than guessing at a layout for a case I cannot test, but it is a choice worth making deliberately.