Adding APM configuration in AEC dump.

webrtc/modules/audio_processing/audio_processing_impl.cc

Index: webrtc/modules/audio_processing/audio_processing_impl.cc
diff --git a/webrtc/modules/audio_processing/audio_processing_impl.cc b/webrtc/modules/audio_processing/audio_processing_impl.cc
index 4ef4e6da6a307e6533e8ac9ddf39e888c2c52713..9d0632649f49c9b99cfe9897271e271a809ea9bf 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -42,15 +42,6 @@ extern "C" {
 #include "webrtc/system_wrappers/interface/logging.h"
 #include "webrtc/system_wrappers/interface/metrics.h"
 
-#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP

[Andrew MacDonald, 2015/10/01 06:33:28]

Why was this removed?

[minyue-webrtc, 2015/10/01 20:18:26]

This is moved to .h since audioproc::Config is needed there.

-// Files generated at build-time by the protobuf compiler.
-#ifdef WEBRTC_ANDROID_PLATFORM_BUILD
-#include "external/webrtc/webrtc/modules/audio_processing/debug.pb.h"
-#else
-#include "webrtc/audio_processing/debug.pb.h"
-#endif
-#endif  // WEBRTC_AUDIOPROC_DEBUG_DUMP
-
 #define RETURN_ON_ERR(expr) \
   do {                      \
     int err = (expr);       \
@@ -59,6 +50,15 @@ extern "C" {
     }                       \
   } while (0)
 
+#define UPDATE_CONFIG(field_name, new_value) \
+  { \
+    const auto value = new_value; \
+    if (!config_->has_##field_name() || config_->field_name() != value) { \
+      config_->set_##field_name(value); \
+      changed = true; \
+    } \
+  }
+
 namespace webrtc {
 namespace {
 
@@ -194,6 +194,7 @@ AudioProcessingImpl::AudioProcessingImpl(const Config& config,
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
       debug_file_(FileWrapper::Create()),
       event_msg_(new audioproc::Event()),
+      config_(new audioproc::Config()),
 #endif
       api_format_({{{kSampleRate16kHz, 1, false},
                     {kSampleRate16kHz, 1, false},
@@ -250,6 +251,10 @@ AudioProcessingImpl::AudioProcessingImpl(const Config& config,
   gain_control_for_new_agc_.reset(new GainControlForNewAgc(gain_control_));
 
   SetExtraOptions(config);
+
+#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
+  UpdateCurrentConfig();
+#endif
 }
 
 AudioProcessingImpl::~AudioProcessingImpl() {
@@ -560,6 +565,12 @@ int AudioProcessingImpl::ProcessStream(const float* const* src,
          api_format_.input_stream().num_frames());
 
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
+  if (debug_file_->Open() && UpdateCurrentConfig()) {
+    RETURN_ON_ERR(WriteConfigMessage());
+  }
+#endif

[Andrew MacDonald, 2015/10/01 06:33:28]

Combine these #ifdef blocks.

[minyue-webrtc, 2015/10/01 20:18:26]

Done.

+
+#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   if (debug_file_->Open()) {
     event_msg_->set_type(audioproc::Event::STREAM);
     audioproc::Stream* msg = event_msg_->mutable_stream();
@@ -946,7 +957,13 @@ int AudioProcessingImpl::StartDebugRecording(
     return kFileError;
   }
 
-  int err = WriteInitMessage();
+  UpdateCurrentConfig();
+  int err = WriteConfigMessage();
+  if (err != kNoError) {
+    return err;
+  }
+
+  err = WriteInitMessage();
   if (err != kNoError) {
     return err;
   }
@@ -975,7 +992,13 @@ int AudioProcessingImpl::StartDebugRecording(FILE* handle) {
     return kFileError;
   }
 
-  int err = WriteInitMessage();
+  UpdateCurrentConfig();
+  int err = WriteConfigMessage();
+  if (err != kNoError) {
+    return err;
+  }
+
+  err = WriteInitMessage();
   if (err != kNoError) {
     return err;
   }
@@ -1255,6 +1278,52 @@ int AudioProcessingImpl::WriteInitMessage() {
 
   return kNoError;
 }
+
+bool AudioProcessingImpl::UpdateCurrentConfig() {

[Andrew MacDonald, 2015/10/01 06:33:28]

I wonder if rather than checking every individual field, it would be cleaner to
generate the message, serialize it to a string and check if that was different
than the existing serialization. You might have to rework how the messages get
written.

[minyue-webrtc, 2015/10/01 20:18:26]

I did a similar trial (not a string compare, but a struct compare) before patch
set 4. But it feels that an additional struct is redundant, since
audioproc::Config is a good container of all configs that need to be stored.

The reason of not generating a temp audioproc::Config and compare it with the
current audioproc::Config is
1) there is no "==" defined, we need to define it;
2) We omit memory usage of a temp audioproc::Config

Or do you suggest dropping protobuf for Config at all? Then do we need to invent
a new dump format?

[Andrew MacDonald, 2015/10/02 02:15:19]

I wouldn't worry about that, as it would only be used when recording aecdumps.
No. I meant serializing to a string and comparing the strings. Defining an
operator== for audioproc::Config is an interesting option, but I think we'd have
problems keeping it updated.

[minyue-webrtc, 2015/10/02 05:29:01]

I still have some questions on this. Do you mean

class AudioProcessingImpl {
  private:
    std::string config_;
}

std::string current_config = GetCurrentConfig();  // serialize all configs to a
string.
if (strcmp(current_config, config_) != 0) {
  WriteConfigMessage();
  config_ = current_config;
}

If this, we may need to check through all configs twice, one in GetCurrentConfig
and one in WriteConfigMessage. The cumbersomeness is similar to Patch set 3.

Or do you even mean to write the string as a message, i.e., doing, in protobuf
message Config {
  optional string all_configs;
}
Then what is benefit of protobuf, we need to write encode and decode.

[Andrew MacDonald, 2015/10/02 06:07:39]

To be clear, I mean this:
https://codereview.webrtc.org/1384653002/
(based on your CL, and essentially the same as the first snippet you posted)

Yes, it serializes Config twice, but since this is only enabled during debug
recordings, I'm not that worried.

+  bool changed = false;
+
+  // Acoustic echo canceler.
+  UPDATE_CONFIG(aec_enabled, echo_cancellation_->is_enabled());

[peah-webrtc, 2015/09/30 13:13:54]

Sorry, missed to send this before, had a draft that was never sent. 
Sending it now instead.
I'd prefer a function instead of a macro in this case. Using a macro instead of
a function "only" saves 2 parameters. Furthermore, it optionally allows a more
descriptive printout than rather just the field name. The style guide also also
cautious about macros
(http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Preprocessor_...)

If you want to have a macro anyway, I'm fine with that as well though.

[minyue-webrtc, 2015/09/30 18:44:52]

I would like to use function but it does not seem to be as convenient. The
config field name determines the function name (see the string concatenation ##
in the macro). Is there a better way you may suggest me to maintain this
convenience of macro?

[peah-webrtc, 2015/10/01 05:31:31]

I'd use a function instead with more parameters.

The only benefit I see is that the macro implementation is the convenience of
requiring 2 parameters instead of the 3 (or 4) that a function implementation
would. Personally, I don't think that warrants the cost in readability and
debuggability that a macro incurs.

[Andrew MacDonald, 2015/10/01 06:33:28]

Agree with Per, I probably would not use a macro for this.

[kwiberg-webrtc, 2015/10/01 08:26:22]

A good compromise might be to implement almost all of it as a function with 3-4
arguments, and have a macro with 2 arguments whose only job is to magick up the
1-2 extra parameters and call the function.

[minyue-webrtc, 2015/10/01 20:18:26]

It is rather complicated to use inline function, since it needs a callback to a
non-static function, "set_xxx".

Plus the fact that if we do not take the advantage of ## in macro, the benefit
of inline function is quite minor, I suggest just do a plain coding. See the new
patch.

+  UPDATE_CONFIG(aec_delay_agnostic,
+                echo_cancellation_->is_delay_agnostic_enabled());
+  UPDATE_CONFIG(aec_drift_compensation,
+                echo_cancellation_->is_drift_compensation_enabled());
+  UPDATE_CONFIG(aec_extended_filter,
+                echo_cancellation_->is_extended_filter_enabled());
+  UPDATE_CONFIG(aec_suppression_level,
+                static_cast<int>(echo_cancellation_->suppression_level()));
+  // Mobile AEC.
+  UPDATE_CONFIG(aecm_enabled, echo_control_mobile_->is_enabled());
+  UPDATE_CONFIG(aecm_comfort_noise,
+                echo_control_mobile_->is_comfort_noise_enabled());
+  UPDATE_CONFIG(aecm_routing_mode,
+                static_cast<int>(echo_control_mobile_->routing_mode()));
+  // Automatic gain controller.
+  UPDATE_CONFIG(agc_enabled, gain_control_->is_enabled());
+  UPDATE_CONFIG(agc_experiment, use_new_agc_);
+  UPDATE_CONFIG(agc_mode, static_cast<int>(gain_control_->mode()));
+  UPDATE_CONFIG(agc_limiter, gain_control_->is_limiter_enabled());
+  // High pass filter.
+  UPDATE_CONFIG(hpf_enabled, high_pass_filter_->is_enabled());
+  // Noise suppression.
+  UPDATE_CONFIG(ns_enabled, noise_suppression_->is_enabled());
+  UPDATE_CONFIG(ns_experiment, transient_suppressor_enabled_);
+  UPDATE_CONFIG(ns_level, static_cast<int>(noise_suppression_->level()));
+
+  return changed;
+}
+
+int AudioProcessingImpl::WriteConfigMessage() {
+  event_msg_->set_type(audioproc::Event::CONFIG);
+  event_msg_->mutable_config()->CopyFrom(*config_);
+
+  int err = WriteMessageToDebugFile();
+  if (err != kNoError) {
+    return err;
+  }
+
+  return kNoError;
+}
 #endif  // WEBRTC_AUDIOPROC_DEBUG_DUMP
 
 }  // namespace webrtc