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

webrtc/modules/audio_processing/test/audio_processing_unittest.cc

Index: webrtc/modules/audio_processing/test/audio_processing_unittest.cc
diff --git a/webrtc/modules/audio_processing/test/audio_processing_unittest.cc b/webrtc/modules/audio_processing/test/audio_processing_unittest.cc
index 53f667d50d7815287ae1b873606598bd02120149..da695ec03842f4aa81d0dd90d01321a6ac8c8b27 100644
--- a/webrtc/modules/audio_processing/test/audio_processing_unittest.cc
+++ b/webrtc/modules/audio_processing/test/audio_processing_unittest.cc
@@ -55,8 +55,8 @@ 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
@@ -435,11 +435,7 @@ void ApmTest::SetUp() {
   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() {
@@ -1039,18 +1035,6 @@ TEST_F(ApmTest, DISABLED_EchoCancellationReportsCorrectDelays) {
 }
 
 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));
@@ -1974,6 +1958,7 @@ TEST_F(ApmTest, FloatAndIntInterfacesGiveSimilarResults) {
                                         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;
@@ -2012,18 +1997,13 @@ TEST_F(ApmTest, FloatAndIntInterfacesGiveSimilarResults) {
         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;
         }
       }
 
@@ -2039,6 +2019,16 @@ TEST_F(ApmTest, FloatAndIntInterfacesGiveSimilarResults) {
       // 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_);
   }
@@ -2560,9 +2550,9 @@ TEST_P(AudioProcessingTest, Formats) {
       } 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(
@@ -2645,9 +2635,7 @@ TEST_P(AudioProcessingTest, Formats) {
         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);
@@ -2729,9 +2717,9 @@ INSTANTIATE_TEST_CASE_P(
                     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),
@@ -2742,15 +2730,15 @@ INSTANTIATE_TEST_CASE_P(
                     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),