AecDump interface

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 900a8ec437abdf069ec29e02afe678ea7fb939c5..cd244cd8385de0907921d74d74545a25b7f5017c 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -155,6 +155,23 @@ class HighPassFilterImpl : public HighPassFilter {
   RTC_DISALLOW_IMPLICIT_CONSTRUCTORS(HighPassFilterImpl);
 };
 
+webrtc::InternalAPMStreamsConfig ToStreamsConfig(
+    const ProcessingConfig& api_format) {
+  webrtc::InternalAPMStreamsConfig result;
+  result.input_sample_rate = api_format.input_stream().sample_rate_hz();
+  result.input_num_channels = api_format.input_stream().num_channels();
+  result.output_num_channels = api_format.output_stream().num_channels();
+  result.render_input_num_channels =
+      api_format.reverse_input_stream().num_channels();
+  result.render_input_sample_rate =
+      api_format.reverse_input_stream().sample_rate_hz();
+  result.output_sample_rate = api_format.output_stream().sample_rate_hz();
+  result.render_output_sample_rate =
+      api_format.reverse_output_stream().sample_rate_hz();
+  result.render_output_num_channels =
+      api_format.reverse_output_stream().num_channels();
+  return result;
+}
 }  // namespace
 
 // Throughout webrtc, it's assumed that success is represented by zero.
@@ -529,7 +546,9 @@ int AudioProcessingImpl::InitializeLocked() {
     }
   }
 #endif
-
+  if (aec_dump_) {
+    aec_dump_->WriteInitMessage(ToStreamsConfig(formats_.api_format));
+  }
   return kNoError;
 }
 
@@ -830,6 +849,10 @@ int AudioProcessingImpl::ProcessStream(const float* const* src,
   }
 #endif
 
+  if (aec_dump_) {
+    RecordUnprocessedCaptureStream(src);
+  }
+
   capture_.capture_audio->CopyFrom(src, formats_.api_format.input_stream());
   RETURN_ON_ERR(ProcessCaptureStreamLocked());
   capture_.capture_audio->CopyTo(formats_.api_format.output_stream(), dest);
@@ -847,7 +870,9 @@ int AudioProcessingImpl::ProcessStream(const float* const* src,
                                           &crit_debug_, &debug_dump_.capture));
   }
 #endif
-
+  if (aec_dump_) {
+    RecordProcessedCaptureStream(dest);
+  }
   return kNoError;
 }
 
@@ -1086,6 +1111,10 @@ int AudioProcessingImpl::ProcessStream(AudioFrame* frame) {
     return kBadDataLengthError;
   }
 
+  if (aec_dump_) {
+    RecordUnprocessedCaptureStream(*frame);
+  }
+
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   if (debug_dump_.debug_file->is_open()) {
     RETURN_ON_ERR(WriteConfigMessage(false));
@@ -1103,6 +1132,9 @@ int AudioProcessingImpl::ProcessStream(AudioFrame* frame) {
   capture_.capture_audio->InterleaveTo(
       frame, submodule_states_.CaptureMultiBandProcessingActive());
 
+  if (aec_dump_) {
+    RecordProcessedCaptureStream(*frame);
+  }
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   if (debug_dump_.debug_file->is_open()) {
     audioproc::Stream* msg = debug_dump_.capture.event_msg->mutable_stream();
@@ -1384,7 +1416,14 @@ int AudioProcessingImpl::AnalyzeReverseStreamLocked(
                                           &crit_debug_, &debug_dump_.render));
   }
 #endif
-
+  if (aec_dump_) {
+    const size_t channel_size =
+        formats_.api_format.reverse_input_stream().num_frames();
+    const size_t num_channels =
+        formats_.api_format.reverse_input_stream().num_channels();
+    aec_dump_->WriteRenderStreamMessage(
+        FloatAudioFrame(src, num_channels, channel_size));
+  }
   render_.render_audio->CopyFrom(src,
                                  formats_.api_format.reverse_input_stream());
   return ProcessRenderStreamLocked();
@@ -1437,6 +1476,10 @@ int AudioProcessingImpl::ProcessReverseStream(AudioFrame* frame) {
                                           &crit_debug_, &debug_dump_.render));
   }
 #endif
+  if (aec_dump_) {
+    aec_dump_->WriteRenderStreamMessage(*frame);
+  }
+
   render_.render_audio->DeinterleaveFrom(frame);
   RETURN_ON_ERR(ProcessRenderStreamLocked());
   render_.render_audio->InterleaveTo(
@@ -1523,6 +1566,31 @@ int AudioProcessingImpl::delay_offset_ms() const {
   return capture_.delay_offset_ms;
 }
 
+void AudioProcessingImpl::AttachAecDump(std::unique_ptr<AecDump> aec_dump) {
+  std::unique_ptr<AecDump> old_aec_dump;
+  {
+    rtc::CritScope cs_render(&crit_render_);
+    rtc::CritScope cs_capture(&crit_capture_);
+    old_aec_dump = std::move(aec_dump_);

[the sun, 2017/05/22 22:32:55]

We don't have to use std::move for everything.

You could instead do:

RTC_DCHECK(aec_dump);
[claim locks]
aec_dump_.swap(aec_dump);
aec_dump_->WriteConfig(...

and avoid the extra scope and moves.

[aleloi, 2017/05/23 10:39:04]

Thanks!

I was worried the destructor of the previously attached AecDump would be called
while holding the locks. Hence the extra scope. With the new version it's called
when the 'aec_dump' parameter is destroyed. That happens after the function
exits and after the locks are released.

Standard quote (quoted from SO thread
https://stackoverflow.com/questions/2506793): 

Temporary objects are destroyed as the last step in evaluating the
full-expression that (lexically) contains the point where they were created.

The 'aec_dump' parameter is a temporary object. In a call

apm->AttachAecDump(aec_dump);

'aec_dump' with the previously attached dump is destroyed at the last ')', which
is after locks are released.

Sorry for wall of text. I wanted to convince myself that the code still does the
same thing.

+    RTC_DCHECK(aec_dump);
+    aec_dump_ = std::move(aec_dump);
+    aec_dump_->WriteConfig(CollectApmConfig(), true);
+    aec_dump_->WriteInitMessage(ToStreamsConfig(formats_.api_format));
+  }
+}
+
+void AudioProcessingImpl::DetachAecDump() {
+  // The d-tor of a task-queue based AecDump blocks until all pending
+  // tasks are done. This construction avoids blocking while holding
+  // the render and capture locks.
+  std::unique_ptr<AecDump> aec_dump = nullptr;
+  {
+    rtc::CritScope cs_render(&crit_render_);
+    rtc::CritScope cs_capture(&crit_capture_);
+    aec_dump = std::move(aec_dump_);

[the sun, 2017/05/22 22:32:56]

If you use
aec_dump_.reset(nullptr);
you don't need the extra scope and the local unique_ptr.

[aleloi, 2017/05/23 10:39:04]

Then the AecDump d-tor would be called during the 'reset' call, which is while
holding the locks.

We could make by without the extra scope by relying on destroying variables in
reverse order of initialization. This way:

Detach() {
  aec_dump = nullptr;
  lock1 = ...;
  lock2 = ...;
  std::swap(aec_dump_, aec_dump); // or move, which I think is more explicit

  // automatically call local var d-tors in reverse order of initialization:
  // 1. lock2 released
  // 2. lock1 released
  // 3. previously attached aec_dump destroyed
}

I think that either construction is fine, but that the one in the current PS
(with creating an extra scope) is more explicit.

+  }
+}
+
 int AudioProcessingImpl::StartDebugRecording(
     const char filename[AudioProcessing::kMaxFilenameSize],
     int64_t max_log_size_bytes) {
@@ -1594,11 +1662,12 @@ int AudioProcessingImpl::StartDebugRecordingForPlatformFile(
 }
 
 int AudioProcessingImpl::StopDebugRecording() {
+  DetachAecDump();
+
+#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   // Run in a single-threaded manner.
   rtc::CritScope cs_render(&crit_render_);
   rtc::CritScope cs_capture(&crit_capture_);
-
-#ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   // We just return if recording hasn't started.
   debug_dump_.debug_file->CloseFile();
   return kNoError;
@@ -1848,6 +1917,113 @@ void AudioProcessingImpl::UpdateHistogramsOnCallEnd() {
   capture_.last_aec_system_delay_ms = 0;
 }
 
+InternalAPMConfig AudioProcessingImpl::CollectApmConfig() const {
+  std::string experiments_description =
+      public_submodules_->echo_cancellation->GetExperimentsDescription();
+  // TODO(peah): Add semicolon-separated concatenations of experiment
+  // descriptions for other submodules.
+  if (capture_nonlocked_.level_controller_enabled) {
+    experiments_description += "LevelController;";
+  }
+  if (constants_.agc_clipped_level_min != kClippedLevelMin) {
+    experiments_description += "AgcClippingLevelExperiment;";
+  }
+  if (capture_nonlocked_.echo_canceller3_enabled) {
+    experiments_description += "EchoCanceller3;";
+  }
+
+  InternalAPMConfig apm_config;
+
+  apm_config.aec_enabled = public_submodules_->echo_cancellation->is_enabled();
+  apm_config.aec_delay_agnostic_enabled =
+      public_submodules_->echo_cancellation->is_delay_agnostic_enabled();
+  apm_config.aec_drift_compensation_enabled =
+      public_submodules_->echo_cancellation->is_drift_compensation_enabled();
+  apm_config.aec_extended_filter_enabled =
+      public_submodules_->echo_cancellation->is_extended_filter_enabled();
+  apm_config.aec_suppression_level = static_cast<int>(
+      public_submodules_->echo_cancellation->suppression_level());
+
+  apm_config.aecm_enabled =
+      public_submodules_->echo_control_mobile->is_enabled();
+  apm_config.aecm_comfort_noise_enabled =
+      public_submodules_->echo_control_mobile->is_comfort_noise_enabled();
+  apm_config.aecm_routing_mode =
+      static_cast<int>(public_submodules_->echo_control_mobile->routing_mode());
+
+  apm_config.agc_enabled = public_submodules_->gain_control->is_enabled();
+  apm_config.agc_mode =
+      static_cast<int>(public_submodules_->gain_control->mode());
+  apm_config.agc_limiter_enabled =
+      public_submodules_->gain_control->is_limiter_enabled();
+  apm_config.noise_robust_agc_enabled = constants_.use_experimental_agc;
+
+  apm_config.hpf_enabled = config_.high_pass_filter.enabled;
+
+  apm_config.ns_enabled = public_submodules_->noise_suppression->is_enabled();
+  apm_config.ns_level =
+      static_cast<int>(public_submodules_->noise_suppression->level());
+
+  apm_config.transient_suppression_enabled =
+      capture_.transient_suppressor_enabled;
+  apm_config.intelligibility_enhancer_enabled =
+      capture_nonlocked_.intelligibility_enabled;
+  apm_config.experiments_description = experiments_description;
+  return apm_config;
+}
+
+void AudioProcessingImpl::RecordUnprocessedCaptureStream(
+    const float* const* src) {
+  RTC_DCHECK(aec_dump_);
+  aec_dump_->WriteConfig(CollectApmConfig(), false);

[the sun, 2017/05/22 22:32:55]

Why do we always write the config?

[peah-webrtc, 2017/05/23 06:15:28]

That is because of the APM API which allows access to submodules. In order to be
able to store changes of settings, we need to poll the config for changes, and
store them if there are any. The "false" means that the storing is only done if
there are changes.

+
+  const size_t channel_size = formats_.api_format.input_stream().num_frames();
+  const size_t num_channels = formats_.api_format.input_stream().num_channels();
+  aec_dump_->AddCaptureStreamInput(
+      FloatAudioFrame(src, num_channels, channel_size));
+  RecordAudioProcessingState();
+}
+
+void AudioProcessingImpl::RecordUnprocessedCaptureStream(
+    const AudioFrame& capture_frame) {
+  RTC_DCHECK(aec_dump_);
+
+  aec_dump_->AddCaptureStreamInput(capture_frame);
+  RecordAudioProcessingState();
+  aec_dump_->WriteConfig(CollectApmConfig(), false);

[the sun, 2017/05/22 22:32:56]

Why is the config written after stream input in this variant, but not in the one
above?

[aleloi, 2017/05/23 10:39:04]

Fixed, thanks! This doesn't change the order in which the pb objects are written
to file. It's still CONFIG then STREAM, because the STREAM is written in
RecordProcessedCaptureStream(), which happens after this method is called.

+}
+
+void AudioProcessingImpl::RecordProcessedCaptureStream(
+    const float* const* processed_capture_stream) {
+  RTC_DCHECK(aec_dump_);
+
+  const size_t channel_size = formats_.api_format.output_stream().num_frames();
+  const size_t num_channels =
+      formats_.api_format.output_stream().num_channels();
+  aec_dump_->AddCaptureStreamOutput(
+      FloatAudioFrame(processed_capture_stream, num_channels, channel_size));
+  aec_dump_->WriteCaptureStreamMessage();
+}
+
+void AudioProcessingImpl::RecordProcessedCaptureStream(
+    const AudioFrame& processed_capture_frame) {
+  RTC_DCHECK(aec_dump_);
+
+  aec_dump_->AddCaptureStreamOutput(processed_capture_frame);
+  aec_dump_->WriteCaptureStreamMessage();
+}
+
+void AudioProcessingImpl::RecordAudioProcessingState() {
+  RTC_DCHECK(aec_dump_);
+  AecDump::AudioProcessingState audio_proc_state;
+  audio_proc_state.delay = capture_nonlocked_.stream_delay_ms;
+  audio_proc_state.drift =
+      public_submodules_->echo_cancellation->stream_drift_samples();
+  audio_proc_state.level = gain_control()->stream_analog_level();
+  audio_proc_state.keypress = capture_.key_pressed;
+  aec_dump_->AddAudioProcessingState(audio_proc_state);
+}
+
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
 int AudioProcessingImpl::WriteMessageToDebugFile(
     FileWrapper* debug_file,