Compensate for the IntelligibilityEnhancer processing delay in high bands

webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc

Index: webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc
diff --git a/webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc b/webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc
index 45f338c642540acaabf8fb96652b55c28f51cce0..372a8935177ab7f18a071e1282d5f304f5ecafd6 100644
--- a/webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc
+++ b/webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc
@@ -202,11 +202,12 @@ static_assert(arraysize(kTestCenterFreqs) ==
 const float kMaxTestError = 0.005f;
 
 // Enhancer initialization parameters.
-const int kSamples = 1000;
+const int kSamples = 10000;

[peah-webrtc, 2016/09/13 13:30:00]

What is the motivation behind the changed number of samples?

[aluebs-webrtc, 2016/09/14 00:35:55]

The number samples before where not enough for the IE to adapt. The test passed
because the test did not compensate for the delay introduced by the IE.

 const int kSampleRate = 4000;

[peah-webrtc, 2016/09/13 13:30:00]

What is the purpose of the samplerate constant of 4000? 

[aluebs-webrtc, 2016/09/14 00:35:54]

To set a sample rate for the tests.

[peah-webrtc, 2016/09/15 15:06:20]

You mean that the test is running at a sample rate of 4 kHz? Why not use one of
the native rates instead? 4 kHz is never used in practice when using the IE.

[aluebs-webrtc, 2016/09/15 23:45:25]

Other test (like the bitexactness and the one I just added) use native rates. I
don't see anything wrong with also using different sample rates than used in
practice. Plus it means having less samples until convergence, making the test
quicker.
But anyway, if we would decide to change it, this is not the CL for doing that.
It is unrelated.

[peah-webrtc, 2016/09/16 13:35:56]

It makes sense to test for other sample rates. But in this case the IE is tested
for a case which it never (being located beneath APM) will operate in.

I'd claim that IE cannot support arbitrary sample rates. For instance, one
requirement is that the sample rate produces 10 ms frames of an even number of
samples. Same goes for the VAD. 4 kHz fulfills that requirement though.

The problem that I see with testing for sample rates that will never be used in
practice is that if the code is changed in a way that does not support that
sample rate, the test will need to be changed in vain. Furthermore, the added
test coverage by using a different sample rate is not useful as it tests the
code in a way that it will not be used.

But you are correct that it is not related to this CL.

[aluebs-webrtc, 2016/09/17 00:48:48]

Acknowledged.

 const int kNumChannels = 1;
 const int kFragmentSize = kSampleRate / 100;
 const size_t kNumNoiseBins = 129;
+const size_t kNumBands = 1;
 
 // Number of frames to process in the bitexactness tests.
 const size_t kNumFramesToProcess = 1000;
@@ -228,10 +229,7 @@ void ProcessOneFrame(int sample_rate_hz,
     capture_audio_buffer->SplitIntoFrequencyBands();
   }
 
-  intelligibility_enhancer->ProcessRenderAudio(
-      render_audio_buffer->split_channels_f(kBand0To8kHz),
-      IntelligibilityEnhancerSampleRate(sample_rate_hz),
-      render_audio_buffer->num_channels());
+  intelligibility_enhancer->ProcessRenderAudio(render_audio_buffer);
 
   noise_suppressor->AnalyzeCaptureAudio(capture_audio_buffer);
   noise_suppressor->ProcessCaptureAudio(capture_audio_buffer);
@@ -276,7 +274,8 @@ void RunBitexactnessTest(int sample_rate_hz,
 
   IntelligibilityEnhancer intelligibility_enhancer(
       IntelligibilityEnhancerSampleRate(sample_rate_hz),
-      render_config.num_channels(), NoiseSuppressionImpl::num_noise_bins());
+      render_config.num_channels(), kNumBands,
+      NoiseSuppressionImpl::num_noise_bins());
 
   for (size_t frame_no = 0u; frame_no < kNumFramesToProcess; ++frame_no) {
     ReadFloatSamplesFromStereoFile(render_buffer.num_frames(),
@@ -320,24 +319,34 @@ float float_rand() {
 class IntelligibilityEnhancerTest : public ::testing::Test {
  protected:
   IntelligibilityEnhancerTest()
-      : clear_data_(kSamples), noise_data_(kSamples), orig_data_(kSamples) {
+      : clear_buffer_(kFragmentSize,
+                      kNumChannels,
+                      kFragmentSize,
+                      kNumChannels,
+                      kFragmentSize),
+        stream_config_(kSampleRate, kNumChannels),
+        clear_data_(kSamples),
+        noise_data_(kNumNoiseBins),
+        orig_data_(kSamples) {
     std::srand(1);
-    enh_.reset(
-        new IntelligibilityEnhancer(kSampleRate, kNumChannels, kNumNoiseBins));
+    enh_.reset(new IntelligibilityEnhancer(kSampleRate, kNumChannels, kNumBands,
+                                           kNumNoiseBins));
   }
 
   bool CheckUpdate() {

[peah-webrtc, 2016/09/13 13:30:00]

I'd like a more descriptive name here. This method actually processes the audio
using the IE and checks whether the resulting output has been processed. I think
the readability of the test code below would benefit a lot from having a more
descriptive name that matches what is actually done in the method. WDYT?

[aluebs-webrtc, 2016/09/14 00:35:54]

I think that it is a great idea, but I don't think we should clutter this CL
with naming, since it is not related. It should be done in a separate CL.

[peah-webrtc, 2016/09/15 15:06:20]

Acknowledged.

-    enh_.reset(
-        new IntelligibilityEnhancer(kSampleRate, kNumChannels, kNumNoiseBins));
+    enh_.reset(new IntelligibilityEnhancer(kSampleRate, kNumChannels, kNumBands,
+                                           kNumNoiseBins));
     float* clear_cursor = clear_data_.data();

[peah-webrtc, 2016/09/13 13:30:00]

The name cursor I think is quite misleading as I cannot see the connection
between a cursor and a buffer position. I know it was present in the former code
but I think that it would be good to change it here. What about about
clear_buffer_position? WDYT?

[aluebs-webrtc, 2016/09/14 00:35:54]

See above about naming.

-    float* noise_cursor = noise_data_.data();
     for (int i = 0; i < kSamples; i += kFragmentSize) {
-      enh_->ProcessRenderAudio(&clear_cursor, kSampleRate, kNumChannels);
+      enh_->SetCaptureNoiseEstimate(noise_data_, 1);
+      clear_buffer_.CopyFrom(&clear_cursor, stream_config_);
+      enh_->ProcessRenderAudio(&clear_buffer_);
+      clear_buffer_.CopyTo(stream_config_, &clear_cursor);
       clear_cursor += kFragmentSize;
-      noise_cursor += kFragmentSize;
     }
-    for (int i = 0; i < kSamples; i++) {
-      if (std::fabs(clear_data_[i] - orig_data_[i]) > kMaxTestError) {
+    for (int i = initial_delay_; i < kSamples; i++) {
+      if (std::fabs(clear_data_[i] - orig_data_[i - initial_delay_]) >

[peah-webrtc, 2016/09/13 13:30:00]

As far as I can see, this does not verify that the delay of the upper bands is
correct as well as the delay for the lower bands. Please add a test for that.

[aluebs-webrtc, 2016/09/14 00:35:54]

Added test.

+          kMaxTestError) {
         return true;
       }
     }
@@ -345,22 +354,29 @@ class IntelligibilityEnhancerTest : public ::testing::Test {
   }
 
   std::unique_ptr<IntelligibilityEnhancer> enh_;
+  AudioBuffer clear_buffer_;

[peah-webrtc, 2016/09/13 13:30:00]

What is the reason for this name? Please explain, either in comments or by a
more descriptive variable name what it is that makes this buffer "clear". I
guess you mean that it is noise-free but then it is better to name it "speech",
"clean_speech", or "render_audio_buffer".

[aluebs-webrtc, 2016/09/14 00:35:54]

To be consistent with the naming in the test I am leaving it as is. But added
comment.

+  StreamConfig stream_config_;
   std::vector<float> clear_data_;
   std::vector<float> noise_data_;
   std::vector<float> orig_data_;
+  size_t initial_delay_;
 };
 
 // For each class of generated data, tests that render stream is updated when
 // it should be.
 TEST_F(IntelligibilityEnhancerTest, TestRenderUpdate) {
+  initial_delay_ = enh_->render_mangler_->initial_delay();
   std::fill(noise_data_.begin(), noise_data_.end(), 0.f);
   std::fill(orig_data_.begin(), orig_data_.end(), 0.f);
   std::fill(clear_data_.begin(), clear_data_.end(), 0.f);
   EXPECT_FALSE(CheckUpdate());
-  std::generate(noise_data_.begin(), noise_data_.end(), float_rand);
+  std::generate(clear_data_.begin(), clear_data_.end(), float_rand);
+  orig_data_ = clear_data_;
   EXPECT_FALSE(CheckUpdate());
   std::generate(clear_data_.begin(), clear_data_.end(), float_rand);
   orig_data_ = clear_data_;
+  std::generate(noise_data_.begin(), noise_data_.end(), float_rand);
+  FloatToFloatS16(noise_data_.data(), noise_data_.size(), noise_data_.data());
   EXPECT_TRUE(CheckUpdate());
 }
 
@@ -418,7 +434,8 @@ TEST_F(IntelligibilityEnhancerTest, TestNoiseGainHasExpectedResult) {
   float* clear_cursor = clear_data_.data();
   for (size_t i = 0; i < kNumFramesToProcess; ++i) {
     enh_->SetCaptureNoiseEstimate(noise, kGain);
-    enh_->ProcessRenderAudio(&clear_cursor, kSampleRate, kNumChannels);
+    clear_buffer_.CopyFrom(&clear_cursor, stream_config_);

[peah-webrtc, 2016/09/13 13:30:00]

Why don't you update the clear cursor counter here, in the same way as you do
the line 345?

[aluebs-webrtc, 2016/09/14 00:35:54]

Because it is irrelevant to the Noise PSD estimation, but the number of frames
to process is higher than the actual buffer size.

[peah-webrtc, 2016/09/15 15:06:20]

I see, so basically you don't really care about what values are in clear_buffer
since you only need to perform a number of ProcessRenderAudio calls in order to
verify that the noise gain is what it should, right?

I think this test code really would benefit from renaming or more commenting. 
The fact that the  same buffer is passed several times to ProcessRenderAudio
looks (incorrectly) like a clear bug as it is not the way IE is supposed to work
in practice (e.g., being called multiple times with the same render input data).

Another thing with this code that makes it looks buggy as that
kNumFramesToProcess does not match the length of clear_buffer_.


A comment would clarify that, or even more, a variable name of clear_buffer_ to
dummy_input_frame_ or empty_input_frame_ would make this clearer. 
Furthermore, it would make sense not reuse clear_buffer for this purpose as the
fact that it is much longer indicates that it should be used as a buffer and not
as a dummy frame.

[aluebs-webrtc, 2016/09/15 23:45:25]

Agreed. But it is unrelated to this CL, so we should do it in a different one.

+    enh_->ProcessRenderAudio(&clear_buffer_);
   }
   const std::vector<float>& estimated_psd =
       enh_->noise_power_estimator_.power();