APM QA unit tests, fake POLQA binary and minor fixes

This CL includes the following changes:
- BUILD file reorganized, unit tests now have dedicated targets.
- "fake_polqa" is a binary producing fake output in the same format of PolqaOem64; the binary is injected for unit tests instead of the actual POLQA tool.
- Minor refactoring to inject the path to the POLQA binary instead of its parent folder.
- Unit tests for the evaluation score workers.
- Unit tests for the ApmModuleSimulator class.
- Unit tests for the test data generators: ReverberationTestDataGenerator added.

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

[AleBzk, 2017-04-11 08:52:30]

Description was changed from

==========
simulation unit tests finalized
fake_polqa for unit testing
fake_polqa injected into EvaluationScoreWorkerFactory for unit testing
eval score workers ut

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

to

==========
This CL includes the following changes:
- BUILD file reorganized, unit tests now have dedicated targets.
- "fake_polqa" is a binary producing fake output in the same format of
PolqaOem64; the binary is injected for unit tests instead of the actual POLQA
tool.
- Minor refactoring to inject the path to the POLQA binary instead of its parent
folder.
- Unit tests for the evaluation score workers.
- Unit tests for the ApmModuleSimulator class.
- Unit tests for the test data generators: ReverberationTestDataGenerator added.

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

[AleBzk, 2017-04-11 08:52:30]

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

[AleBzk, 2017-04-11 08:59:35]

Hi Alex,

It'd be great if you review this CL since we will use the AMP QA tool for our
next project. The CL is mainly about unit tests.

Cheers,
Alessio

[aleloi, 2017-04-11 09:08:42]

Will do! Just a few APM CLs first...

[AleBzk, 2017-04-11 09:31:56]

Yup

[aleloi, 2017-04-12 09:37:05]

LG! Only minor comments.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/py_quality_assessment/BUILD.gn
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/BUILD.gn:124: deps =
[]
I think deps can be left out if it's empty.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/BUILD.gn:128:
testonly = true
visibility = ...

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/BUILD.gn:135: deps =
[]
Same here.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores_unittest.py
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores_unittest.py:67:
os.path.dirname((__file__)), 'fake_polqa')))
In my experience, relative paths always give trouble. How about
os.path.dirname(os.path.abspath(__file__)) ?

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/fake_polqa.cc
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/fake_polqa.cc:44:
}
I think using gflags.h will simplify the parsing. I'm not sure, but I think it
will handle things like 
  -Out "/path/with spaces/".

It's simple to add documentation to a flag and general usage description.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation_unittest.py
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation_unittest.py:12:
import operator
Not necessary (see below).

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation_unittest.py:86:
len(config_files), len(input_files), len(test_data_generators)], 1)
len(config_files) * len(input_files) * len(test_data_generators) is shorter and
simpler to read.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_unittest.py
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_unittest.py:39:
]
I don't understand what happens here. How are the air db irs used?

[AleBzk, 2017-04-12 12:41:04]

Thanks Alex! You can find my answers below.
Alessio

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/py_quality_assessment/BUILD.gn
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/BUILD.gn:124: deps =
[]
On 2017/04/12 09:37:05, aleloi wrote:
> I think deps can be left out if it's empty.

Done.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/BUILD.gn:128:
testonly = true
On 2017/04/12 09:37:05, aleloi wrote:
> visibility = ...

Done.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/BUILD.gn:135: deps =
[]
On 2017/04/12 09:37:05, aleloi wrote:
> Same here.

Done.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores_unittest.py
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores_unittest.py:67:
os.path.dirname((__file__)), 'fake_polqa')))
On 2017/04/12 09:37:05, aleloi wrote:
> In my experience, relative paths always give trouble. How about
> os.path.dirname(os.path.abspath(__file__)) ?

Done.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/fake_polqa.cc
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/fake_polqa.cc:44:
}
On 2017/04/12 09:37:05, aleloi wrote:
> I think using gflags.h will simplify the parsing. I'm not sure, but I think it
> will handle things like 
>   -Out "/path/with spaces/".
> 
> It's simple to add documentation to a flag and general usage description.

Thanks for your comment.
Actually, that was my first choice.
But there was something annoying. If the caller adds args that are not define
with gflags, the arg parsing fails.
Unfortunately, this is the case and it would require to add several unnecessary
args making the code longer.
Plus, I don't need args doc since this is just a file that mimics POLQA
generating fake output.

I strongly prefer this way. WDYT?

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation_unittest.py
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation_unittest.py:12:
import operator
On 2017/04/12 09:37:05, aleloi wrote:
> Not necessary (see below).

Done.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation_unittest.py:86:
len(config_files), len(input_files), len(test_data_generators)], 1)
On 2017/04/12 09:37:05, aleloi wrote:
> len(config_files) * len(input_files) * len(test_data_generators) is shorter
and
> simpler to read.

Done.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_unittest.py
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_unittest.py:39:
]
On 2017/04/12 09:37:05, aleloi wrote:
> I don't understand what happens here. How are the air db irs used?

The for loop below creates random impulse responses, one for each file name in
|impulse_response_mat_file_names|.
The generated impulse responses are stored in Matlab files under the temp
directory self._fake_air_db_path.

The latter path is passed to the TestDataGeneratorFactory factory further down.
You can see this as dependency injection: instead of passing the path to the
actual AIR DB, we create a fake version of AIR DB and pass its path.

There is one thing that can be improved here. At this stage, the class making
use of AIR DB requires the files named air_binaural_lecture_0_0_1.mat and
air_binaural_booth_0_0_1.mat (see
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...).
The test fixture uses this knowledge instead of "pulling" the list of file names
directly from the ReverberationTestDataGenerator class. This will change soonish
since I planned to allow the user to decide which AIR DB files should be used.

To clarify this I added a TODO here.
WDYT?

[aleloi, 2017-04-12 13:05:40]

lgtm

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/fake_polqa.cc
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/fake_polqa.cc:44:
}
Ok, this way seems best then.

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_unittest.py
(right):

https://codereview.webrtc.org/2811953002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_unittest.py:39:
]
On 2017/04/12 12:41:04, AleBzk wrote:
> On 2017/04/12 09:37:05, aleloi wrote:
> > I don't understand what happens here. How are the air db irs used?
> 
> The for loop below creates random impulse responses, one for each file name in
> |impulse_response_mat_file_names|.
> The generated impulse responses are stored in Matlab files under the temp
> directory self._fake_air_db_path.
> 
> The latter path is passed to the TestDataGeneratorFactory factory further
down.
> You can see this as dependency injection: instead of passing the path to the
> actual AIR DB, we create a fake version of AIR DB and pass its path.
> 
> There is one thing that can be improved here. At this stage, the class making
> use of AIR DB requires the files named air_binaural_lecture_0_0_1.mat and
> air_binaural_booth_0_0_1.mat (see
>
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/modules/aud...).
> The test fixture uses this knowledge instead of "pulling" the list of file
names
> directly from the ReverberationTestDataGenerator class. This will change
soonish
> since I planned to allow the user to decide which AIR DB files should be used.
> 
> To clarify this I added a TODO here.
> WDYT?

Acknowledged.

[AleBzk, 2017-04-12 13:30:52]

The CQ bit was checked by alessiob@webrtc.org

[commit-bot: I haz the power, 2017-04-12 13:30:59]

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-12 13:56:25]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1492003852973230,
"parent_rev": "103ac7e7d9c3548bf43ffa3a24b5e89689695c02", "commit_rev":
"a79143f3e9ee94d332d52462356a0a955b05d63b"}

[commit-bot: I haz the power, 2017-04-12 13:56:29]

Description was changed from

==========
This CL includes the following changes:
- BUILD file reorganized, unit tests now have dedicated targets.
- "fake_polqa" is a binary producing fake output in the same format of
PolqaOem64; the binary is injected for unit tests instead of the actual POLQA
tool.
- Minor refactoring to inject the path to the POLQA binary instead of its parent
folder.
- Unit tests for the evaluation score workers.
- Unit tests for the ApmModuleSimulator class.
- Unit tests for the test data generators: ReverberationTestDataGenerator added.

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

to

==========
This CL includes the following changes:
- BUILD file reorganized, unit tests now have dedicated targets.
- "fake_polqa" is a binary producing fake output in the same format of
PolqaOem64; the binary is injected for unit tests instead of the actual POLQA
tool.
- Minor refactoring to inject the path to the POLQA binary instead of its parent
folder.
- Unit tests for the evaluation score workers.
- Unit tests for the ApmModuleSimulator class.
- Unit tests for the test data generators: ReverberationTestDataGenerator added.

BUG=webrtc:7218

Review-Url: https://codereview.webrtc.org/2811953002
Cr-Commit-Position: refs/heads/master@{#17674}
Committed:
https://chromium.googlesource.com/external/webrtc/+/a79143f3e9ee94d332d524623...
==========

[commit-bot: I haz the power, 2017-04-12 13:56:31]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/external/webrtc/+/a79143f3e9ee94d332d524623...