Conversational Speech tool, MultiEndCall::CheckTiming() and tests

webrtc/modules/audio_processing/test/conversational_speech/multiend_call.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_TEST_CONVERSATIONAL_SPEECH_MULTIEND_CALL_H_
 #define WEBRTC_MODULES_AUDIO_PROCESSING_TEST_CONVERSATIONAL_SPEECH_MULTIEND_CALL_H_
 
 #include <map>
 #include <memory>
 #include <set>
 #include <string>
+#include <utility>
+#include <vector>
 
 #include "webrtc/base/array_view.h"
 #include "webrtc/base/constructormagic.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 {
 
 class MultiEndCall {
  public:
+  struct SpeakingTurn {
+    // Constructor required in order to use std::vector::emaplace().
+    SpeakingTurn(std::string new_speaker_name,
+                 std::string new_audiotrack_file_name,
+                 std::size_t new_begin, std::size_t new_end)

[hlundin-webrtc, 2017/04/06 08:10:04]

We tend to use size_t instead of std::size_t. Include e.g., stddef.h for
declaration.

[AleBzk, 2017/04/06 16:42:42]

Done.

+        : speaker_name(std::move(new_speaker_name)),

[hlundin-webrtc, 2017/04/06 08:10:04]

In not an expert in move semantics, but this looks weird to me. Pruning it down
to a simple example, you are doing something like this:

class B; 

struct A {
  A(B new_b) : b(std::move(new_b) {}

  B b;
}

// Using the ctor:
B my_b;
A my_a(my_b);

If I'm not mistaken, this will result in the ctor parameter b being a copy,
which then is moved from.

I think you can make the ctor parameter a const ref, and do the copy inside the
ctor instead:
  A(const B& new_b) : b(new_b) {}
This will also result in one copy, but saves the unnecessary (and confusing)
move operation.

If you really want to move from my_b, you will have to declare the ctor
parameter as using &&:
  A(B&& new_b) : b(std::move(new_b)) {}
and call it with
A my_a(std::move(my_b));  // my_b is invalid after this.

[kwiberg-webrtc, 2017/04/06 08:35:32]

Well, yes. It's up to the caller to create the B that's passed to A's
constructor, and it can do that with the copy or move constructor. The following
will result in two moves and no copy:

  B my_b;
  A my_a(std::move(my_b));
That's a valid way to do it, but as you say it'll always do a copy. I think the
way the CL currently does it is better.
That'll change two things, compared to what the CL does now: (1) result in a
total of just one move instead of two, and (2) force the caller to do std::move
or something equivalent---copying won't be an option.

[AleBzk, 2017/04/06 16:42:42]

Thanks for your comments on this point.
Before answering inline below, two important notes:
- as reported in the comment before SpeakingTurn is defined, these objects are
created via std::vector::emplace_back()
- the original code in multiend_call.cc does not use std::move (search
"speaking_turns_.emplace_back" in MultiEndCall::CheckTiming()) and *should not*

On 2017/04/06 08:35:32, kwiberg-webrtc wrote:
We can't go for this last option otherwise, if I'm correct, I have to use
std::move while passing the ctor args in emplace_back. But what I pass there is
used afterwards, hence std::move is not possible.
The only alternative is going for const std::string& in the SpeakingTurn ctor as
Henrik suggested.

Result: for now I didn't change anything because we have a tie. Karl finds the
current CL better than const std::string&, but Henrik finds better the latter
for readability. Do the details I shared above about the caller (namely,
emplace_back) help in making a final decision?

[hlundin-webrtc, 2017/04/07 10:24:09]

You can keep this as is. I was mainly concerned that you *thought* you were
using move semantics all the way, avoiding copying completely, but were in fact
not.

[AleBzk, 2017/04/07 11:37:06]

Acknowledged.

+          audiotrack_file_name(std::move(new_audiotrack_file_name)),
+          begin(new_begin), end(new_end) {}
+    std::string speaker_name;
+    std::string audiotrack_file_name;
+    std::size_t begin;
+    std::size_t end;
+  };
+
   MultiEndCall(
       rtc::ArrayView<const Turn> timing, const std::string& audiotracks_path,
       std::unique_ptr<WavReaderAbstractFactory> wavreader_abstract_factory);
   ~MultiEndCall();
 
   const std::set<std::string>& speaker_names() const;
   const std::map<std::string, std::unique_ptr<WavReaderInterface>>&
       audiotrack_readers() const;
+  bool valid() const;
+  std::size_t total_duration_samples() const;

[hlundin-webrtc, 2017/04/06 08:10:04]

size_t

[AleBzk, 2017/04/06 16:42:42]

Done.

+  const std::vector<SpeakingTurn>& speaking_turns() const;
 
  private:
-  // Find unique speaker names.
+  // Finds unique speaker names.
   void FindSpeakerNames();
 
-  // Create one WavReader instance for each unique audiotrack.
+  // Creates one WavReader instance for each unique audiotrack.
   void CreateAudioTrackReaders();
 
-  // Check the speaking turns timing.
-  void CheckTiming();
+  // Validates the speaking turns timing information. Accepts cross-talk, but
+  // only up to 2 speakers. Rejects unordered turns and self cross-talk.
+  bool CheckTiming();
+
+  // Detects cross-talk, which occurs when two turns from the same speaker
+  // overlap in time.
+  bool DetectSelfCrossTalk(
+      const std::vector<std::size_t>& speaking_turn_indices) const;

[hlundin-webrtc, 2017/04/06 08:10:04]

size_t

[AleBzk, 2017/04/06 16:42:42]

Done.

 
   rtc::ArrayView<const Turn> timing_;
   const std::string& audiotracks_path_;
   std::unique_ptr<WavReaderAbstractFactory> wavreader_abstract_factory_;
   std::set<std::string> speaker_names_;
   std::map<std::string, std::unique_ptr<WavReaderInterface>>
       audiotrack_readers_;
+  bool valid_;
+  std::size_t total_duration_samples_;

[hlundin-webrtc, 2017/04/06 08:10:04]

size_t

[AleBzk, 2017/04/06 16:42:42]

Done.

+  std::vector<SpeakingTurn> speaking_turns_;
 
   RTC_DISALLOW_COPY_AND_ASSIGN(MultiEndCall);
 };
 
 }  // namespace conversational_speech
 }  // namespace test
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_PROCESSING_TEST_CONVERSATIONAL_SPEECH_MULTIEND_CALL_H_