Adding APM configuration in AEC dump.

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 1726 matching lines @@
            msg.num_output_channels(),
            msg.num_reverse_channels(),
            false);
       if (first_init) {
         // StartDebugRecording() writes an additional init message. Don't start
         // recording until after the first init to avoid the extra message.
         EXPECT_NOERR(apm_->StartDebugRecording(out_filename.c_str()));
         first_init = false;
       }
 
+    } else if (event_msg.type() == audioproc::Event::CONFIG) {

[Andrew MacDonald, 2015/09/26 02:36:20]

Correct me if I'm wrong, but because the ref03.aecdump won't contain this
message, this isn't testing anything, right?

Instead, add a new focused test where you set a few of these settings, start
debug recording, read the file and verify the config message contains what you
expect.

Too bad StartDebugRecording doesn't take some kind of abstract Writer class so
you wouldn't have to involve the file-system.

[minyue-webrtc, 2015/09/26 16:26:44]

ref03.aecdump doesn't contain Config message. But the test is valid. The test
does the following
1. takes ref03.aecdump and re-process it to ref_xxx, now it contains Config
2. use ref_xxx and re-process it to out_xxx, also contains Config
3. check if ref_xxx === out_xxx

So it checks both backward compatibility and new fields

[Andrew MacDonald, 2015/09/28 06:22:33]

OK, thanks. Still not totally convinced this is a great way to test this. All
components are enabled in this test, and if one of those fails to get logged,
this wouldn't catch it. Right?

Also, we want to remove all these run-time setters anyway. So this feels like a
step backwards.

[minyue-webrtc, 2015/09/29 04:51:49]

True, it does not test all combination of configs,which is an over-kill also.
I may do this
try many combinations offline (just need to change line 1883) , and maybe leave
a test or two in there.

+      const audioproc::Config msg = event_msg.config();
+      Config config;
+      // AEC (5)
+      if (msg.has_aec_enabled()) {
+        apm_->echo_cancellation()->Enable(msg.aec_enabled());
+      }
+      if (msg.has_aec_delay_agnostic()) {
+        config.Set<DelayAgnostic>(new DelayAgnostic(msg.aec_delay_agnostic()));
+      }
+      if (msg.has_aec_drift_compensation()) {
+        apm_->echo_cancellation()->enable_drift_compensation(
+            msg.aec_drift_compensation());
+      }
+      if (msg.has_aec_extended_filter()) {
+        config.Set<ExtendedFilter>(new ExtendedFilter(
+            msg.aec_extended_filter()));
+      }
+      if (msg.has_aec_suppression_level()) {
+        apm_->echo_cancellation()->set_suppression_level(
+            static_cast<webrtc::EchoCancellation::SuppressionLevel>(
+                msg.aec_suppression_level()));
+      }
+      // AEC-M (3)
+      if (msg.has_aecm_enabled()) {
+        apm_->echo_control_mobile()->Enable(msg.aecm_enabled());
+      }
+      if (msg.has_aecm_comfort_noise()) {
+        apm_->echo_control_mobile()->enable_comfort_noise(
+            msg.aecm_comfort_noise());
+      }
+      if (msg.has_aecm_routing_mode()) {
+        apm_->echo_control_mobile()->set_routing_mode(
+            static_cast<webrtc::EchoControlMobile::RoutingMode>(
+            msg.aecm_routing_mode()));
+      }
+      // AGC (4)
+      if (msg.has_agc_enabled()) {
+        apm_->gain_control()->Enable(msg.agc_enabled());
+      }
+      if (msg.has_agc_experiment()) {
+        // agc_experiment can only be set in the ctor of APM. So there should be
+        // nothing to do, unless the |apm_| is wrongly initialized.
+      }
+      if (msg.has_agc_mode()) {
+        apm_->gain_control()->set_mode(
+            static_cast<webrtc::GainControl::Mode>(msg.agc_mode()));
+      }
+      if (msg.has_agc_limiter()) {
+        apm_->gain_control()->enable_limiter(msg.agc_limiter());
+      }
+      // HPF (1)
+      if (msg.has_hpf_enabled()) {
+        apm_->high_pass_filter()->Enable(msg.hpf_enabled());
+      }
+      // NS (3)
+      if (msg.has_ns_enabled()) {
+        apm_->noise_suppression()->Enable(msg.ns_enabled());
+      }
+      if (msg.has_ns_experiment()) {
+        config.Set<ExperimentalNs>(new ExperimentalNs(msg.ns_experiment()));
+      }
+      if (msg.has_ns_level()) {
+        apm_->noise_suppression()->set_level(
+            static_cast<webrtc::NoiseSuppression::Level>(msg.ns_level()));
+      }
+      apm_->SetExtraOptions(config);

[Andrew MacDonald, 2015/09/26 02:36:20]

This API is deprecated. Don't add more calls to it.

[minyue-webrtc, 2015/09/26 16:26:44]

Ok, thanks for the info. But how should one set ns_experiment / da-aec /
aec-extended for now? 

[Andrew MacDonald, 2015/09/28 06:22:33]

Through Create().

[minyue-webrtc, 2015/09/29 04:51:49]

Yes I see, do you suggest recreate APM here?

[Andrew MacDonald, 2015/09/29 05:05:17]

No, I suggest not testing this feature here, as in the other comment.

[minyue-webrtc, 2015/09/29 21:05:05]

Ok. I'd remove these changes.

I will try to add some tests later, preferably in a separate CL. I'd do tests
(without submit) with these modifies offline anyway to verify the new fields.

[Andrew MacDonald, 2015/09/29 23:38:49]

Why in a separate CL? If you're going to add a unit test (and I think you
should) you should do it in this CL.

     } else if (event_msg.type() == audioproc::Event::REVERSE_STREAM) {
       const audioproc::ReverseStream msg = event_msg.reverse_stream();
 
       if (msg.channel_size() > 0) {
         ASSERT_EQ(revframe_->num_channels_, msg.channel_size());
         for (int i = 0; i < msg.channel_size(); ++i) {
            memcpy(revfloat_cb_->channels()[i],
                   msg.channel(i).data(),
                   msg.channel(i).size());
         }
@@ skipping 75 matching lines @@
     EXPECT_EQ(ref_size, out_size);
     EXPECT_EQ(0, memcmp(ref_bytes.get(), out_bytes.get(), ref_size));
     ref_size = ReadMessageBytesFromFile(ref_file, &ref_bytes);
     out_size = ReadMessageBytesFromFile(out_file, &out_bytes);
   }
   EXPECT_GT(bytes_read, 0u);
   EXPECT_NE(0, feof(ref_file));
   EXPECT_NE(0, feof(out_file));
   ASSERT_EQ(0, fclose(ref_file));
   ASSERT_EQ(0, fclose(out_file));
-  remove(ref_filename.c_str());
-  remove(out_filename.c_str());
+//  remove(ref_filename.c_str());

[ivoc, 2015/09/25 09:24:21]

Why is this commented out? I think it's needed, isn't it?

[minyue-webrtc, 2015/09/25 09:49:09]

sorry sorry, I needed they to help debugging. 

+//  remove(out_filename.c_str());
 }
 
 TEST_F(ApmTest, VerifyDebugDumpInt) {
   VerifyDebugDumpTest(kIntFormat);
 }
 
 TEST_F(ApmTest, VerifyDebugDumpFloat) {
   VerifyDebugDumpTest(kFloatFormat);
 }
 #endif
@@ skipping 894 matching lines @@
                     std::tr1::make_tuple(16000, 16000, 32000, 16000, 40, 20),
                     std::tr1::make_tuple(16000, 16000, 16000, 16000, 0, 0)));
 #endif
 
 // TODO(henrike): re-implement functionality lost when removing the old main
 //                function. See
 //                https://code.google.com/p/webrtc/issues/detail?id=1981
 
 }  // namespace
 }  // namespace webrtc