Finalization of the first version of EchoCanceller 3

webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc

Index: webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc
diff --git a/webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc b/webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc
index 1c019937e5086ddd3ffe6ab4f7242dc9d2ce1dd9..b001dd852ff4220c10595870a26c652ccdbe9a72 100644
--- a/webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc
+++ b/webrtc/modules/audio_processing/aec3/echo_remover_unittest.cc
@@ -10,12 +10,16 @@
 
 #include "webrtc/modules/audio_processing/aec3/echo_remover.h"
 
+#include <algorithm>
 #include <memory>
+#include <numeric>
 #include <sstream>
 #include <string>
 
+#include "webrtc/base/random.h"
 #include "webrtc/modules/audio_processing/aec3/aec3_constants.h"
 #include "webrtc/modules/audio_processing/logging/apm_data_dumper.h"
+#include "webrtc/modules/audio_processing/test/echo_canceller_test_tools.h"
 #include "webrtc/test/gtest.h"
 
 namespace webrtc {
@@ -27,6 +31,12 @@ std::string ProduceDebugText(int sample_rate_hz) {
   return ss.str();
 }
 
+std::string ProduceDebugText(int sample_rate_hz, int delay) {
+  std::ostringstream ss(ProduceDebugText(sample_rate_hz));
+  ss << ", Delay: " << delay;
+  return ss.str();
+}
+
 }  // namespace
 
 // Verifies the basic API call sequence
@@ -155,4 +165,62 @@ TEST(EchoRemover, NullCapture) {
 
 #endif
 
+// Performs a sanity check that the echo_remover is being able to properly
+// remove echoes.
+TEST(EchoRemover, BasicEchoRemoval) {

[aleloi, 2017/02/09 17:02:59]

In this test, the near-end signal is comprised entirely of echo. Is it possible
to write similar test/tests for echo + noise?

[peah-webrtc, 2017/02/20 07:37:16]

Absolutely, but I'm not sure if we should have that here/yet as the AEC
functionality is still being developed. I prefer to stay with having  this
sanity check test for now and instead consider adding more refined tests later.
Is that ok?

[aleloi, 2017/02/23 10:56:45]

Acknowledged.

+  constexpr int kNumBlocksToProcess = 500;
+  Random random_generator(42U);
+  for (auto rate : {8000, 16000, 32000, 48000}) {
+    std::vector<std::vector<float>> x(NumBandsForRate(rate),
+                                      std::vector<float>(kBlockSize, 0.f));
+    std::vector<std::vector<float>> y(NumBandsForRate(rate),
+                                      std::vector<float>(kBlockSize, 0.f));
+    EchoPathVariability echo_path_variability(false, false);
+    for (size_t delay_samples : {0, 64, 150, 200, 301}) {
+      ProduceDebugText(rate, delay_samples);

[aleloi, 2017/02/09 17:02:59]

This doesn't do anything: ProduceDebugText is a function that returns a string
and has no side effects. It's used the same way in decimator_by_4_unittest.
Should it be logged with SCOPED_TRACE?

[peah-webrtc, 2017/02/20 07:37:16]

Done.

+      std::unique_ptr<EchoRemover> remover(EchoRemover::Create(rate));
+      std::vector<std::unique_ptr<DelayBuffer<float>>> delay_buffers(x.size());
+      for (size_t j = 0; j < x.size(); ++j) {
+        delay_buffers[j].reset(new DelayBuffer<float>(delay_samples));
+      }
+
+      bool silence = false;
+      float input_energy = 0.f;
+      float output_energy = 0.f;
+      for (int k = 0; k < kNumBlocksToProcess; ++k) {
+        if (silence) {
+          silence = (k % 100) != 0;
+        } else {
+          silence = (k % 10) == 0;
+        }

[aleloi, 2017/02/09 17:02:59]

The 'silence' updating construct seems a little strange. Only frames 100-109,
200-209, 300-309, 400-409 will have nonzero samples. Why do we start with 100
blocks of silence?

Also, I think it's rather hard to understand what happens here. Can you change
it to

for k = 0..:
   const bool silence = <t/f condition calculated from 'k'>

?

To keep it working as is, it would be 

 'const bool silence = k < 100 || (k % 100 >= 10)' 

which I think is much better (but still a bit complex).

[peah-webrtc, 2017/02/20 07:37:16]

I changed to your proposed one-linear.

The purpose of the silence and speech parts is to ensure that the comfort noise
generator has an estimated noise level that is lower than the non-silence parts,
as that will then allow us to detect the removal of those in the AEC output.

The initial silence of 100 frames is not important, it could as well  have been
the other way around.

+
+        for (size_t j = 0; j < x.size(); ++j) {
+          if (silence) {
+            std::fill(x[j].begin(), x[j].end(), 0.f);
+          } else {
+            RandomizeSampleVector(&random_generator, x[j]);
+          }
+          delay_buffers[j]->Delay(x[j], y[j]);
+        }
+
+        if (k > kNumBlocksToProcess / 2) {
+          for (size_t j = 0; j < x.size(); ++j) {
+            input_energy = std::inner_product(y[j].begin(), y[j].end(),
+                                              y[j].begin(), input_energy);
+          }
+
+          remover->ProcessBlock(rtc::Optional<size_t>(delay_samples),
+                                echo_path_variability, false, x, &y);

[aleloi, 2017/02/09 17:02:59]

The remover only sees the last 250 of the samples. That seems wrong. I think the
remover should process all of the samples, but that energy levels should only be
counted starting from kNumBlocksToProcess/2.

[peah-webrtc, 2017/02/20 07:37:16]

Good find!

Done.

+
+          for (size_t j = 0; j < x.size(); ++j) {
+            output_energy = std::inner_product(y[j].begin(), y[j].end(),
+                                               y[j].begin(), output_energy);
+          }
+        }
+      }
+      EXPECT_GT(input_energy, 10.f * output_energy);
+    }
+  }
+}
+
 }  // namespace webrtc