Echo canceller 3 improvements for setups with headsets.

Echo canceller 3 improvements for setups with headsets.

This CL improves the echo cancellation performance on setups where 
headsets are used (systems with such low echo path gain 
that no correlation between the render and capture signals
can be found) in 4 ways:
1) The echo path gain for systems with headsets is assumed to be 
nonzero.
2) The stationary component of the render power is not included
in nonlinear echo power estimate.
3) The behavior after echo path gain changes is made less cautious.
4) The detection of systems with headsets is made more rapid.

BUG=chromium:712651, webrtc:6018
Review-Url: https://codereview.webrtc.org/2823903003
Cr-Commit-Position: refs/heads/master@{#17768}
Committed: https://chromium.googlesource.com/external/webrtc/+/e52a203a56c7876bc226c04464563555433fa0c1

[peah-webrtc, 2017-04-19 12:02:14]

Description was changed from

==========
Bugfix


Bugfix

BUG=
==========

to

==========
Echo canceller 3 improvements for setups with headsets.

This CL improves the echo cancellation performance on setups where 
headsets are used (systems with such low echo path gain 
that no correlation between the render and capture signals
can be found) in 4 ways:
1) The echo path gain for systems with headsets is assumed to be 
nonzero.
2) The stationary component of the render power is not included
in nonlinear echo power estimate.
3) The behavior after echo path gain changes is made less cautious.
4) The detection of systems with headsets is made more rapid.

BUG=
==========

[peah-webrtc, 2017-04-19 12:03:08]

Description was changed from

==========
Echo canceller 3 improvements for setups with headsets.

This CL improves the echo cancellation performance on setups where 
headsets are used (systems with such low echo path gain 
that no correlation between the render and capture signals
can be found) in 4 ways:
1) The echo path gain for systems with headsets is assumed to be 
nonzero.
2) The stationary component of the render power is not included
in nonlinear echo power estimate.
3) The behavior after echo path gain changes is made less cautious.
4) The detection of systems with headsets is made more rapid.

BUG=
==========

to

==========
Echo canceller 3 improvements for setups with headsets.

This CL improves the echo cancellation performance on setups where 
headsets are used (systems with such low echo path gain 
that no correlation between the render and capture signals
can be found) in 4 ways:
1) The echo path gain for systems with headsets is assumed to be 
nonzero.
2) The stationary component of the render power is not included
in nonlinear echo power estimate.
3) The behavior after echo path gain changes is made less cautious.
4) The detection of systems with headsets is made more rapid.

BUG=chromium:712651, webrtc:6018
==========

[peah-webrtc, 2017-04-19 12:03:17]

Patchset #1 (id:1) has been deleted

[peah-webrtc, 2017-04-19 12:03:23]

Patchset #1 (id:20001) has been deleted

[peah-webrtc, 2017-04-19 12:04:37]

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

[peah-webrtc, 2017-04-19 12:04:37]

Hi,
This is another CL for AEC3 that improves the transparency and echo removal for
systems where headsets are used.

Could you PTAL.

[peah-webrtc, 2017-04-19 14:04:08]

The CQ bit was checked by peah@webrtc.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 14:04:14]

Dry run: 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-19 14:39:42]

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

[commit-bot: I haz the power, 2017-04-19 14:39:43]

Dry run: This issue passed the CQ dry run.

[ivoc, 2017-04-19 14:47:47]

Looks mostly good, see a few comments and questions below.

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:129: active_render_block &&
(!SaturatedCapture()) ? 1 : 0;
This looks kind of strange to me. The result of "active_render_block &&
(!SaturatedCapture()) ? 1 : 0" is a bool, right? I don't think it's good to add
a bool to an int.

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right):

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:69:
std::max((*X2_noise_floor)[k] * 1.1f, kNoiseFloorMin);
Shouldn't (*X2_noise_floor_counter)[k] be reset to 0 here? Otherwise this will
keep getting multiplied by 1.1 on each iteration.

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:158:
X2_noise_floor_counter_.fill(kNoiseFloorCounterMax);
Shouldn't the counters be initialized at zero?

[peah-webrtc, 2017-04-19 14:56:21]

Thanks for the comments. I have some explanations to the code constructs as
response. PTAL

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:129: active_render_block &&
(!SaturatedCapture()) ? 1 : 0;
On 2017/04/19 14:47:47, ivoc wrote:
> This looks kind of strange to me. The result of "active_render_block &&
> (!SaturatedCapture()) ? 1 : 0" is a bool, right? I don't think it's good to
add
> a bool to an int.

No, it is actually an int. That is a conditional using the ternary operator ( of
the type test ? result a : result b;). That expression should return an int if I
did not get this wrong.

PTAL

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right):

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:69:
std::max((*X2_noise_floor)[k] * 1.1f, kNoiseFloorMin);
On 2017/04/19 14:47:47, ivoc wrote:
> Shouldn't (*X2_noise_floor_counter)[k] be reset to 0 here? Otherwise this will
> keep getting multiplied by 1.1 on each iteration.

Yes, that is the purpose of this. When the noise floor has not been updated
downwards for kNoiseFloorCounterMax blocks, the minimum statistics starts to
exponentially increase the noise floor until it is again updated downwards
(which occurs when the noise floor has exceeded the power at hand).

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:158:
X2_noise_floor_counter_.fill(kNoiseFloorCounterMax);
On 2017/04/19 14:47:47, ivoc wrote:
> Shouldn't the counters be initialized at zero?

No, they are initialized to kNoiseFloorCounterMax on purpose. I don't want the
initial value to be stuck fot kNoiseFloorCounterMax blocks until the noise floor
starts increasing upwards.  With this initialization it starts increasing
upwards immediately, thereby as quickly as possible reaching the true noise
floor.

The alternative is to initialize the noise floor to a huge value from the
beginning and then decrease, but since that easily may have the effect that the
noise floor is initially overestimated which in turn may cause echo leakage, I
prefer to do this in the implemented way.

WDYT?

[ivoc, 2017-04-19 15:04:00]

LGTM, but see my reply about braces below.

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:129: active_render_block &&
(!SaturatedCapture()) ? 1 : 0;
On 2017/04/19 14:56:20, peah-webrtc wrote:
> On 2017/04/19 14:47:47, ivoc wrote:
> > This looks kind of strange to me. The result of "active_render_block &&
> > (!SaturatedCapture()) ? 1 : 0" is a bool, right? I don't think it's good to
> add
> > a bool to an int.
> 
> No, it is actually an int. That is a conditional using the ternary operator (
of
> the type test ? result a : result b;). That expression should return an int if
I
> did not get this wrong.
> 
> PTAL

Ok, in that case I suggest adding some braces to make this more clear to the
reader: (active_render_block && !SaturatedCapture()) ? 1 : 0;

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc (right):

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:69:
std::max((*X2_noise_floor)[k] * 1.1f, kNoiseFloorMin);
On 2017/04/19 14:56:20, peah-webrtc wrote:
> On 2017/04/19 14:47:47, ivoc wrote:
> > Shouldn't (*X2_noise_floor_counter)[k] be reset to 0 here? Otherwise this
will
> > keep getting multiplied by 1.1 on each iteration.
> 
> Yes, that is the purpose of this. When the noise floor has not been updated
> downwards for kNoiseFloorCounterMax blocks, the minimum statistics starts to
> exponentially increase the noise floor until it is again updated downwards
> (which occurs when the noise floor has exceeded the power at hand).

Acknowledged.

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/residual_echo_estimator.cc:158:
X2_noise_floor_counter_.fill(kNoiseFloorCounterMax);
On 2017/04/19 14:56:20, peah-webrtc wrote:
> On 2017/04/19 14:47:47, ivoc wrote:
> > Shouldn't the counters be initialized at zero?
> 
> No, they are initialized to kNoiseFloorCounterMax on purpose. I don't want the
> initial value to be stuck fot kNoiseFloorCounterMax blocks until the noise
floor
> starts increasing upwards.  With this initialization it starts increasing
> upwards immediately, thereby as quickly as possible reaching the true noise
> floor.
> 
> The alternative is to initialize the noise floor to a huge value from the
> beginning and then decrease, but since that easily may have the effect that
the
> noise floor is initially overestimated which in turn may cause echo leakage, I
> prefer to do this in the implemented way.
> 
> WDYT?

Ok, sounds like you've thought about this :-)

[peah-webrtc, 2017-04-19 15:16:14]

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

[peah-webrtc, 2017-04-19 15:16:42]

Thanks for the comments! I added the suggested braces.

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/aec_state.cc (right):

https://codereview.webrtc.org/2823903003/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/aec_state.cc:129: active_render_block &&
(!SaturatedCapture()) ? 1 : 0;
On 2017/04/19 15:04:00, ivoc wrote:
> On 2017/04/19 14:56:20, peah-webrtc wrote:
> > On 2017/04/19 14:47:47, ivoc wrote:
> > > This looks kind of strange to me. The result of "active_render_block &&
> > > (!SaturatedCapture()) ? 1 : 0" is a bool, right? I don't think it's good
to
> > add
> > > a bool to an int.
> > 
> > No, it is actually an int. That is a conditional using the ternary operator
(
> of
> > the type test ? result a : result b;). That expression should return an int
if
> I
> > did not get this wrong.
> > 
> > PTAL
> 
> Ok, in that case I suggest adding some braces to make this more clear to the
> reader: (active_render_block && !SaturatedCapture()) ? 1 : 0;

Done.

[peah-webrtc, 2017-04-19 15:17:05]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-04-19 15:17:06]

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/2823903003/#ps60001 (title:
"Changes in response to reviewer comments")

[commit-bot: I haz the power, 2017-04-19 15:17:16]

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-19 16:03:41]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1492615025684600,
"parent_rev": "d5c77abbaa893f0940acf584ab1abfab1f14345f", "commit_rev":
"e52a203a56c7876bc226c04464563555433fa0c1"}

[commit-bot: I haz the power, 2017-04-19 16:03:44]

Description was changed from

==========
Echo canceller 3 improvements for setups with headsets.

This CL improves the echo cancellation performance on setups where 
headsets are used (systems with such low echo path gain 
that no correlation between the render and capture signals
can be found) in 4 ways:
1) The echo path gain for systems with headsets is assumed to be 
nonzero.
2) The stationary component of the render power is not included
in nonlinear echo power estimate.
3) The behavior after echo path gain changes is made less cautious.
4) The detection of systems with headsets is made more rapid.

BUG=chromium:712651, webrtc:6018
==========

to

==========
Echo canceller 3 improvements for setups with headsets.

This CL improves the echo cancellation performance on setups where 
headsets are used (systems with such low echo path gain 
that no correlation between the render and capture signals
can be found) in 4 ways:
1) The echo path gain for systems with headsets is assumed to be 
nonzero.
2) The stationary component of the render power is not included
in nonlinear echo power estimate.
3) The behavior after echo path gain changes is made less cautious.
4) The detection of systems with headsets is made more rapid.

BUG=chromium:712651, webrtc:6018

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

[commit-bot: I haz the power, 2017-04-19 16:03:45]

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