Add support for 5.1.4 and 7.1.4 audio channel layouts [chromium/src : main]
Syed AbuTalib (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert added 3 comments![]( "Open in Gerrit")
if (layout == CHANNEL_LAYOUT_DISCRETE) {
switch (channels) {
case 10:
// Map to canonical AAUDIO_CHANNEL_5POINT1POINT4 channel mask for
// 10-channel PCM MediaCodec decoded audio. This ensures
// compatibility with Android devices for signaling 10-channel output.
return AAUDIO_CHANNEL_5POINT1POINT4;
case 12:
// Map to canonical AAUDIO_CHANNEL_7POINT1POINT4 channel mask for
// 12-channel PCM MediaCodec decoded audio. This ensures
// compatibility with Android devices for signaling 12-channel output.
return AAUDIO_CHANNEL_7POINT1POINT4;
default:
return std::nullopt;
}
}This should be cleaned up whenever we allow 12 channels. Add a TODO to track this?
static const std::optional<int> kChannelMap[] = {
CRAS_CH_FL, CRAS_CH_FR, CRAS_CH_FC, CRAS_CH_LFE, CRAS_CH_RL, CRAS_CH_RR,
CRAS_CH_FLC, CRAS_CH_FRC, CRAS_CH_RC, CRAS_CH_SL, CRAS_CH_SR,
// CRAS doesn't currently define explicit mappings for 7.1.4 height
// channels.
std::nullopt, std::nullopt, std::nullopt, std::nullopt};The optional is a bit strange here. I see you're trying to respect the static_assert, but to add these `std::nullopt` only to skip them is a bit odd.
A `constexpr int kUnsupportedTopChannel` instead of `std::nullopt` would already be a bit clearer in its intent, even though I still think it looks odd, since we will never loop over them.
Another alternative:
I think one of Jordan's CLs will introduce a direct layout to mask function: http://crrev.com/c/7631615/14/media/base/channel_layout.h
If you wanted things to be fancy and explicit in the intent, cou could take the output of that function, make a bit mask for "supported cras channels" and `&` the two masks, and then populate `layout` accordingly.
static_assert(std::size(kChannelMap) == CHANNELS_MAX + 1,Same comment
Related details
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 3 comments![]( "Open in Gerrit")
if (layout == CHANNEL_LAYOUT_DISCRETE) {
switch (channels) {
case 10:
// Map to canonical AAUDIO_CHANNEL_5POINT1POINT4 channel mask for
// 10-channel PCM MediaCodec decoded audio. This ensures
// compatibility with Android devices for signaling 10-channel output.
return AAUDIO_CHANNEL_5POINT1POINT4;
case 12:
// Map to canonical AAUDIO_CHANNEL_7POINT1POINT4 channel mask for
// 12-channel PCM MediaCodec decoded audio. This ensures
// compatibility with Android devices for signaling 12-channel output.
return AAUDIO_CHANNEL_7POINT1POINT4;
default:
return std::nullopt;
}
}This should be cleaned up whenever we allow 12 channels. Add a TODO to track this?
Done
static const std::optional<int> kChannelMap[] = {
CRAS_CH_FL, CRAS_CH_FR, CRAS_CH_FC, CRAS_CH_LFE, CRAS_CH_RL, CRAS_CH_RR,
CRAS_CH_FLC, CRAS_CH_FRC, CRAS_CH_RC, CRAS_CH_SL, CRAS_CH_SR,
// CRAS doesn't currently define explicit mappings for 7.1.4 height
// channels.
std::nullopt, std::nullopt, std::nullopt, std::nullopt};The optional is a bit strange here. I see you're trying to respect the static_assert, but to add these `std::nullopt` only to skip them is a bit odd.
A `constexpr int kUnsupportedTopChannel` instead of `std::nullopt` would already be a bit clearer in its intent, even though I still think it looks odd, since we will never loop over them.
Another alternative:
I think one of Jordan's CLs will introduce a direct layout to mask function: http://crrev.com/c/7631615/14/media/base/channel_layout.h
If you wanted things to be fancy and explicit in the intent, cou could take the output of that function, make a bit mask for "supported cras channels" and `&` the two masks, and then populate `layout` accordingly.
Ended up going with the `constexpr int kUnsupportedTopChannel` route. Marking as resolved.
static_assert(std::size(kChannelMap) == CHANNELS_MAX + 1,Syed AbuTalibSame comment
Done
Related details
- Thomas Guilbert
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Thomas Guilbert (Gerrit)
Thomas Guilbert voted and added 6 comments![]( "Open in Gerrit")
Votes added by Thomas Guilbert
| Code-Review | +1 |
6 comments
* CRAS does not support height channels, created `kUnsupportedChannel` to ignore such channels.NIT: format the description to fit in 80 char (overly long URL is fine though)
static constexpr int kUnsupportedChannel = -1;Nit: in `layout` itself, `-1` means no mapping. Try using a different invalid constant, like `-123` or anything else that should help catch errors if this code is changed.
// CRAS doesn't currently define explicit mappings for 7.1.4 heightNit: height channels are not limited to `7.1.4`. `x.y.4 height channels` or just `height channels` is more correct here.
static constexpr int kUnsupportedChannel = -1;Ditto.
// CRAS doesn't currently define explicit mappings for 7.1.4 heightDitto ditto.
Related details
- Syed AbuTalib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib added 5 comments![]( "Open in Gerrit")
* CRAS does not support height channels, created `kUnsupportedChannel` to ignore such channels.NIT: format the description to fit in 80 char (overly long URL is fine though)
Done
Nit: in `layout` itself, `-1` means no mapping. Try using a different invalid constant, like `-123` or anything else that should help catch errors if this code is changed.
Went with -4 in honor of the number of height channels.
// CRAS doesn't currently define explicit mappings for 7.1.4 heightNit: height channels are not limited to `7.1.4`. `x.y.4 height channels` or just `height channels` is more correct here.
Done
static constexpr int kUnsupportedChannel = -1;Syed AbuTalibDitto.
Done
// CRAS doesn't currently define explicit mappings for 7.1.4 heightSyed AbuTalibDitto ditto.
Done
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Syed AbuTalib (Gerrit)
Syed AbuTalib voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
10 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: media/audio/cras/cras_input.cc
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: media/audio/cras/cras_unified.cc
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
Change information
Add support for 5.1.4 and 7.1.4 audio channel layouts
Add new `5_1_4` and `7_1_4` enums, as well as the associated height
Channels. Update switch cases to handle these new enums. Note that we
are not upgrading kMaxConcurrentChannels until later, leading us to skip
select tests for now.
Adding such enums leads to OS specific updates:
* Add the height channels to `kMediaChannelToAAudioChannel` (refer to https://developer.android.com/ndk/reference/group/audio#group___audio_1gac36f475ca5b446f4fde4c9b90bec77c8)
* CRAS does not support height channels, created `kUnsupportedChannel` to
ignore such channels.
* Add `kAudioChannelLabel_*` to MacOS specific cases.
- M media/audio/android/aaudio_stream_wrapper.cc
- M media/audio/cras/cras_input.cc
- M media/audio/cras/cras_unified.cc
- M media/base/channel_layout.cc
- M media/base/channel_layout.h
- M media/base/channel_mixer_unittest.cc
- M media/base/channel_mixing_matrix_unittest.cc
- M media/base/mac/channel_layout_util_mac.cc
- M media/base/mac/channel_layout_util_mac_unittest.cc
Code-Review: +1 by Thomas Guilbert
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |