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 1f73c5984a68af29d7dd941a64f88a834fd9e72a..1221140953014ed2ae02aafd0b951b653697e8e9 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -21,6 +21,7 @@
 #include "webrtc/common_audio/signal_processing/include/signal_processing_library.h"
 #include "webrtc/modules/audio_processing/aec/aec_core.h"
 #include "webrtc/modules/audio_processing/aec3/echo_canceller3.h"
+#include "webrtc/modules/audio_processing/aec_dumper/null_aec_dumper.h"
 #include "webrtc/modules/audio_processing/agc/agc_manager_direct.h"
 #include "webrtc/modules/audio_processing/audio_buffer.h"
 #include "webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.h"
@@ -305,6 +306,7 @@ AudioProcessingImpl::AudioProcessingImpl(const webrtc::Config& config)
 AudioProcessingImpl::AudioProcessingImpl(const webrtc::Config& config,
                                          NonlinearBeamformer* beamformer)
     : high_pass_filter_impl_(new HighPassFilterImpl(this)),
+      aec_dumper_(AecDumper::CreateNullDumper()),

[peah-webrtc, 2017/03/31 07:24:43]

Why cannot we use an empty aecdumper?

[aleloi, 2017/04/06 15:46:11]

It's possible. Then we would have to add 'if (aec_dumper_)' checks everywhere.
This way there is an invariant: aec_dumper_ is never empty. There are no special
cases and the code is slightly simpler in this file.

[peah-webrtc, 2017/04/07 12:57:15]

Absolutely, I definitely like the simpler code. But is comes at quite a large
cost.

[aleloi, 2017/04/12 11:05:29]

I checked: virtual calls are always generated in release-mode, even when
rtc_enable_protobuf is false. Now null dump is replaced with nullptr.

       public_submodules_(new ApmPublicSubmodules()),
       private_submodules_(new ApmPrivateSubmodules(beamformer)),
       constants_(config.Get<ExperimentalAgc>().startup_min_volume,
@@ -525,6 +527,7 @@ int AudioProcessingImpl::InitializeLocked() {
     }
   }
 #endif
+  aec_dumper_->WriteInitMessage(formats_.api_format);
 
   return kNoError;
 }
@@ -815,7 +818,31 @@ int AudioProcessingImpl::ProcessStream(const float* const* src,
   }
 #endif
 
+  std::unique_ptr<AecDumper::CaptureStreamInfo> stream_info =
+      aec_dumper_->GetCaptureStreamInfo();
+  const size_t channel_size =
+      sizeof(float) * formats_.api_format.input_stream().num_frames();
+
+  {
+    std::vector<rtc::ArrayView<const float>> src_view;

[peah-webrtc, 2017/03/31 07:24:43]

Afaics, the emplace_back call will cause arrayview objects to dynamically be
created in the vector, right? Furthermore, the vector may dynamically change
it's size during the loop. On top of that, line 1418 will dynamically allocate
the vector data.

Is this really necessary? That is a lot of dynamic memory allocation for
something that we basically already know the maximum size of. Is there no way
that we can use a stack array of a maximum size for this?

[aleloi, 2017/04/06 15:46:11]

We could use a static array of large enough size. Or reserve vector size in
declaration of src_view.

I profiled (link below) and found that this part doesn't have a noticeable
impact on performance. Perhaps leave it as simple and readable as possible

Profiling report: no aec processing but resampling and aec-dump recording
https://drive.google.com/open?id=0B0ohpnzd-DTsVHBta0t6UGJObTQ

[peah-webrtc, 2017/04/07 12:57:15]

Yes, the impact of this is definitely not noticeable. I still think it makes
sense to as a rule not add dynamic allocations if it is not needed. 
Some sources state that a heap allocation costs about 200-500 cycles, compared
to 0 cycles for an array. I'm not sure about the cost. However, in this case we
actually know the number of channels, as well as the maximum possible number of
channels so any cost required for this is actually not necessary.

+    for (size_t i = 0; i < formats_.api_format.input_stream().num_channels();
+         ++i) {
+      src_view.emplace_back(src[i], channel_size);
+    }
+    stream_info->AddInput(src_view);
+  }
   capture_.capture_audio->CopyFrom(src, formats_.api_format.input_stream());
+
+  // This earlier happened in ProcessCaptureStreamLocked().

[peah-webrtc, 2017/03/31 07:24:43]

Please remove this comment. This is more of a comment to the reviewers, and I
don't think it will make any sense to people later reading the code.

[aleloi, 2017/04/06 15:46:11]

Done.

+  RTC_DCHECK(!(public_submodules_->echo_cancellation->is_enabled() &&
+               public_submodules_->echo_control_mobile->is_enabled()));

[peah-webrtc, 2017/03/31 07:24:43]

Why were there DCHECKs moved to here?

[aleloi, 2017/04/06 15:46:11]

The conditions were DCHECKed before the protobuf stream object was filled with
delay, drift, etc. That was done in ProcessCaptureStreamLocked. To avoid sending
the CaptureStreamInfo object to ProcessCaptureStreamLocked, it was moved here.
The DCHECKS were copied here to ensure everything is executed in the same order
as before this change. I think it should be safe to leave them in
ProcessCaptureStreamLocked().

[peah-webrtc, 2017/04/07 12:57:15]

I agree, it should be safe to leave them there. In case there is a DCHECK, the
recording will regardless be halted and incomplete. Having one or more objects
sent for recording does not matter in that case.

Please move these back to where they were.

+
+  stream_info->set_delay(capture_nonlocked_.stream_delay_ms);
+  stream_info->set_drift(
+      public_submodules_->echo_cancellation->stream_drift_samples());
+  stream_info->set_level(gain_control()->stream_analog_level());
+  stream_info->set_keypress(capture_.key_pressed);
+
   RETURN_ON_ERR(ProcessCaptureStreamLocked());
   capture_.capture_audio->CopyTo(formats_.api_format.output_stream(), dest);
 
@@ -832,6 +859,17 @@ int AudioProcessingImpl::ProcessStream(const float* const* src,
                                           &crit_debug_, &debug_dump_.capture));
   }
 #endif
+  {
+    const size_t channel_size =
+        sizeof(float) * formats_.api_format.output_stream().num_frames();
+    std::vector<rtc::ArrayView<const float>> dest_view;
+    for (size_t i = 0; i < formats_.api_format.output_stream().num_channels();
+         ++i) {
+      dest_view.emplace_back(dest[i], channel_size);

[peah-webrtc, 2017/03/31 07:24:43]

Afaics, the emplace_back call will cause arrayview objects to dynamically be
created in the vector, right? Furthermore, the vector may dynamically change
it's size during the loop. On top of that, line 1418 will dynamically allocate
the vector data.

Is this really necessary? That is a lot of dynamic memory allocation for
something that we basically already know the maximum size of. Is there no way
that we can use a stack array of a maximum size for this?

[aleloi, 2017/04/06 15:46:11]

Acknowledged.

+    }
+    stream_info->AddOutput(dest_view);
+  }
+  aec_dumper_->WriteCaptureStreamMessage(std::move(stream_info));
 
   return kNoError;
 }
@@ -1069,6 +1107,9 @@ int AudioProcessingImpl::ProcessStream(AudioFrame* frame) {
     return kBadDataLengthError;
   }
 
+  std::unique_ptr<AecDumper::CaptureStreamInfo> stream_info =
+      aec_dumper_->GetCaptureStreamInfo();
+  stream_info->AddInput(*frame);
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   if (debug_dump_.debug_file->is_open()) {
     RETURN_ON_ERR(WriteConfigMessage(false));
@@ -1082,10 +1123,22 @@ int AudioProcessingImpl::ProcessStream(AudioFrame* frame) {
 #endif
 
   capture_.capture_audio->DeinterleaveFrom(frame);
+
+  RTC_DCHECK(!(public_submodules_->echo_cancellation->is_enabled() &&

[peah-webrtc, 2017/03/31 07:24:43]

Why did you move these DCHECKs here?

[aleloi, 2017/04/06 15:46:11]

Same as the DCHECKs above.

+               public_submodules_->echo_control_mobile->is_enabled()));
+
+  stream_info->set_delay(capture_nonlocked_.stream_delay_ms);
+  stream_info->set_drift(
+      public_submodules_->echo_cancellation->stream_drift_samples());
+  stream_info->set_level(gain_control()->stream_analog_level());
+  stream_info->set_keypress(capture_.key_pressed);
+
   RETURN_ON_ERR(ProcessCaptureStreamLocked());
   capture_.capture_audio->InterleaveTo(
       frame, submodule_states_.CaptureMultiBandProcessingActive());
 
+  stream_info->AddOutput(*frame);
+  aec_dumper_->WriteCaptureStreamMessage(std::move(stream_info));
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   if (debug_dump_.debug_file->is_open()) {
     audioproc::Stream* msg = debug_dump_.capture.event_msg->mutable_stream();
@@ -1362,6 +1415,15 @@ int AudioProcessingImpl::AnalyzeReverseStreamLocked(
                                           &crit_debug_, &debug_dump_.render));
   }
 #endif
+  std::vector<rtc::ArrayView<const float>> src_view;
+  const size_t channel_size =
+      sizeof(float) * formats_.api_format.reverse_input_stream().num_frames();
+
+  for (size_t i = 0;
+       i < formats_.api_format.reverse_input_stream().num_channels(); ++i) {
+    src_view.emplace_back(src[i], channel_size);

[peah-webrtc, 2017/03/31 07:24:43]

Afaics, the emplace_back call will cause arrayview objects to dynamically be
created in the vector, right? Furthermore, the vector may dynamically change
it's size during the loop. On top of that, line 1418 will dynamically allocate
the vector data.

Is this really necessary? That is a lot of dynamic memory allocation for
something that we basically already know the maximum size of. Is there no way
that we can use a stack array of a maximum size for this?

[aleloi, 2017/04/06 15:46:11]

Acknowledged.

+  }
+  aec_dumper_->WriteReverseStreamMessage(src_view);
 
   render_.render_audio->CopyFrom(src,
                                  formats_.api_format.reverse_input_stream());
@@ -1415,6 +1477,8 @@ int AudioProcessingImpl::ProcessReverseStream(AudioFrame* frame) {
                                           &crit_debug_, &debug_dump_.render));
   }
 #endif
+  aec_dumper_->WriteReverseStreamMessage(*frame);

[peah-webrtc, 2017/03/31 07:24:43]

Does this work?  This accesses aec_dumper_ holding the crit_render_lock. But on
1646 aec_dumper is reassigned without holding any lock. I guess it is similar
for the crit_capture_lock

[aleloi, 2017/04/06 15:46:11]

Good point. Accesses to AecDump are thread safe, but accesses to the unique_ptr
are not. Holding a lock when reassigning to aec_dump_ is enough. I've changed
that.

+
   render_.render_audio->DeinterleaveFrom(frame);
   RETURN_ON_ERR(ProcessRenderStreamLocked());
   render_.render_audio->InterleaveTo(
@@ -1502,7 +1566,8 @@ int AudioProcessingImpl::delay_offset_ms() const {
 
 int AudioProcessingImpl::StartDebugRecording(
     const char filename[AudioProcessing::kMaxFilenameSize],
-    int64_t max_log_size_bytes) {
+    int64_t max_log_size_bytes,
+    rtc::TaskQueue* worker_queue) {
   // Run in a single-threaded manner.
   rtc::CritScope cs_render(&crit_render_);
   rtc::CritScope cs_capture(&crit_capture_);
@@ -1512,6 +1577,8 @@ int AudioProcessingImpl::StartDebugRecording(
     return kNullPointerError;
   }
 
+  aec_dumper_ = AecDumper::Create(filename, max_log_size_bytes, worker_queue);
+  aec_dumper_->WriteInitMessage(formats_.api_format);
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   debug_dump_.num_bytes_left_for_log_ = max_log_size_bytes;
   // Stop any ongoing recording.
@@ -1530,7 +1597,8 @@ int AudioProcessingImpl::StartDebugRecording(
 }
 
 int AudioProcessingImpl::StartDebugRecording(FILE* handle,
-                                             int64_t max_log_size_bytes) {
+                                             int64_t max_log_size_bytes,
+                                             rtc::TaskQueue* worker_queue) {
   // Run in a single-threaded manner.
   rtc::CritScope cs_render(&crit_render_);
   rtc::CritScope cs_capture(&crit_capture_);
@@ -1539,6 +1607,8 @@ int AudioProcessingImpl::StartDebugRecording(FILE* handle,
     return kNullPointerError;
   }
 
+  aec_dumper_ = AecDumper::Create(handle, max_log_size_bytes, worker_queue);
+  aec_dumper_->WriteInitMessage(formats_.api_format);
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   debug_dump_.num_bytes_left_for_log_ = max_log_size_bytes;
 
@@ -1557,20 +1627,23 @@ int AudioProcessingImpl::StartDebugRecording(FILE* handle,
 #endif  // WEBRTC_AUDIOPROC_DEBUG_DUMP
 }
 
-int AudioProcessingImpl::StartDebugRecording(FILE* handle) {
-  return StartDebugRecording(handle, -1);
+int AudioProcessingImpl::StartDebugRecording(FILE* handle,
+                                             rtc::TaskQueue* worker_queue) {
+  return StartDebugRecording(handle, -1, worker_queue);
 }
 
 int AudioProcessingImpl::StartDebugRecordingForPlatformFile(
-    rtc::PlatformFile handle) {
+    rtc::PlatformFile handle,
+    rtc::TaskQueue* worker_queue) {
   // Run in a single-threaded manner.
   rtc::CritScope cs_render(&crit_render_);
   rtc::CritScope cs_capture(&crit_capture_);
   FILE* stream = rtc::FdopenPlatformFileForWriting(handle);
-  return StartDebugRecording(stream, -1);
+  return StartDebugRecording(stream, -1, worker_queue);
 }
 
 int AudioProcessingImpl::StopDebugRecording() {
+  aec_dumper_ = AecDumper::CreateNullDumper();

[peah-webrtc, 2017/03/31 07:24:43]

Why do we need to have a null dumper? Is it not sufficient to have an null
(empty) unique pointer?

[aleloi, 2017/04/06 15:46:11]

It is. I can remove the null dumper if there are sufficiently many voices for
that. There is one reason for the null dumper (not relevant at this line): it
allows to define a null factory, which can be included by the build system when
protobuf support is off.

   // Run in a single-threaded manner.
   rtc::CritScope cs_render(&crit_render_);
   rtc::CritScope cs_capture(&crit_capture_);
@@ -1938,6 +2011,43 @@ int AudioProcessingImpl::WriteConfigMessage(bool forced) {
   config.set_intelligibility_enhancer_enabled(
       capture_nonlocked_.intelligibility_enabled);
 
+  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;
+
   std::string experiments_description =
       public_submodules_->echo_cancellation->GetExperimentsDescription();
   // TODO(peah): Add semicolon-separated concatenations of experiment
@@ -1953,6 +2063,9 @@ int AudioProcessingImpl::WriteConfigMessage(bool forced) {
   }
   config.set_experiments_description(experiments_description);
 
+  apm_config.experiments_description = experiments_description;
+  aec_dumper_->WriteConfig(apm_config, forced);
+
   std::string serialized_config = config.SerializeAsString();
   if (!forced &&
       debug_dump_.capture.last_serialized_config == serialized_config) {