Support 4 channel mic in Windows Core Audio

webrtc/audio/utility/audio_frame_operations.cc

Index: webrtc/audio/utility/audio_frame_operations.cc
diff --git a/webrtc/audio/utility/audio_frame_operations.cc b/webrtc/audio/utility/audio_frame_operations.cc
index 6fcb84e722d5a07cfcf9d04c90641e4e238464fd..b97d1d3c902d81148c74d71b6b255d769f1975b9 100644
--- a/webrtc/audio/utility/audio_frame_operations.cc
+++ b/webrtc/audio/utility/audio_frame_operations.cc
@@ -114,6 +114,79 @@ int AudioFrameOperations::StereoToMono(AudioFrame* frame) {
   return 0;
 }
 
+void AudioFrameOperations::QuadToMono(const int16_t* src_audio,

[hlundin-webrtc, 2017/02/24 09:46:35]

The order of the methods in the .cc file and the .h file should be the same.

[jens.nielsen, 2017/02/24 14:55:48]

Done.

+                                      size_t samples_per_channel,
+                                      int16_t* dst_audio) {
+  for (size_t i = 0; i < samples_per_channel; i++) {
+    dst_audio[i] = (src_audio[4 * i] + src_audio[4 * i + 1] +
+                    src_audio[4 * i + 2] + src_audio[4 * i + 3]) >> 2;

[aleloi, 2017/02/24 10:39:31]

I think this can overflow. If so, can the sum be stored in a larger temporary
variable? Or does the binary arithmetic work out anyway?

[jens.nielsen, 2017/02/24 11:11:18]

I was too, and was about to fix that here as well as the StereoToMono case when
I saw there's already a unit test to verify that it in fact does not overflow...
In a spirit of "don't touch it if not needed" I just followed pattern here... :)

[aleloi, 2017/02/24 13:32:52]

I tried to look it up:
http://en.cppreference.com/w/cpp/language/operator_arithmetic#Overflows

When signed integer arithmetic operation overflows (the result does not fit in
the result type), the behavior is undefined: it may wrap around according to the
rules of the representation (typically 2's complement), it may trap on some
platforms or due to compiler options (e.g. -ftrapv in GCC and Clang), or may be
completely optimized out by the compiler.


IIUC, this is undefined behavior. On the other hand, all the compilers seem to
do integer promotion: https://godbolt.org/g/f0hiqu. Also, our undefined-behavior
test bots don't catch it.

But can we add a static_cast<int32_t> anyway? It will generate the same binary
code, but I will sleep better at night :)

[jens.nielsen, 2017/02/24 14:55:48]

Done.

+  }
+}
+
+int AudioFrameOperations::QuadToMono(AudioFrame* frame) {
+  if (frame->num_channels_ != 4) {
+    return -1;
+  }
+

[aleloi, 2017/02/24 10:39:31]

I think this is a good place to RTC_DCHECK_LE that samples_per_channel_ * 4 <=
AudioFrame::kMaxDataSizeSamples.

[jens.nielsen, 2017/02/24 14:55:48]

Done. But the damage is already done here if the buffer is too small, this
should be checked when the 4 channel AudioFrame is created and/or written to
(afaik never used)

+  QuadToMono(frame->data_, frame->samples_per_channel_, frame->data_);
+  frame->num_channels_ = 1;
+
+  return 0;
+}
+
+void AudioFrameOperations::QuadToStereo(const int16_t* src_audio,
+                                        size_t samples_per_channel,
+                                        int16_t* dst_audio) {
+  for (size_t i = 0; i < samples_per_channel; i++) {
+    dst_audio[i * 2] = (src_audio[4 * i] + src_audio[4 * i + 1]) >> 1;
+    dst_audio[i * 2 + 1] = (src_audio[4 * i + 2] + src_audio[4 * i + 3]) >> 1;

[aleloi, 2017/02/24 10:39:31]

I have concerns for overflows here as well.

[jens.nielsen, 2017/02/24 14:55:48]

Done.

+  }
+}
+
+int AudioFrameOperations::QuadToStereo(AudioFrame* frame) {
+  if (frame->num_channels_ != 4) {
+    return -1;
+  }
+

[aleloi, 2017/02/24 10:39:31]

Please add

RTC_DCHECK_LE(samples_per_channel_ * 4, AudioFrame::kMaxDataSizeSamples)

[jens.nielsen, 2017/02/24 14:55:48]

Done.

+  QuadToStereo(frame->data_, frame->samples_per_channel_, frame->data_);
+  frame->num_channels_ = 2;
+
+  return 0;
+}
+
+void AudioFrameOperations::DownmixChannels(const int16_t* src_audio,
+                                           size_t src_channels,
+                                           size_t samples_per_channel,
+                                           int16_t* dst_audio,
+                                           size_t dst_channels) {
+  if (src_channels == 2 && dst_channels == 1) {
+    StereoToMono(src_audio, samples_per_channel, dst_audio);
+  } else if (src_channels == 4 && dst_channels == 2) {
+    QuadToStereo(src_audio, samples_per_channel, dst_audio);
+  } else if (src_channels == 4 && dst_channels == 1) {
+    QuadToMono(src_audio, samples_per_channel, dst_audio);
+  }

[hlundin-webrtc, 2017/02/24 09:46:36]

I'd like to see some kind of DCHECK that the number of input and output channels
are valid. One way to accomplish this is to add a return to each if statement,
and then at the very bottom add
RTC_NOTREACHED() << "src_channels: " << src_channels << ", dst_channels: " <<
dst_channels;

[jens.nielsen, 2017/02/24 14:55:48]

Done.

+}
+
+int AudioFrameOperations::DownmixChannels(AudioFrame* frame,
+                                          size_t dst_channels) {
+  if (frame->num_channels_ >= dst_channels) {

[hlundin-webrtc, 2017/02/24 09:46:35]

This is not needed. You are explicitly testing all valid combinations below, and
return -1 if that fails.

[jens.nielsen, 2017/02/24 14:55:48]

Done.

+    return -1;
+  }
+
+  if (frame->num_channels_ == 2 && dst_channels == 1) {
+    return StereoToMono(frame);
+  } else if (frame->num_channels_ == 4 && dst_channels == 2) {
+    return QuadToStereo(frame);
+  } else if (frame->num_channels_ == 4 && dst_channels == 1) {
+    return QuadToMono(frame);
+  } else {

[hlundin-webrtc, 2017/02/24 09:46:36]

Delete the else statement and simply return -1 unconditionally.

[jens.nielsen, 2017/02/24 14:55:48]

Done.

+    return -1;
+  }
+
+  return 0;
+}
+
 void AudioFrameOperations::SwapStereoChannels(AudioFrame* frame) {
   RTC_DCHECK(frame);
   if (frame->num_channels_ != 2) {