AecDump implementation.

webrtc/modules/audio_processing/aec_dump/aec_dump.cc

Index: webrtc/modules/audio_processing/aec_dump/aec_dump.cc
diff --git a/webrtc/modules/audio_processing/aec_dump/aec_dump.cc b/webrtc/modules/audio_processing/aec_dump/aec_dump.cc
index efc56d2bf844920278b20022ba09200ef2326606..c9e3665f067dc58d5017044a99778fecb98c1386 100644
--- a/webrtc/modules/audio_processing/aec_dump/aec_dump.cc
+++ b/webrtc/modules/audio_processing/aec_dump/aec_dump.cc
@@ -10,55 +10,171 @@
 
 #include "webrtc/modules/audio_processing/aec_dump/aec_dump.h"
 
+#include "webrtc/base/checks.h"
+#include "webrtc/base/event.h"
+#include "webrtc/base/ignore_wundef.h"
+#include "webrtc/base/protobuf_utils.h"
+#include "webrtc/modules/audio_processing/aec_dump/write_to_file_task.h"
+
+// Files generated at build-time by the protobuf compiler.
+RTC_PUSH_IGNORING_WUNDEF()
+#ifdef WEBRTC_ANDROID_PLATFORM_BUILD
+#include "external/webrtc/webrtc/modules/audio_processing/debug.pb.h"
+#else
+#include "webrtc/modules/audio_processing/debug.pb.h"
+#endif
+RTC_POP_IGNORING_WUNDEF()
+
 namespace webrtc {
 
 InternalAPMConfig::InternalAPMConfig() = default;
 InternalAPMConfig::InternalAPMConfig(const InternalAPMConfig&) = default;
 InternalAPMConfig::InternalAPMConfig(InternalAPMConfig&&) = default;
 
-AecDump::AecDump(int64_t max_log_size_bytes, rtc::TaskQueue* worker_queue) {}
+AecDump::AecDump(int64_t max_log_size_bytes, rtc::TaskQueue* worker_queue)
+    : debug_file_(FileWrapper::Create()),
+      num_bytes_left_for_log_(max_log_size_bytes),
+      worker_queue_(worker_queue) {}
 
 AecDump::AecDump(rtc::PlatformFile file,
                  int64_t max_log_size_bytes,
-                 rtc::TaskQueue* worker_queue) {}
+                 rtc::TaskQueue* worker_queue)
+    : AecDump(max_log_size_bytes, worker_queue) {
+  FILE* handle = rtc::FdopenPlatformFileForWriting(file);
+  RTC_DCHECK(handle);
+  debug_file_->OpenFromFileHandle(handle);
+}
 
 AecDump::AecDump(std::string file_name,
                  int64_t max_log_size_bytes,
-                 rtc::TaskQueue* worker_queue) {}
+                 rtc::TaskQueue* worker_queue)
+    : AecDump(max_log_size_bytes, worker_queue) {
+  RTC_DCHECK(debug_file_);
+  debug_file_->OpenFile(file_name.c_str(), false);
+}
 
 AecDump::AecDump(FILE* handle,
                  int64_t max_log_size_bytes,
-                 rtc::TaskQueue* worker_queue) {}
+                 rtc::TaskQueue* worker_queue)
+    : AecDump(max_log_size_bytes, worker_queue) {
+  RTC_DCHECK(debug_file_);
+  debug_file_->OpenFromFileHandle(handle);
+}
 
-AecDump::~AecDump() {}
+AecDump::~AecDump() {
+  // Block until all tasks have finished running.
+  rtc::Event thread_sync_event(false /* manual_reset */, false);
+  worker_queue_->PostTask([&thread_sync_event] { thread_sync_event.Set(); });
+  thread_sync_event.Wait(rtc::Event::kForever);

[peah-webrtc, 2017/05/09 07:14:57]

What does this do? Can this wait forever in the destructor?

[aleloi, 2017/05/12 13:07:56]

It pauses execution until 'thread_sync_event.Set()' in the lambda function on
line 67 has been run. This is to ensure the task queue has pointers to an
AecDump object from inside pending tasks when the d-tor has finished.

It shouldn't wait forever.

The same pattern is used in AudioSendStream, VideoSendStream and rtc event
logging.

I've added a comment.

+}
 
-AecDump::CaptureStreamInfo::CaptureStreamInfo() = default;
-AecDump::CaptureStreamInfo::~CaptureStreamInfo() = default;
+void AecDump::WriteInitMessage(const InternalAPMStreamsConfig& streams_config) {
+  auto event = std::unique_ptr<audioproc::Event>(new audioproc::Event());
+  event->set_type(audioproc::Event::INIT);
+  audioproc::Init* msg = event->mutable_init();
+
+  msg->set_sample_rate(streams_config.input_sample_rate);
+  msg->set_output_sample_rate(streams_config.output_sample_rate);
+  msg->set_reverse_sample_rate(streams_config.render_input_sample_rate);
+  msg->set_reverse_output_sample_rate(streams_config.render_output_sample_rate);
+
+  msg->set_num_input_channels(
+      static_cast<int32_t>(streams_config.input_num_channels));
+  msg->set_num_output_channels(
+      static_cast<int32_t>(streams_config.output_num_channels));
+  msg->set_num_reverse_channels(
+      static_cast<int32_t>(streams_config.render_input_num_channels));
+  msg->set_num_reverse_output_channels(
+      streams_config.render_output_num_channels);
+
+  PostTask(std::move(event));
+}
 
-void AecDump::CaptureStreamInfo::AddInput(FloatAudioFrame src) {}
-void AecDump::CaptureStreamInfo::AddOutput(FloatAudioFrame src) {}
+void AecDump::WriteRenderStreamMessage(const AudioFrame& frame) {
+  auto event = std::unique_ptr<audioproc::Event>(new audioproc::Event());
 
-void AecDump::CaptureStreamInfo::AddInput(const AudioFrame& frame) {}
-void AecDump::CaptureStreamInfo::AddOutput(const AudioFrame& frame) {}
+  event->set_type(audioproc::Event::REVERSE_STREAM);
+  audioproc::ReverseStream* msg = event->mutable_reverse_stream();
+  const size_t data_size =
+      sizeof(int16_t) * frame.samples_per_channel_ * frame.num_channels_;
+  msg->set_data(frame.data_, data_size);
 
-void AecDump::CaptureStreamInfo::set_delay(int delay) {}
-void AecDump::CaptureStreamInfo::set_drift(int drift) {}
-void AecDump::CaptureStreamInfo::set_level(int level) {}
-void AecDump::CaptureStreamInfo::set_keypress(bool keypress) {}
+  PostTask(std::move(event));
+}
 
-std::unique_ptr<audioproc::Event> GetEventMsg();
+void AecDump::WriteRenderStreamMessage(FloatAudioFrame src) {

[peah-webrtc, 2017/05/09 07:14:57]

Instead of passing a FloatAudioFrame as
aec_dump_->WriteRenderStreamMessage(FloatAudioFrame(src, num_channels,
channel_size));
have you considered changing the call to
aec_dump_->WriteRenderStreamMessage(src, num_channels, channel_size);

That would eliminate the need for FloatAudioFrame.
The for loop would then instead be
for (size_t i = 0; i < num_channels; ++i) {
  const auto& channel_view = src.channel(i);
  msg->add_channel(&src[i], sizeof(float) * channel_size);
}

WDYT?

[aleloi, 2017/05/12 13:07:56]

I have a mild preference for leaving FloatAudioFrame. If we were to change the
interface to (float** data, num_channels, channel_size), there would be three
arguments for what is conceptually the same thing.

FloadAudioFrame helps to use the interface correctly by showing how the data
will be used through the public method channel(idx). It also helps in the
implementation of an AecDump subclass by showing how the data is supposed to be
used.

+  auto event = std::unique_ptr<audioproc::Event>(new audioproc::Event());
+  event->set_type(audioproc::Event::REVERSE_STREAM);
 
-void AecDump::WriteInitMessage(const InternalAPMStreamsConfig& streams_config) {
-}
+  audioproc::ReverseStream* msg = event->mutable_reverse_stream();
 
-void AecDump::WriteRenderStreamMessage(const AudioFrame& frame) {}
+  for (size_t i = 0; i < src.num_channels(); ++i) {
+    const auto& channel_view = src.channel(i);
+    msg->add_channel(channel_view.begin(), sizeof(float) * channel_view.size());
+  }
 
-void AecDump::WriteRenderStreamMessage(FloatAudioFrame src) {}
+  PostTask(std::move(event));
+}
 
 void AecDump::WriteCaptureStreamMessage(
-    CaptureStreamInfo* capture_stream_info) {}
+    CaptureStreamInfo* capture_stream_info) {
+  auto event_ptr = capture_stream_info->GetEventMsg();
+  if (event_ptr) {
+    PostTask(std::move(event_ptr));
+  }
+}
+
+void CopyFromConfigToEvent(const webrtc::InternalAPMConfig& config,
+                           webrtc::audioproc::Config* pb_cfg) {
+  pb_cfg->set_aec_enabled(config.aec_enabled);
+  pb_cfg->set_aec_delay_agnostic_enabled(config.aec_delay_agnostic_enabled);
+  pb_cfg->set_aec_drift_compensation_enabled(
+      config.aec_drift_compensation_enabled);
+  pb_cfg->set_aec_extended_filter_enabled(config.aec_extended_filter_enabled);
+  pb_cfg->set_aec_suppression_level(config.aec_suppression_level);
+
+  pb_cfg->set_aecm_enabled(config.aecm_enabled);
+  pb_cfg->set_aecm_comfort_noise_enabled(config.aecm_comfort_noise_enabled);
+  pb_cfg->set_aecm_routing_mode(config.aecm_routing_mode);
+
+  pb_cfg->set_agc_enabled(config.agc_enabled);
+  pb_cfg->set_agc_mode(config.agc_mode);
+  pb_cfg->set_agc_limiter_enabled(config.agc_limiter_enabled);
+  pb_cfg->set_noise_robust_agc_enabled(config.noise_robust_agc_enabled);
+
+  pb_cfg->set_hpf_enabled(config.hpf_enabled);
+
+  pb_cfg->set_ns_enabled(config.ns_enabled);
+  pb_cfg->set_ns_level(config.ns_level);
 
-void AecDump::WriteConfig(const InternalAPMConfig& config, bool forced) {}
+  pb_cfg->set_transient_suppression_enabled(
+      config.transient_suppression_enabled);
+  pb_cfg->set_intelligibility_enhancer_enabled(
+      config.intelligibility_enhancer_enabled);
 
-void AecDump::PostTask(std::unique_ptr<audioproc::Event> event) {}
+  pb_cfg->set_experiments_description(config.experiments_description);
+}
+
+void AecDump::WriteConfig(const InternalAPMConfig& config, bool forced) {
+  auto event = std::unique_ptr<audioproc::Event>(new audioproc::Event());

[peah-webrtc, 2017/05/09 07:14:57]

I'd prefer to make this part of the WriteToFileTask. As it is now, it is
injected into WriteToFileTask but due to that construction it needs to be
created separately.

Would it be possible to do this as the following instead.

std::unique_ptr<rtc::QueuedTask> task(new WriteToFileTask(debug_file_.get(),
&num_bytes_left_for_log_)));
audioproc::Event* = task.GetEvent();
....
[update the fields of Event]
....
worker_queue_->PostTask(std::move(task));

That approach would eliminate the need for a separate newing up  of Event().
Furthermore, the postTask method would not be needed.

WDYT?

[aleloi, 2017/05/12 13:07:56]

That makes everything a little simpler. I like it!

+  event->set_type(audioproc::Event::CONFIG);
+  CopyFromConfigToEvent(config, event->mutable_config());
+
+  ProtoString serialized_config = event->mutable_config()->SerializeAsString();
+  {
+    rtc::CritScope cs(&config_string_lock_);
+    if (!forced && serialized_config == last_serialized_capture_config_) {
+      return;
+    }
+    last_serialized_capture_config_ = serialized_config;
+  }
+
+  PostTask(std::move(event));
+}
+
+void AecDump::PostTask(std::unique_ptr<audioproc::Event> event) {
+  RTC_DCHECK(event);
+  worker_queue_->PostTask(std::unique_ptr<rtc::QueuedTask>(new WriteToFileTask(
+      debug_file_.get(), std::move(event), &num_bytes_left_for_log_)));
+}
 }  // namespace webrtc