Test submission of complete AEC-dump refactoring.

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/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 {
+  InternalAPMConfig();
+  InternalAPMConfig(InternalAPMConfig&);

[kwiberg-webrtc, 2017/03/28 08:54:58]

Is this intended to be a copy constructor? In that case, you want const T&.
Also, since the struct contains a std::string, consider also adding a move
constructor.

[aleloi, 2017/03/29 08:41:31]

I now pass by const ref and have deleted the other c-tors.

+  ~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 = "";
+};
+
+inline InternalAPMConfig::InternalAPMConfig(InternalAPMConfig&) = default;
+inline InternalAPMConfig::InternalAPMConfig() = default;
+inline InternalAPMConfig::~InternalAPMConfig() = default;

[kwiberg-webrtc, 2017/03/28 08:54:58]

Either drop the "inline" and move these lines to the .cc file, or just write
"=default" on the lines in the class declaration. (But the second option is
equivalent to simply not writing the lines even in the class declaration, so in
that case just drop them.)

[aleloi, 2017/03/29 08:41:31]

I've moved them to a cc file. I think the chrome style plugin thinks this is a
complex class (probably because of std::string) and requires definitions outside
class decl.

+
+class AecDumper {
+ 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
+  // AecDumper::GetCaptureStreamInfo.  Add the input and output to
+  // it. Then call AecDumper::WriteCaptureStreamMessage.
+  class CaptureStreamInfo {
+   public:
+    CaptureStreamInfo() = default;

[kwiberg-webrtc, 2017/03/28 08:54:58]

This is a pure interface, so don't try to add a constructor. It won't do
anything...

[aleloi, 2017/03/29 08:41:31]

Thanks! I've just read tip of the week #131. There should be a compiler warning
flag for when '= default' doesn't generate anything.

+    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;

[kwiberg-webrtc, 2017/03/28 08:54:59]

Drop the outermost "const" here. You're simply taking the vectors by value.

Would a typical implementation actually std::move the passed vectors and save
them (as opposed to just reading from it)? If not, consider making the outer
layer an ArrayView too.

[aleloi, 2017/03/29 08:41:31]

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;

[kwiberg-webrtc, 2017/03/28 08:54:58]

Again: pure interface, so no need to even mention a constructor.

[aleloi, 2017/03/29 08:41:31]

Done.

+
+  virtual ~AecDumper() = default;
+
+  // TODO(aleloi): update comments to new creation scheme.
+  // If called when a recording is active, that file is closed, and a
+  // new file is opened. Messages waiting to be written asynchronously
+  // to the old file may be lost. Returns true iff opening file for
+  // writing succeeded.
+
+  // Closes associated file. Messages waiting to be written to file
+  // asynchronously may be lost. This method is safe to call when no
+  // recording is active. A recording does not have to be closed
+  // manually with this method; instead the AecDumper instance may be
+  // destroyed.
+  static std::unique_ptr<AecDumper> Create(std::string file_name,
+                                           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();

[kwiberg-webrtc, 2017/03/28 08:54:58]

For modularity, consider having the interface and the create functions in
different build targets. That way, it becomes possible to implement the
interface without having to link with the create functions. (It's totally fine
to have a .h file that has *just* these three functions in it.)

It's fine to keep them here, too, *if* it's acceptable that everyone that
implements the interface has to link with the implementations of the create
functions.

[aleloi, 2017/03/29 08:41:31]

The whole interface is rather specialized for use from inside
audio_processing_impl.cc. I can't think of where else this could be
use used. Therefore I'd like to keep the build targets as they
currently are. Declaring Create methods in a different target seems
something that shouldn't be difficult to do later if it's needed.

+
+  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 AecDumper can't keep up with
+  // the flow of messages, some will be silently dropped.
+  virtual void WriteInitMessage(const ProcessingConfig& api_format) = 0;
+
+  virtual void WriteReverseStreamMessage(const AudioFrame& frame) = 0;
+
+  virtual void WriteReverseStreamMessage(
+      const std::vector<rtc::ArrayView<const float>> src) = 0;

[kwiberg-webrtc, 2017/03/28 08:54:59]

The comment for AddInput/AddOutput applies here too.

[aleloi, 2017/03/29 08:41:31]

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(InternalAPMConfig config, bool forced) = 0;

[kwiberg-webrtc, 2017/03/28 08:54:59]

InternalAPMConfig is somewhat big, so isn't exactly free even to move. Pass by
const ref?

[aleloi, 2017/03/29 08:41:31]

Done.

+};
+}  // namespace webrtc
+
+#endif  // WEBRTC_MODULES_AUDIO_PROCESSING_AEC_DUMPER_AEC_DUMPER_H_