APM QA refactoring: render stream support, echo path simulation, new export engine

This CL is a refactoring of the APM QA tool; it includes the following changes:
- render stream support, required to assess AEC;
- echo path simulation and input mixer, to generate echo and add it to the
speech signal;
- export engine: improved UI, switch to Pandas DataFrames;
- minor design improvements and needed adaptions.

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

[AleBzk, 2017-04-11 14:54:54]

Description was changed from

==========
TODOs for EchoPathSimulator in simulation
EchoPathSimulator docstring added
interface, 2 empty implementations and simple unit test
reverse input support into APM QA pipeline

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

to

==========
This CL modifies the APM QA tool pipeline in order to allow reverse input. When
the latter is used, it is also required to specify an echo path simulator with
which the forward input for APM is created. In general, an echo path simulator
mixes the near-end and the far-end simulating what a microphone records due to
the presence of echo paths (e.g., mechanical or electrical path).

The EchoPathSimulator and two concrete classes are defined, but the latter are
not implemented yet. Before proceeding, we should agree on design and naming.

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

[AleBzk, 2017-04-11 14:54:54]

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

[AleBzk, 2017-04-11 14:57:03]

Description was changed from

==========
This CL modifies the APM QA tool pipeline in order to allow reverse input. When
the latter is used, it is also required to specify an echo path simulator with
which the forward input for APM is created. In general, an echo path simulator
mixes the near-end and the far-end simulating what a microphone records due to
the presence of echo paths (e.g., mechanical or electrical path).

The EchoPathSimulator and two concrete classes are defined, but the latter are
not implemented yet. Before proceeding, we should agree on design and naming.

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

to

==========
This CL modifies the APM QA tool pipeline in order to allow reverse input. When
the latter is used, it is also required to specify an echo path simulator with
which the forward input for APM is created. In general, an echo path simulator
mixes the near-end and the far-end simulating what a microphone records due to
the presence of echo paths (e.g., mechanical or electrical path).

The EchoPathSimulator interface and two concrete classes are defined, but the
latter are not implemented yet. Before proceeding, we should agree on design and
naming.

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

[AleBzk, 2017-04-11 15:00:00]

Hi Per,

This should be straightforward to review. The CL is about echo paths simulation
using the APM QA tool. I would like to avoid issues with unclear naming, that's
why I only uploaded the interface and left the implementation for next CLs.

Alessio

[peah-webrtc, 2017-04-21 05:28:45]

Thanks for the CL. I have some comments.

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py
(right):

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py:25:
_AUDIOPROC_F_BIN_PATH = os.path.abspath(os.path.join(
What does this mean? Is the location of audioproc_f hardcoded? Would it not be
better to have it either as part of the searchpath, and/or allow a specific path
to be specified on the command line?

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py:41:
reverse_input_filepath=None):
I'd prefer if capture and render is used instead of forward and reverse. I think
those make more sense, and the plan is to change the code to use that naming
instead.

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py:69:
if reverse_input_filepath is not None:
Can't this be simplified to
 if reverse_input_filepath:

?

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py
(right):

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:60:
tracks and reverse input audio tracks has to equal. The two lists are used
have?

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:105:
# TODO(alessiob): Instance EchoPathSimulator through factory.
Instantiate ?

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:219:
def _SetProbingSignalFilePaths(self, input_filepaths,
The naming of this method does not seem intuitive or descriptive to me. What is
ProbingSignals?

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:223:
Pairs the input and rever input files by storing the file paths into two
reverse ?

[AleBzk, 2017-04-24 13:29:59]

Patchset #3 (id:40001) has been deleted

[AleBzk, 2017-04-24 14:30:42]

Thanks for your comments Per! Everything has been addressed.

Alessio

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py
(right):

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py:25:
_AUDIOPROC_F_BIN_PATH = os.path.abspath(os.path.join(
On 2017/04/21 05:28:45, peah-webrtc wrote:
> What does this mean? Is the location of audioproc_f hardcoded? Would it not be
> better to have it either as part of the searchpath, and/or allow a specific
path
> to be specified on the command line?

True, that would be better, but I didn't want to add too much boilerplate.
Instead, I made audioproc_f a dependency (see the BUILD file).
Since we know the relative path between audioproc_f and this wrapper Python
file, I see safe leaving everything as it is now. WDYT?

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py:41:
reverse_input_filepath=None):
On 2017/04/21 05:28:45, peah-webrtc wrote:
> I'd prefer if capture and render is used instead of forward and reverse. I
think
> those make more sense, and the plan is to change the code to use that naming
> instead.

Done, including changes in the rest of the APM QA tool.

As discussed offline, I won't touch audioproc_f, which currently uses the
forward/reverse naming convention.
Switching to the capture/render naming convention may break things, hence
changes in audioproc_f are left for separate CLs.

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py:69:
if reverse_input_filepath is not None:
On 2017/04/21 05:28:45, peah-webrtc wrote:
> Can't this be simplified to
>  if reverse_input_filepath:
> 
> ?

Personally I don't like implicit casts in this case, but what you suggest also
covers the case of an empty string.
Let's do this: I will improve the code checking if the file exists and raising
an exception if it doesn't.

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py
(right):

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:60:
tracks and reverse input audio tracks has to equal. The two lists are used
On 2017/04/21 05:28:45, peah-webrtc wrote:
> have?

Done.

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:105:
# TODO(alessiob): Instance EchoPathSimulator through factory.
On 2017/04/21 05:28:45, peah-webrtc wrote:
> Instantiate ?

Done.

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:219:
def _SetProbingSignalFilePaths(self, input_filepaths,
On 2017/04/21 05:28:45, peah-webrtc wrote:
> The naming of this method does not seem intuitive or descriptive to me. What
is
> ProbingSignals?

Renamed to _SetTestInputSignalFilePaths().

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:223:
Pairs the input and rever input files by storing the file paths into two
On 2017/04/21 05:28:45, peah-webrtc wrote:
> reverse ?

Done.

[peah-webrtc, 2017-05-03 08:10:08]

lgtm conditioned that you address the comments about the comments in the code.

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py
(right):

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py:25:
_AUDIOPROC_F_BIN_PATH = os.path.abspath(os.path.join(
On 2017/04/24 14:30:41, AleBzk wrote:
> On 2017/04/21 05:28:45, peah-webrtc wrote:
> > What does this mean? Is the location of audioproc_f hardcoded? Would it not
be
> > better to have it either as part of the searchpath, and/or allow a specific
> path
> > to be specified on the command line?
> 
> True, that would be better, but I didn't want to add too much boilerplate.
> Instead, I made audioproc_f a dependency (see the BUILD file).
> Since we know the relative path between audioproc_f and this wrapper Python
> file, I see safe leaving everything as it is now. WDYT?

Sounds good!

https://codereview.webrtc.org/2813883002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/audioproc_wrapper.py:69:
if reverse_input_filepath is not None:
On 2017/04/24 14:30:41, AleBzk wrote:
> On 2017/04/21 05:28:45, peah-webrtc wrote:
> > Can't this be simplified to
> >  if reverse_input_filepath:
> > 
> > ?
> 
> Personally I don't like implicit casts in this case, but what you suggest also
> covers the case of an empty string.
> Let's do this: I will improve the code checking if the file exists and raising
> an exception if it doesn't.

Sounds good! I'm definitely not a python Guru but I have the impression that the
check falsy (not false) values is very common. 
Both variants are fine with me though.

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
(right):

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:107:
'length of --capture_input_files')
"of" seems wrong in this sentence, please reformulate.

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py
(right):

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:18:
In general, an echo path simulator mixes the capture and the render signals
I would not characterize an echo path simulator as that. I think an echo path
simulator only simulates the echo path and I would not expect any mixing of the
capture signal to be part of the echo path simulation. Is it possible to
reformulate this accordingly? Or should we rather change the name of the class?

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:61:
It applies a given impulse response to the render input and then it sums the
Please replace "It" with something else, as it is a bit ambiguous. Suggestions:
It applies -> Applies
It applies  -> This class applies
It applies -> This simulator applies

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:61:
It applies a given impulse response to the render input and then it sums the
This is not clear to me: then it sums the signal to the capture input signal.

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:82:
# @EchoPathSimulator.RegisterClass
What makes RecordedEchoPathSimulator from LinearEchoPathSimulator? The latter is
registered, but not the former. And neither of them yet contain the simulation
code. it would make sense to treat those similarly in terms of
@EchoPathSimulator.RegisterClass

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:86:
It uses the clean capture input file name to build the file name of the
This is an echopath simulator, and I think it would be better if the comment
mentioned more about how the simulation is done. As it is now, it mentions the
construction of file names which is a bit misleading I think.

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:87:
corresponding recording containing capture input and echo. Such file is
Such a file

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py
(right):

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:61:
equal. The two lists are used to form pairs of capture and render input.
be equal

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:61:
equal. The two lists are used to form pairs of capture and render input.
input->inputs

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:106:
# TODO(alessiob): Instanciate EchoPathSimulator through factory.
Instanciate -> Instantiate

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:116:
without_render_input = self._render_input_filepaths is None
suggestion: without_render_input -> no_render_input

That seems to work better in the statements below:
None if without_render_input -> None if no_render_input
if without_render_input -> if no_render_input

[AleBzk, 2017-05-12 15:39:04]

Description was changed from

==========
This CL modifies the APM QA tool pipeline in order to allow reverse input. When
the latter is used, it is also required to specify an echo path simulator with
which the forward input for APM is created. In general, an echo path simulator
mixes the near-end and the far-end simulating what a microphone records due to
the presence of echo paths (e.g., mechanical or electrical path).

The EchoPathSimulator interface and two concrete classes are defined, but the
latter are not implemented yet. Before proceeding, we should agree on design and
naming.

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

to

==========
This CL modifies the APM QA tool pipeline in order to support the render stream
analysis. When the latter is used, it is also required to specify an echo path
simulator, which is mixed to speech in order to form the capture signal for APM.

In general, an echo path simulator simulates what a microphone records due to
the presence of echo paths (e.g., mechanical or electrical path).

This CL also includes a new export engine. The change was necessary because of
the changes described above. 

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

[AleBzk, 2017-05-15 08:20:28]

Description was changed from

==========
This CL modifies the APM QA tool pipeline in order to support the render stream
analysis. When the latter is used, it is also required to specify an echo path
simulator, which is mixed to speech in order to form the capture signal for APM.

In general, an echo path simulator simulates what a microphone records due to
the presence of echo paths (e.g., mechanical or electrical path).

This CL also includes a new export engine. The change was necessary because of
the changes described above. 

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

to

==========
This CL is a refactoring of the APM QA tool; it includes the following changes:
- render stream support, required to assess AEC;
- echo path simulation and input mixer, to generate echo and add it to the
speech signal;
- export engine: improved UI, switch to Pandas DataFrames;
- minor design improvements and needed adaptions.

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

[AleBzk, 2017-05-15 08:20:29]

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

[AleBzk, 2017-05-15 08:22:58]

Hi all,

This is a big CL that I could not split. I therefore added multiple reviewer,
asking everyone to check different parts (see below).

[Per]

I addressed your comments from the last PS by decoupling the echo path
simulation and the mixing of speech and echo. I therefore added the input_mixer
module, for which we had an offline discussion concluding that the gain should
not be adjusted and that a warning should be shown in case of saturation.
 - quality_assessment/audioproc_wrapper.py
 - quality_assessment/echo_path_simulation.py
 - quality_assessment/echo_path_simulation_factory.py
 - quality_assessment/echo_path_simulation_unittest.py
 - quality_assessment/input_mixer.py
 - quality_assessment/input_mixer_unittest.py
 - quality_assessment/signal_processing.py

[Alex]

The export script was using an ad-hoc nested dictionary with which the data was
efficiently exported into an HTML file. I removed this coupling by switching to
Pandas DataFrames. Then, I also rewrote almost from scratch the export class. It
now uses Material Design Lite (https://getmdl.io/) and the results are not shown
at once anymore. Instead, one stats table is generated for each score and by
clicking on a cell, one can find more details. Finally, it is now possible to
listen to the simulated echo audio track.
 - apm_quality_assessment_export.py
 - quality_assessment/export.py
 - quality_assessment/results.css
 - quality_assessment/results.js

[Ivo]

The simulation module now supports render streams and it became responsible
for naming output directories and files.
 - quality_assessment/simulation.py
 - quality_assessment/simulation_unittest.py

Output directory prefix added, the simulation module is now responsible for
naming the output directories.
 - quality_assessment/test_data_generation.py
 - quality_assessment/test_data_generation_factory.py
 - quality_assessment/test_data_generation_unittest.py

Score file name prefix added, the simulation module is now responsible for
naming the output files.
 - quality_assessment/eval_scores.py
 - quality_assessment/eval_scores_factory.py
 - quality_assessment/eval_scores_unittest.py

Minor changes.
 - BUILD.gn
 - README.md
 - apm_quality_assessment.py
 - apm_quality_assessment.sh
 - quality_assessment/data_access.py
 - quality_assessment/exceptions.py

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
(right):

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:107:
'length of --capture_input_files')
On 2017/05/03 08:10:08, peah-webrtc wrote:
> "of" seems wrong in this sentence, please reformulate.

Done.

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py
(right):

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:18:
In general, an echo path simulator mixes the capture and the render signals
On 2017/05/03 08:10:08, peah-webrtc wrote:
> I would not characterize an echo path simulator as that. I think an echo path
> simulator only simulates the echo path and I would not expect any mixing of
the
> capture signal to be part of the echo path simulation. Is it possible to
> reformulate this accordingly? Or should we rather change the name of the
class?

Thanks a lot for this comment.
I mixed two responsibilities in this class.
As we discussed offline, I decoupled them adding ApmInputMixer.

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:61:
It applies a given impulse response to the render input and then it sums the
On 2017/05/03 08:10:08, peah-webrtc wrote:
> This is not clear to me: then it sums the signal to the capture input signal.

Doesn't apply anymore.

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:82:
# @EchoPathSimulator.RegisterClass
On 2017/05/03 08:10:08, peah-webrtc wrote:
> What makes RecordedEchoPathSimulator from LinearEchoPathSimulator? The latter
is
> registered, but not the former. And neither of them yet contain the simulation
> code. it would make sense to treat those similarly in terms of
> @EchoPathSimulator.RegisterClass

Fixed. I also added the implementation and a "no echo" simulator.

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:86:
It uses the clean capture input file name to build the file name of the
On 2017/05/03 08:10:08, peah-webrtc wrote:
> This is an echopath simulator, and I think it would be better if the comment
> mentioned more about how the simulation is done. As it is now, it mentions the
> construction of file names which is a bit misleading I think.

Well, I could describe the setup to record echo.
But I don't think this class is the right place for doing that.
Let's discuss this point offline.

https://codereview.webrtc.org/2813883002/diff/60001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/echo_path_simulation.py:87:
corresponding recording containing capture input and echo. Such file is
On 2017/05/03 08:10:08, peah-webrtc wrote:
> Such a file

Done.

[peah-webrtc, 2017-05-15 10:52:14]

I have only some comments about the hard clipping detection. 

I will, however, have probably more comments once the CL for the actual tests
that are implemented with the framework are constructed, but lets do those
changes when that happens.

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py
(right):

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:152:
def DetectHardClipping(cls, signal, threshold=3):
Where does this value (3) come from? Is it tailored to a specific test? If not,
I'd rather have expected 1 as a default value.
However, as this is likely very specific for what test is done, I'd rather have
this as a mandatory requirement.

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/signal_processing.py:175:
if samples.typecode != 'h':
I don't see why we should have this restriction.

[ivoc, 2017-05-15 14:56:55]

LGTM, see some minor comments below.

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py
(right):

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:68:
return cls._PREFIX_APM_CONFIG
How about making these constants public instead? (So without the _)

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:102:
input audio tracks and the number of render input audio tracks have to
input audio tracks have to be equal ("be" is missing)

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:235:
clean_capture_input_filepath: clean capture input audio track file to
It would be good to explain briefly what "clean" means in this context.

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:267:
echo_path_filpath = echo_path_simulator.Simulate(
Should be "echo_path_filepath" I presume?

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:342:
return os.path.splitext(os.path.split(filepath)[1])[0]
Why does this take [1] after splitting? Is that the final part with the filename
in it? In that case wouldn't -1 be more robust?

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation.py
(right):

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation.py:46:
expected at the output of the APM module when the latter is fed with the nosiy
noisy

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_factory.py
(right):

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_factory.py:34:
"""
Please describe the return value.

[AleBzk, 2017-05-19 15:45:35]

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py
(right):

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:68:
return cls._PREFIX_APM_CONFIG
On 2017/05/15 14:56:55, ivoc wrote:
> How about making these constants public instead? (So without the _)

It'd be much less boilerplate, but nothing prevents the caller to change the
value.
If you think that formatting the var name as const is safe enough, then I can
simplify that.
The only automatic complain would come from the linter. And a reviewer can
quickly see that a constant is redefined.

WDYT?

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:102:
input audio tracks and the number of render input audio tracks have to
On 2017/05/15 14:56:55, ivoc wrote:
> input audio tracks have to be equal ("be" is missing)

Done.

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:235:
clean_capture_input_filepath: clean capture input audio track file to
On 2017/05/15 14:56:55, ivoc wrote:
> It would be good to explain briefly what "clean" means in this context.

Done.

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:267:
echo_path_filpath = echo_path_simulator.Simulate(
On 2017/05/15 14:56:55, ivoc wrote:
> Should be "echo_path_filepath" I presume?

Done.

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:342:
return os.path.splitext(os.path.split(filepath)[1])[0]
On 2017/05/15 14:56:55, ivoc wrote:
> Why does this take [1] after splitting? Is that the final part with the
filename
> in it? In that case wouldn't -1 be more robust?

Done.

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation.py
(right):

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation.py:46:
expected at the output of the APM module when the latter is fed with the nosiy
On 2017/05/15 14:56:55, ivoc wrote:
> noisy

Done.

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
File
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_factory.py
(right):

https://codereview.webrtc.org/2813883002/diff/100001/webrtc/modules/audio_pro...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/test_data_generation_factory.py:34:
"""
On 2017/05/15 14:56:55, ivoc wrote:
> Please describe the return value.

Done.

[AleBzk, 2017-07-31 12:07:17]

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

[AleBzk, 2017-08-01 11:16:42]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-08-01 11:16:43]

The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org,
ivoc@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2813883002/#ps140001 (title:
"merge")

[commit-bot: I haz the power, 2017-08-01 11:16:46]

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-08-01 11:37:22]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1501586202895920,
"parent_rev": "8a1d2a315fb44a936cdf6eee646b7067e66ee8ea", "commit_rev":
"fdd568eb25d2211a4702fd69a604f66bd561101f"}

[commit-bot: I haz the power, 2017-08-01 11:37:27]

Description was changed from

==========
This CL is a refactoring of the APM QA tool; it includes the following changes:
- render stream support, required to assess AEC;
- echo path simulation and input mixer, to generate echo and add it to the
speech signal;
- export engine: improved UI, switch to Pandas DataFrames;
- minor design improvements and needed adaptions.

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

to

==========
This CL is a refactoring of the APM QA tool; it includes the following changes:
- render stream support, required to assess AEC;
- echo path simulation and input mixer, to generate echo and add it to the
speech signal;
- export engine: improved UI, switch to Pandas DataFrames;
- minor design improvements and needed adaptions.

BUG=webrtc:7218

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

[commit-bot: I haz the power, 2017-08-01 11:37:28]

Committed patchset #7 (id:140001) as
https://chromium.googlesource.com/external/webrtc/+/fdd568eb25d2211a4702fd69a...