Optionally disable APM limiter in AudioMixer.

webrtc/modules/audio_mixer/frame_combiner.cc

+/*
+ *  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/audio/utility/audio_frame_operations.h"
+#include "webrtc/base/logging.h"
+#include "webrtc/modules/audio_mixer/audio_frame_manipulator.h"
+#include "webrtc/modules/audio_mixer/audio_mixer_impl.h"
+
+namespace webrtc {
+namespace {
+
+// Stereo, 48 kHz, 10 ms.
+constexpr int kMaximalFrameSize = 2 * 48 * 10;
+
+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()) {

[hlundin-webrtc, 2017/02/17 11:04:13]

Under what conditions can this happen? Wouldn't this warrant a (D)CHECK instead?
This gives a better context on why there was no limiter object, instead of
finding out only when it was about to be used.

[aleloi, 2017/02/20 10:41:33]

Done.

+    return nullptr;
+  }
+
+  if (limiter->gain_control()->set_mode(GainControl::kFixedDigital) !=

[hlundin-webrtc, 2017/02/17 11:04:13]

Same here.

[aleloi, 2017/02/20 10:41:33]

Done.

+      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) {

[hlundin-webrtc, 2017/02/17 11:04:13]

Same here.

[aleloi, 2017/02/20 10:41:33]

Done.

+    return nullptr;
+  }
+
+  if (limiter->gain_control()->set_compression_gain_db(0) !=

[hlundin-webrtc, 2017/02/17 11:04:13]

Same here.

[aleloi, 2017/02/20 10:41:33]

Done.

+      limiter->kNoError) {
+    return nullptr;
+  }
+
+  if (limiter->gain_control()->enable_limiter(true) != limiter->kNoError) {

[hlundin-webrtc, 2017/02/17 11:04:13]

Same here.

[aleloi, 2017/02/20 10:41:33]

Done.

+    return nullptr;
+  }
+
+  if (limiter->gain_control()->Enable(true) != limiter->kNoError) {

[hlundin-webrtc, 2017/02/17 11:04:12]

Same here.

[aleloi, 2017/02/20 10:41:33]

Done.

+    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 {

[hlundin-webrtc, 2017/02/17 11:04:13]

RTC_DCHECK(audio_frame_for_mixing);

[aleloi, 2017/02/20 10:41:33]

Done.

+  const size_t kSamplesPerChannel = static_cast<size_t>(

[hlundin-webrtc, 2017/02/17 11:04:12]

I don't think it is correct to use kCamelCase naming for consts that depend on
runtime variables. See
https://google.github.io/styleguide/cppguide.html#Constant_Names.

[aleloi, 2017/02/20 10:41:33]

Good find! Fixed.

+      (sample_rate * webrtc::AudioMixerImpl::kFrameDurationInMs) / 1000);
+
+  for (const auto* frame : mix_list) {
+    RTC_DCHECK_EQ(kSamplesPerChannel, frame->samples_per_channel_);

[hlundin-webrtc, 2017/02/17 11:04:13]

A for loop only for DCHECKing? Why don't you perform those DCHECKs in the next
loop (line 99)?

[aleloi, 2017/02/20 10:41:33]

It's two against one now!

I wanted to separate checking from modifying. Note that the DCHECK loop will be
optimized out of existence in non-debug builds.

WDYT, can I leave it?

[hlundin-webrtc, 2017/02/20 14:16:31]

Alright. I'll allow it.

+    RTC_DCHECK_EQ(sample_rate, frame->sample_rate_hz_);
+  }
+
+  // Frames could be both stereo and mono.
+  for (auto* frame : mix_list) {
+    RemixFrame(number_of_channels, frame);
+  }
+
+  // TODO(aleloi): Issue bugs.webrtc.org/3390.
+  // Audio frame timestamp. The 'timestamp_' field is set to dummy
+  // value '0', because it is only supported in the one channel case and
+  // is then updated in the helper functions.
+  audio_frame_for_mixing->UpdateFrame(
+      -1, 0, nullptr, kSamplesPerChannel, sample_rate, AudioFrame::kUndefined,
+      AudioFrame::kVadUnknown, 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) const {
+  RTC_DCHECK(!input_frames.empty());

[hlundin-webrtc, 2017/02/17 11:04:13]

RTC_DCHECK(audio_frame_for_mixing);

[aleloi, 2017/02/20 10:41:33]

Done.

+
+  const size_t frame_length = input_frames.front().size();
+  for (const auto& frame : input_frames) {

[hlundin-webrtc, 2017/02/17 11:04:13]

Again, incorporate this DCHECK into another loop.

[aleloi, 2017/02/20 10:41:33]

I think it's more readable as is. Do you still want it in the other loop?

[hlundin-webrtc, 2017/02/20 14:16:31]

Leave it as it is.

+    RTC_DCHECK_EQ(frame_length, frame.size());
+  }
+
+  // Algorithm: int16 frames are added to a sufficiently large
+  // statically allocated int32 buffer. For > 2 participants this is
+  // more efficient than addition in place in the int16 audio
+  // frame. The audio quality loss due to halving the samples is
+  // smaller than 16-bit addition in place.
+  RTC_DCHECK_GE(kMaximalFrameSize, frame_length);
+  std::array<int32_t, kMaximalFrameSize> add_buffer;
+
+  add_buffer.fill(0);
+
+  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_) {
+    // Halve all samples to avoid saturation before limiting.
+    std::transform(add_buffer.begin(), add_buffer.begin() + frame_length,
+                   audio_frame_for_mixing->data_, [](int32_t a) {
+                     return rtc::saturated_cast<int16_t>(a / 2);
+                   });
+
+    // Smoothly limit the audio.
+    RTC_DCHECK(limiter_);

[hlundin-webrtc, 2017/02/17 11:04:13]

With your current implementation of CreateLimiter(), it is not "impossible" to
arrive here with a nullptr, making it somewhat incorrect to DCHECK. But with my
suggested changes, this DCHECK can remain.

[aleloi, 2017/02/20 10:41:33]

Acknowledged.

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