Adding second layer of the echo canceller 3 functionality.

webrtc/modules/audio_processing/aec3/block_processor.cc

Index: webrtc/modules/audio_processing/aec3/block_processor.cc
diff --git a/webrtc/modules/audio_processing/aec3/block_processor.cc b/webrtc/modules/audio_processing/aec3/block_processor.cc
index 066e2f71ac1078ddaddfbfc8d28600a9853ec8b4..3eb48c9ceeb2b042ce6b298f37ebe4dadc87b8cb 100644
--- a/webrtc/modules/audio_processing/aec3/block_processor.cc
+++ b/webrtc/modules/audio_processing/aec3/block_processor.cc
@@ -9,9 +9,16 @@
  */
 #include "webrtc/modules/audio_processing/aec3/block_processor.h"
 
+#include <memory>

[hlundin-webrtc, 2017/01/18 13:08:48]

memory already included in .h file.

[peah-webrtc, 2017/01/19 15:33:04]

Done.

[hlundin-webrtc, 2017/01/20 09:31:44]

No, not done. I meant that you should remove it from this file.

[peah-webrtc, 2017/01/23 14:16:39]

Sorry. 

Done.

+
 #include "webrtc/base/atomicops.h"
+#include "webrtc/base/constructormagic.h"
 #include "webrtc/base/optional.h"
 #include "webrtc/modules/audio_processing/aec3/aec3_constants.h"
+#include "webrtc/modules/audio_processing/aec3/echo_path_variability.h"
+#include "webrtc/modules/audio_processing/aec3/echo_remover.h"
+#include "webrtc/modules/audio_processing/logging/apm_data_dumper.h"
+#include "webrtc/system_wrappers/include/logging.h"
 
 namespace webrtc {
 namespace {
@@ -19,50 +26,120 @@ namespace {
 class BlockProcessorImpl final : public BlockProcessor {
  public:
   explicit BlockProcessorImpl(int sample_rate_hz);
+  BlockProcessorImpl(int sample_rate_hz,
+                     std::unique_ptr<RenderDelayBuffer> render_buffer);
+  BlockProcessorImpl(int sample_rate_hz,
+                     std::unique_ptr<RenderDelayBuffer> render_buffer,
+                     std::unique_ptr<RenderDelayController> delay_controller,
+                     std::unique_ptr<EchoRemover> echo_remover);
+
   ~BlockProcessorImpl() override;
 
-  void ProcessCapture(bool known_echo_path_change,
-                      bool saturated_microphone_signal,
+  void ProcessCapture(bool echo_path_gain_change,
+                      bool capture_signal_saturation,
                       std::vector<std::vector<float>>* capture_block) override;
 
   bool BufferRender(std::vector<std::vector<float>>* block) override;
 
-  void ReportEchoLeakage(bool leakage_detected) override;
+  void UpdateEchoLeakageStatus(bool leakage_detected) override;
 
  private:
-  const size_t sample_rate_hz_;
   static int instance_count_;
   std::unique_ptr<ApmDataDumper> data_dumper_;
+  const size_t sample_rate_hz_;
+  std::unique_ptr<RenderDelayBuffer> render_buffer_;
+  std::unique_ptr<RenderDelayController> delay_controller_;
+  std::unique_ptr<EchoRemover> echo_remover_;
   RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(BlockProcessorImpl);
 };
 
+static const size_t kRenderBufferSize = 250;
 int BlockProcessorImpl::instance_count_ = 0;
+constexpr size_t kMaxApiJitter = 30;
 
 BlockProcessorImpl::BlockProcessorImpl(int sample_rate_hz)
-    : sample_rate_hz_(sample_rate_hz),
-      data_dumper_(
-          new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))) {}
+    : data_dumper_(
+          new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))),

[hlundin-webrtc, 2017/01/18 13:08:47]

You only need one ctor (the one with 4 parameters), and have the Create
functions create the dependencies when needed.

[peah-webrtc, 2017/01/19 15:33:04]

Good point! I needed to do some other changes as well as it is in that
construction no longer possible nicely pass around the pointer to the
ApmDataDumper. I therefore let the other objects create their own ApmDataDumper
objects. 

PTAL

Done.

+      sample_rate_hz_(sample_rate_hz),
+      render_buffer_(RenderDelayBuffer::Create(kRenderBufferSize,
+                                               NumBandsForRate(sample_rate_hz_),
+                                               kMaxApiJitter)),
+      delay_controller_(RenderDelayController::Create(data_dumper_.get(),
+                                                      sample_rate_hz,
+                                                      *render_buffer_)),
+      echo_remover_(EchoRemover::Create(data_dumper_.get(), sample_rate_hz)) {}
+
+BlockProcessorImpl::BlockProcessorImpl(
+    int sample_rate_hz,
+    std::unique_ptr<RenderDelayBuffer> render_buffer)
+    : data_dumper_(
+          new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))),
+      sample_rate_hz_(sample_rate_hz),
+      render_buffer_(std::move(render_buffer)),
+      delay_controller_(RenderDelayController::Create(data_dumper_.get(),
+                                                      sample_rate_hz,
+                                                      *render_buffer_)),
+      echo_remover_(EchoRemover::Create(data_dumper_.get(), sample_rate_hz)) {}
+
+BlockProcessorImpl::BlockProcessorImpl(
+    int sample_rate_hz,
+    std::unique_ptr<RenderDelayBuffer> render_buffer,
+    std::unique_ptr<RenderDelayController> delay_controller,
+    std::unique_ptr<EchoRemover> echo_remover)
+    : data_dumper_(
+          new ApmDataDumper(rtc::AtomicOps::Increment(&instance_count_))),
+      sample_rate_hz_(sample_rate_hz),
+      render_buffer_(std::move(render_buffer)),
+      delay_controller_(std::move(delay_controller)),
+      echo_remover_(std::move(echo_remover)) {}
 
 BlockProcessorImpl::~BlockProcessorImpl() = default;
 
 void BlockProcessorImpl::ProcessCapture(
-    bool known_echo_path_change,
-    bool saturated_microphone_signal,
+    bool echo_path_gain_change,
+    bool capture_signal_saturation,
     std::vector<std::vector<float>>* capture_block) {
   RTC_DCHECK(capture_block);
   RTC_DCHECK_EQ(NumBandsForRate(sample_rate_hz_), capture_block->size());
   RTC_DCHECK_EQ(kBlockSize, (*capture_block)[0].size());
+
+  const size_t delay = delay_controller_->SelectDelay((*capture_block)[0]);
+  const bool render_delay_change = delay != render_buffer_->Delay();
+
+  if (render_delay_change) {
+    render_buffer_->SetDelay(delay);
+  }
+
+  if (render_buffer_->IsBlockAvailable()) {
+    const std::vector<std::vector<float>>& render_block =

[hlundin-webrtc, 2017/01/18 13:08:47]

auto

[peah-webrtc, 2017/01/19 15:33:04]

Done.

+        render_buffer_->GetNext();
+    echo_remover_->ProcessBlock(
+        delay_controller_->AlignmentHeadroom(),
+        EchoPathVariability(echo_path_gain_change, render_delay_change),
+        capture_signal_saturation, render_block, capture_block);
+  } else {
+    LOG(LS_INFO) << "AEC3 empty render buffer";

[hlundin-webrtc, 2017/01/18 13:08:48]

Is this an error? When does it happen? Should we (D)CHECK?

[peah-webrtc, 2017/01/19 15:33:04]

I'm not sure how we should handle this. It is basically related to clock-drift
in that it happens when there are more samples being rendered than there is
being captured. As the clock-drift handling is not yet ready, I'd like to keep
it as it is. I neither want to DCHECK as it is is a valid condition for
clockdrift.

WDYT, can we keep it as it is and change this when we land the code for handling
clock-drift?

[hlundin-webrtc, 2017/01/20 09:31:44]

Sure.

[peah-webrtc, 2017/01/23 14:16:39]

Acknowledged.

+  }
 }
 
-bool BlockProcessorImpl::BufferRender(
-    std::vector<std::vector<float>>* render_block) {
-  RTC_DCHECK(render_block);
-  RTC_DCHECK_EQ(NumBandsForRate(sample_rate_hz_), render_block->size());
-  RTC_DCHECK_EQ(kBlockSize, (*render_block)[0].size());
+bool BlockProcessorImpl::BufferRender(std::vector<std::vector<float>>* block) {
+  RTC_DCHECK(block);
+  RTC_DCHECK_EQ(NumBandsForRate(sample_rate_hz_), block->size());
+  RTC_DCHECK_EQ(kBlockSize, (*block)[0].size());
+
+  bool overrun = !delay_controller_->AnalyzeRender((*block)[0]);

[hlundin-webrtc, 2017/01/18 13:08:47]

I think this code would be easier to read if you stored the results in two
distinct variables:

const bool overrun1 = !delay_controller_->AnalyzeRender((*block)[0]);
const bool overrun2 = !render_buffer_->Insert(block);
if (overrun1 || overrun2) {
...

(Better names are encouraged...)

[peah-webrtc, 2017/01/19 15:33:04]

Good point! Agree I do.

Done.

+  overrun = !render_buffer_->Insert(block) || overrun;
+  if (overrun) {
+    LOG(LS_INFO) << "AEC3 Buffer overrrun";

[hlundin-webrtc, 2017/01/18 13:08:47]

buffer

[peah-webrtc, 2017/01/19 15:33:04]

Done.

+    return true;

[hlundin-webrtc, 2017/01/18 13:08:48]

I guess "true" means "overrun happened". But we usually have "true" mean
"success". Would it be ok to change to that?

[peah-webrtc, 2017/01/19 15:33:04]

Makes sense. I did that change in some other places as well.

Done.

+  }
+
   return false;
 }
 
-void BlockProcessorImpl::ReportEchoLeakage(bool leakage_detected) {}
+void BlockProcessorImpl::UpdateEchoLeakageStatus(bool leakage_detected) {
+  echo_remover_->UpdateEchoLeakageStatus(leakage_detected);
+}
 
 }  // namespace
 
@@ -70,4 +147,20 @@ BlockProcessor* BlockProcessor::Create(int sample_rate_hz) {
   return new BlockProcessorImpl(sample_rate_hz);
 }
 
+BlockProcessor* BlockProcessor::Create(
+    int sample_rate_hz,
+    std::unique_ptr<RenderDelayBuffer> render_buffer) {
+  return new BlockProcessorImpl(sample_rate_hz, std::move(render_buffer));
+}
+
+BlockProcessor* BlockProcessor::Create(
+    int sample_rate_hz,
+    std::unique_ptr<RenderDelayBuffer> render_buffer,
+    std::unique_ptr<RenderDelayController> delay_controller,
+    std::unique_ptr<EchoRemover> echo_remover) {
+  return new BlockProcessorImpl(sample_rate_hz, std::move(render_buffer),
+                                std::move(delay_controller),
+                                std::move(echo_remover));
+}
+
 }  // namespace webrtc