AecDump interface

webrtc/modules/audio_processing/audio_processing_impl.h

 /*
  *  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.
  */
 
 #ifndef WEBRTC_MODULES_AUDIO_PROCESSING_AUDIO_PROCESSING_IMPL_H_
 #define WEBRTC_MODULES_AUDIO_PROCESSING_AUDIO_PROCESSING_IMPL_H_
 
 #include <list>
 #include <memory>
 #include <string>
 #include <vector>
 
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/function_view.h"
 #include "webrtc/base/gtest_prod_util.h"
 #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"
 #include "webrtc/modules/audio_processing/rms_level.h"
 #include "webrtc/system_wrappers/include/file_wrapper.h"
 
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
 // Files generated at build-time by the protobuf compiler.
 RTC_PUSH_IGNORING_WUNDEF()
 #ifdef WEBRTC_ANDROID_PLATFORM_BUILD
@@ skipping 24 matching lines @@
   int Initialize(int capture_input_sample_rate_hz,
                  int capture_output_sample_rate_hz,
                  int render_sample_rate_hz,
                  ChannelLayout capture_input_layout,
                  ChannelLayout capture_output_layout,
                  ChannelLayout render_input_layout) override;
   int Initialize(const ProcessingConfig& processing_config) override;
   void ApplyConfig(const AudioProcessing::Config& config) override;
   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
   // multi-threaded manner. Acquire the capture lock.
   int ProcessStream(AudioFrame* frame) override;
   int ProcessStream(const float* const* src,
                     size_t samples_per_channel,
                     int input_sample_rate_hz,
                     ChannelLayout input_layout,
                     int output_sample_rate_hz,
@@ skipping 208 matching lines @@
   int WriteConfigMessage(bool forced) EXCLUSIVE_LOCKS_REQUIRED(crit_capture_)
       EXCLUSIVE_LOCKS_REQUIRED(crit_capture_);
 
   // Critical section.
   rtc::CriticalSection crit_debug_;
 
   // Debug dump state.
   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_;
 
   // Struct containing the Config specifying the behavior of APM.
   AudioProcessing::Config config_;
 
   // Class containing information about what submodules are active.
   ApmSubmoduleStates submodule_states_;
 
@@ skipping 116 matching lines @@
   std::unique_ptr<
       SwapQueue<std::vector<int16_t>, RenderQueueItemVerifier<int16_t>>>
       agc_render_signal_queue_;
   std::unique_ptr<SwapQueue<std::vector<float>, RenderQueueItemVerifier<float>>>
       red_render_signal_queue_;
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_PROCESSING_AUDIO_PROCESSING_IMPL_H_