AecDump interface

webrtc/modules/audio_processing/audio_processing_impl.h

Index: webrtc/modules/audio_processing/audio_processing_impl.h
diff --git a/webrtc/modules/audio_processing/audio_processing_impl.h b/webrtc/modules/audio_processing/audio_processing_impl.h
index 37b15c28935f29cb6450f5f5cec3d53fe640f50f..9e76e3abbae5782c6ef4265b17b01eee4aa433eb 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.h
+++ b/webrtc/modules/audio_processing/audio_processing_impl.h
@@ -23,6 +23,7 @@
 #include "webrtc/base/swap_queue.h"
 #include "webrtc/base/thread_annotations.h"
 #include "webrtc/modules/audio_processing/audio_buffer.h"
+#include "webrtc/modules/audio_processing/include/aec_dump.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
 #include "webrtc/modules/audio_processing/render_queue_item_verifier.h"
 #include "webrtc/modules/audio_processing/rms_level.h"
@@ -66,6 +67,7 @@ class AudioProcessingImpl : public AudioProcessing {
   void ApplyConfig(const AudioProcessing::Config& config) override;
   void SetExtraOptions(const webrtc::Config& config) override;
   void UpdateHistogramsOnCallEnd() override;
+  void StartDebugRecording(std::unique_ptr<AecDump> aec_dump) override;

[the sun, 2017/04/28 11:06:00]

nit: perhaps "Start" isn't the right verb here. "Set" seems more appropriate.
Perhaps even SetAecDump()?

[aleloi, 2017/05/03 13:58:19]

Ok, changing to SetAecDump().

   int StartDebugRecording(const char filename[kMaxFilenameSize],
                           int64_t max_log_size_bytes) override;
   int StartDebugRecording(FILE* handle, int64_t max_log_size_bytes) override;
@@ -275,6 +277,38 @@ class AudioProcessingImpl : public AudioProcessing {
       EXCLUSIVE_LOCKS_REQUIRED(crit_render_);
   int ProcessRenderStreamLocked() EXCLUSIVE_LOCKS_REQUIRED(crit_render_);
 
+  // Collects configuration settings from public and private
+  // submodules to be saved as an audioproc::Config message.
+  InternalAPMConfig CollectApmConfig() const
+      EXCLUSIVE_LOCKS_REQUIRED(crit_capture_)
+          EXCLUSIVE_LOCKS_REQUIRED(crit_capture_);
+
+  // Creates and returns new CaptureStreamInfo filled with the capture
+  // stream and data (delay, drift etc).
+  std::unique_ptr<AecDump::CaptureStreamInfo> RecordUnprocessedCaptureStream(
+      const float* const* capture_stream) const
+      EXCLUSIVE_LOCKS_REQUIRED(crit_capture_);
+
+  std::unique_ptr<AecDump::CaptureStreamInfo> RecordUnprocessedCaptureStream(
+      const AudioFrame& capture_frame) const
+      EXCLUSIVE_LOCKS_REQUIRED(crit_capture_);
+
+  // Fills the CaptureStreamInfo object with the processed capture
+  // stream and sends it to be written with AecDump.
+  void RecordProcessedCaptureStream(
+      const float* const* processed_capture_stream,
+      std::unique_ptr<AecDump::CaptureStreamInfo> stream_info) const
+      EXCLUSIVE_LOCKS_REQUIRED(crit_capture_);
+
+  void RecordProcessedCaptureStream(
+      const AudioFrame& processed_capture_frame,
+      std::unique_ptr<AecDump::CaptureStreamInfo> stream_info) const
+      EXCLUSIVE_LOCKS_REQUIRED(crit_capture_);
+
+  // Copies data (delay, drift, etc) into the |stream_info| object.
+  void PopulateStreamInfoWithConfig(AecDump::CaptureStreamInfo* stream_info)
+      const EXCLUSIVE_LOCKS_REQUIRED(crit_capture_);
+
 // Debug dump methods that are internal and called without locks.
 // TODO(peah): Make thread safe.
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
@@ -299,6 +333,10 @@ class AudioProcessingImpl : public AudioProcessing {
   ApmDebugDumpState debug_dump_;
 #endif
 
+  // AecDump instance used for optionally logging APM config, input
+  // and output to file in the AEC-dump format defined in debug.proto.
+  std::unique_ptr<AecDump> aec_dump_;

[the sun, 2017/04/28 11:06:00]

EXCLUSIVE_LOCKS_REQUIRED(crit_render_, crit_capture_)

[aleloi, 2017/05/03 13:58:19]

Calling methods concurrently on a AecDump instance is supposed to be safe.
Modifying the aec_dump_ pointer concurrently is not.

Currently, calls to the AecDump are guarded by *some* of the render/capture
locks.
Changes to the pointer are guarded by both locks. The locking scheme used
currently is safe, but it cannot be expressed with compiler annotations.

I don't think it's worth adding extra locks where they are not needed for
correctness to be able to have compiler-checked annotations.

+
   // Critical sections.
   rtc::CriticalSection crit_render_ ACQUIRED_BEFORE(crit_capture_);
   rtc::CriticalSection crit_capture_;