Optionally disable APM limiter in AudioMixer.

webrtc/modules/audio_mixer/frame_combiner.cc

Index: webrtc/modules/audio_mixer/frame_combiner.cc
diff --git a/webrtc/modules/audio_mixer/frame_combiner.cc b/webrtc/modules/audio_mixer/frame_combiner.cc
new file mode 100644
index 0000000000000000000000000000000000000000..7456e5dc1a0106350dba38c6ae585c49d71b06f5
--- /dev/null
+++ b/webrtc/modules/audio_mixer/frame_combiner.cc
@@ -0,0 +1,175 @@
+/*
+ *  Copyright (c) 2017 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 "webrtc/modules/audio_mixer/frame_combiner.h"
+
+#include <algorithm>
+#include <array>
+#include <functional>
+#include <memory>
+
+#include "webrtc/base/logging.h"

[ivoc, 2017/02/15 16:58:47]

Order of includes

[aleloi, 2017/02/16 14:01:36]

Done.

+#include "webrtc/audio/utility/audio_frame_operations.h"
+#include "webrtc/modules/audio_mixer/audio_frame_manipulator.h"
+#include "webrtc/modules/audio_mixer/audio_mixer_impl.h"
+
+namespace webrtc {
+namespace {
+
+void CombineZeroFrames(AudioFrame* audio_frame_for_mixing) {
+  audio_frame_for_mixing->elapsed_time_ms_ = -1;
+  AudioFrameOperations::Mute(audio_frame_for_mixing);
+}
+
+void CombineOneFrame(const AudioFrame* input_frame,
+                     AudioFrame* audio_frame_for_mixing) {
+  audio_frame_for_mixing->timestamp_ = input_frame->timestamp_;
+  audio_frame_for_mixing->elapsed_time_ms_ = input_frame->elapsed_time_ms_;
+  std::copy(input_frame->data_,
+            input_frame->data_ +
+                input_frame->num_channels_ * input_frame->samples_per_channel_,
+            audio_frame_for_mixing->data_);
+}
+
+std::unique_ptr<AudioProcessing> CreateLimiter() {
+  Config config;
+  config.Set<ExperimentalAgc>(new ExperimentalAgc(false));
+  std::unique_ptr<AudioProcessing> limiter(AudioProcessing::Create(config));
+  if (!limiter.get()) {
+    return nullptr;
+  }
+
+  if (limiter->gain_control()->set_mode(GainControl::kFixedDigital) !=
+      limiter->kNoError) {
+    return nullptr;
+  }
+
+  // We smoothly limit the mixed frame to -7 dbFS. -6 would correspond to the
+  // divide-by-2 but -7 is used instead to give a bit of headroom since the
+  // AGC is not a hard limiter.
+  if (limiter->gain_control()->set_target_level_dbfs(7) != limiter->kNoError) {
+    return nullptr;
+  }
+
+  if (limiter->gain_control()->set_compression_gain_db(0) !=
+      limiter->kNoError) {
+    return nullptr;
+  }
+
+  if (limiter->gain_control()->enable_limiter(true) != limiter->kNoError) {
+    return nullptr;
+  }
+
+  if (limiter->gain_control()->Enable(true) != limiter->kNoError) {
+    return nullptr;
+  }
+  return limiter;
+}
+}  // namespace
+
+FrameCombiner::FrameCombiner(bool use_apm_limiter)
+    : use_apm_limiter_(use_apm_limiter),
+      limiter_(use_apm_limiter ? CreateLimiter() : nullptr) {}
+
+FrameCombiner::~FrameCombiner() = default;
+
+void FrameCombiner::Combine(const std::vector<AudioFrame*>& mix_list,
+                            size_t number_of_channels,
+                            int sample_rate,
+                            AudioFrame* audio_frame_for_mixing) {
+  const size_t kSamplesPerChannel = static_cast<size_t>(
+      (sample_rate * webrtc::AudioMixerImpl::kFrameDurationInMs) / 1000);
+
+  for (const auto& frame : mix_list) {
+    RTC_DCHECK_EQ(kSamplesPerChannel, frame->samples_per_channel_);
+    RTC_DCHECK_EQ(sample_rate, frame->sample_rate_hz_);
+  }
+
+  // Frames could be both stereo and mono.
+  for (const auto& frame : mix_list) {

[ivoc, 2017/02/15 16:58:47]

Can be merged with the previous loop.

[aleloi, 2017/02/16 14:01:36]

Yes, but I'd like to keep the modification from the checks. I've changed the
'constness' so the first loop iterates over const frames, the 2nd one does not.
With NDEBUG on, the compiler will eliminate the above loop anyway.

[ivoc, 2017/02/21 09:59:16]

Acknowledged.

+    RemixFrame(number_of_channels, frame);
+  }
+
+  // TODO(aleloi): Issue 3390.

[ivoc, 2017/02/15 16:58:47]

A url to the issue would be helpful here.

[aleloi, 2017/02/16 14:01:35]

Done.

+  // Audio frame timestamp . The 'timestamp_' field is set to dummy

[ivoc, 2017/02/15 16:58:46]

The space between "timestamp" and the "." bothers me :-)

[aleloi, 2017/02/16 14:01:36]

Done.

+  // value '0', because it is only supported in one channel case and

[ivoc, 2017/02/15 16:58:46]

the one channel case

[aleloi, 2017/02/16 14:01:36]

Done.

+  // is then updated in the helper functions.
+  audio_frame_for_mixing->UpdateFrame(
+      -1, 0, NULL, kSamplesPerChannel, sample_rate, AudioFrame::kNormalSpeech,
+      AudioFrame::kVadPassive, number_of_channels);
+
+  if (mix_list.size() == 0) {
+    CombineZeroFrames(audio_frame_for_mixing);
+  } else if (mix_list.size() == 1) {
+    CombineOneFrame(mix_list.front(), audio_frame_for_mixing);
+  } else {
+    std::vector<rtc::ArrayView<const int16_t>> input_frames;
+    for (size_t i = 0; i < mix_list.size(); ++i) {
+      input_frames.push_back(rtc::ArrayView<const int16_t>(
+          mix_list[i]->data_, kSamplesPerChannel * number_of_channels));
+    }
+    CombineMultipleFrames(input_frames, audio_frame_for_mixing);
+  }
+}
+
+void FrameCombiner::CombineMultipleFrames(
+    const std::vector<rtc::ArrayView<const int16_t>>& input_frames,
+    AudioFrame* audio_frame_for_mixing) {
+  RTC_DCHECK(!input_frames.empty());
+
+  const size_t frame_length = input_frames.front().size();
+  for (const auto& frame : input_frames) {
+    RTC_DCHECK_EQ(frame_length, frame.size());
+  }
+
+  // Maximal frame size: stereo, 48 kHz, 10 ms.
+  RTC_DCHECK_GE(2 * 48 * 10, frame_length);
+  std::array<int32_t, 2 * 48 * 10> add_buffer;

[ivoc, 2017/02/15 16:58:47]

I suggest declaring these constants as constexprs in the anonymous namespace. 
Also, is it possible to avoid the intermediate variable and write to the
audio_frame_for_mixing directly?

[aleloi, 2017/02/16 14:01:36]

Done. Reasons for having an intermediate 32-bit array: 

* profiling shows it's a little faster for >= 3 streams (because of less calls
to rtc::saturated_cast) and the difference is rather small for 2 streams
* We lose less precision by halving at the end compared to halving before adding
* This prepares for the hypothetical future, in which a new limiter could
operate on the 32-bit result (although then everything should probably be
re-done in floats...)

[ivoc, 2017/02/21 09:59:16]

Thanks for the profiling, that makes sense.

+
+  add_buffer.fill(0);

[ivoc, 2017/02/15 16:58:47]

It's more efficient to copy the first frame into the add_buffer instead of
initializing it to zero. It eliminates one pass through the array.

[aleloi, 2017/02/16 14:01:35]

I profiled this and some other changes. On x86-64 with a GN release build
(Clang++ 4.0.0), there is almost no change in execution speed. The numbers I got
are

82.49	81.43	80.67	81.74

where the first (82.49) is the current version and the other 3 are 
* 136 -> fill(add_buffer.begin(), add_buffer.begin() + frame_length)
* 136 -> copy 1st frame to buffer + loops start from 2nd frame
* the above + last step combined with last frame.

Since the changes would lead to messier code, and there is no gain, I'd like to
leave everything as is. I've added comments about the profiling in the code.

[ivoc, 2017/02/21 09:59:16]

Great! Thanks for looking into this. I agree that if the effect is this small we
should leave it as-is.

+
+  for (const auto& frame : input_frames) {
+    std::transform(frame.begin(), frame.end(), add_buffer.begin(),
+                   add_buffer.begin(), std::plus<int32_t>());
+  }
+
+  if (use_apm_limiter_) {
+    // Half all samples to avoid saturation before limiting.

[ivoc, 2017/02/15 16:58:47]

Halve

[aleloi, 2017/02/16 14:01:36]

Done.

+    std::transform(add_buffer.begin(), add_buffer.begin() + frame_length,

[ivoc, 2017/02/15 16:58:47]

For maximum efficiency it would be more efficient to combine this with adding
the last frame from input_frames (add it and saturate_cast the result). It would
eliminate another pass through the buffer.

[aleloi, 2017/02/16 14:01:36]

Actually no performance improvement. See comment above.

[ivoc, 2017/02/21 09:59:16]

Acknowledged.

+                   audio_frame_for_mixing->data_, [](int32_t a) {
+                     return rtc::saturated_cast<int16_t>(a / 2);
+                   });
+
+    // Smoothly limit the audio.
+    RTC_DCHECK(limiter_);
+    const int error = limiter_->ProcessStream(audio_frame_for_mixing);
+    if (error != limiter_->kNoError) {
+      LOG_F(LS_ERROR) << "Error from AudioProcessing: " << error;
+      RTC_NOTREACHED();
+    }
+
+    // And now we can safely restore the level. This procedure results in
+    // some loss of resolution, deemed acceptable.
+    //
+    // It's possible to apply the gain in the AGC (with a target level of 0 dbFS
+    // and compression gain of 6 dB). However, in the transition frame when this
+    // is enabled (moving from one to two audio sources) it has the potential to
+    // create discontinuities in the mixed frame.
+    //
+    // Instead we double the frame (with addition since left-shifting a
+    // negative value is undefined).
+    AudioFrameOperations::Add(*audio_frame_for_mixing, audio_frame_for_mixing);
+  } else {
+    std::transform(add_buffer.begin(), add_buffer.begin() + frame_length,

[ivoc, 2017/02/15 16:58:46]

See remark about combining with adding last frame.

[aleloi, 2017/02/16 14:01:36]

Here as well.

+                   audio_frame_for_mixing->data_,
+                   [](int32_t a) { return rtc::saturated_cast<int16_t>(a); });
+  }
+}
+}  // namespace webrtc