AecDump interface

webrtc/modules/audio_processing/aec_dumper/aec_dumper.h

+/*
+ *  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_AEC_DUMPER_AEC_DUMPER_H_
+#define WEBRTC_MODULES_AUDIO_PROCESSING_AEC_DUMPER_AEC_DUMPER_H_
+
+#include <memory>
+#include <string>
+#include <vector>
+
+#include "webrtc/base/array_view.h"
+#include "webrtc/base/constructormagic.h"
+#include "webrtc/modules/audio_processing/include/audio_processing.h"
+
+namespace audioproc {
+class Event;
+}  // namespace audioproc
+
+namespace rtc {
+class TaskQueue;
+}  // namespace rtc
+
+namespace webrtc {
+
+class AudioFrame;
+
+// Struct for passing current config from APM without having to
+// include protobuf headers.
+struct InternalAPMConfig {

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

Is it possible to extend the APM::Config to include the information missing
here, instead of adding another type? APM::Config could be extended with the
information here which is missing, *without* using that data to actually
configure the APM.

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

Probably. For some reason, I thought we were moving away from the template based
webrtc::Config. We would have to add twice as many ConfigOptionsIDs. I don't
know if that's a good idea: setting the new configuration variables in
webrtc::Config would not have any effect when configuring/creating APM. That
would complicate a public interface.

[the sun, 2017/04/06 17:39:37]

Oh, misunderstanding. Not the template based global config. Please help us
obliterate that one.

There's a simple struct in APM which is currently used to set up HPF and newer
components:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...
Ideally we'd get rid of all the component-specific interfaces and only set up
the APM with this config.

+  InternalAPMConfig();
+
+  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 = "";
+
+ private:
+  RTC_DISALLOW_COPY_AND_ASSIGN(InternalAPMConfig);
+};
+
+class AecDumper {

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

Would it not make sense to at the same time change the name to ApmDumper?
aecdump recordings are quite improperly named, as we actually use the to
evaluate quite a lot more than just the AEC. I guess the name has stuck for
legacy reasons but it would be good to take the opportunity to change that now,
at least internally.

WDYT?

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

Let's keep AecDump for now.
1) The concept is already established when discussing these recordings.
2) In Chrome we have other types of recordings as well (input/output capture).
3) Hopefully, there is a future without a monolithic APM, in which case talking
about APM recordings makes as much (or as little) sense as talking about AEC
dumps.

I'd suggest you call the interface AecDump and drop the "er". 

[peah-webrtc, 2017/04/05 04:41:50]

Makes sense. In particular I think 1) is a very important point.

Agree to this I do.

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

Changed to AecDump.

+ public:
+  // A capture stream frame is logged before and after processing in
+  // the same protobuf message. To facilitate that, a

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

Is really a line break needed here due to the length of the line? If
CaptureStreamInfo can be fit into line 65, "Output" will fit into line 66.

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

Done.

+  // CaptureStreamInfo instance is first filled with Input, then
+  // Output.
+  //
+  // To log an input/output pair, first call
+  // AecDumper::GetCaptureStreamInfo.  Add the input and output to
+  // it. Then call AecDumper::WriteCaptureStreamMessage.
+  class CaptureStreamInfo {
+   public:
+    virtual ~CaptureStreamInfo() = default;
+    virtual void AddInput(std::vector<rtc::ArrayView<const float>> src) = 0;

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

Won't this cause a copy of src? Would it not be better to pass it as a constant
reference?

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

Done.

+    virtual void AddOutput(std::vector<rtc::ArrayView<const float>> src) = 0;

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

See above

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

Done.

+
+    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;
+  };
+
+  AecDumper() = default;
+
+  virtual ~AecDumper() = default;
+
+  // An AecDumper is always associated with a single file and task
+  // queue. The file is safely closed when the dtor is called. The

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

Is this the case as it is now used in WebRTC? 

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

I think using a single file / AecDump instance makes the interface simpler. That
a task queue must outlive objects that post on it is assumed in other places in
WebRTC.

I've removed mentions of files, queues and handles from this interface.

+  // task queue must outlive the created AecDumper instance.
+  static std::unique_ptr<AecDumper> Create(std::string file_name,

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

For the reasons we've discussed offline (proto library support) it is better to
separate the interface from the implementations/factory methods in this case.
The interface should live in audio_processing/include, but the implementations
(one build target for each type) lives in a separate folder.

The null implementation should probably live with the interface, though I'm not
sure in this case there should be one, or if it is better to make the calls to
the AECDump implementation conditional in AudioProcessingImpl... hmm.

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

Done, I think. I prefer having a null implementation to conditionally making
calls on a pointer: less logic in audio_processing_impl, less branches and less
potential for pointer errors.

+                                           int64_t max_log_size_bytes,
+                                           rtc::TaskQueue* worker_queue);
+  static std::unique_ptr<AecDumper> Create(FILE* handle,
+                                           int64_t max_log_size_bytes,
+                                           rtc::TaskQueue* worker_queue);
+
+  static std::unique_ptr<AecDumper> CreateNullDumper();
+
+  virtual std::unique_ptr<CaptureStreamInfo> GetCaptureStreamInfo() = 0;
+
+  virtual void WriteInitMessage(const ProcessingConfig& api_format) = 0;
+
+  virtual void WriteReverseStreamMessage(const AudioFrame& frame) = 0;

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

Would it make sense to change "Reverse" to "Render"? That would match the
"Capture" term that is used in WriteCaptureStreamMessage.

I think that "Reverse" is a bit ambiguous and not as descriptive as it could be.
I personally prefer "Render".

WDYT?

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

+1! "Reverse" stream makes my head explode.

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

Done.

+
+  virtual void WriteReverseStreamMessage(
+      std::vector<rtc::ArrayView<const float>> src) = 0;

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

Won't this cause a copy of src? Would it not be better to pass it as a constant
reference?

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

Done.

+
+  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;
+};
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_DUMPER_AEC_DUMPER_H_