Support 4 channel mic in Windows Core Audio

webrtc/audio/utility/audio_frame_operations.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 96 matching lines @@
   if (frame->num_channels_ != 2) {
     return -1;
   }
 
   StereoToMono(frame->data_, frame->samples_per_channel_, frame->data_);
   frame->num_channels_ = 1;
 
   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) {
     return;
   }
 
   for (size_t i = 0; i < frame->samples_per_channel_ * 2; i += 2) {
     int16_t temp_data = frame->data_[i];
     frame->data_[i] = frame->data_[i + 1];
     frame->data_[i + 1] = temp_data;
@@ skipping 90 matching lines @@
       frame.data_[i] = -32768;
     } else if (temp_data > 32767) {
       frame.data_[i] = 32767;
     } else {
       frame.data_[i] = static_cast<int16_t>(temp_data);
     }
   }
   return 0;
 }
 }  // namespace webrtc