Added first layer of the echo canceller 3 functionality

webrtc/modules/audio_processing/aec3/echo_canceller3.cc

Index: webrtc/modules/audio_processing/aec3/echo_canceller3.cc
diff --git a/webrtc/modules/audio_processing/aec3/echo_canceller3.cc b/webrtc/modules/audio_processing/aec3/echo_canceller3.cc
index e69ccdcbc63fe068a09058e97e32518d6050af05..1b716de2529f188c3f7472dcc7e17ccc53b3d87f 100644
--- a/webrtc/modules/audio_processing/aec3/echo_canceller3.cc
+++ b/webrtc/modules/audio_processing/aec3/echo_canceller3.cc
@@ -10,35 +10,252 @@
 #include "webrtc/modules/audio_processing/aec3/echo_canceller3.h"
 
 #include "webrtc/base/atomicops.h"
-#include "webrtc/system_wrappers/include/logging.h"
+#include "webrtc/modules/audio_processing/logging/apm_data_dumper.h"
 
 namespace webrtc {
 
+namespace {
+
+bool DetectSaturation(rtc::ArrayView<const float> y) {
+  for (auto y_k : y) {
+    if (y_k > 32767.0f || y_k < -32768.0f) {

[ivoc, 2016/12/19 11:30:22]

How about if the signals are equal to the limits? 

[peah-webrtc, 2016/12/20 10:10:25]

Good find!
Done.

+      return true;
+    }
+  }
+  return false;
+}
+
+void ProcessCaptureFrameContent(AudioBuffer* capture,
+                                bool known_echo_path_change,
+                                bool saturated_microphone_signal,
+                                size_t subframe_index,
+                                FrameBlocker* capture_blocker,
+                                BlockFramer* output_framer,
+                                BlockProcessor* block_processor) {
+  float capture_block[kMaxNumBands * kBlockSize];
+  capture_blocker->InsertFrameAndExtractBlock(
+      subframe_index, capture->split_bands_f(0), capture_block);
+  block_processor->ProcessCapture(known_echo_path_change,
+                                  saturated_microphone_signal, capture_block);
+  output_framer->InsertBlock(capture_block);
+  output_framer->ExtractFrame(subframe_index, capture->split_bands_f(0));
+}
+
+void ProcessRemainingCaptureFrameContent(bool known_echo_path_change,
+                                         bool saturated_microphone_signal,
+                                         FrameBlocker* capture_blocker,
+                                         BlockFramer* output_framer,
+                                         BlockProcessor* block_processor) {
+  if (capture_blocker->IsBlockAvailable()) {

[hlundin-webrtc, 2016/12/16 10:04:47]

Nit: I prefer early return.
if (!capture_blocker->IsBlockAvailable()) {
  return;
}
...

[peah-webrtc, 2016/12/20 10:10:25]

Done.

+    float capture_block[kMaxNumBands * kBlockSize];
+    capture_blocker->ExtractBlockIfAvailable(capture_block);

[hlundin-webrtc, 2016/12/16 10:04:47]

...IfAvailable? I thought we established that it was available.

[peah-webrtc, 2016/12/20 10:10:25]

True, I changed the implementation of FrameBlocker accordingly.

Done.

+    block_processor->ProcessCapture(known_echo_path_change,
+                                    saturated_microphone_signal, capture_block);
+    output_framer->InsertBlock(capture_block);
+  }
+}
+
+bool BufferRenderFrameContent(rtc::ArrayView<const float> render_frame,
+                              size_t subframe_index,
+                              FrameBlocker* render_blocker,
+                              BlockProcessor* block_processor) {
+  return block_processor->BufferRender(
+      [render_blocker, render_frame,
+       subframe_index](rtc::ArrayView<float> block) mutable {
+        render_blocker->InsertFrameAndExtractBlock(subframe_index, render_frame,
+                                                   block);
+      });

[ivoc, 2016/12/19 11:30:22]

Can you explain what this is supposed to do? The way the code looks now doesn't
make much sense to me. For example, this lambda is passed to the block processor
(why?), and is never executed there. I guess some steps will be added later?

[peah-webrtc, 2016/12/20 10:10:25]

Yes, the reason for this construct is to provide a cheap way of producing a
render block from a frame and passing it to the BufferRender method. The reason
for why it is not used in the block_processor is that that functionality is to
be added in the next CL.

However, with the code changes in the new patch, this lambda is no longer used
and I instead use the vector-swap functionality for efficiently passing the
blocks.

PTAL

[ivoc, 2016/12/21 13:04:04]

It looks easier to understand now.

[peah-webrtc, 2016/12/21 23:13:48]

Acknowledged.

+}
+
+bool BufferRemainingRenderFrameContent(FrameBlocker* render_blocker,
+                                       BlockProcessor* block_processor) {
+  if (render_blocker->IsBlockAvailable()) {
+    return block_processor->BufferRender(
+        [render_blocker](rtc::ArrayView<float> block) mutable {
+          render_blocker->ExtractBlockIfAvailable(block);
+        });
+  }
+  return false;
+}
+
+void CopyAudioBufferIntoFrame(AudioBuffer* buffer,

[hlundin-webrtc, 2016/12/16 10:04:47]

I assume this cannot be made a const argument because of magic happening inside
the buffer.

[peah-webrtc, 2016/12/20 10:10:25]

Afaics, that is the case.

+                              size_t num_bands,
+                              size_t frame_length,
+                              rtc::ArrayView<float> frame) {
+  RTC_DCHECK_EQ(num_bands * frame_length, frame.size());
+  for (size_t i = 0; i < num_bands; ++i) {
+    rtc::ArrayView<float> buffer_view(&buffer->split_bands_f(0)[i][0],
+                                      frame_length);
+    std::copy(buffer_view.begin(), buffer_view.end(),
+              frame.begin() + i * frame_length);
+  }
+}
+
+// [B,A] = butter(2,100/4000,'high')
+const CascadedBiQuadFilter::BiQuadCoefficients
+    kHighPassFilterCoefficients_8kHz = {
+        {0.945976856002790, -1.891953712005580, 0.945976856002790},
+        {-1.889033079394525, 0.894874344616636}};
+const int kNumberOfHighPassBiQuads_8kHz = 1;
+
+// [B,A] = butter(2,100/8000,'high')
+const CascadedBiQuadFilter::BiQuadCoefficients
+    kHighPassFilterCoefficients_16kHz = {
+        {0.972613898499844, -1.945227796999688, 0.972613898499844},
+        {-1.944477657767094, 0.945977936232282}};
+const int kNumberOfHighPassBiQuads_16kHz = 1;
+
+}  // namespace
+
+EchoCanceller3::RenderWriterState::RenderWriterState(
+    ApmDataDumper* data_dumper,
+    RenderTransferBufferWriter* transfer_buffer_writer,
+    std::unique_ptr<CascadedBiQuadFilter> render_highpass_filter,
+    int sample_rate_hz,
+    int frame_length,
+    int num_bands)
+    : data_dumper_(data_dumper),

[hlundin-webrtc, 2016/12/16 10:04:47]

Is nullptr valid? Otherwise, add a DCHECK.

[peah-webrtc, 2016/12/20 10:10:25]

Done.

+      sample_rate_hz_(sample_rate_hz),
+      frame_length_(frame_length),
+      num_bands_(num_bands),
+      transfer_buffer_writer_(transfer_buffer_writer),
+      render_highpass_filter_(std::move(render_highpass_filter)),
+      render_queue_input_frame_(num_bands_ * frame_length_, 0.f) {}
+
+EchoCanceller3::RenderWriterState::~RenderWriterState() = default;
+
+bool EchoCanceller3::RenderWriterState::Insert(AudioBuffer* render) {

[hlundin-webrtc, 2016/12/16 10:04:46]

"render" is a dubious name here. I think "input" would me more descriptive.

[peah-webrtc, 2016/12/20 10:10:25]

Done.

+  RTC_DCHECK_EQ(1u, render->num_channels());

[hlundin-webrtc, 2016/12/16 10:04:47]

Nit: I think you no longer need the 'u' suffix, as a result of
https://codereview.webrtc.org/2535593002.

[peah-webrtc, 2016/12/20 10:10:25]

Done.

+  RTC_DCHECK_EQ(frame_length_, render->num_frames_per_band());
+  data_dumper_->DumpWav("aec3_render_input", frame_length_,
+                        &render->split_bands_f(0)[0][0],
+                        LowestBandRate(sample_rate_hz_), 1);
+
+  CopyAudioBufferIntoFrame(render, num_bands_, frame_length_,
+                           render_queue_input_frame_);
+
+  if (render_highpass_filter_) {
+    render_highpass_filter_->Process(
+        rtc::ArrayView<float>(&render_queue_input_frame_[0], frame_length_));
+  }
+
+  return transfer_buffer_writer_->Insert(&render_queue_input_frame_);
+}
+
 int EchoCanceller3::instance_count_ = 0;
 
-EchoCanceller3::EchoCanceller3(int sample_rate_hz, bool use_anti_hum_filter) {
-  int band_sample_rate_hz = (sample_rate_hz == 8000 ? sample_rate_hz : 16000);
-  frame_length_ = rtc::CheckedDivExact(band_sample_rate_hz, 100);
+EchoCanceller3::EchoCanceller3(int sample_rate_hz, bool use_highpass_filter)
+    : data_dumper_(new ApmDataDumper(instance_count_)),
+      sample_rate_hz_(sample_rate_hz),
+      num_bands_(sample_rate_hz_ == 8000 ? 1 : sample_rate_hz / 16000),

[hlundin-webrtc, 2016/12/16 10:04:47]

You have this expression defined as NumBandsForRate(sample_rate_hz) in
aec3_constants.h.

[peah-webrtc, 2016/12/20 10:10:25]

Done.

+      frame_length_(rtc::CheckedDivExact(LowestBandRate(sample_rate_hz_), 100)),
+      output_framer_(num_bands_, frame_length_),
+      capture_blocker_(num_bands_, frame_length_),
+      render_blocker_(num_bands_, frame_length_),
+      render_transfer_buffer_(num_bands_, frame_length_),
+      block_processor_(data_dumper_.get(), sample_rate_hz, num_bands_),
+      render_queue_output_frame_(num_bands_ * frame_length_, 0.f) {
+  std::unique_ptr<CascadedBiQuadFilter> render_highpass_filter;
+  if (use_highpass_filter) {
+    render_highpass_filter.reset(new CascadedBiQuadFilter(
+        sample_rate_hz_ == 8000 ? kHighPassFilterCoefficients_8kHz
+                                : kHighPassFilterCoefficients_16kHz,
+        sample_rate_hz_ == 8000 ? kNumberOfHighPassBiQuads_8kHz
+                                : kNumberOfHighPassBiQuads_16kHz));
+    capture_highpass_filter_.reset(new CascadedBiQuadFilter(
+        sample_rate_hz_ == 8000 ? kHighPassFilterCoefficients_8kHz
+                                : kHighPassFilterCoefficients_16kHz,
+        sample_rate_hz_ == 8000 ? kNumberOfHighPassBiQuads_8kHz
+                                : kNumberOfHighPassBiQuads_16kHz));
+  }
+
+  render_writer_.reset(
+      new RenderWriterState(data_dumper_.get(), &render_transfer_buffer_,
+                            std::move(render_highpass_filter), sample_rate_hz_,

[hlundin-webrtc, 2016/12/16 10:04:47]

What happens when you move from an (explicitly) uninitialized unique_ptr? Are
you sure you get a nullptr as expected? From
http://en.cppreference.com/w/cpp/memory/unique_ptr/unique_ptr, it seems that the
default ctor value-initializes the pointer, which I assume sets it to nullptr.

[peah-webrtc, 2016/12/20 10:10:25]

Good point! I cannot find whether it actually initializes it to null though. I
added that specifically.

PTAL 

+                            frame_length_, num_bands_));
 
-  LOG(LS_INFO) << "AEC3 created : "
-               << "{ instance_count: " << instance_count_ << "}";
+  RTC_DCHECK_EQ(num_bands_, std::max(sample_rate_hz_, 16000) / 16000);
+  RTC_DCHECK_GE(kMaxNumBands, num_bands_);
   instance_count_ = rtc::AtomicOps::Increment(&instance_count_);
 }
 
 EchoCanceller3::~EchoCanceller3() = default;
 
 bool EchoCanceller3::AnalyzeRender(AudioBuffer* render) {
-  RTC_DCHECK_EQ(1u, render->num_channels());
-  RTC_DCHECK_EQ(frame_length_, render->num_frames_per_band());
-  return true;
+  return render_writer_->Insert(render);
+}
+
+bool EchoCanceller3::EmptyRenderQueue() {

[hlundin-webrtc, 2016/12/16 10:04:47]

Check the order of the methods in this file, and make sure it is the same as in
the h-file.

[peah-webrtc, 2016/12/20 10:10:25]

Done.

+  bool render_buffer_overrun = false;
+  bool frame_to_buffer =
+      render_transfer_buffer_.Remove(&render_queue_output_frame_);
+  while (frame_to_buffer) {
+    render_buffer_overrun |= BufferRenderFrameContent(
+        render_queue_output_frame_, 0, &render_blocker_, &block_processor_);
+
+    if (sample_rate_hz_ != 8000) {
+      render_buffer_overrun |= BufferRenderFrameContent(
+          render_queue_output_frame_, 1, &render_blocker_, &block_processor_);
+    }
+
+    render_buffer_overrun |=
+        BufferRemainingRenderFrameContent(&render_blocker_, &block_processor_);
+
+    frame_to_buffer =
+        render_transfer_buffer_.Remove(&render_queue_output_frame_);
+  }
+  return render_buffer_overrun;
 }
 
-void EchoCanceller3::AnalyzeCapture(AudioBuffer* capture) {}
+void EchoCanceller3::AnalyzeCapture(AudioBuffer* capture) {
+  data_dumper_->DumpWav("aec3_capture_analyze_input", frame_length_,
+                        capture->channels_f()[0], sample_rate_hz_, 1);
+
+  saturated_microphone_signal_ = false;

[hlundin-webrtc, 2016/12/16 10:04:47]

You reset saturated_microphone_signal_ both here and at the end of
ProcessCapture. Is this intentional?

[peah-webrtc, 2016/12/20 10:10:25]

Done.

+  for (size_t k = 0; k < capture->num_channels(); ++k) {
+    saturated_microphone_signal_ |=

[hlundin-webrtc, 2016/12/16 10:04:47]

I assume you will expand the work of this for loop later one. Otherwise, you can
do early return on finding the first saturation.
for () {
  if (DetectSaturation(blah) {
    saturated_microphone_signal_ = true;
    return;
  }
}

[peah-webrtc, 2016/12/20 10:10:25]

There will be no more work on this loop. I added early return.

Done.

+        DetectSaturation(rtc::ArrayView<const float>(capture->channels_f()[k],
+                                                     capture->num_frames()));
+  }
+}
 
 void EchoCanceller3::ProcessCapture(AudioBuffer* capture,
                                     bool known_echo_path_change) {
   RTC_DCHECK_EQ(1u, capture->num_channels());
   RTC_DCHECK_EQ(frame_length_, capture->num_frames_per_band());
+
+  rtc::ArrayView<float> capture_lower_band =
+      rtc::ArrayView<float>(&capture->split_bands_f(0)[0][0], frame_length_);
+
+  data_dumper_->DumpWav("aec3_capture_input", capture_lower_band,
+                        LowestBandRate(sample_rate_hz_), 1);
+
+  bool render_buffer_overrun = EmptyRenderQueue();
+  RTC_DCHECK(!render_buffer_overrun);
+
+  if (capture_highpass_filter_) {
+    capture_highpass_filter_->Process(capture_lower_band);
+  }
+
+  ProcessCaptureFrameContent(capture, known_echo_path_change,
+                             saturated_microphone_signal_, 0, &capture_blocker_,
+                             &output_framer_, &block_processor_);
+
+  if (sample_rate_hz_ != 8000) {
+    ProcessCaptureFrameContent(
+        capture, known_echo_path_change, saturated_microphone_signal_, 1,
+        &capture_blocker_, &output_framer_, &block_processor_);
+  }
+
+  ProcessRemainingCaptureFrameContent(
+      known_echo_path_change, saturated_microphone_signal_, &capture_blocker_,
+      &output_framer_, &block_processor_);
+
+  data_dumper_->DumpWav("aec3_capture_output", frame_length_,
+                        &capture->split_bands_f(0)[0][0],
+                        LowestBandRate(sample_rate_hz_), 1);
+
+  saturated_microphone_signal_ = false;
 }
 
 std::string EchoCanceller3::ToString(