Conversational Speech generator, unit test

Conversational Speech generator, adding unit test.

Test for the conversational_speech::Config class and renaming.

BUG=webrtc:7218
Review-Url: https://codereview.webrtc.org/2749573002
Cr-Commit-Position: refs/heads/master@{#17301}
Committed: https://chromium.googlesource.com/external/webrtc/+/3b51065c55e622867db89ec50a5e3c35691ebad4

[AleBzk, 2017-03-13 15:15:44]

Description was changed from

==========
ConvSpeechGeneratorSettings unit test

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

to

==========
Conversational Speech generator, unit test.

Test for the ConvSpeechGeneratorSettings class.

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

[AleBzk, 2017-03-13 15:15:44]

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

[AleBzk, 2017-03-13 15:19:47]

Hi Fredrik,

I'm creating a new tool to simulate conversation calls (for more details, see
the README.md file at
https://chromium.googlesource.com/external/webrtc/+/1d395dfb36e9ada73fc16aff0...).

I just added a first and basic unit test.

Alessio

[the sun, 2017-03-13 18:51:20]

On 2017/03/13 15:19:47, AleBzk wrote:
> Hi Fredrik,
> 
> I'm creating a new tool to simulate conversation calls (for more details, see
> the README.md file at
>
https://chromium.googlesource.com/external/webrtc/+/1d395dfb36e9ada73fc16aff0...).
> 
> I just added a first and basic unit test.
> 
> Alessio

Hi,

This is a bit of a nit, but there's not that much else to comment on at this
stage. :)

I have a problem with abbreviations in general, and in particular when they are
inconsistently used.

The folder name is conversational_speech, then the file is called
convspeech_gen_unittest.cc. Why the verbosity? Do you intend to have a
non-conversational speech generator in the same folder? Could you use
"generator_unittest.cc" instead and rename the other files?
conversational_speech/generator.cc, .h etc - that shorter and nicer IMO.

Besides, in signal processing "conv" is often short for "convolution".

(Same reasoning applies to the build targetes in the .gn)

[AleBzk, 2017-03-14 08:40:19]

Hi Fredrik,

Thanks for your comments!

I simplified the names. Only note the rule names in BUILD.gn: for the main
binary and the unit test I had to use the full name (with no abbreviation) since
the produced binaries are deployed in e.g., out/Default. Hence, "generator" and
"generator_unittest" would have been wrong - "conversational_speech_generator"
and "conversational_speech_generator_unittest" are deployed instead.

Alessio

[the sun, 2017-03-14 12:21:58]

lgtm

https://codereview.webrtc.org/2749573002/diff/40001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
(right):

https://codereview.webrtc.org/2749573002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:25:
ConvSpeechGeneratorSettings settings(
Here's another one of those abbreviations. Namespaces and scoping inside classes
are useful for things like this, e.g.:

namespace conversational_speech {

class Generator {
  struct Settings { // <-- In a lot of places we call this a "Config" (yeah, I
know, that's an abbreviation..)

[AleBzk, 2017-03-15 15:08:18]

The CQ bit was checked by alessiob@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-15 15:08:29]

Dry run: 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-03-15 15:09:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-15 15:10:00]

Dry run: Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/18607)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23891)

[AleBzk, 2017-03-18 09:37:49]

Description was changed from

==========
Conversational Speech generator, unit test.

Test for the ConvSpeechGeneratorSettings class.

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

to

==========
Conversational Speech generator, adding unit test.

Test for the conversational_speech::Config class and renaming.

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

[AleBzk, 2017-03-18 09:47:48]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-03-18 09:47:48]

The patchset sent to the CQ was uploaded after l-g-t-m from solenberg@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2749573002/#ps80001 (title:
"rebase")

[commit-bot: I haz the power, 2017-03-18 09:47:52]

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-03-18 09:49:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 09:49:22]

Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_dbg/builds/23999)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22789)

[AleBzk, 2017-03-18 09:57:07]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-03-18 09:57:07]

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

[commit-bot: I haz the power, 2017-03-18 09:57: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-03-18 09:58:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-18 09:58:44]

Try jobs failed on following builders:
  ios32_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios32_sim_ios9_dbg/buil...)
  ios64_sim_ios10_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios10_dbg/bui...)
  ios64_sim_ios9_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios64_sim_ios9_dbg/buil...)
  ios_arm64_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_arm64_dbg/builds/18716)
  ios_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/ios_rel/builds/22790)

[AleBzk, 2017-03-18 10:23:39]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-03-18 10:23:39]

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

[commit-bot: I haz the power, 2017-03-18 10:23:43]

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-03-18 10:45:31]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1489832619036270,
"parent_rev": "5f32aca85ef6c2ded2ec89b4b2a49f62b151223a", "commit_rev":
"3b51065c55e622867db89ec50a5e3c35691ebad4"}

[commit-bot: I haz the power, 2017-03-18 10:45:35]

Description was changed from

==========
Conversational Speech generator, adding unit test.

Test for the conversational_speech::Config class and renaming.

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

to

==========
Conversational Speech generator, adding unit test.

Test for the conversational_speech::Config class and renaming.

BUG=webrtc:7218

Review-Url: https://codereview.webrtc.org/2749573002
Cr-Commit-Position: refs/heads/master@{#17301}
Committed:
https://chromium.googlesource.com/external/webrtc/+/3b51065c55e622867db89ec50...
==========

[commit-bot: I haz the power, 2017-03-18 10:45:36]

Committed patchset #7 (id:120001) as
https://chromium.googlesource.com/external/webrtc/+/3b51065c55e622867db89ec50...

[AleBzk, 2017-03-20 08:38:40]

https://codereview.webrtc.org/2749573002/diff/40001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc
(right):

https://codereview.webrtc.org/2749573002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/conversational_speech/generator_unittest.cc:25:
ConvSpeechGeneratorSettings settings(
On 2017/03/14 12:21:57, the sun wrote:
> Here's another one of those abbreviations. Namespaces and scoping inside
classes
> are useful for things like this, e.g.:
> 
> namespace conversational_speech {
> 
> class Generator {
>   struct Settings { // <-- In a lot of places we call this a "Config" (yeah, I
> know, that's an abbreviation..)

Done.

[kjellander (google.com), 2017-03-23 06:24:35]

kjellander@google.com changed reviewers:
+ kjellander@google.com

[kjellander (google.com), 2017-03-23 06:24:36]

https://codereview.webrtc.org/2749573002/diff/120001/webrtc/modules/audio_pro...
File webrtc/modules/audio_processing/test/conversational_speech/BUILD.gn
(right):

https://codereview.webrtc.org/2749573002/diff/120001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/conversational_speech/BUILD.gn:43:
rtc_test("conversational_speech_generator_unittest") {
Adding a test like this doesn't make it run. Please add "generator_unittest.cc"
to modules_unittests instead and delete this rtc_test target. We need to keep
the number of tests down since it's a lot of overhead introducing new ones due
to many different systems are involved in their configuration.