Conversational Speech tool, WavReaderAdaptor and unit test

WavReaderAdaptor is a simple adaptor of the existing class WavReader from webrtc/common_audio/wav_file.h. The adaptor was mainly needed to use dependency injection and easily test the MultiEndCall class (see https://codereview.webrtc.org/2761853002/).

The unit test ConversationalSpeechTest.MultiEndCallWavReaderAdaptorSine uses CreateSineWavFile() and writes temporary wav files that are used for test (deleted only if the test passes).

BUG=webrtc:7218
Review-Url: https://codereview.webrtc.org/2774423005
Cr-Commit-Position: refs/heads/master@{#17608}
Committed: https://chromium.googlesource.com/external/webrtc/+/36e6a8f1bd1fd91ed5d9f6c29e6e0ae77881a710

[AleBzk, 2017-03-28 16:05:04]

Description was changed from

==========
WavReaderAdaptor implemented, audio track properties unit test.

BUG=webrtc:7218
==========

to

==========
WavReaderAdaptor is a simple adaptor of the existing class WavReader from
webrtc/common_audio/wav_file.h. The adaptor was mainly needed to use dependency
injection and easily test the MultiEndCall class (see
https://codereview.webrtc.org/2761853002/).

The unit test ConversationalSpeechTest.MultiEndCallWavReaderAdaptorSine uses
CreateSineWavFile() and writes temporary wav files that are used for test
(deleted only if the test passes).

BUG=webrtc:7218
==========

[AleBzk, 2017-03-28 16:07:05]

alessiob@webrtc.org changed reviewers:
+ kwiberg@webrtc.org

[AleBzk, 2017-03-28 16:07:06]

Hi Karl, I finalized the implementation of an adaptor of WavReader. There is
also a new unit test for it. Alessio

[kwiberg-webrtc, 2017-03-28 17:52:45]

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
(right):

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:36:
#define _USE_MATH_DEFINES
A comment on what effect this has, and on which header file?

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:108:
const double two_pi = 2.0 * M_PI;
constexpr

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:109:
std::unique_ptr<int16_t[]> samples(new int16_t[params.num_samples]);
It's easier to avoid bugs if you use e.g. a std::vector<int16_t>.

If you expect the number of samples to be large (or if performance was a
concern), it would've been better to allocate a fixed-size std::array on the
stack and repeatedly fill it and write it to disk.

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:472:
std::size_t num_samples = duration_seconds * sample_rate;
const?

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.h
(right):

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.h:32:
size_t ReadInt16Samples(size_t num_samples, int16_t* samples) override;
Not this CL's fault, but the arguments here really should have been ArrayViews.

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.h:38:
WavReader wav_reader_;
When you add this private member variable, you have to #include wav_file.h,
which is visible to anyone who #includes this file.

If you're open to moving code around a bit, I'd suggest moving this entire class
definition to the .cc file, and keeping just one thing in this header:

  std::unique_ptr<WavReaderInterface> CreateWavReaderAdaptor(const std::string&
filepath);

As an added bonus, everything in the .cc file (except for the definition of that
function) can then be in an anonymous namespace.

[AleBzk, 2017-03-29 08:56:45]

Thanks a lot. Everything addressed.

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
(right):

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:36:
#define _USE_MATH_DEFINES
On 2017/03/28 17:52:45, kwiberg-webrtc wrote:
> A comment on what effect this has, and on which header file?

Done.

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:108:
const double two_pi = 2.0 * M_PI;
On 2017/03/28 17:52:45, kwiberg-webrtc wrote:
> constexpr

Done.

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:109:
std::unique_ptr<int16_t[]> samples(new int16_t[params.num_samples]);
On 2017/03/28 17:52:45, kwiberg-webrtc wrote:
> It's easier to avoid bugs if you use e.g. a std::vector<int16_t>.
> 
> If you expect the number of samples to be large (or if performance was a
> concern), it would've been better to allocate a fixed-size std::array on the
> stack and repeatedly fill it and write it to disk.

Done.

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:472:
std::size_t num_samples = duration_seconds * sample_rate;
On 2017/03/28 17:52:44, kwiberg-webrtc wrote:
> const?

Done.

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.h
(right):

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.h:32:
size_t ReadInt16Samples(size_t num_samples, int16_t* samples) override;
On 2017/03/28 17:52:45, kwiberg-webrtc wrote:
> Not this CL's fault, but the arguments here really should have been
ArrayViews.

Done.

https://codereview.webrtc.org/2774423005/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.h:38:
WavReader wav_reader_;
On 2017/03/28 17:52:45, kwiberg-webrtc wrote:
> When you add this private member variable, you have to #include wav_file.h,
> which is visible to anyone who #includes this file.
> 
> If you're open to moving code around a bit, I'd suggest moving this entire
class
> definition to the .cc file, and keeping just one thing in this header:
> 
>   std::unique_ptr<WavReaderInterface> CreateWavReaderAdaptor(const
std::string&
> filepath);
> 
> As an added bonus, everything in the .cc file (except for the definition of
that
> function) can then be in an anonymous namespace.

Done.

[kwiberg-webrtc, 2017-03-29 09:08:28]

lgtm, but see nits

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.cc
(right):

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.cc:26:
class WavReaderAdaptor : public WavReaderInterface {
final? Like const, it's usually a good idea to use it whenever the compiler lets
you.

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/conversational_speech/wavreader_factory.cc
(right):

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_factory.cc:26:
}
Hmm. Can you eliminate either WavReaderFactory::Create() or
CreateWavReaderAdaptor()? They seem to do exactly the same thing...

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/conversational_speech/wavreader_interface.h
(right):

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_interface.h:34:
virtual size_t num_samples() const = 0;
Opinion: lower_case names are allowed for trivial getters/setters and the like.
Since these are virtual, the interface can't know what the implementations will
do, so it isn't a great fit to use lower_case names.

[AleBzk, 2017-03-29 09:34:43]

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.cc
(right):

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_adaptor.cc:26:
class WavReaderAdaptor : public WavReaderInterface {
On 2017/03/29 09:08:28, kwiberg-webrtc wrote:
> final? Like const, it's usually a good idea to use it whenever the compiler
lets
> you.

Done.

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/conversational_speech/wavreader_factory.cc
(right):

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_factory.cc:26:
}
On 2017/03/29 09:08:28, kwiberg-webrtc wrote:
> Hmm. Can you eliminate either WavReaderFactory::Create() or
> CreateWavReaderAdaptor()? They seem to do exactly the same thing...

Right. I can move everything into the factory.

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/conversational_speech/wavreader_interface.h
(right):

https://codereview.webrtc.org/2774423005/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/conversational_speech/wavreader_interface.h:34:
virtual size_t num_samples() const = 0;
On 2017/03/29 09:08:28, kwiberg-webrtc wrote:
> Opinion: lower_case names are allowed for trivial getters/setters and the
like.
> Since these are virtual, the interface can't know what the implementations
will
> do, so it isn't a great fit to use lower_case names.

Right. I also got this comment in a previous CL. I wanted to keep the same
interface of WavReader (webrtc/common_audio/wav_file.h). Since the interface has
anyway changed, I will follow your tip.

[kwiberg-webrtc, 2017-03-29 10:44:48]

lgtm!

[AleBzk, 2017-04-10 07:31:00]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-04-10 07:31:01]

The patchset sent to the CQ was uploaded after l-g-t-m from kwiberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2774423005/#ps80001 (title:
"BUILD deps fixed")

[commit-bot: I haz the power, 2017-04-10 07:31:12]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2017-04-10 07:53:54]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1491809460591390,
"parent_rev": "2042c16be026f44a66dda8373ed812ce38fd2a55", "commit_rev":
"36e6a8f1bd1fd91ed5d9f6c29e6e0ae77881a710"}

[commit-bot: I haz the power, 2017-04-10 07:53:57]

Description was changed from

==========
WavReaderAdaptor is a simple adaptor of the existing class WavReader from
webrtc/common_audio/wav_file.h. The adaptor was mainly needed to use dependency
injection and easily test the MultiEndCall class (see
https://codereview.webrtc.org/2761853002/).

The unit test ConversationalSpeechTest.MultiEndCallWavReaderAdaptorSine uses
CreateSineWavFile() and writes temporary wav files that are used for test
(deleted only if the test passes).

BUG=webrtc:7218
==========

to

==========
WavReaderAdaptor is a simple adaptor of the existing class WavReader from
webrtc/common_audio/wav_file.h. The adaptor was mainly needed to use dependency
injection and easily test the MultiEndCall class (see
https://codereview.webrtc.org/2761853002/).

The unit test ConversationalSpeechTest.MultiEndCallWavReaderAdaptorSine uses
CreateSineWavFile() and writes temporary wav files that are used for test
(deleted only if the test passes).

BUG=webrtc:7218

Review-Url: https://codereview.webrtc.org/2774423005
Cr-Commit-Position: refs/heads/master@{#17608}
Committed:
https://chromium.googlesource.com/external/webrtc/+/36e6a8f1bd1fd91ed5d9f6c29...
==========

[commit-bot: I haz the power, 2017-04-10 07:53:58]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/external/webrtc/+/36e6a8f1bd1fd91ed5d9f6c29...