Transparency improvements in the echo canceller 3

Transparency improvements in the echo canceller 3

This CL adds two changes:
-Adaptive adjustment of the echo suppression to both cover the cases
when the echo path well covers the room, and when when it does not.
-Identification of the case when the echo is too low to be audible
and adaptive handling of this case in the echo suppression.


BUG=webrtc:7519, webrtc:7956, webrtc:7957
Review-Url: https://codereview.webrtc.org/2974583004
Cr-Commit-Position: refs/heads/master@{#18962}
Committed: https://chromium.googlesource.com/external/webrtc/+/2910357621dee4368bd3eaa0040cec82ac230dad

[peah-webrtc, 2017-07-07 13:51:12]

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

[peah-webrtc, 2017-07-07 13:51:12]

Hi ivoc@,
Could you please take a look at this CL?

[ivoc, 2017-07-10 13:53:56]

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:515:
std::copy(h.begin(), h.begin() + kFftLengthBy2,
Is it possible to do the operations directly in h_ and avoid having to copy the
data?

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec_state.cc:219: for (size_t k = 1; k <
tail_index; ++k) {
You could use std::max_element instead of this loop.

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec_state.cc:259: if
(reverb_decay_candidate_residual_ < 0.f) {
This is always true (since reverb_decay_candidate_residual_ is set to -1 on the
previous line)

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec_state.cc:263: reverb_decay_ *=
reverb_decay_candidate_;
Is this for-loop more efficient than using pow?

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec_state.cc:283: const float x_min =
*std::min_element(x.begin(), x.end());
There is a std::minmax function that calculates both the max and min in a single
pass. I'm not sure if we can use it (it's a C++11 library), but it would be more
efficient.

[peah-webrtc, 2017-07-10 23:18:09]

Thanks for the comments!
I have uploaded a new patchset in response to  the comments.

PTAL

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc (right):

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/adaptive_fir_filter.cc:515:
std::copy(h.begin(), h.begin() + kFftLengthBy2,
On 2017/07/10 13:53:55, ivoc wrote:
> Is it possible to do the operations directly in h_ and avoid having to copy
the
> data?

That would be good, but unfortunately it is not possible. h goes into the Ifft
on line 508 and that needs h to be of size 128.  By construction, only the last
64 values in h contain the desired data which are copied into h_.

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec_state.cc:219: for (size_t k = 1; k <
tail_index; ++k) {
On 2017/07/10 13:53:55, ivoc wrote:
> You could use std::max_element instead of this loop.

Good point!
Done.

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec_state.cc:259: if
(reverb_decay_candidate_residual_ < 0.f) {
On 2017/07/10 13:53:55, ivoc wrote:
> This is always true (since reverb_decay_candidate_residual_ is set to -1 on
the
> previous line)

Good find!
Done.

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec_state.cc:263: reverb_decay_ *=
reverb_decay_candidate_;
On 2017/07/10 13:53:55, ivoc wrote:
> Is this for-loop more efficient than using pow?

That seems a bit unclear. However, this is only done once per frame, so I think
for code clarity, it makes sense to go for pow.

Done.

https://codereview.webrtc.org/2974583004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec3/aec_state.cc:283: const float x_min =
*std::min_element(x.begin(), x.end());
On 2017/07/10 13:53:55, ivoc wrote:
> There is a std::minmax function that calculates both the max and min in a
single
> pass. I'm not sure if we can use it (it's a C++11 library), but it would be
more
> efficient.

Ah! Great! I did not know about that one. Afaics, we are allowed to use that, so
I change to using that.

Done.

[ivoc, 2017-07-11 08:24:40]

lgtm!

[peah-webrtc, 2017-07-11 08:27:07]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2017-07-11 08:27:14]

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-07-11 08:36:03]

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

[commit-bot: I haz the power, 2017-07-11 08:36:04]

Try jobs failed on following builders:
  linux_asan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/26255)
  linux_ubsan on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan/builds/13776)
  win_clang_dbg on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/15300)
  win_x64_rel on master.tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/27118)

[peah-webrtc, 2017-07-11 09:21:38]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-07-11 09:21:39]

The patchset sent to the CQ was uploaded after l-g-t-m from ivoc@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2974583004/#ps40001 (title:
"Corrected wrong echo estimate vector in unittest")

[commit-bot: I haz the power, 2017-07-11 09:21: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-07-11 09:54:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1499764898946780,
"parent_rev": "863f03ba38f397c078931d2bd27d40a4d60efc02", "commit_rev":
"2910357621dee4368bd3eaa0040cec82ac230dad"}

[commit-bot: I haz the power, 2017-07-11 09:54:08]

Description was changed from

==========
Transparency improvements in the echo canceller 3

This CL adds two changes:
-Adaptive adjustment of the echo suppression to both cover the cases
when the echo path well covers the room, and when when it does not.
-Identification of the case when the echo is too low to be audible
and adaptive handling of this case in the echo suppression.


BUG=webrtc:7519, webrtc:7956,webrtc:7957
==========

to

==========
Transparency improvements in the echo canceller 3

This CL adds two changes:
-Adaptive adjustment of the echo suppression to both cover the cases
when the echo path well covers the room, and when when it does not.
-Identification of the case when the echo is too low to be audible
and adaptive handling of this case in the echo suppression.


BUG=webrtc:7519, webrtc:7956,webrtc:7957

Review-Url: https://codereview.webrtc.org/2974583004
Cr-Commit-Position: refs/heads/master@{#18962}
Committed:
https://chromium.googlesource.com/external/webrtc/+/2910357621dee4368bd3eaa00...
==========

[commit-bot: I haz the power, 2017-07-11 09:54:09]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/external/webrtc/+/2910357621dee4368bd3eaa00...