Updating APM unittests on the echo metrics.

webrtc/modules/audio_processing/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 186 matching lines @@
   for (size_t i = 1; i < length; i++) {
     max_data = std::max(max_data, AbsValue(frame.data_[i]));
   }
 
   return max_data;
 }
 
 #if defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE)
 void TestStats(const AudioProcessing::Statistic& test,
                const audioproc::Test::Statistic& reference) {
-  EXPECT_NEAR(reference.instant(), test.instant, 2);
-  EXPECT_NEAR(reference.average(), test.average, 2);
-  EXPECT_NEAR(reference.maximum(), test.maximum, 3);
-  EXPECT_NEAR(reference.minimum(), test.minimum, 2);
+  ASSERT_EQ(reference.instant(), test.instant);

[peah-webrtc, 2016/05/20 12:21:23]

Why not use EXPECT_EQ? Does it make sense to continue the test if this fails?

[minyue-webrtc, 2016/05/23 03:19:45]

It was EXPECT_EQ because the test checked the values only once, now we added
multiple checks, therefore, it is better to quit early.

[peah-webrtc, 2016/05/23 05:11:53]

I don't agree with that. Please see the related comment below.

+  ASSERT_EQ(reference.average(), test.average);
+  ASSERT_EQ(reference.maximum(), test.maximum);
+  ASSERT_EQ(reference.minimum(), test.minimum);
 }
 
 void WriteStatsMessage(const AudioProcessing::Statistic& output,
                        audioproc::Test::Statistic* msg) {
   msg->set_instant(output.instant);
   msg->set_average(output.average);
   msg->set_maximum(output.maximum);
   msg->set_minimum(output.minimum);
 }
 #endif
 
 void OpenFileAndWriteMessage(const std::string filename,
                              const ::google::protobuf::MessageLite& msg) {
-#if defined(WEBRTC_LINUX) && !defined(WEBRTC_ANDROID)
   FILE* file = fopen(filename.c_str(), "wb");
   ASSERT_TRUE(file != NULL);
 
   int32_t size = msg.ByteSize();
   ASSERT_GT(size, 0);
   std::unique_ptr<uint8_t[]> array(new uint8_t[size]);
   ASSERT_TRUE(msg.SerializeToArray(array.get(), size));
 
   ASSERT_EQ(1u, fwrite(&size, sizeof(size), 1, file));
   ASSERT_EQ(static_cast<size_t>(size),
       fwrite(array.get(), sizeof(array[0]), size, file));
   fclose(file);
-#else
-  std::cout << "Warning: Writing new reference is only allowed on Linux!"
-      << std::endl;
-#endif
 }
 
 std::string ResourceFilePath(std::string name, int sample_rate_hz) {
   std::ostringstream ss;
   // Resource files are all stereo.
   ss << name << sample_rate_hz / 1000 << "_stereo";
   return test::ResourcePath(ss.str(), "pcm");
 }
 
 // Temporary filenames unique to this process. Used to be able to run these
@@ skipping 1843 matching lines @@
          true);
 
     int frame_count = 0;
     int has_echo_count = 0;
     int has_voice_count = 0;
     int is_saturated_count = 0;
     int analog_level = 127;
     int analog_level_average = 0;
     int max_output_average = 0;
     float ns_speech_prob_average = 0.0f;
+#if defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE)
+  int stats_index = 0;
+#endif
 
     while (ReadFrame(far_file_, revframe_) && ReadFrame(near_file_, frame_)) {
       EXPECT_EQ(apm_->kNoError, apm_->ProcessReverseStream(revframe_));
 
       frame_->vad_activity_ = AudioFrame::kVadUnknown;
 
       EXPECT_EQ(apm_->kNoError, apm_->set_stream_delay_ms(0));
       apm_->echo_cancellation()->set_stream_drift_samples(0);
       EXPECT_EQ(apm_->kNoError,
           apm_->gain_control()->set_stream_analog_level(analog_level));
@@ skipping 27 matching lines @@
       size_t frame_size = frame_->samples_per_channel_ * frame_->num_channels_;
       size_t write_count = fwrite(frame_->data_,
                                   sizeof(int16_t),
                                   frame_size,
                                   out_file_);
       ASSERT_EQ(frame_size, write_count);
 
       // Reset in case of downmixing.
       frame_->num_channels_ = static_cast<size_t>(test->num_input_channels());
       frame_count++;
+
+#if defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE)
+      const int kStatsAggregationFrameNum = 100;  // 1 second.
+      if (frame_count % kStatsAggregationFrameNum == 0) {
+        // Get echo metrics.
+        EchoCancellation::Metrics echo_metrics;
+        EXPECT_EQ(apm_->kNoError,
+                  apm_->echo_cancellation()->GetMetrics(&echo_metrics));
+
+        // Get delay metrics.
+        int median = 0;
+        int std = 0;
+        float fraction_poor_delays = 0;
+        EXPECT_EQ(apm_->kNoError,
+                  apm_->echo_cancellation()->GetDelayMetrics(
+                      &median, &std, &fraction_poor_delays));
+
+        // Get RMS.
+        int rms_level = apm_->level_estimator()->RMS();
+        EXPECT_LE(0, rms_level);
+        EXPECT_GE(127, rms_level);
+
+        if (!write_ref_data) {
+          const audioproc::Test::EchoMetrics& reference =
+              test->echo_metrics(stats_index);
+          TestStats(echo_metrics.residual_echo_return_loss,
+                    reference.residual_echo_return_loss());
+          TestStats(echo_metrics.echo_return_loss,
+                    reference.echo_return_loss());
+          TestStats(echo_metrics.echo_return_loss_enhancement,
+                    reference.echo_return_loss_enhancement());
+          TestStats(echo_metrics.a_nlp,
+                    reference.a_nlp());
+          ASSERT_EQ(echo_metrics.divergent_filter_fraction,
+                    reference.divergent_filter_fraction());
+
+          const audioproc::Test::DelayMetrics& reference_delay =
+              test->delay_metrics(stats_index);
+          ASSERT_EQ(reference_delay.median(), median);

[peah-webrtc, 2016/05/20 12:21:23]

Why have you changed this to ASSERT_EQ? That does not make the test stronger,
right? The change makes sense if the test will not make sense at all after one
change, but I think it these tests it would make sense to continue the test even
though one of these fail. WDYT?

[minyue-webrtc, 2016/05/23 03:19:45]

I think if it fails one time, no benefits can be obtained by letting it continue
to run. In such situation, the maker of the change should investigate whether
the change is valid and a new reference file should be created if the change is
valid.

[peah-webrtc, 2016/05/23 05:11:53]

That depends on how it is to be used. If it is to be used as a bitexactness test
for the metrics, I partly agree as there you are not interested in the actual
data points.

But even though you are not interested in the actual data points of the metrics,
you actually loose all the information about what goes wrong in this way. 
For instance, if the test fails due to the echo_metrics.echo_return_loss stats
it is not possible to tell whether the echo_metrics.echo_return_loss_enhancement
failed as well.

I think it is better to use EXPECT_EQ here as you will gain more information
when the test fails. The drawback is the extra complexity of running more checks
than needed when it fails but I think that is definitely worth it if that gives
us more information about the failing test.

+          ASSERT_EQ(reference_delay.std(), std);
+          ASSERT_EQ(reference_delay.fraction_poor_delays(),
+                    fraction_poor_delays);
+
+          ASSERT_EQ(test->rms_level(stats_index), rms_level);
+
+          ++stats_index;
+        } else {
+          audioproc::Test::EchoMetrics* message =
+              test->add_echo_metrics();
+          WriteStatsMessage(echo_metrics.residual_echo_return_loss,
+                            message->mutable_residual_echo_return_loss());
+          WriteStatsMessage(echo_metrics.echo_return_loss,
+                            message->mutable_echo_return_loss());
+          WriteStatsMessage(echo_metrics.echo_return_loss_enhancement,
+                            message->mutable_echo_return_loss_enhancement());
+          WriteStatsMessage(echo_metrics.a_nlp,
+                            message->mutable_a_nlp());
+          message->set_divergent_filter_fraction(
+              echo_metrics.divergent_filter_fraction);
+
+          audioproc::Test::DelayMetrics* message_delay =
+              test->add_delay_metrics();
+          message_delay->set_median(median);
+          message_delay->set_std(std);
+          message_delay->set_fraction_poor_delays(fraction_poor_delays);
+
+          test->add_rms_level(rms_level);
+        }
+      }
+#endif  // defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE).
     }
     max_output_average /= frame_count;
     analog_level_average /= frame_count;
     ns_speech_prob_average /= frame_count;
 
-#if defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE)
-    EchoCancellation::Metrics echo_metrics;
-    EXPECT_EQ(apm_->kNoError,
-              apm_->echo_cancellation()->GetMetrics(&echo_metrics));
-    int median = 0;
-    int std = 0;
-    float fraction_poor_delays = 0;
-    EXPECT_EQ(apm_->kNoError,
-              apm_->echo_cancellation()->GetDelayMetrics(
-                  &median, &std, &fraction_poor_delays));
-
-    int rms_level = apm_->level_estimator()->RMS();
-    EXPECT_LE(0, rms_level);
-    EXPECT_GE(127, rms_level);
-#endif
-
     if (!write_ref_data) {
       const int kIntNear = 1;
       // When running the test on a N7 we get a {2, 6} difference of
       // |has_voice_count| and |max_output_average| is up to 18 higher.
       // All numbers being consistently higher on N7 compare to ref_data.
       // TODO(bjornv): If we start getting more of these offsets on Android we
       // should consider a different approach. Either using one slack for all,
       // or generate a separate android reference.
 #if defined(WEBRTC_ANDROID)
       const int kHasVoiceCountOffset = 3;
       const int kHasVoiceCountNear = 4;
       const int kMaxOutputAverageOffset = 9;
       const int kMaxOutputAverageNear = 9;
 #else
       const int kHasVoiceCountOffset = 0;
       const int kHasVoiceCountNear = kIntNear;
       const int kMaxOutputAverageOffset = 0;
       const int kMaxOutputAverageNear = kIntNear;
 #endif
       EXPECT_NEAR(test->has_echo_count(), has_echo_count, kIntNear);
       EXPECT_NEAR(test->has_voice_count(),
                   has_voice_count - kHasVoiceCountOffset,
                   kHasVoiceCountNear);
       EXPECT_NEAR(test->is_saturated_count(), is_saturated_count, kIntNear);
 
       EXPECT_NEAR(test->analog_level_average(), analog_level_average, kIntNear);
       EXPECT_NEAR(test->max_output_average(),
                   max_output_average - kMaxOutputAverageOffset,
                   kMaxOutputAverageNear);
-
 #if defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE)

[peah-webrtc, 2016/05/20 12:21:23]

Why were only parts of the stats moved to test for each frame? Would it not be
possible to change to testing everything for each frame?

[minyue-webrtc, 2016/05/23 03:19:45]

Because has_echo_count, has_voice_count, analog_level_average etc. are per call.

[peah-webrtc, 2016/05/23 05:13:09]

Acknowledged.

-      audioproc::Test::EchoMetrics reference = test->echo_metrics();
-      TestStats(echo_metrics.residual_echo_return_loss,
-                reference.residual_echo_return_loss());
-      TestStats(echo_metrics.echo_return_loss,
-                reference.echo_return_loss());
-      TestStats(echo_metrics.echo_return_loss_enhancement,
-                reference.echo_return_loss_enhancement());
-      TestStats(echo_metrics.a_nlp,
-                reference.a_nlp());
-
       const double kFloatNear = 0.0005;
-      audioproc::Test::DelayMetrics reference_delay = test->delay_metrics();
-      EXPECT_NEAR(reference_delay.median(), median, kIntNear);
-      EXPECT_NEAR(reference_delay.std(), std, kIntNear);
-      EXPECT_NEAR(reference_delay.fraction_poor_delays(), fraction_poor_delays,
-                  kFloatNear);
-
-      EXPECT_NEAR(test->rms_level(), rms_level, kIntNear);
-
       EXPECT_NEAR(test->ns_speech_probability_average(),
                   ns_speech_prob_average,
                   kFloatNear);
 #endif
     } else {
       test->set_has_echo_count(has_echo_count);
       test->set_has_voice_count(has_voice_count);
       test->set_is_saturated_count(is_saturated_count);
 
       test->set_analog_level_average(analog_level_average);
       test->set_max_output_average(max_output_average);
 
 #if defined(WEBRTC_AUDIOPROC_FLOAT_PROFILE)
-      audioproc::Test::EchoMetrics* message = test->mutable_echo_metrics();
-      WriteStatsMessage(echo_metrics.residual_echo_return_loss,
-                        message->mutable_residual_echo_return_loss());
-      WriteStatsMessage(echo_metrics.echo_return_loss,
-                        message->mutable_echo_return_loss());
-      WriteStatsMessage(echo_metrics.echo_return_loss_enhancement,
-                        message->mutable_echo_return_loss_enhancement());
-      WriteStatsMessage(echo_metrics.a_nlp,
-                        message->mutable_a_nlp());
-
-      audioproc::Test::DelayMetrics* message_delay =
-          test->mutable_delay_metrics();
-      message_delay->set_median(median);
-      message_delay->set_std(std);
-      message_delay->set_fraction_poor_delays(fraction_poor_delays);
-
-      test->set_rms_level(rms_level);
-
       EXPECT_LE(0.0f, ns_speech_prob_average);
       EXPECT_GE(1.0f, ns_speech_prob_average);
       test->set_ns_speech_probability_average(ns_speech_prob_average);
 #endif
     }
 
     rewind(far_file_);
     rewind(near_file_);
   }
 
@@ skipping 489 matching lines @@
                     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, 35, 20),
                     std::tr1::make_tuple(16000, 16000, 16000, 16000, 0, 0)));
 #endif
 
 }  // namespace
 }  // namespace webrtc