AecDump interface

webrtc/modules/audio_processing/include/aec_dump.h

Index: webrtc/modules/audio_processing/include/aec_dump.h
diff --git a/webrtc/modules/audio_processing/include/aec_dump.h b/webrtc/modules/audio_processing/include/aec_dump.h
new file mode 100644
index 0000000000000000000000000000000000000000..f4167981ab6d5fda2a2f3b7013307d36436c8542
--- /dev/null
+++ b/webrtc/modules/audio_processing/include/aec_dump.h
@@ -0,0 +1,147 @@
+/*
+ *  Copyright (c) 2017 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_INCLUDE_AEC_DUMP_H_
+#define WEBRTC_MODULES_AUDIO_PROCESSING_INCLUDE_AEC_DUMP_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "webrtc/base/array_view.h"
+#include "webrtc/modules/audio_processing/include/audio_processing.h"
+
+namespace audioproc {
+class Event;
+}  // namespace audioproc
+
+namespace webrtc {
+
+class AudioFrame;
+
+// Struct for passing current config from APM without having to
+// include protobuf headers.
+struct InternalAPMConfig {
+  InternalAPMConfig();
+
+  InternalAPMConfig(const InternalAPMConfig&) = delete;
+  InternalAPMConfig(const InternalAPMConfig&&) = delete;
+  InternalAPMConfig& operator=(const InternalAPMConfig&) = delete;
+  InternalAPMConfig& operator=(const InternalAPMConfig&&) = delete;
+
+  bool aec_enabled = false;
+  bool aec_delay_agnostic_enabled = false;
+  bool aec_drift_compensation_enabled = false;
+  bool aec_extended_filter_enabled = false;
+  int aec_suppression_level = 0;
+  bool aecm_enabled = false;
+  bool aecm_comfort_noise_enabled = false;
+  int aecm_routing_mode = 0;
+  bool agc_enabled = false;
+  int agc_mode = 0;
+  bool agc_limiter_enabled = false;
+  bool hpf_enabled = false;
+  bool ns_enabled = false;
+  int ns_level = 0;
+  bool transient_suppression_enabled = false;
+  bool intelligibility_enhancer_enabled = false;
+  bool noise_robust_agc_enabled = false;
+  std::string experiments_description = "";
+};
+
+class AecDump {
+ public:
+  // A capture stream frame is logged before and after processing in
+  // the same protobuf message. To facilitate that, a CaptureStreamInfo
+  // instance is first filled with Input, then Output.
+  //
+  // To log an input/output pair, first call
+  // AecDump::GetCaptureStreamInfo.  Add the input and output to
+  // it. Then call AecDump::WriteCaptureStreamMessage.
+  class CaptureStreamInfo {
+   public:
+    virtual ~CaptureStreamInfo() = default;
+    virtual void AddInput(
+        const std::vector<rtc::ArrayView<const float>>& src) = 0;
+    virtual void AddOutput(
+        const std::vector<rtc::ArrayView<const float>>& src) = 0;
+
+    virtual void AddInput(const AudioFrame& frame) = 0;
+    virtual void AddOutput(const AudioFrame& frame) = 0;
+
+    virtual void set_delay(int delay) = 0;
+    virtual void set_drift(int drift) = 0;
+    virtual void set_level(int level) = 0;
+    virtual void set_keypress(bool keypress) = 0;
+  };
+
+  virtual ~AecDump() = default;
+
+  static std::unique_ptr<AecDump> CreateNullDump();
+
+  virtual std::unique_ptr<CaptureStreamInfo> GetCaptureStreamInfo() = 0;
+
+  // The Write* methods are always safe to call. If no recording is in
+  // progress, calls will have no effect. Messages are written to file
+  // in a 'best effort' manner. If the AecDump can't keep up with
+  // the flow of messages, some will be silently dropped.

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

I'm not that happy that messages will be dropped. In particular, it is not at
all good that that happens silently. If the aecdump recordings are to be used as
a tool for debugging issues on platforms, they must be reliable. Otherwise this
will just cause another reason of doubt for what caused an issue.

What is the reason that they need to be dropped?

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

Sorry, missed to update the comment after I removed the EventPool (a pool of
protobuf objects). Now log requests will be put on a task queue, but not be
dropped.

+  virtual void WriteInitMessage(const ProcessingConfig& api_format) = 0;

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

Even though the ProcessingConfig class contains all the information needed, it
is fairly heavily based on StreamConfig. I'm not sure that we will be wanting to
maintain either ProcessingConfig nor StreamConfig in the long run. 

Would it be possible to implement this without adding further usage of
ProcessingConfig? Or do you think that it will be easy to refactor this if we
want to remove/change ProcessingConfig in the near future?

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

I have added a new type similar to InternalAPMConfig. Apart from not having to
maintain ProcessConfig/StreamConfig, there are other benefits:

* consistent naming (no more reverse_input)
* no dependency on audio_processing.h in aec_dump.h, which makes the cyclic
include check happy

+
+  virtual void WriteRenderStreamMessage(const AudioFrame& frame) = 0;
+
+  virtual void WriteRenderStreamMessage(
+      std::vector<rtc::ArrayView<const float>> src) = 0;

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

This call will copy a vector. Is that desired? Would it be possible to pass it
as a const-ref?

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

I have changed this to take the value by const& exactly as AddInput in
CaptureStreamInfo.

+
+  virtual void WriteCaptureStreamMessage(
+      std::unique_ptr<CaptureStreamInfo> stream_info) = 0;
+
+  // If not |forced|, only writes the current config if it is
+  // different from the last saved one; if |forced|, writes the config
+  // regardless of the last saved.
+  virtual void WriteConfig(const InternalAPMConfig& config, bool forced) = 0;
+};
+
+class NullCaptureStreamInfo final : public AecDump::CaptureStreamInfo {

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

Where is this classed used?

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

It was previously used for AecDumps that did nothing and in the unit test. It
has now been deleted. The unit test will use a real and possibly a mock AecDump
after the impl has been uploaded.

+  void AddInput(const std::vector<rtc::ArrayView<const float>>& src) override{};
+  void AddOutput(
+      const std::vector<rtc::ArrayView<const float>>& src) override{};
+
+  void AddInput(const AudioFrame& frame) override{};
+  void AddOutput(const AudioFrame& frame) override{};
+  void set_delay(int delay) override{};
+  void set_drift(int drift) override{};
+  void set_level(int level) override{};
+  void set_keypress(bool keypress) override{};
+};
+
+// Implementation of AecDump that doesn't do anything. It is used to
+// avoid handling special cases for when logging is enabled/disabled
+// in AudioProcessingImpl.
+class NullAecDump final : public AecDump {
+ public:
+  ~NullAecDump() override = default;
+
+  std::unique_ptr<CaptureStreamInfo> GetCaptureStreamInfo() override;

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

This should also need an implementation, right?

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

It was previously in aec_dump.cc. Now it's removed.

+
+  void WriteInitMessage(const ProcessingConfig& api_format) override{};

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

I assume that this method, as well as the ones below, would be inlined by the
compiler to a no-op? E.g., there would be no method call overhead in practice?
How does this work when the call is virtual?

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

I checked by compiling in release-mode and disassembling and stepping with gdb.
The methods were not inlined.

+
+  void WriteRenderStreamMessage(const AudioFrame& frame) override{};
+
+  void WriteRenderStreamMessage(
+      std::vector<rtc::ArrayView<const float>> src) override{};

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

This call will copy a vector. Is that desired? Would it be possible to pass it
as a const-ref?

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

Changed.

+
+  void WriteCaptureStreamMessage(
+      std::unique_ptr<CaptureStreamInfo> stream_info) override{};
+
+  void WriteConfig(const InternalAPMConfig& config, bool forced) override{};
+};
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_INCLUDE_AEC_DUMP_H_