Move functionality out from AudioFrame and into AudioFrameOperations.

webrtc/audio/utility/audio_frame_operations.cc

Index: webrtc/audio/utility/audio_frame_operations.cc
diff --git a/webrtc/modules/utility/source/audio_frame_operations.cc b/webrtc/audio/utility/audio_frame_operations.cc
similarity index 61%
rename from webrtc/modules/utility/source/audio_frame_operations.cc
rename to webrtc/audio/utility/audio_frame_operations.cc
index 102407d0f0f0fd223f6eaf9836dfa2b18278a5f7..cc2b603b42397aba7c6f50bc61406bbd24088643 100644
--- a/webrtc/modules/utility/source/audio_frame_operations.cc
+++ b/webrtc/audio/utility/audio_frame_operations.cc
@@ -8,18 +8,69 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
+#include <algorithm>
+
+#include "webrtc/audio/utility/audio_frame_operations.h"
 #include "webrtc/modules/include/module_common_types.h"
-#include "webrtc/modules/utility/include/audio_frame_operations.h"
 #include "webrtc/base/checks.h"
 
 namespace webrtc {
-namespace  {
+namespace {
 
 // 2.7ms @ 48kHz, 4ms @ 32kHz, 8ms @ 16kHz.
 const size_t kMuteFadeFrames = 128;
 const float kMuteFadeInc = 1.0f / kMuteFadeFrames;
 
-}  // namespace {
+}  // namespace
+
+void AudioFrameOperations::Add(const AudioFrame& frame_to_add,
+                               AudioFrame* result_frame) {
+  // Sanity check
+  RTC_DCHECK_GT(result_frame->num_channels_, 0u);

[hlundin-webrtc, 2016/11/30 11:24:36]

I know we usually don't allow other than plain mono and stereo, but is there
anything particular to _this_ method that requires this limitation. Can we not
add two 5-channel frames?

[aleloi, 2016/11/30 11:33:39]

Not really. If there is a chance that poly-channel frames will become a reality,
I think we should remove the checks from functions that don't require it. There
are lots places where it is an assumption (e.g. the mixer).

[kwiberg-webrtc, 2016/11/30 11:36:04]

But there's no good reason to not remove the unnecessary checks right away, is
there?

[aleloi, 2016/11/30 11:48:34]

I think methods should in general only check their preconditions. If they also
check all the things that are true for the system at the point of writing them,
there will be more to keep track of when functionality changes (more work to
make a change), and some old checks will probably be left when they shouldn't. I
don't think the benefit of tests like this is high enough.

[hlundin-webrtc, 2016/11/30 11:51:05]

[Logic evaluation stalled due to double negation]
[Attempting simplification]
["no good reason to not" -> "good reason to"]
[Substitute into original expression]
[-> "But there's good reason to remove the unnecessary checks right away, is
there?"]
[Ignoring positive question tag]
[-> "But there's good reason to remove the unnecessary checks right away"]
[Computer says: yes!]

[kwiberg-webrtc, 2016/11/30 11:55:04]

Yes, that's what I meant. I (mis-)interpreted Alex's original reply as saying
that we should remove the checks later. But it seems like we all agree that the
checks should (at least in principle) be removed immediately.

+  RTC_DCHECK_LT(result_frame->num_channels_, 3u);

[kwiberg-webrtc, 2016/11/30 09:38:34]

You don't need the "u"s anymore!

[aleloi, 2016/11/30 10:11:48]

Done. Has the implementation of the DCHECK macros changed? (Hurray for that in
that case!)

[kwiberg-webrtc, 2016/11/30 11:32:44]

Yes. (I sent a mail about it yesterday to discuss-webrtc.)

+  if ((result_frame->num_channels_ > 2) || (result_frame->num_channels_ < 1))
+    return;
+  if (result_frame->num_channels_ != frame_to_add.num_channels_)
+    return;
+
+  bool noPrevData = false;

[hlundin-webrtc, 2016/11/30 11:24:36]

no_previous_data

+  if (result_frame->samples_per_channel_ != frame_to_add.samples_per_channel_) {
+    if (result_frame->samples_per_channel_ == 0) {
+      // special case we have no data to start with

[hlundin-webrtc, 2016/11/30 11:24:36]

Capital start and end with '.'

+      result_frame->samples_per_channel_ = frame_to_add.samples_per_channel_;
+      noPrevData = true;
+    } else {
+      return;
+    }
+  }
+
+  if ((result_frame->vad_activity_ == AudioFrame::kVadActive) ||
+      frame_to_add.vad_activity_ == result_frame->kVadActive) {

[kwiberg-webrtc, 2016/11/30 09:38:34]

Remove one layer of parentheses, since the relative precedence of == and || is
well-known.

[aleloi, 2016/11/30 10:11:48]

Done.

+    result_frame->vad_activity_ = AudioFrame::kVadActive;
+  } else if (result_frame->vad_activity_ == AudioFrame::kVadUnknown ||
+             frame_to_add.vad_activity_ == AudioFrame::kVadUnknown) {
+    result_frame->vad_activity_ = AudioFrame::kVadUnknown;
+  }
+
+  if (result_frame->speech_type_ != frame_to_add.speech_type_)
+    result_frame->speech_type_ = AudioFrame::kUndefined;
+
+  if (noPrevData) {
+    std::copy(frame_to_add.data_, frame_to_add.data_ +
+                                      frame_to_add.samples_per_channel_ *
+                                          result_frame->num_channels_,
+              result_frame->data_);
+  } else {
+    for (size_t i = 0;
+         i < result_frame->samples_per_channel_ * result_frame->num_channels_;
+         i++) {
+      int32_t wrap_guard = static_cast<int32_t>(result_frame->data_[i]) +

[kwiberg-webrtc, 2016/11/30 09:38:34]

const

[aleloi, 2016/11/30 10:11:48]

Done.

+                           static_cast<int32_t>(frame_to_add.data_[i]);
+      result_frame->data_[i] = ClampToInt16(wrap_guard);
+    }
+  }
+  return;
+}
 
 void AudioFrameOperations::MonoToStereo(const int16_t* src_audio,
                                         size_t samples_per_channel,
@@ -68,7 +119,8 @@ int AudioFrameOperations::StereoToMono(AudioFrame* frame) {
 }
 
 void AudioFrameOperations::SwapStereoChannels(AudioFrame* frame) {
-  if (frame->num_channels_ != 2) return;
+  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];
@@ -77,7 +129,8 @@ void AudioFrameOperations::SwapStereoChannels(AudioFrame* frame) {
   }
 }
 
-void AudioFrameOperations::Mute(AudioFrame* frame, bool previous_frame_muted,
+void AudioFrameOperations::Mute(AudioFrame* frame,
+                                bool previous_frame_muted,
                                 bool current_frame_muted) {
   RTC_DCHECK(frame);
   if (!previous_frame_muted && !current_frame_muted) {
@@ -125,14 +178,29 @@ void AudioFrameOperations::Mute(AudioFrame* frame, bool previous_frame_muted,
   }
 }
 
+void AudioFrameOperations::Mute(AudioFrame* frame) {
+  Mute(frame, true, true);
+}
+
+void AudioFrameOperations::ApplyHalfGain(AudioFrame* frame) {
+  RTC_DCHECK_GT(frame->num_channels_, 0u);
+  RTC_DCHECK_LT(frame->num_channels_, 3u);

[kwiberg-webrtc, 2016/11/30 09:38:34]

Remove "u"s.

[aleloi, 2016/11/30 10:11:48]

Done.

+  if ((frame->num_channels_ > 2) || (frame->num_channels_ < 1))

[kwiberg-webrtc, 2016/11/30 09:38:34]

Remove the extra parentheses.

[aleloi, 2016/11/30 10:11:48]

Done.

[hlundin-webrtc, 2016/11/30 11:24:36]

Again, does this function really need the restriction to one or two channels?

[aleloi, 2016/11/30 11:33:38]

No, there is no such assumption here either.

+    return;
+
+  for (size_t i = 0; i < frame->samples_per_channel_ * frame->num_channels_;
+       i++) {
+    frame->data_[i] = static_cast<int16_t>(frame->data_[i] >> 1);
+  }
+}
+
 int AudioFrameOperations::Scale(float left, float right, AudioFrame& frame) {
   if (frame.num_channels_ != 2) {
     return -1;
   }
 
   for (size_t i = 0; i < frame.samples_per_channel_; i++) {
-    frame.data_[2 * i] =
-        static_cast<int16_t>(left * frame.data_[2 * i]);
+    frame.data_[2 * i] = static_cast<int16_t>(left * frame.data_[2 * i]);
     frame.data_[2 * i + 1] =
         static_cast<int16_t>(right * frame.data_[2 * i + 1]);
   }
@@ -157,4 +225,14 @@ int AudioFrameOperations::ScaleWithSat(float scale, AudioFrame& frame) {
   return 0;
 }
 
+int16_t ClampToInt16(int32_t input) {
+  if (input < -0x00008000) {
+    return -0x8000;
+  } else if (input > 0x00007FFF) {
+    return 0x7FFF;
+  } else {
+    return static_cast<int16_t>(input);
+  }
+}

[kwiberg-webrtc, 2016/11/30 09:38:34]

Instead of defining this function, use rtc::saturated_cast<int16_t>.

[aleloi, 2016/11/30 10:11:48]

Done. Great that we have saturated_cast! I missed that.

I checked that it does the same thing as ClampToInt16 by running 
ClampToInt16.TestCases from audio_frame_operations_unittest.cc on it. I also
replaced the ClampToInt16 copy in module_common_types with saturated_cast. That
way we can remove an anonymous namespace in a header, and delete the method
right away instead of deprecating it.

[kwiberg-webrtc, 2016/11/30 11:32:44]

Except for the name---it should have been "saturating_cast"...

+
 }  // namespace webrtc