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 01b640fcfc0f36332790765fd530f267690cfc9a..26d870f8aa7d4d5d023fc2da9dd5ac0a10efc9a4 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.h
+++ b/webrtc/modules/audio_processing/audio_processing_impl.h
@@ -22,6 +22,7 @@
 #include "webrtc/base/ignore_wundef.h"
 #include "webrtc/base/swap_queue.h"
 #include "webrtc/base/thread_annotations.h"
+#include "webrtc/modules/audio_processing/aec_dumper/aec_dumper.h"
 #include "webrtc/modules/audio_processing/audio_buffer.h"
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
 #include "webrtc/modules/audio_processing/render_queue_item_verifier.h"
@@ -67,10 +68,14 @@ class AudioProcessingImpl : public AudioProcessing {
   void SetExtraOptions(const webrtc::Config& config) override;
   void UpdateHistogramsOnCallEnd() override;
   int StartDebugRecording(const char filename[kMaxFilenameSize],

[the sun, 2017/04/03 14:33:02]

I think we only need one method

StartAecDump(std::unique_ptr<AecDump> aec_dump);

Send in a valid impl to start dumping, send in a nullptr to stop. Whether the
dump is created from a FILE* or a char filename[], whether it uses a TaskQueue
or a separate thread, that should be up to the AecDump implementation and
disconnected from the APM. This class shouldn't include file_wrapper.h and not
debug.bp.h.

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

Good idea! I can't get rid of the other StartDebugRecordings right away, because
there are downstream dependencies and because this CL doesn't include the
AecDump implementation.

-                          int64_t max_log_size_bytes) override;
-  int StartDebugRecording(FILE* handle, int64_t max_log_size_bytes) override;
-  int StartDebugRecording(FILE* handle) override;
-  int StartDebugRecordingForPlatformFile(rtc::PlatformFile handle) override;
+                          int64_t max_log_size_bytes,
+                          rtc::TaskQueue* worker_queue) override;
+  int StartDebugRecording(FILE* handle,
+                          int64_t max_log_size_bytes,
+                          rtc::TaskQueue* worker_queue) override;
+  int StartDebugRecording(FILE* handle, rtc::TaskQueue* worker_queue) override;
+  int StartDebugRecordingForPlatformFile(rtc::PlatformFile handle,
+                                         rtc::TaskQueue* worker_queue) override;
   int StopDebugRecording() override;
 
   // Capture-side exclusive methods possibly running APM in a
@@ -299,6 +304,8 @@ class AudioProcessingImpl : public AudioProcessing {
   ApmDebugDumpState debug_dump_;
 #endif
 
+  // TODO(aleloi) doc.

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

Please change the comment according to the style guidelines

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

Done.

+  std::unique_ptr<AecDumper> aec_dumper_;

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

Won't this be accessed concurrently? In that case, it would be good to annotate
it with the corresponding lock.

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

The implementation is actually thread-safe. The methods construct pb-objects
without changing state. Then pb-objects are then sent to the task queue, which
is safe to do from different threads.

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

I've added locks for writing to aec_dump_, and an AecDump instance by itself is
thread safe. I don't think the code is safe as is.

Suppose StopRecording is called. Locks are aquired and aec_dump_ is destroyed.
Suppose another thread is inside an unlocked part of ProcessStream(). It has
unguarded 'if (aec_dump_)' blocks, which is a data race.

I will fix that somehow in the next patch set.

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