APM Quality Generator, noise generator and evaluation score workers factory + echo noise generator

I added two factory classes to address an important comment I got in another CL from kjellander@webrtc.org.
This CL also includes the implementation of the echo noise generator.
I added comments in the diff to help the reviewers.

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2715233003
Cr-Commit-Position: refs/heads/master@{#17507}
Committed: https://chromium.googlesource.com/external/webrtc/+/54ad3df1d290b202bbd3917b170df5e7cbf95f29

[AleBzk, 2017-02-28 16:04:31]

Description was changed from

==========
Echo noise generator.

BUG=webrtc:7218
NOTRY=True
==========

to

==========
Echo noise generator.

BUG=webrtc:7218
NOTRY=True
==========

[AleBzk, 2017-02-28 16:04:31]

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

[AleBzk, 2017-02-28 16:18:50]

This is another CL about noise generation. This time it implements local echo
(not echo path). It essentially is a convolution applied to the input signal.

[opp90350, 2017-03-06 11:17:23]

opp90350@gmail.com changed reviewers:
+ opp90350@gmail.com

[opp90350, 2017-03-06 11:17:24]

opp90350@gmail.com changed reviewers:
+ opp90350@gmail.com

[AleBzk, 2017-03-06 11:24:13]

alessiob@webrtc.org changed reviewers:
- opp90350@gmail.com

[AleBzk, 2017-03-06 11:25:20]

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

[AleBzk, 2017-03-06 11:29:01]

Description was changed from

==========
Echo noise generator.

BUG=webrtc:7218
NOTRY=True
==========

to

==========
I added two factory classes to address an important comment I got in another CL
from kjellander@webrtc.org.
This CL also includes the implementation of the echo noise generator.
I added comments in the diff to help the reviewers.

BUG=webrtc:7218
NOTRY=True
==========

[AleBzk, 2017-03-06 11:29:54]

Hi,

I updated this CL and added Henrik as reviewer since I start addressing in this
CL a request I got from him on another CL.

Best,
Alessio

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:80:

@kjellander: the environment variable, if defined, is now only used as default
for the shell arguments

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:93:
polqa_tool_path=args.polqa_path)
@kjellander: the required paths are now passed to the APM simulator

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:34:
return class_to_register
bug fix, a class decorator must return the class

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/eval_scores.py:125:
self._polqa_tool_path = polqa_tool_path
@kjellander: now the POLQA path is passed in the constructor

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:50:
return class_to_register
Bug fix: the class decorator MUST return the class

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:357:
self._aechen_ir_database_path = aechen_ir_database_path
@kjellander: the same comment I got from you about the POLQA path also applies
to the AIR database path, I also corrected this

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:35:
polqa_tool_path=polqa_tool_path))
I added factory classes with which evaluation score and noise generator workers
are instantiated. The factory objects take the parameters (e.g., paths) from the
caller (in this case, the APM simulator) and use them to instantiate the
workers.

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:54:
noise_generator_names)]
Now getting an instance through the factory object.

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/simulation.py:60:
name) in eval_score_names]
Now getting an instance through the factory object.

[kjellander_webrtc, 2017-03-09 07:49:32]

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:80:

On 2017/03/06 11:29:53, AleBzk wrote:
> @kjellander: the environment variable, if defined, is now only used as default
> for the shell arguments

That's better, but I'd still like to never even attempt to read an environment
variable though. I understand it's annoying to have to pass these flags every
time and that's why you want to define an environment variable. How about just
having a sensible default path instead, and eliminate the environment variable
access entirely?

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:12:
import scipy.io
This is not part of the standard library. Is it possible to pull it down via
DEPS to avoid relying on system-installed libraries?

Please implement a user-friendly error message if the module is missing on
import.

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:335:
_IMPULSE_RESPONSES = {
I know nothing about the signal processing involved here, so I suggest you pick
a more suitable reviewer for that. I'm only reviewing style and basic logic from
here on.

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:359:
def _generate(
Add docstring

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:417:
impulse_response_filepath; then save to noise_track_filepath.
Add docstring for arguments

[AleBzk, 2017-03-09 11:31:19]

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

[AleBzk, 2017-03-09 11:31:20]

Hi Henrik,

Everything fixed except for the 3rd-party pkgs which IMHO require a separate CL.
See my comments below.

Alessio

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:80:

On 2017/03/09 07:49:31, kjellander_webrtc wrote:
> On 2017/03/06 11:29:53, AleBzk wrote:
> > @kjellander: the environment variable, if defined, is now only used as
default
> > for the shell arguments
> 
> That's better, but I'd still like to never even attempt to read an environment
> variable though. I understand it's annoying to have to pass these flags every
> time and that's why you want to define an environment variable. How about just
> having a sensible default path instead, and eliminate the environment variable
> access entirely?

Done. Now both paths are required and with no default. See the changes in
apm_quality_assessment.sh taking into account that the script is a sample
customizable script provided with the tool. Still, I defined a default path in
case the environment vars are not defined. But again, taking care of the paths
will be up to the user and now the Python code does not depend anymore on any
environment var.

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:12:
import scipy.io
On 2017/03/09 07:49:31, kjellander_webrtc wrote:
> This is not part of the standard library. Is it possible to pull it down via
> DEPS to avoid relying on system-installed libraries?
> 
> Please implement a user-friendly error message if the module is missing on
> import.

True. And in another file I also use another 3rd part package (pydub).
If you could point me on a guideline to add 3rd party libs in chromium or
webrtc, I will do that in two separate CLs (one for scipy and one for pydub).
I have done this before on another repository, so I know that I should take care
of licensing stuff.
As a reference I guess I can look at
https://chromium.googlesource.com/chromium/src/third_party/jinja2/+/master
Feel free to contact me privately to advise on this.

As for the user-friendly message, I added a try-except block around import.

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:335:
_IMPULSE_RESPONSES = {
On 2017/03/09 07:49:31, kjellander_webrtc wrote:
> I know nothing about the signal processing involved here, so I suggest you
pick
> a more suitable reviewer for that. I'm only reviewing style and basic logic
from
> here on.

Yup

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:359:
def _generate(
On 2017/03/09 07:49:31, kjellander_webrtc wrote:
> Add docstring

Done.

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:417:
impulse_response_filepath; then save to noise_track_filepath.
On 2017/03/09 07:49:31, kjellander_webrtc wrote:
> Add docstring for arguments

Done.

[kjellander_webrtc, 2017-03-09 14:05:42]

lgtm, 

I'll leave the decision to you if you want an additional reviewer for the signal
processing specifics.

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:80:

On 2017/03/09 11:31:19, AleBzk wrote:
> On 2017/03/09 07:49:31, kjellander_webrtc wrote:
> > On 2017/03/06 11:29:53, AleBzk wrote:
> > > @kjellander: the environment variable, if defined, is now only used as
> default
> > > for the shell arguments
> > 
> > That's better, but I'd still like to never even attempt to read an
environment
> > variable though. I understand it's annoying to have to pass these flags
every
> > time and that's why you want to define an environment variable. How about
just
> > having a sensible default path instead, and eliminate the environment
variable
> > access entirely?
> 
> Done. Now both paths are required and with no default. See the changes in
> apm_quality_assessment.sh taking into account that the script is a sample
> customizable script provided with the tool. Still, I defined a default path in
> case the environment vars are not defined. But again, taking care of the paths
> will be up to the user and now the Python code does not depend anymore on any
> environment var.

Thanks! Environment variables are the cause of much evil in development and
infrastructure. That's why I'm so heavily against them.

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

https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:12:
import scipy.io
On 2017/03/09 11:31:20, AleBzk wrote:
> On 2017/03/09 07:49:31, kjellander_webrtc wrote:
> > This is not part of the standard library. Is it possible to pull it down via
> > DEPS to avoid relying on system-installed libraries?
> > 
> > Please implement a user-friendly error message if the module is missing on
> > import.
> 
> True. And in another file I also use another 3rd part package (pydub).
> If you could point me on a guideline to add 3rd party libs in chromium or
> webrtc, I will do that in two separate CLs (one for scipy and one for pydub).
> I have done this before on another repository, so I know that I should take
care
> of licensing stuff.
> As a reference I guess I can look at
> https://chromium.googlesource.com/chromium/src/third_party/jinja2/+/master
> Feel free to contact me privately to advise on this.
> 
> As for the user-friendly message, I added a try-except block around import.

This is fine. If this never is going to run on any bot, we don't need to DEPS
them in.
Otherwise you'll have to create a mirror of the source code at
https://chromium.googlesource.com/ and pull that in via DEPS.
It can be auto-mirrored from github.com repos, but if it's not there, we might
rely on manually importing to a deps repo, assuming the license of these
libraries allows redistribution in source form. If they have native code as
well, it becomes an even harder problem (I assume these have?).

[AleBzk, 2017-03-10 08:40:53]

Hi again,

Great work thanks to your comments.
I'm very satisfied!

I will add someone from my team to double check the signal processing part.
As for DEPS, at this time we don't foresee a bot scenario, so for simplicity I'd
leave everything as it is.
I have a final CL with a detailed README.md file to help the user with deps.
That should be sufficient for now.

Cheers,
Alessio

On 2017/03/09 14:05:42, kjellander_webrtc wrote:
> lgtm, 
> 
> I'll leave the decision to you if you want an additional reviewer for the
signal
> processing specifics.
> 
>
https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
> File
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py
> (right):
> 
>
https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
>
webrtc/modules/audio_processing/test/py_quality_assessment/apm_quality_assessment.py:80:
> 
> On 2017/03/09 11:31:19, AleBzk wrote:
> > On 2017/03/09 07:49:31, kjellander_webrtc wrote:
> > > On 2017/03/06 11:29:53, AleBzk wrote:
> > > > @kjellander: the environment variable, if defined, is now only used as
> > default
> > > > for the shell arguments
> > > 
> > > That's better, but I'd still like to never even attempt to read an
> environment
> > > variable though. I understand it's annoying to have to pass these flags
> every
> > > time and that's why you want to define an environment variable. How about
> just
> > > having a sensible default path instead, and eliminate the environment
> variable
> > > access entirely?
> > 
> > Done. Now both paths are required and with no default. See the changes in
> > apm_quality_assessment.sh taking into account that the script is a sample
> > customizable script provided with the tool. Still, I defined a default path
in
> > case the environment vars are not defined. But again, taking care of the
paths
> > will be up to the user and now the Python code does not depend anymore on
any
> > environment var.
> 
> Thanks! Environment variables are the cause of much evil in development and
> infrastructure. That's why I'm so heavily against them.
> 
>
https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
> File
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py
> (right):
> 
>
https://codereview.webrtc.org/2715233003/diff/20001/webrtc/modules/audio_proc...
>
webrtc/modules/audio_processing/test/py_quality_assessment/quality_assessment/noise_generation.py:12:
> import scipy.io
> On 2017/03/09 11:31:20, AleBzk wrote:
> > On 2017/03/09 07:49:31, kjellander_webrtc wrote:
> > > This is not part of the standard library. Is it possible to pull it down
via
> > > DEPS to avoid relying on system-installed libraries?
> > > 
> > > Please implement a user-friendly error message if the module is missing on
> > > import.
> > 
> > True. And in another file I also use another 3rd part package (pydub).
> > If you could point me on a guideline to add 3rd party libs in chromium or
> > webrtc, I will do that in two separate CLs (one for scipy and one for
pydub).
> > I have done this before on another repository, so I know that I should take
> care
> > of licensing stuff.
> > As a reference I guess I can look at
> > https://chromium.googlesource.com/chromium/src/third_party/jinja2/+/master
> > Feel free to contact me privately to advise on this.
> > 
> > As for the user-friendly message, I added a try-except block around import.
> 
> This is fine. If this never is going to run on any bot, we don't need to DEPS
> them in.
> Otherwise you'll have to create a mirror of the source code at
> https://chromium.googlesource.com/ and pull that in via DEPS.
> It can be auto-mirrored from http://github.com repos, but if it's not there,
we might
> rely on manually importing to a deps repo, assuming the license of these
> libraries allows redistribution in source form. If they have native code as
> well, it becomes an even harder problem (I assume these have?).

[AleBzk, 2017-04-03 15:09:18]

The CQ bit was checked by alessiob@webrtc.org

[AleBzk, 2017-04-03 15:09:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
kjellander@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2715233003/#ps60001 (title:
"rebase + style fix")

[commit-bot: I haz the power, 2017-04-03 15:09:28]

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-03 15:12:45]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491232158802290,
"parent_rev": "8a1b3c9d1129a326afde9b853482a86969d720d2", "commit_rev":
"54ad3df1d290b202bbd3917b170df5e7cbf95f29"}

[commit-bot: I haz the power, 2017-04-03 15:12:50]

Description was changed from

==========
I added two factory classes to address an important comment I got in another CL
from kjellander@webrtc.org.
This CL also includes the implementation of the echo noise generator.
I added comments in the diff to help the reviewers.

BUG=webrtc:7218
NOTRY=True
==========

to

==========
I added two factory classes to address an important comment I got in another CL
from kjellander@webrtc.org.
This CL also includes the implementation of the echo noise generator.
I added comments in the diff to help the reviewers.

BUG=webrtc:7218
NOTRY=True

Review-Url: https://codereview.webrtc.org/2715233003
Cr-Commit-Position: refs/heads/master@{#17507}
Committed:
https://chromium.googlesource.com/external/webrtc/+/54ad3df1d290b202bbd3917b1...
==========

[commit-bot: I haz the power, 2017-04-03 15:12:51]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/external/webrtc/+/54ad3df1d290b202bbd3917b1...