Tell CoreAudio how to interpret Chrome's channel layout.

Tell CoreAudio how to interpret Chrome's channel layout.

Previously we always assumed the default layout which causes channels
to be mapped incorrectly if the user has manually assigned channels.

Instead, configure the AUHAL with our known channel layout, or in the
case of discrete the most likely layout so that macOS will remap the
channels in the appropriate order during playout.

BUG=671308, 675787
TEST=5.1 virtual device configured with mixed channels plays the
correct order.
https://storage.googleapis.com/dalecurtis/dolby6.mp4
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2683033003
Cr-Commit-Position: refs/heads/master@{#449694}
Committed: https://chromium.googlesource.com/chromium/src/+/30068b4b24fc18a1a6473866dcc9a8ff28d4ec30

[DaleCurtis, 2017-02-09 02:28:35]

Description was changed from

==========
Tell CoreAudio how to interpret Chrome's channel layout.

Previously we always assumed the default layout which causes channels
to be mapped incorrectly if the user has manually assigned channels.

Instead, configure the AUHAL with our known channel layout, or in the
case of discrete the most likely layout so that OSX will remap the
channels in the appropriate order during playout.

BUG=671308,651172
TEST=5.1 virtual device configured with mixed channels plays the
correct order.
https://storage.googleapis.com/dalecurtis/dolby6.mp4
==========

to

==========
Tell CoreAudio how to interpret Chrome's channel layout.

Previously we always assumed the default layout which causes channels
to be mapped incorrectly if the user has manually assigned channels.

Instead, configure the AUHAL with our known channel layout, or in the
case of discrete the most likely layout so that OSX will remap the
channels in the appropriate order during playout.

BUG=671308,651172
TEST=5.1 virtual device configured with mixed channels plays the
correct order.
https://storage.googleapis.com/dalecurtis/dolby6.mp4
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[DaleCurtis, 2017-02-09 02:30:41]

dalecurtis@chromium.org changed reviewers:
+ hongchan@chromium.org, rtoy@chromium.org

[DaleCurtis, 2017-02-09 02:30:41]

+folk whose help I need in testing this :)

[Avi (use Gerrit), 2017-02-09 02:44:38]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2017-02-09 02:44:38]

https://codereview.chromium.org/2683033003/diff/1/media/audio/mac/audio_auhal...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2683033003/diff/1/media/audio/mac/audio_auhal...
media/audio/mac/audio_auhal_mac.cc:48: "Channel positions must max OSX channel
order.");
"must match"? (And it's "macOS" nowadays.)

https://codereview.chromium.org/2683033003/diff/1/media/audio/mac/audio_auhal...
media/audio/mac/audio_auhal_mac.cc:593: //
https://developer.apple.com/library/content/qa/qa1627/_index.html
Can you clarify in the comments why it appears you're doing this in a very weird
way? e.g.

1. We define FIELD_OFFSET rather than use offsetof from <cstddef> because
AudioChannelLayout is a C struct that ends with a variable-length array:

typedef struct AudioChannelLayout {
    // ...
    AudioChannelDescription mChannelDescriptions[1];
} AudioChannelLayout;

and you can't use offsetof in that case.

2. We allocate the AudioChannelLayout dynamically rather than statically because
of this reason.

-----

etc. BTW, _can_ you use offsetof instead? It would be cool if you could.

https://codereview.chromium.org/2683033003/diff/1/media/base/channel_layout.h
File media/base/channel_layout.h (right):

https://codereview.chromium.org/2683033003/diff/1/media/base/channel_layout.h...
media/base/channel_layout.h:116: // ordering to operate correctly. E.g., OSX
channel layout computations.
macOS

[DaleCurtis, 2017-02-09 03:17:58]

Patchset #2 (id:20001) has been deleted

[DaleCurtis, 2017-02-09 03:18:39]

Thanks for the drive by!

https://codereview.chromium.org/2683033003/diff/1/media/audio/mac/audio_auhal...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2683033003/diff/1/media/audio/mac/audio_auhal...
media/audio/mac/audio_auhal_mac.cc:48: "Channel positions must max OSX channel
order.");
On 2017/02/09 at 02:44:38, Avi wrote:
> "must match"? (And it's "macOS" nowadays.)

Done, switched to CoreAudio like we use elsewhere for consistency.

https://codereview.chromium.org/2683033003/diff/1/media/audio/mac/audio_auhal...
media/audio/mac/audio_auhal_mac.cc:593: //
https://developer.apple.com/library/content/qa/qa1627/_index.html
On 2017/02/09 at 02:44:38, Avi wrote:
> Can you clarify in the comments why it appears you're doing this in a very
weird way? e.g.
> 
> 1. We define FIELD_OFFSET rather than use offsetof from <cstddef> because
AudioChannelLayout is a C struct that ends with a variable-length array:
> 
> typedef struct AudioChannelLayout {
>     // ...
>     AudioChannelDescription mChannelDescriptions[1];
> } AudioChannelLayout;
> 
> and you can't use offsetof in that case.
> 
> 2. We allocate the AudioChannelLayout dynamically rather than statically
because of this reason.
> 
> -----
> 
> etc. BTW, _can_ you use offsetof instead? It would be cool if you could.

I thought (1) would prevent this, but offsetof() works! Thanks for the
suggestion!

https://codereview.chromium.org/2683033003/diff/1/media/base/channel_layout.h
File media/base/channel_layout.h (right):

https://codereview.chromium.org/2683033003/diff/1/media/base/channel_layout.h...
media/base/channel_layout.h:116: // ordering to operate correctly. E.g., OSX
channel layout computations.
On 2017/02/09 at 02:44:38, Avi wrote:
> macOS

Switched in favor of CoreAudio.

[Avi (use Gerrit), 2017-02-09 03:33:46]

lgtm

drive-by approval then

[hongchan, 2017-02-09 04:10:25]

I will test the CL against my 16 channel audio interface.

https://codereview.chromium.org/2683033003/diff/40001/media/audio/mac/audio_a...
File media/audio/mac/audio_auhal_mac.cc (right):

https://codereview.chromium.org/2683033003/diff/40001/media/audio/mac/audio_a...
media/audio/mac/audio_auhal_mac.cc:598: //
https://developer.apple.com/library/content/qa/qa1627/_index.html
Thanks so much for doing this. That is one convoluted way of setting the layout.
:)

[DaleCurtis, 2017-02-09 17:33:45]

Description was changed from

==========
Tell CoreAudio how to interpret Chrome's channel layout.

Previously we always assumed the default layout which causes channels
to be mapped incorrectly if the user has manually assigned channels.

Instead, configure the AUHAL with our known channel layout, or in the
case of discrete the most likely layout so that OSX will remap the
channels in the appropriate order during playout.

BUG=671308,651172
TEST=5.1 virtual device configured with mixed channels plays the
correct order.
https://storage.googleapis.com/dalecurtis/dolby6.mp4
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Tell CoreAudio how to interpret Chrome's channel layout.

Previously we always assumed the default layout which causes channels
to be mapped incorrectly if the user has manually assigned channels.

Instead, configure the AUHAL with our known channel layout, or in the
case of discrete the most likely layout so that OSX will remap the
channels in the appropriate order during playout.

BUG=671308,675787
TEST=5.1 virtual device configured with mixed channels plays the
correct order.
https://storage.googleapis.com/dalecurtis/dolby6.mp4
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[hongchan, 2017-02-09 20:31:39]

On 2017/02/09 04:10:25, hongchan wrote:
> I will test the CL against my 16 channel audio interface.
> 
>
https://codereview.chromium.org/2683033003/diff/40001/media/audio/mac/audio_a...
> File media/audio/mac/audio_auhal_mac.cc (right):
> 
>
https://codereview.chromium.org/2683033003/diff/40001/media/audio/mac/audio_a...
> media/audio/mac/audio_auhal_mac.cc:598: //
> https://developer.apple.com/library/content/qa/qa1627/_index.html
> Thanks so much for doing this. That is one convoluted way of setting the
layout.
> :)

Okay, here's what I did:
(macOS Sierra 10.12.2)

1. Apply the patch to ToT.
2. Connect FCA1616.
3. Open Audio MIDI setup > Select FCA1616 > Configure Speakers > Select 7.1 Rear
Surround.
4. Launch ToT + Patch release build.
5. Play dolby6.mp4. (by drag-and-dropping the file to the tab.)

I can confirm it plays 5.1 channels content correctly. (The output channels
matches the diagram in Audio/MIDI setup speaker config app.)

[hongchan, 2017-02-09 21:23:46]

On 2017/02/09 20:31:39, hongchan wrote:
> On 2017/02/09 04:10:25, hongchan wrote:
> > I will test the CL against my 16 channel audio interface.
> > 
> >
>
https://codereview.chromium.org/2683033003/diff/40001/media/audio/mac/audio_a...
> > File media/audio/mac/audio_auhal_mac.cc (right):
> > 
> >
>
https://codereview.chromium.org/2683033003/diff/40001/media/audio/mac/audio_a...
> > media/audio/mac/audio_auhal_mac.cc:598: //
> > https://developer.apple.com/library/content/qa/qa1627/_index.html
> > Thanks so much for doing this. That is one convoluted way of setting the
> layout.
> > :)
> 
> Okay, here's what I did:
> (macOS Sierra 10.12.2)
> 
> 1. Apply the patch to ToT.
> 2. Connect FCA1616.
> 3. Open Audio MIDI setup > Select FCA1616 > Configure Speakers > Select 7.1
Rear
> Surround.
> 4. Launch ToT + Patch release build.
> 5. Play dolby6.mp4. (by drag-and-dropping the file to the tab.)
> 
> I can confirm it plays 5.1 channels content correctly. (The output channels
> matches the diagram in Audio/MIDI setup speaker config app.)

Okay, this is what I got from media-internals with the set up of AudioContext
(16 channels):

channel_layout	DISCRETE
channels	16
device_id	AppleUSBAudioEngine:BEHRINGER:FCA1616:0x1564002aafe:2,1
device_type	pcm_low_latency
effects	NO_EFFECTS
frames_per_buffer	256
sample_rate	48000
status	started
volume	1

Also I found that the CL is not compatible with the change I made
(https://codereview.chromium.org/810763006) for the channel layout. With this
CL, now I don't hear anything beyond channel #7 in the following manual test.

WebKit/ManualTests/webaudio/multichannel.html

This is a small manual test for any type of multichannel audio device. Note that
I get all the audio output up to 16 channels with the current stable version.

[hongchan, 2017-02-09 23:13:00]

I confirmed that PS3 fixes the web audio multichannel (8 >) issue.

lgtm

[DaleCurtis, 2017-02-09 23:25:24]

Great thanks for review! rtoy@ did you have anything to add or ready for CQ?

[Raymond Toy, 2017-02-09 23:58:14]

On 2017/02/09 23:25:24, DaleCurtis wrote:
> Great thanks for review! rtoy@ did you have anything to add or ready for CQ?

Not really.  hongchan@ and I did discuss what happens if I feed in a 6-channel
audio file to an audio tag and play it on mac, windows, and linux would the
expected audio come out the same set of speakers.  (This would make Chrome at
least self-consistent, even if it didn't match anything else in the world.)

But that's probably out of scope of this CL.

[DaleCurtis, 2017-02-10 00:10:17]

On 2017/02/09 at 23:58:14, rtoy wrote:
> On 2017/02/09 23:25:24, DaleCurtis wrote:
> > Great thanks for review! rtoy@ did you have anything to add or ready for CQ?
> 
> Not really.  hongchan@ and I did discuss what happens if I feed in a 6-channel
audio file to an audio tag and play it on mac, windows, and linux would the
expected audio come out the same set of speakers.  (This would make Chrome at
least self-consistent, even if it didn't match anything else in the world.)
> 
> But that's probably out of scope of this CL.

Assuming your speakers are labeled correctly it should be true that things play
out consistently. OSX is the only platform with DISCRETE layout support FWIW.

[DaleCurtis, 2017-02-10 00:10:24]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2017-02-10 00:10:25]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org
Link to the patchset: https://codereview.chromium.org/2683033003/#ps60001
(title: "Fix 8ch+ discrete layouts.")

[commit-bot: I haz the power, 2017-02-10 00:10:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[DaleCurtis, 2017-02-10 00:10:52]

Description was changed from

==========
Tell CoreAudio how to interpret Chrome's channel layout.

Previously we always assumed the default layout which causes channels
to be mapped incorrectly if the user has manually assigned channels.

Instead, configure the AUHAL with our known channel layout, or in the
case of discrete the most likely layout so that OSX will remap the
channels in the appropriate order during playout.

BUG=671308,675787
TEST=5.1 virtual device configured with mixed channels plays the
correct order.
https://storage.googleapis.com/dalecurtis/dolby6.mp4
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Tell CoreAudio how to interpret Chrome's channel layout.

Previously we always assumed the default layout which causes channels
to be mapped incorrectly if the user has manually assigned channels.

Instead, configure the AUHAL with our known channel layout, or in the
case of discrete the most likely layout so that macOS will remap the
channels in the appropriate order during playout.

BUG=671308,675787
TEST=5.1 virtual device configured with mixed channels plays the
correct order.
https://storage.googleapis.com/dalecurtis/dolby6.mp4
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[commit-bot: I haz the power, 2017-02-10 00:59:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 00:59:04]

Try jobs failed on following builders:
  mac_optional_gpu_tests_rel on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_optional_gpu_...)

[DaleCurtis, 2017-02-10 02:13:30]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2017-02-10 02:13:51]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 08:14:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 08:14:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2017-02-10 17:51:51]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2017-02-10 17:52:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-10 19:39:49]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486749111687410,
"parent_rev": "ed8f1828942b62be160096076aef279b1367f2bb", "commit_rev":
"30068b4b24fc18a1a6473866dcc9a8ff28d4ec30"}

[commit-bot: I haz the power, 2017-02-10 19:40:21]

Description was changed from

==========
Tell CoreAudio how to interpret Chrome's channel layout.

Previously we always assumed the default layout which causes channels
to be mapped incorrectly if the user has manually assigned channels.

Instead, configure the AUHAL with our known channel layout, or in the
case of discrete the most likely layout so that macOS will remap the
channels in the appropriate order during playout.

BUG=671308,675787
TEST=5.1 virtual device configured with mixed channels plays the
correct order.
https://storage.googleapis.com/dalecurtis/dolby6.mp4
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Tell CoreAudio how to interpret Chrome's channel layout.

Previously we always assumed the default layout which causes channels
to be mapped incorrectly if the user has manually assigned channels.

Instead, configure the AUHAL with our known channel layout, or in the
case of discrete the most likely layout so that macOS will remap the
channels in the appropriate order during playout.

BUG=671308,675787
TEST=5.1 virtual device configured with mixed channels plays the
correct order.
https://storage.googleapis.com/dalecurtis/dolby6.mp4
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2683033003
Cr-Commit-Position: refs/heads/master@{#449694}
Committed:
https://chromium.googlesource.com/chromium/src/+/30068b4b24fc18a1a6473866dcc9...
==========

[commit-bot: I haz the power, 2017-02-10 19:40:23]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/30068b4b24fc18a1a6473866dcc9...