Fix for CoreAudio stereo problem for unknown speakers

Fix for CoreAudio stereo problem for unknown speakers

BUG=85100
TEST=HTML5 videos with stereo channel layout on Mac with unconfigured speakers can be heard.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88339

[annacc, 2011-06-07 21:22:21]

This change fixes a problem with no audio processing for unconfigured (unknown)
speaker arrangements.

Before, unknown speakers were ignored, but we would notice that the channel
layouts between the user's device and the audio source were different and so we
would attempt to swizzle to match the device layout.  But, because we didn't
understand the device layout (unknown channels), we were swizzling to nowhere.

Now, if all channels are detected as unknown, we leave the audio buffer alone
(no swizzling or down sampling) and leave it up to the device to make sense of
the audio.

[scherkus (not reviewing), 2011-06-07 22:43:22]

nits

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:99: AudioObjectID device_id = 0;
double checking.. intended change?

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:183: bool allChannelsUnknown = true;
unix_hacker style: all_channels_unknown

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:187: if (label !=
kAudioChannelLabel_Unknown) {
nit: instead check for opposite condition + continue

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:249: if (!allChannelsUnknown) {
nit: check for opposite condition and return early -- also I'd include a comment
for this case and perhaps a LOG(INFO) message

[annacc, 2011-06-07 23:09:44]

Thanks Andrew!

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:99: AudioObjectID device_id = 0;
On 2011/06/07 22:43:22, scherkus wrote:
> double checking.. intended change?

Yep, acts the same way, just looks prettier.

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:183: bool allChannelsUnknown = true;
On 2011/06/07 22:43:22, scherkus wrote:
> unix_hacker style: all_channels_unknown

Arg, I forget your crazy conventions. :)  Thanks.  Done.

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:187: if (label !=
kAudioChannelLabel_Unknown) {
On 2011/06/07 22:43:22, scherkus wrote:
> nit: instead check for opposite condition + continue

Done.

http://codereview.chromium.org/7134001/diff/2001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:249: if (!allChannelsUnknown) {
On 2011/06/07 22:43:22, scherkus wrote:
> nit: check for opposite condition and return early -- also I'd include a
comment
> for this case and perhaps a LOG(INFO) message

Done.

[scherkus (not reviewing), 2011-06-07 23:12:15]

sanity checks!

http://codereview.chromium.org/7134001/diff/5001/media/audio/mac/audio_output...
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/7134001/diff/5001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:188: all_channels_unknown = false;
shouldn't this be outside the if statement?

http://codereview.chromium.org/7134001/diff/5001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:236: case kAudioChannelLabel_Unknown:
this code will never get executed due to if statement

http://codereview.chromium.org/7134001/diff/5001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:274: CheckForAdjustedLayout(CENTER, RIGHT);
this code fixes the mono bug?

[annacc, 2011-06-07 23:34:26]

http://codereview.chromium.org/7134001/diff/5001/media/audio/mac/audio_output...
File media/audio/mac/audio_output_mac.cc (right):

http://codereview.chromium.org/7134001/diff/5001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:188: all_channels_unknown = false;
On 2011/06/07 23:12:15, scherkus wrote:
> shouldn't this be outside the if statement?

Whoa!  Movin' too fast.  Good catch!!

http://codereview.chromium.org/7134001/diff/5001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:236: case kAudioChannelLabel_Unknown:
On 2011/06/07 23:12:15, scherkus wrote:
> this code will never get executed due to if statement

True, it's gone.

http://codereview.chromium.org/7134001/diff/5001/media/audio/mac/audio_output...
media/audio/mac/audio_output_mac.cc:274: CheckForAdjustedLayout(CENTER, RIGHT);
On 2011/06/07 23:12:15, scherkus wrote:
> this code fixes the mono bug?

Yes, for people who have speakers already configured.

[scherkus (not reviewing), 2011-06-07 23:35:31]

LGTM

[commit-bot: I haz the power, 2011-06-08 15:32:06]

Try job failure for 7134001-4002 on linux for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

[M-A Ruel, 2011-06-08 15:34:42]

On 2011/06/08 15:32:06, commit-bot wrote:
> Try job failure for 7134001-4002 on linux for step "compile" (clobber build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux&numb...

I'm adding code to skip over broken revisions.

[M-A Ruel, 2011-06-08 15:35:16]

On 2011/06/08 15:34:42, Marc-Antoine Ruel wrote:
> I'm adding code to skip over broken revisions.

Requeueing again.

[commit-bot: I haz the power, 2011-06-08 16:30:24]

Change committed as 88339