Reland: Drop the 16kHz sample rate restriction on AECM and zero out higher bands

webrtc/modules/audio_processing/test/audio_processing_unittest.cc

 /*
  *  Copyright (c) 2012 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.
  */
 
@@ skipping 37 matching lines @@
 // re-implemented; see comment at the bottom of this file. For now, the user has
 // to hard code the |write_ref_data| value.
 // When false, this will compare the output data with the results stored to
 // file. This is the typical case. When the file should be updated, it can
 // be set to true with the command-line switch --write_ref_data.
 bool write_ref_data = false;
 const google::protobuf::int32 kChannels[] = {1, 2};
 const int kSampleRates[] = {8000, 16000, 32000, 48000};
 
 #if defined(WEBRTC_AUDIOPROC_FIXED_PROFILE)
-// AECM doesn't support super-wb.
-const int kProcessSampleRates[] = {8000, 16000};
+// Android doesn't support 48kHz.
+const int kProcessSampleRates[] = {8000, 16000, 32000};
 #elif defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE)
 const int kProcessSampleRates[] = {8000, 16000, 32000, 48000};
 #endif
 
 enum StreamDirection { kForward = 0, kReverse };
 
 void ConvertToFloat(const int16_t* int_data, ChannelBuffer<float>* cb) {
   ChannelBuffer<int16_t> cb_int(cb->num_frames(),
                                 cb->num_channels());
   Deinterleave(int_data,
@@ skipping 358 matching lines @@
   config.Set<ExperimentalAgc>(new ExperimentalAgc(false));
   apm_.reset(AudioProcessing::Create(config));
 }
 
 void ApmTest::SetUp() {
   ASSERT_TRUE(apm_.get() != NULL);
 
   frame_ = new AudioFrame();
   revframe_ = new AudioFrame();
 
-#if defined(WEBRTC_AUDIOPROC_FIXED_PROFILE)
-  Init(16000, 16000, 16000, 2, 2, 2, false);
-#else
   Init(32000, 32000, 32000, 2, 2, 2, false);
-#endif
 }
 
 void ApmTest::TearDown() {
   if (frame_) {
     delete frame_;
   }
   frame_ = NULL;
 
   if (revframe_) {
     delete revframe_;
@@ skipping 579 matching lines @@
                                  delay_max_ms);
     // 5) A delay above the maximum delay should give an estimate at the
     //    boundary (= (kMaxDelayBlocks - 1) * num_ms_per_block).
     delay_ms += 20;
     ProcessDelayVerificationTest(delay_ms, kSystemDelayMs, delay_min_ms,
                                  delay_max_ms);
   }
 }
 
 TEST_F(ApmTest, EchoControlMobile) {
-  // AECM won't use super-wideband.
-  SetFrameSampleRate(frame_, 32000);
-  EXPECT_NOERR(apm_->ProcessStream(frame_));
-  EXPECT_EQ(apm_->kBadSampleRateError,
-            apm_->echo_control_mobile()->Enable(true));
-  SetFrameSampleRate(frame_, 16000);
-  EXPECT_NOERR(apm_->ProcessStream(frame_));
-  EXPECT_EQ(apm_->kNoError,
-            apm_->echo_control_mobile()->Enable(true));
-  SetFrameSampleRate(frame_, 32000);
-  EXPECT_EQ(apm_->kUnsupportedComponentError, apm_->ProcessStream(frame_));
-
   // Turn AECM on (and AEC off)
   Init(16000, 16000, 16000, 2, 2, 2, false);
   EXPECT_EQ(apm_->kNoError, apm_->echo_control_mobile()->Enable(true));
   EXPECT_TRUE(apm_->echo_control_mobile()->is_enabled());
 
   // Toggle routing modes
   EchoControlMobile::RoutingMode mode[] = {
       EchoControlMobile::kQuietEarpieceOrHeadset,
       EchoControlMobile::kEarpiece,
       EchoControlMobile::kLoudEarpiece,
@@ skipping 903 matching lines @@
 
     Init(test->sample_rate(), test->sample_rate(), test->sample_rate(),
          num_input_channels, num_output_channels, num_render_channels, true);
     Init(fapm.get());
 
     ChannelBuffer<int16_t> output_cb(samples_per_channel, num_input_channels);
     ChannelBuffer<int16_t> output_int16(samples_per_channel,
                                         num_input_channels);
 
     int analog_level = 127;
+    size_t num_bad_chunks = 0;
     while (ReadFrame(far_file_, revframe_, revfloat_cb_.get()) &&
            ReadFrame(near_file_, frame_, float_cb_.get())) {
       frame_->vad_activity_ = AudioFrame::kVadUnknown;
 
       EXPECT_NOERR(apm_->AnalyzeReverseStream(revframe_));
       EXPECT_NOERR(fapm->AnalyzeReverseStream(
           revfloat_cb_->channels(),
           samples_per_channel,
           test->sample_rate(),
           LayoutFromChannels(num_render_channels)));
@@ skipping 18 matching lines @@
           LayoutFromChannels(num_output_channels),
           float_cb_->channels()));
       for (size_t j = 0; j < num_output_channels; ++j) {
         FloatToS16(float_cb_->channels()[j],
                    samples_per_channel,
                    output_cb.channels()[j]);
         float variance = 0;
         float snr = ComputeSNR(output_int16.channels()[j],
                                output_cb.channels()[j],
                                samples_per_channel, &variance);
-  #if defined(WEBRTC_AUDIOPROC_FIXED_PROFILE)
-        // There are a few chunks in the fixed-point profile that give low SNR.
-        // Listening confirmed the difference is acceptable.
-        const float kVarianceThreshold = 150;
-        const float kSNRThreshold = 10;
-  #else
+
         const float kVarianceThreshold = 20;
         const float kSNRThreshold = 20;
-  #endif
+
         // Skip frames with low energy.
-        if (sqrt(variance) > kVarianceThreshold) {
-          EXPECT_LT(kSNRThreshold, snr);
+        if (sqrt(variance) > kVarianceThreshold && snr < kSNRThreshold) {
+          ++num_bad_chunks;
         }
       }
 
       analog_level = fapm->gain_control()->stream_analog_level();
       EXPECT_EQ(apm_->gain_control()->stream_analog_level(),
                 fapm->gain_control()->stream_analog_level());
       EXPECT_EQ(apm_->echo_cancellation()->stream_has_echo(),
                 fapm->echo_cancellation()->stream_has_echo());
       EXPECT_NEAR(apm_->noise_suppression()->speech_probability(),
                   fapm->noise_suppression()->speech_probability(),
                   0.01);
 
       // Reset in case of downmixing.
       frame_->num_channels_ = static_cast<size_t>(test->num_input_channels());
     }
+
+#if defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE)
+    const size_t kMaxNumBadChunks = 0;
+#elif defined(WEBRTC_AUDIOPROC_FIXED_PROFILE)
+    // There are a few chunks in the fixed-point profile that give low SNR.
+    // Listening confirmed the difference is acceptable.
+    const size_t kMaxNumBadChunks = 60;
+#endif
+    EXPECT_LE(num_bad_chunks, kMaxNumBadChunks);
+
     rewind(far_file_);
     rewind(near_file_);
   }
 }
 
 // TODO(andrew): Add a test to process a few frames with different combinations
 // of enabled components.
 
 TEST_F(ApmTest, Process) {
   GOOGLE_PROTOBUF_VERIFY_VERSION;
@@ skipping 501 matching lines @@
 
       if (min_ref_rate > 32000) {
         ref_rate = 48000;
       } else if (min_ref_rate > 16000) {
         ref_rate = 32000;
       } else if (min_ref_rate > 8000) {
         ref_rate = 16000;
       } else {
         ref_rate = 8000;
       }
-#ifdef WEBRTC_AUDIOPROC_FIXED_PROFILE
+#ifdef WEBRTC_ARCH_ARM_FAMILY
       if (file_direction == kForward) {
-        ref_rate = std::min(ref_rate, 16000);
+        ref_rate = std::min(ref_rate, 32000);
       }
 #endif
       FILE* out_file = fopen(
           OutputFilePath("out", input_rate_, output_rate_, reverse_input_rate_,
                          reverse_output_rate_, cf[i].num_input,
                          cf[i].num_output, cf[i].num_reverse_input,
                          cf[i].num_reverse_output, file_direction).c_str(),
           "rb");
       // The reference files always have matching input and output channels.
       FILE* ref_file = fopen(
@@ skipping 62 matching lines @@
                 << reverse_input_rate_ << ", " << reverse_output_rate_ << ", "
                 << cf[i].num_input << ", " << cf[i].num_output << ", "
                 << cf[i].num_reverse_input << ", " << cf[i].num_reverse_output
                 << ", " << file_direction << "): ";
       if (sq_error > 0) {
         double snr = 10 * log10(variance / sq_error);
         EXPECT_GE(snr, expected_snr);
         EXPECT_NE(0, expected_snr);
         std::cout << "SNR=" << snr << " dB" << std::endl;
       } else {
-        EXPECT_EQ(expected_snr, 0);
-        std::cout << "SNR="
-                  << "inf dB" << std::endl;
+        std::cout << "SNR=inf dB" << std::endl;

[peah-webrtc, 2016/03/11 11:34:39]

From offline discussions, I got the impression that this is needed as for some
platforms the 48 kHz processing is done in 32 kHz internally and then upsampled
to 48 kHz, right?

I don't see why that should cause this change to be needed? It seem like this
change narrows the scope of the test. I'd rather have expected a change in the
test inputs that specifically for this platforms specifies a different behavior.

Afaics, this change makes the test less specific for the platforms where the 48
kHz processing is actually done in 48 kHz internally.
I don't know this test code that well, but I think that only a flaw in the
actual test should motivate a weakening of the testcases thare are currently in
place.

[aluebs-webrtc, 2016/03/11 13:27:05]

This is not a weaken of the test. When we specify the minimum expected SNR for
each test case, we happen to use "0" to represent an infinite SNR, which is
weird. If the actual SNR is infinite (no error), any minimum expected SNR
specified should be ok, since infinite is larger than any number. This is
necessary for the case when certain platforms have an infinite SNR and other
have not. There we specify the minimum, but we don't want the infinite SNR
breaking.

[peah-webrtc, 2016/03/11 21:37:28]

I actually think that is a weakening of the test. What you do is to increase the
range of all platforms as certain platforms behave differently. And furthermore,
there is an EXPECT_EQ which is removed when sq_error ==0.

[aluebs-webrtc, 2016/03/14 17:30:40]

I see that a EXPECT_EQ has been removed, but it is one that didn't make sense in
the first place. Let me try to explain using an example. As it is today, if in
one platform the SNR is 20dB and on another 30dB, we just use 20dB as a minimum
SNR and both test pass. Similarly if one platform has a SNR of 30dB and the
other one infdB (so an sq_error of 0) it would make sense to use 30dB as the
minimum SNR (since inf is higher than 30) and have both test pass, but it would
actually fail because in the inf case it would EXPECT_EQ the minimum SNR to be 0
(our weird way of representing inf). And when having a restriction of 32kHz for
ARM it makes sense that only on those platforms the SNR is below inf, so this
would be a valid use-case. If you still think think we should keep the
EXPECT_EQ, I am happy to revert this test to always use 16kHz (as it was before)
and have less coverage, but in my opinion that would be sub-optimal. Else I am
open to suggestions of how we should handle this case.

[peah-webrtc, 2016/03/15 15:22:54]

Thanks for the explanation! Now I get it :-) Awesome change!!!

       }
 
       fclose(out_file);
       fclose(ref_file);
     }
   }
 }
 
 #if defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE)
 INSTANTIATE_TEST_CASE_P(
@@ skipping 61 matching lines @@
                     std::tr1::make_tuple(48000, 44100, 48000, 44100, 15, 20),
                     std::tr1::make_tuple(48000, 44100, 32000, 44100, 15, 15),
                     std::tr1::make_tuple(48000, 44100, 16000, 44100, 15, 15),
                     std::tr1::make_tuple(48000, 32000, 48000, 32000, 20, 35),
                     std::tr1::make_tuple(48000, 32000, 32000, 32000, 20, 0),
                     std::tr1::make_tuple(48000, 32000, 16000, 32000, 20, 20),
                     std::tr1::make_tuple(48000, 16000, 48000, 16000, 20, 20),
                     std::tr1::make_tuple(48000, 16000, 32000, 16000, 20, 20),
                     std::tr1::make_tuple(48000, 16000, 16000, 16000, 20, 0),
 
-                    std::tr1::make_tuple(44100, 48000, 48000, 48000, 20, 0),
-                    std::tr1::make_tuple(44100, 48000, 32000, 48000, 20, 30),
-                    std::tr1::make_tuple(44100, 48000, 16000, 48000, 20, 20),
+                    std::tr1::make_tuple(44100, 48000, 48000, 48000, 15, 0),
+                    std::tr1::make_tuple(44100, 48000, 32000, 48000, 15, 30),
+                    std::tr1::make_tuple(44100, 48000, 16000, 48000, 15, 20),
                     std::tr1::make_tuple(44100, 44100, 48000, 44100, 15, 20),
                     std::tr1::make_tuple(44100, 44100, 32000, 44100, 15, 15),
                     std::tr1::make_tuple(44100, 44100, 16000, 44100, 15, 15),
                     std::tr1::make_tuple(44100, 32000, 48000, 32000, 20, 35),
                     std::tr1::make_tuple(44100, 32000, 32000, 32000, 20, 0),
                     std::tr1::make_tuple(44100, 32000, 16000, 32000, 20, 20),
                     std::tr1::make_tuple(44100, 16000, 48000, 16000, 20, 20),
                     std::tr1::make_tuple(44100, 16000, 32000, 16000, 20, 20),
                     std::tr1::make_tuple(44100, 16000, 16000, 16000, 20, 0),
 
-                    std::tr1::make_tuple(32000, 48000, 48000, 48000, 20, 0),
-                    std::tr1::make_tuple(32000, 48000, 32000, 48000, 20, 30),
-                    std::tr1::make_tuple(32000, 48000, 16000, 48000, 20, 20),
-                    std::tr1::make_tuple(32000, 44100, 48000, 44100, 15, 20),
-                    std::tr1::make_tuple(32000, 44100, 32000, 44100, 15, 15),
-                    std::tr1::make_tuple(32000, 44100, 16000, 44100, 15, 15),
-                    std::tr1::make_tuple(32000, 32000, 48000, 32000, 20, 35),
-                    std::tr1::make_tuple(32000, 32000, 32000, 32000, 20, 0),
-                    std::tr1::make_tuple(32000, 32000, 16000, 32000, 20, 20),
+                    std::tr1::make_tuple(32000, 48000, 48000, 48000, 35, 0),

[peah-webrtc, 2016/03/11 21:37:28]

Why did these values need to be changed? Are these only used when AECM is
active?

[aluebs-webrtc, 2016/03/14 17:30:40]

Here I only improved the SNRs, because the test stopped being limited to the
16kHz because of the AECM.

[peah-webrtc, 2016/03/15 15:22:54]

Awesome! Thanks for the explanation!

+                    std::tr1::make_tuple(32000, 48000, 32000, 48000, 65, 30),
+                    std::tr1::make_tuple(32000, 48000, 16000, 48000, 40, 20),
+                    std::tr1::make_tuple(32000, 44100, 48000, 44100, 20, 20),
+                    std::tr1::make_tuple(32000, 44100, 32000, 44100, 20, 15),
+                    std::tr1::make_tuple(32000, 44100, 16000, 44100, 20, 15),
+                    std::tr1::make_tuple(32000, 32000, 48000, 32000, 35, 35),
+                    std::tr1::make_tuple(32000, 32000, 32000, 32000, 0, 0),
+                    std::tr1::make_tuple(32000, 32000, 16000, 32000, 40, 20),
                     std::tr1::make_tuple(32000, 16000, 48000, 16000, 20, 20),
                     std::tr1::make_tuple(32000, 16000, 32000, 16000, 20, 20),
                     std::tr1::make_tuple(32000, 16000, 16000, 16000, 20, 0),
 
                     std::tr1::make_tuple(16000, 48000, 48000, 48000, 25, 0),
                     std::tr1::make_tuple(16000, 48000, 32000, 48000, 25, 30),
                     std::tr1::make_tuple(16000, 48000, 16000, 48000, 25, 20),
                     std::tr1::make_tuple(16000, 44100, 48000, 44100, 15, 20),
                     std::tr1::make_tuple(16000, 44100, 32000, 44100, 15, 15),
                     std::tr1::make_tuple(16000, 44100, 16000, 44100, 15, 15),
                     std::tr1::make_tuple(16000, 32000, 48000, 32000, 25, 35),
                     std::tr1::make_tuple(16000, 32000, 32000, 32000, 25, 0),
                     std::tr1::make_tuple(16000, 32000, 16000, 32000, 25, 20),
                     std::tr1::make_tuple(16000, 16000, 48000, 16000, 35, 20),
                     std::tr1::make_tuple(16000, 16000, 32000, 16000, 40, 20),
                     std::tr1::make_tuple(16000, 16000, 16000, 16000, 0, 0)));
 #endif
 
 }  // namespace
 }  // namespace webrtc