Add IntelligibilityEnhancer support to audioproc_float

Add IntelligibilityEnhancer support to audioproc_float

R=peah@webrtc.org

Committed: https://crrev.com/98c69a0ee785adeb9d95fffeb55cdb6cedbe82c6
Cr-Commit-Position: refs/heads/master@{#12147}

[aluebs-webrtc, 2016-03-16 01:56:47]

aluebs@webrtc.org changed reviewers:
+ peah@webrtc.org, turaj@webrtc.org

[peah-webrtc, 2016-03-16 06:56:57]

peah@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org

[peah-webrtc, 2016-03-16 07:04:50]

On 2016/03/16 06:56:57, peah-webrtc wrote:
> mailto:peah@webrtc.org changed reviewers:
> + mailto:henrik.lundin@webrtc.org

I have a concern about the test tools that we have for this. Currently we have
audioproc_float and process_test. Afaics, audioproc_float is still not
sufficiently ready to reproduce a protobuf recording. For instance at a first
glance I found that the suppression level for the AEC is not specified via the
echo_cancellation()->set_suppression_level() command is not set at all (please
correct me if I missed anything for this). 

Before adding new functionality only to the audioproc_float tool we need to
ensure that it is able to in a bitexact manner replace process_test. Have we
done any such testing? Until then, I think that the process_test tool should be
considered the default tool and then it makes more sense to place the new IE
functionality there.

[aluebs-webrtc, 2016-03-16 16:40:11]

On 2016/03/16 07:04:50, peah-webrtc wrote:
> On 2016/03/16 06:56:57, peah-webrtc wrote:
> > mailto:peah@webrtc.org changed reviewers:
> > + mailto:henrik.lundin@webrtc.org
> 
> I have a concern about the test tools that we have for this. Currently we have
> audioproc_float and process_test. Afaics, audioproc_float is still not
> sufficiently ready to reproduce a protobuf recording. For instance at a first
> glance I found that the suppression level for the AEC is not specified via the
> echo_cancellation()->set_suppression_level() command is not set at all (please
> correct me if I missed anything for this). 
> 
> Before adding new functionality only to the audioproc_float tool we need to
> ensure that it is able to in a bitexact manner replace process_test. Have we
> done any such testing? Until then, I think that the process_test tool should
be
> considered the default tool and then it makes more sense to place the new IE
> functionality there.

I have a diametrically opposed opinion here. process_test is a legacy tool,
which has too many options (see .raw vs .wav for example), is buggy and has tons
of repeated and complicated code (I spend more time debugging and fixing that
tool than I would like to admit). So ajm set out to simplify this by creating
the new version from scratch, audioproc_float, which I couldn't be more happy
about. We still want to keep process_test around for now just because, as you
say, there might be some functionality that was never implemented in
audioproc_float and I think we can all help in porting the features that make
sense to audioproc_float. But the whole point of audioproc_float is to not get
as bloated as process_test, so it would defeat the purpose to port ALL features.
But on the other hand I don't think we should spend any more time on
process_test, since we want to deprecate it in the hopefully-not-so-long term.
And in the worst case, I don't see how a CL adding functionality to
audioproc_float should be blocked on one adding it to process_test, they should
be independent. henrik and turaj, what are your opinions?

[aluebs-webrtc, 2016-03-16 17:38:38]

Answering to turaj's comments on the email thread: Yes, we do have a
intelligibility tool that does something similar than audioproc_float for the
IE, but the truth is that this also tests how the IE and NS are called in the
APM. For example, just having this locally showed me that the results for
audioproc_float and the intelligibility tool are completely different and I am
looking into why this is.

[peah-webrtc, 2016-03-16 19:22:47]

On 2016/03/16 17:38:38, aluebs-webrtc wrote:
> Answering to turaj's comments on the email thread: Yes, we do have a
> intelligibility tool that does something similar than audioproc_float for the
> IE, but the truth is that this also tests how the IE and NS are called in the
> APM. For example, just having this locally showed me that the results for
> audioproc_float and the intelligibility tool are completely different and I am
> looking into why this is.

I definitely agree that the process_test either needs refactoring/rewriting or
should be replaced. (I do, however, not know of any bugs in the call
reproduction in process_test so if you know any, please file a bug).

At the moment process_test is, however, to my knowledge the only tool we have
available for that is capable of reproducing a WebRTC call from protobuf
recording. Please correct me if I'm wrong about that though since I'm not really
familiar with the audioproc_float tool. And in order to be able to handle
incoming bugs we need a tool that is able to reproduce a WebRTC call in a
bitexact manner. Therefore, I'm concerned when new functionality is primarily
added to another tool. (We currently have that situation with the beamformer.)

I'm definitely not against adding the IE functionality to the audioproc_float
tool as long as there is a clear plan for how to ensure that a tool capable of
reproducing any WebRTC call from a protobuf recording is available before IE is
considered to be released. We need a tool to rely on for handling bugs and today
that tool is process_test.

[aluebs-webrtc, 2016-03-18 02:00:01]

On 2016/03/16 19:22:47, peah-webrtc wrote:
> On 2016/03/16 17:38:38, aluebs-webrtc wrote:
> > Answering to turaj's comments on the email thread: Yes, we do have a
> > intelligibility tool that does something similar than audioproc_float for
the
> > IE, but the truth is that this also tests how the IE and NS are called in
the
> > APM. For example, just having this locally showed me that the results for
> > audioproc_float and the intelligibility tool are completely different and I
am
> > looking into why this is.
> 
> I definitely agree that the process_test either needs refactoring/rewriting or
> should be replaced. (I do, however, not know of any bugs in the call
> reproduction in process_test so if you know any, please file a bug).
> 
> At the moment process_test is, however, to my knowledge the only tool we have
> available for that is capable of reproducing a WebRTC call from protobuf
> recording. Please correct me if I'm wrong about that though since I'm not
really
> familiar with the audioproc_float tool. And in order to be able to handle
> incoming bugs we need a tool that is able to reproduce a WebRTC call in a
> bitexact manner. Therefore, I'm concerned when new functionality is primarily
> added to another tool. (We currently have that situation with the beamformer.)
> 
> I'm definitely not against adding the IE functionality to the audioproc_float
> tool as long as there is a clear plan for how to ensure that a tool capable of
> reproducing any WebRTC call from a protobuf recording is available before IE
is
> considered to be released. We need a tool to rely on for handling bugs and
today
> that tool is process_test.

As we are discussing offline, we don't agree on whether it makes sense to
maintain process_test or how much work it would be to add the necessary settings
to audioproc_float (because audioproc_float does support running a WebRTC call
from an aecdump). But while we decide on how to move forward on that, since you
are not against adding this functionality here, could we please still continue
with the review process here? Thank you very much in advance.

[peah-webrtc, 2016-03-18 09:38:03]

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/audio_file_processor.cc (right):

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/audio_file_processor.cc:55:
process_reverse_stream_(reverse_in_file && reverse_out_file) {
Why is this true only if both reverse_in_file reverse_out_file are specified?
For both the AGC, AEC amd AECM  the ProcessReverseStream (or
AnalyzeReverseStream) calls are needed to ensure proper operation.

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/audio_file_processor.cc:83: if
(process_reverse_stream_) {
This bool is a bit redundant: you should be able to do the check on the
reverse_buffer_writer_ and reverse_buffer_reader_ variables.

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/audioproc_float.cc (right):

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/audioproc_float.cc:90: "All components are
disabled by default. If any bi-directional components\n"
Please change this usage description as it is no longer true for the
intelligibility enhancer (or did I misinterpret bi-directional?)

[aluebs-webrtc, 2016-03-18 18:36:54]

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/audio_file_processor.cc (right):

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/audio_file_processor.cc:55:
process_reverse_stream_(reverse_in_file && reverse_out_file) {
On 2016/03/18 09:38:02, peah-webrtc wrote:
> Why is this true only if both reverse_in_file reverse_out_file are specified?
> For both the AGC, AEC amd AECM  the ProcessReverseStream (or
> AnalyzeReverseStream) calls are needed to ensure proper operation.

In practice, the reverse_out_file is always specified (has a default value) and
I guess I was trying to keep the code as simple as possible. But you have a
great point here. Changed to a finer grained control.

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/audio_file_processor.cc:83: if
(process_reverse_stream_) {
On 2016/03/18 09:38:02, peah-webrtc wrote:
> This bool is a bit redundant: you should be able to do the check on the
> reverse_buffer_writer_ and reverse_buffer_reader_ variables.

I just though it was less bug prone to have one central flag for it, since there
are many pointers that are only set when there is a reverse_in_file. But agreed.
Done.

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/audioproc_float.cc (right):

https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/audioproc_float.cc:90: "All components are
disabled by default. If any bi-directional components\n"
On 2016/03/18 09:38:02, peah-webrtc wrote:
> Please change this usage description as it is no longer true for the
> intelligibility enhancer (or did I misinterpret bi-directional?)

Good catch! Done.

[peah-webrtc, 2016-03-18 21:56:52]

On 2016/03/18 18:36:54, aluebs-webrtc wrote:
>
https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
> File webrtc/modules/audio_processing/test/audio_file_processor.cc (right):
> 
>
https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
> webrtc/modules/audio_processing/test/audio_file_processor.cc:55:
> process_reverse_stream_(reverse_in_file && reverse_out_file) {
> On 2016/03/18 09:38:02, peah-webrtc wrote:
> > Why is this true only if both reverse_in_file reverse_out_file are
specified?
> > For both the AGC, AEC amd AECM  the ProcessReverseStream (or
> > AnalyzeReverseStream) calls are needed to ensure proper operation.
> 
> In practice, the reverse_out_file is always specified (has a default value)
and
> I guess I was trying to keep the code as simple as possible. But you have a
> great point here. Changed to a finer grained control.
> 
>
https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
> webrtc/modules/audio_processing/test/audio_file_processor.cc:83: if
> (process_reverse_stream_) {
> On 2016/03/18 09:38:02, peah-webrtc wrote:
> > This bool is a bit redundant: you should be able to do the check on the
> > reverse_buffer_writer_ and reverse_buffer_reader_ variables.
> 
> I just though it was less bug prone to have one central flag for it, since
there
> are many pointers that are only set when there is a reverse_in_file. But
agreed.
> Done.
> 
>
https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
> File webrtc/modules/audio_processing/test/audioproc_float.cc (right):
> 
>
https://codereview.webrtc.org/1800413002/diff/1/webrtc/modules/audio_processi...
> webrtc/modules/audio_processing/test/audioproc_float.cc:90: "All components
are
> disabled by default. If any bi-directional components\n"
> On 2016/03/18 09:38:02, peah-webrtc wrote:
> > Please change this usage description as it is no longer true for the
> > intelligibility enhancer (or did I misinterpret bi-directional?)
> 
> Good catch! Done.

lgtm with nits

[peah-webrtc, 2016-03-18 21:57:17]

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/audioproc_float.cc (right):

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audioproc_float.cc:90: "All components are
disabled by default. If the AEC is enabled, only\n"
Actually, I'd rather you fully remove this line about the AEC. It is only valid
due to lines 104-106, and the FLAGS_all setting anyway may turn on the AEC when
operating on wav-files. And apart from not being able to set the proper default
settings for the AEC, I see now reason for why the AEC cannot already be run
using wav-files.

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audioproc_float.cc:104: if
(FLAGS_dump.empty() && FLAGS_aec) {
Please remove this statement. The statement is not more true for the AEC than
for NS, AGC or any other component. The test program is not at all yet ready to
operate properly on any of those and and I think that it is then better not to
give the impression that it works on wav files for anything but the AEC.

[aluebs-webrtc, 2016-03-18 23:39:29]

henrik? turaj? Any additional comments?

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/audioproc_float.cc (right):

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audioproc_float.cc:90: "All components are
disabled by default. If the AEC is enabled, only\n"
On 2016/03/18 21:57:17, peah-webrtc wrote:
> Actually, I'd rather you fully remove this line about the AEC. It is only
valid
> due to lines 104-106, and the FLAGS_all setting anyway may turn on the AEC
when
> operating on wav-files. And apart from not being able to set the proper
default
> settings for the AEC, I see now reason for why the AEC cannot already be run
> using wav-files.

Agreed. Done.

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audioproc_float.cc:104: if
(FLAGS_dump.empty() && FLAGS_aec) {
On 2016/03/18 21:57:17, peah-webrtc wrote:
> Please remove this statement. The statement is not more true for the AEC than
> for NS, AGC or any other component. The test program is not at all yet ready
to
> operate properly on any of those and and I think that it is then better not to
> give the impression that it works on wav files for anything but the AEC.

Done. If by "not at all yet ready to operate properly" you mean that some
settings are missing, I agree. Or is there something else missing?

[peah-webrtc, 2016-03-19 01:00:16]

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/audioproc_float.cc (right):

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audioproc_float.cc:104: if
(FLAGS_dump.empty() && FLAGS_aec) {
On 2016/03/18 23:39:29, aluebs-webrtc wrote:
> On 2016/03/18 21:57:17, peah-webrtc wrote:
> > Please remove this statement. The statement is not more true for the AEC
than
> > for NS, AGC or any other component. The test program is not at all yet ready
> to
> > operate properly on any of those and and I think that it is then better not
to
> > give the impression that it works on wav files for anything but the AEC.
> 
> Done. If by "not at all yet ready to operate properly" you mean that some
> settings are missing, I agree. Or is there something else missing?

There is afaics lots of things missing:
AEC:
set_suppression_level
set_stream_drift_compensation
delay agnostic mode
extended filter mode

AECM
routing mode
comfort noise enabled
set echo path

GainControl
set_stream_analog_level
set_mode
set_target_level_dbfs
set_compression_gain_db
enable_limiter
set_analog_level_limits

I would say that basically all of the submodule-specific settings are missing
apart from setting the noise suppression level which is the only one that is
actually implemented. The settings listed above are the settings that are
publicly available in the APM submodule API and which therefore a test
application must support.

[aluebs-webrtc, 2016-03-19 02:09:55]

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/audioproc_float.cc (right):

https://codereview.webrtc.org/1800413002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/audioproc_float.cc:104: if
(FLAGS_dump.empty() && FLAGS_aec) {
On 2016/03/19 01:00:16, peah-webrtc wrote:
> On 2016/03/18 23:39:29, aluebs-webrtc wrote:
> > On 2016/03/18 21:57:17, peah-webrtc wrote:
> > > Please remove this statement. The statement is not more true for the AEC
> than
> > > for NS, AGC or any other component. The test program is not at all yet
ready
> > to
> > > operate properly on any of those and and I think that it is then better
not
> to
> > > give the impression that it works on wav files for anything but the AEC.
> > 
> > Done. If by "not at all yet ready to operate properly" you mean that some
> > settings are missing, I agree. Or is there something else missing?
> 
> There is afaics lots of things missing:
> AEC:
> set_suppression_level
> set_stream_drift_compensation
> delay agnostic mode
> extended filter mode
> 
> AECM
> routing mode
> comfort noise enabled
> set echo path
> 
> GainControl
> set_stream_analog_level
> set_mode
> set_target_level_dbfs
> set_compression_gain_db
> enable_limiter
> set_analog_level_limits
> 
> I would say that basically all of the submodule-specific settings are missing
> apart from setting the noise suppression level which is the only one that is
> actually implemented. The settings listed above are the settings that are
> publicly available in the APM submodule API and which therefore a test
> application must support.

Thank you for doing that analysis. I agree we should support these settings. And
I don't think it is a lot of work to do so.

[aluebs-webrtc, 2016-03-29 17:10:09]

Since more than a week has passed, I am taking that hlundin and turaj don't have
any additional comments. Landing this now.

[aluebs-webrtc, 2016-03-29 17:10:14]

The CQ bit was checked by aluebs@webrtc.org

[aluebs-webrtc, 2016-03-29 17:10:14]

The patchset sent to the CQ was uploaded after l-g-t-m from peah@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1800413002/#ps60001 (title:
"Rebasing")

[commit-bot: I haz the power, 2016-03-29 17:10:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1800413002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1800413002/60001

[hlundin-webrtc, 2016-03-29 18:05:12]

henrik.lundin@webrtc.org changed reviewers:
- henrik.lundin@webrtc.org

[hlundin-webrtc, 2016-03-29 18:05:13]

I've been OOO for more than a week. Sorry about not updating my nickname to
reflect that.

Anyway, I trust that peah@ has done a good review, and silently see myself out
the backdoor.

[commit-bot: I haz the power, 2016-03-29 19:10:49]

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

[commit-bot: I haz the power, 2016-03-29 19:10:51]

Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[aluebs-webrtc, 2016-03-29 19:43:46]

Description was changed from

==========
Add IntelligibilityEnhancer support to audioproc_float
==========

to

==========
Add IntelligibilityEnhancer support to audioproc_float

R=peah@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/98c69a0ee785adeb9d95fffeb...
==========

[aluebs-webrtc, 2016-03-29 19:43:48]

Committed patchset #5 (id:80001) manually as
98c69a0ee785adeb9d95fffeb55cdb6cedbe82c6 (presubmit successful).

[commit-bot: I haz the power, 2016-03-29 19:43:51]

Description was changed from

==========
Add IntelligibilityEnhancer support to audioproc_float

R=peah@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/98c69a0ee785adeb9d95fffeb...
==========

to

==========
Add IntelligibilityEnhancer support to audioproc_float

R=peah@webrtc.org

Committed: https://crrev.com/98c69a0ee785adeb9d95fffeb55cdb6cedbe82c6
Cr-Commit-Position: refs/heads/master@{#12147}
==========

[commit-bot: I haz the power, 2016-03-29 19:43:53]

Patchset 5 (id:??) landed as
https://crrev.com/98c69a0ee785adeb9d95fffeb55cdb6cedbe82c6
Cr-Commit-Position: refs/heads/master@{#12147}