Adding APM configuration in AEC dump.

webrtc/modules/audio_processing/audio_processing_impl.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 24 matching lines @@
 #include "webrtc/modules/audio_processing/noise_suppression_impl.h"
 #include "webrtc/modules/audio_processing/processing_component.h"
 #include "webrtc/modules/audio_processing/transient/transient_suppressor.h"
 #include "webrtc/modules/audio_processing/voice_detection_impl.h"
 #include "webrtc/modules/interface/module_common_types.h"
 #include "webrtc/system_wrappers/interface/critical_section_wrapper.h"
 #include "webrtc/system_wrappers/interface/file_wrapper.h"
 #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);       \
     if (err != kNoError) {  \
       return err;           \
     }                       \
   } 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 {
 
 static bool LayoutHasKeyboard(AudioProcessing::ChannelLayout layout) {
   switch (layout) {
     case AudioProcessing::kMono:
     case AudioProcessing::kStereo:
       return false;
     case AudioProcessing::kMonoAndKeyboard:
     case AudioProcessing::kStereoAndKeyboard:
@@ skipping 115 matching lines @@
       echo_control_mobile_(NULL),
       gain_control_(NULL),
       high_pass_filter_(NULL),
       level_estimator_(NULL),
       noise_suppression_(NULL),
       voice_detection_(NULL),
       crit_(CriticalSectionWrapper::CreateCriticalSection()),
 #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},
                     {kSampleRate16kHz, 1, false},
                     {kSampleRate16kHz, 1, false}}}),
       fwd_proc_format_(kSampleRate16kHz),
       rev_proc_format_(kSampleRate16kHz, 1),
       split_rate_(kSampleRate16kHz),
       stream_delay_ms_(0),
       delay_offset_ms_(0),
@@ skipping 36 matching lines @@
 
   noise_suppression_ = new NoiseSuppressionImpl(this, crit_);
   component_list_.push_back(noise_suppression_);
 
   voice_detection_ = new VoiceDetectionImpl(this, crit_);
   component_list_.push_back(voice_detection_);
 
   gain_control_for_new_agc_.reset(new GainControlForNewAgc(gain_control_));
 
   SetExtraOptions(config);
+
+#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
+  UpdateCurrentConfig();
+#endif
 }
 
 AudioProcessingImpl::~AudioProcessingImpl() {
   {
     CriticalSectionScoped crit_scoped(crit_);
     // Depends on gain_control_ and gain_control_for_new_agc_.
     agc_manager_.reset();
     // Depends on gain_control_.
     gain_control_for_new_agc_.reset();
     while (!component_list_.empty()) {
@@ skipping 141 matching lines @@
     return kBadNumberChannelsError;
   }
 
   api_format_ = config;
 
   // We process at the closest native rate >= min(input rate, output rate)...
   const int min_proc_rate =
       std::min(api_format_.input_stream().sample_rate_hz(),
                api_format_.output_stream().sample_rate_hz());
   int fwd_proc_rate;
   for (size_t i = 0; i < kNumNativeSampleRates; ++i) {

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

oh, this must come from rebasing. Sorry, I worked on a different computer and
could not separate this out.

     fwd_proc_rate = kNativeSampleRatesHz[i];
     if (fwd_proc_rate >= min_proc_rate) {
       break;
     }
   }
   // ...with one exception.
   if (echo_control_mobile_->is_enabled() &&

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

rebase

       min_proc_rate > kMaxAECMSampleRateHz) {
     fwd_proc_rate = kMaxAECMSampleRateHz;
   }
 
   fwd_proc_format_ = StreamConfig(fwd_proc_rate);
 
   // We normally process the reverse stream at 16 kHz. Unless...
   int rev_proc_rate = kSampleRate16kHz;
   if (fwd_proc_format_.sample_rate_hz() == kSampleRate8kHz) {
     // ...the forward stream is at 8 kHz.
@@ skipping 121 matching lines @@
 
   ProcessingConfig processing_config = api_format_;
   processing_config.input_stream() = input_config;
   processing_config.output_stream() = output_config;
 
   RETURN_ON_ERR(MaybeInitializeLocked(processing_config));
   assert(processing_config.input_stream().num_frames() ==
          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();
     const size_t channel_size =
         sizeof(float) * api_format_.input_stream().num_frames();
     for (int i = 0; i < api_format_.input_stream().num_channels(); ++i)
       msg->add_input_channel(src[i], channel_size);
   }
 #endif
 
@@ skipping 21 matching lines @@
     return kNullPointerError;
   }
   // Must be a native rate.
   if (frame->sample_rate_hz_ != kSampleRate8kHz &&
       frame->sample_rate_hz_ != kSampleRate16kHz &&
       frame->sample_rate_hz_ != kSampleRate32kHz &&
       frame->sample_rate_hz_ != kSampleRate48kHz) {
     return kBadSampleRateError;
   }
   if (echo_control_mobile_->is_enabled() &&
       frame->sample_rate_hz_ > kMaxAECMSampleRateHz) {

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

rebase

     LOG(LS_ERROR) << "AECM only supports 16 or 8 kHz sample rates";
     return kUnsupportedComponentError;
   }
 
   // TODO(ajm): The input and output rates and channels are currently
   // constrained to be identical in the int16 interface.
   ProcessingConfig processing_config = api_format_;
   processing_config.input_stream().set_sample_rate_hz(frame->sample_rate_hz_);
   processing_config.input_stream().set_num_channels(frame->num_channels_);
   processing_config.output_stream().set_sample_rate_hz(frame->sample_rate_hz_);
@@ skipping 324 matching lines @@
     if (debug_file_->CloseFile() == -1) {
       return kFileError;
     }
   }
 
   if (debug_file_->OpenFile(filename, false) == -1) {
     debug_file_->CloseFile();
     return kFileError;
   }
 
-  int err = WriteInitMessage();
+  UpdateCurrentConfig();
+  int err = WriteConfigMessage();
   if (err != kNoError) {
     return err;
   }
+
+  err = WriteInitMessage();
+  if (err != kNoError) {
+    return err;
+  }
   return kNoError;
 #else
   return kUnsupportedFunctionError;
 #endif  // WEBRTC_AUDIOPROC_DEBUG_DUMP
 }
 
 int AudioProcessingImpl::StartDebugRecording(FILE* handle) {
   CriticalSectionScoped crit_scoped(crit_);
 
   if (handle == NULL) {
     return kNullPointerError;
   }
 
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   // Stop any ongoing recording.
   if (debug_file_->Open()) {
     if (debug_file_->CloseFile() == -1) {
       return kFileError;
     }
   }
 
   if (debug_file_->OpenFromFileHandle(handle, true, false) == -1) {
     return kFileError;
   }
 
-  int err = WriteInitMessage();
+  UpdateCurrentConfig();
+  int err = WriteConfigMessage();
   if (err != kNoError) {
     return err;
   }
+
+  err = WriteInitMessage();
+  if (err != kNoError) {
+    return err;
+  }
   return kNoError;
 #else
   return kUnsupportedFunctionError;
 #endif  // WEBRTC_AUDIOPROC_DEBUG_DUMP
 }
 
 int AudioProcessingImpl::StartDebugRecordingForPlatformFile(
     rtc::PlatformFile handle) {
   FILE* stream = rtc::FdopenPlatformFileForWriting(handle);
   return StartDebugRecording(stream);
@@ skipping 256 matching lines @@
   msg->set_output_sample_rate(api_format_.output_stream().sample_rate_hz());
   // TODO(ekmeyerson): Add reverse output fields to event_msg_.
 
   int err = WriteMessageToDebugFile();
   if (err != kNoError) {
     return err;
   }
 
   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