Move functionality out from AudioFrame and into AudioFrameOperations.

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.
  */
 
+#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,
                                         int16_t* dst_audio) {
   for (size_t i = 0; i < samples_per_channel; i++) {
     dst_audio[2 * i] = src_audio[i];
     dst_audio[2 * i + 1] = src_audio[i];
   }
 }
 
@@ skipping 28 matching lines @@
     return -1;
   }
 
   StereoToMono(frame->data_, frame->samples_per_channel_, frame->data_);
   frame->num_channels_ = 1;
 
   return 0;
 }
 
 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];
     frame->data_[i] = frame->data_[i + 1];
     frame->data_[i + 1] = temp_data;
   }
 }
 
-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) {
     // Not muted, don't touch.
   } else if (previous_frame_muted && current_frame_muted) {
     // Frame fully muted.
     size_t total_samples = frame->samples_per_channel_ * frame->num_channels_;
     RTC_DCHECK_GE(AudioFrame::kMaxDataSizeSamples, total_samples);
     memset(frame->data_, 0, sizeof(frame->data_[0]) * total_samples);
   } else {
@@ skipping 27 matching lines @@
     for (size_t j = 0; j < channels; ++j) {
       float g = start_g;
       for (size_t i = start * channels; i < end * channels; i += channels) {
         g += inc;
         frame->data_[i + j] *= g;
       }
     }
   }
 }
 
+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]);
   }
   return 0;
 }
 
 int AudioFrameOperations::ScaleWithSat(float scale, AudioFrame& frame) {
   int32_t temp_data = 0;
 
   // Ensure that the output result is saturated [-32768, +32767].
   for (size_t i = 0; i < frame.samples_per_channel_ * frame.num_channels_;
        i++) {
     temp_data = static_cast<int32_t>(scale * frame.data_[i]);
     if (temp_data < -32768) {
       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;
 }
 
+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