Conversational Speech tool, MultiEndCall class and unit tests via mocking

webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc

Index: webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc
diff --git a/webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc b/webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc
new file mode 100644
index 0000000000000000000000000000000000000000..c8734de791a207fe8f637a12b18d7022296f4236
--- /dev/null
+++ b/webrtc/modules/audio_processing/test/conversational_speech/multiend_call.cc
@@ -0,0 +1,82 @@
+/*
+ *  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.
+ */
+
+#include "webrtc/modules/audio_processing/test/conversational_speech/multiend_call.h"
+
+#include <memory>
+#include <set>
+#include <utility>
+
+#include "webrtc/base/array_view.h"

[hlundin-webrtc, 2017/03/24 13:44:05]

Skip this; already included in header.

[AleBzk, 2017/03/27 08:24:14]

Same for the includes below except for the first then.
What about the includes above from std lib? They're also included in .h

[hlundin-webrtc, 2017/03/27 09:46:34]

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
"any includes present in the related header do not need to be included again in
the related cc (i.e., foo.cc can rely on foo.h's includes)"

[AleBzk, 2017/03/27 11:17:06]

Acknowledged.

+#include "webrtc/base/pathutils.h"
+#include "webrtc/modules/audio_processing/test/conversational_speech/timing.h"
+#include "webrtc/modules/audio_processing/test/conversational_speech/wavreader_abstract_factory.h"
+#include "webrtc/modules/audio_processing/test/conversational_speech/wavreader_interface.h"
+
+namespace webrtc {
+namespace test {
+namespace conversational_speech {
+
+MultiEndCall::MultiEndCall(
+    rtc::ArrayView<const Turn> timing, const std::string& audiotracks_path,
+    std::unique_ptr<WavReaderAbstractFactory> wavreader_abstract_factory)
+        : timing_(timing), audiotracks_path_(audiotracks_path),
+          wavreader_abstract_factory_(std::move(wavreader_abstract_factory)) {
+  FindSpeakerNames();
+  CreateAudioTrackReaders();
+  CheckTiming();
+}
+
+MultiEndCall::~MultiEndCall() {}

[hlundin-webrtc, 2017/03/24 13:44:05]

= default;

[AleBzk, 2017/03/27 08:24:13]

Done.

+
+rtc::ArrayView<const std::string> MultiEndCall::speaker_names() const {
+  return speaker_names_;
+}
+
+const std::map<std::string, std::unique_ptr<WavReaderInterface>>&
+    MultiEndCall::audiotrack_readers() const {
+  return audiotrack_readers_;
+}
+
+void MultiEndCall::FindSpeakerNames() {

[hlundin-webrtc, 2017/03/24 13:44:05]

This is only going to be called from the constructor, right? If so, I suggest
you skip the local std::set and insert the names directly into speaker_names_.
DCHECK that the vector is empty before.

void MultiEndCall::FindSpeakerNames() {
  RTC_DCHECK(speaker_names_.empty());
  for (const Turn& turn : timing_) {
    speaker_names_.insert(turn.speaker_name);
  }
}

Now, this is short enough to actually do it in the constructor, and skip this
method. Then you can also skip the DCHECK.

[AleBzk, 2017/03/27 08:24:14]

Note that speaker_names_ is std::vector.
I use speaker_names_set to find the unique names while iterating over the turns.
I then copy the set into the vector so that I can return ArrayView<const
std::string> in the getter speaker_names().
If I'm right, ArrayView doesn't work with std::set.

[hlundin-webrtc, 2017/03/27 09:46:34]

I think you should consider sticking with the set instead of converting to a
vector. This seems like the kind of data that does not benefit from being
represented as a vector. I mean, the actual data in the string objects is most
likely not stored in the continuous array that the vector guarantees.

So, unless the users of the speaker_names_ data will rely on being able to index
the data by numbers (the [] operator), I would recommend to make speaker_names_
itself an std::set, and have speaker_names() return a const ref to it. If you
really rely on the [] indexing, you could consider inserting directly into a
vector, and then run sort and unique on that vector (as in
http://en.cppreference.com/w/cpp/algorithm/unique). But I admit that is not as
elegant.

+  std::set<std::string> speaker_names_set;
+  for (const Turn& turn : timing_) {
+    speaker_names_set.insert(turn.speaker_name);
+  }
+  speaker_names_.assign(speaker_names_set.begin(), speaker_names_set.end());
+}
+
+void MultiEndCall::CreateAudioTrackReaders() {
+  for (const Turn& turn : timing_) {
+    auto it = audiotrack_readers_.find(turn.audiotrack_file_name);
+    if (it != audiotrack_readers_.end())

[hlundin-webrtc, 2017/03/24 13:44:05]

This method is also supposed to be called from the ctor only, right? If so,
simply DCHECK that audiotrack_readers_ is empty, and skip this.

[AleBzk, 2017/03/27 08:24:13]

Yes
Nope. Similarly to MultiEndCall::FindSpeakerNames(), I find the set of unique
audio track files (to avoid loading the same file multiple times).
Line 59 checks whether the audio track file name is already a key of the
collection of WavReaderInterface instances.
If so, it skips to the next turn.

+      continue;
+
+    // Instance Pathname to retrieve the full path to the audiotrack file.
+    const rtc::Pathname audiotrack_file_path(
+        audiotracks_path_, turn.audiotrack_file_name);
+
+    // Map the audiotrack file name to a new instance of WavReaderInterface.
+    std::unique_ptr<WavReaderInterface> wavreader =
+        wavreader_abstract_factory_->Create(audiotrack_file_path.pathname());
+    audiotrack_readers_.insert(

[hlundin-webrtc, 2017/03/24 13:44:05]

I think you can make this somewhat shorter, like
audiotrack_readers_.emplace(turn.audiotrack_file_name, std::move(wavreader));

[AleBzk, 2017/03/27 08:24:14]

Done.

[hlundin-webrtc, 2017/03/27 09:46:34]

Did you try emplace?

[AleBzk, 2017/03/27 11:17:06]

As far as I understand from http://www.cplusplus.com/reference/map/map/emplace/,
emplace() is useful when one can pass the arguments for the constructor.
While this way removes the need to copy/move, it is not applicable to my case
since I need to pull objects from an injected factory.

+        std::pair<std::string, std::unique_ptr<WavReaderInterface>>(
+            turn.audiotrack_file_name, std::move(wavreader)));
+  }
+}
+
+void MultiEndCall::CheckTiming() {
+  // TODO(alessiob): use audiotrack lengths and offset to check whether the
+  // timing is valid.
+}
+
+}  // namespace conversational_speech
+}  // namespace test
+}  // namespace webrtc