Corrected the behavior in AEC3 during buffer overruns and underruns

Corrected the behavior in AEC3 during buffer overruns and underruns

This CL corrects the behavior in AEC3 during buffer overruns 
and underruns in three ways.
1) When there is no render signal available (due to a buffering
 issues, a zero block is inserted instead of the previous render 
 block. This avoids the same block being repeatedly inserted when
 there are many back-to-back calls.
2) The internal counters in the main adaptive filter gain are also
 reset when the filter is reset.
3) The internal counters in the shadow adaptive filter gain are
 reset when the filter is reset.

BUG=chromium:717920, webrtc:7559
Review-Url: https://codereview.webrtc.org/2862533002
Cr-Commit-Position: refs/heads/master@{#17991}
Committed: https://chromium.googlesource.com/external/webrtc/+/debaa442ed0317e215706c8c7d91c880e6bd6051

[peah-webrtc, 2017-05-03 09:29:24]

Patchset #1 (id:1) has been deleted

[peah-webrtc, 2017-05-03 09:42:43]

Description was changed from

==========
Fixed underun behavior


Fixed underun behavior

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

to

==========
Corrected the behavior in AEC3 during buffer overruns and underruns

This CL corrects the behavior in AEC3 during buffer overruns 
and underruns in three ways.
1) When there is no render signal available (due to a buffering
 issues, a zero block is inserted instead of the previous render 
 block. This avoids the same block being repeatedly inserted when
 there are many back-to-back calls.
2) The internal counters in the main adaptive filter gain are also
 reset when the filter is reset.
3) The internal counters in the shadow adaptive filter gain are
 reset when the filter is reset.

BUG=chromium:717920,webrtc:7559
==========

[peah-webrtc, 2017-05-03 09:44:08]

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

[commit-bot: I haz the power, 2017-05-03 09:44:11]

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-05-03 10:00:21]

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

[commit-bot: I haz the power, 2017-05-03 10:00:21]

Dry run: This issue passed the CQ dry run.

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

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

[peah-webrtc, 2017-05-03 10:53:11]

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

[aleloi, 2017-05-03 11:23:40]

lgtm!

A few comments though.

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right):

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:166: if (!underrun)
{
I think if/else statements should check for the positive case, 'if (underrun) {}
else {}', unless there is a reason not to. Then the reader has to do one mental
step less. In this case, I don't see a reason to have the negative case first.

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:176: if (!underrun)
{
Same here.

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc (right):

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc:25: }
Is there a risk that the echo canceler doesn't perform well because small
changes in the echo path cause components to be reset? Should be perhaps track
statistics for the frequency of such resets?

[peah-webrtc, 2017-05-03 11:31:23]

Thanks for the comments and the quick review! I will upload a patch accordingly.

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/render_delay_buffer.cc (right):

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:166: if (!underrun)
{
On 2017/05/03 11:23:39, aleloi wrote:
> I think if/else statements should check for the positive case, 'if (underrun)
{}
> else {}', unless there is a reason not to. Then the reader has to do one
mental
> step less. In this case, I don't see a reason to have the negative case first.

I agree, good point! I prefer to have the most normal case in the if-section and
the exception in the else-section. But with the underrun flag it becomes weird.
Will change.

Done.

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/render_delay_buffer.cc:176: if (!underrun)
{
On 2017/05/03 11:23:39, aleloi wrote:
> Same here.

Done.

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc (right):

https://codereview.webrtc.org/2862533002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/aec3/shadow_filter_update_gain.cc:25: }
On 2017/05/03 11:23:39, aleloi wrote:
> Is there a risk that the echo canceler doesn't perform well because small
> changes in the echo path cause components to be reset? Should be perhaps track
> statistics for the frequency of such resets?

I see that there is a problem with the naming here. The echo path changes
referred are echo path changes introduced by buffering issues or by
nonnegligible AGC changes, and not by small variations in the echo path.

The buffering issues are logged, but maybe not to a sufficient extent. 

Ideally, the calls to HandleEchoPathChange should be very rare, so this change
is safe, in particular as the resets of the counters only pauses the filter
adaptation for 12 blocks = 48 ms.

[peah-webrtc, 2017-05-03 11:34:00]

The CQ bit was checked by peah@webrtc.org

[peah-webrtc, 2017-05-03 11:34:00]

The patchset sent to the CQ was uploaded after l-g-t-m from aleloi@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2862533002/#ps40001 (title:
"Changes in response to reviewer comments")

[commit-bot: I haz the power, 2017-05-03 11:34:11]

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-05-03 12:39:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493811240556490,
"parent_rev": "5af6e25b106f20c269d00ab33f751c0eff6162ad", "commit_rev":
"debaa442ed0317e215706c8c7d91c880e6bd6051"}

[commit-bot: I haz the power, 2017-05-03 12:39:12]

Description was changed from

==========
Corrected the behavior in AEC3 during buffer overruns and underruns

This CL corrects the behavior in AEC3 during buffer overruns 
and underruns in three ways.
1) When there is no render signal available (due to a buffering
 issues, a zero block is inserted instead of the previous render 
 block. This avoids the same block being repeatedly inserted when
 there are many back-to-back calls.
2) The internal counters in the main adaptive filter gain are also
 reset when the filter is reset.
3) The internal counters in the shadow adaptive filter gain are
 reset when the filter is reset.

BUG=chromium:717920,webrtc:7559
==========

to

==========
Corrected the behavior in AEC3 during buffer overruns and underruns

This CL corrects the behavior in AEC3 during buffer overruns 
and underruns in three ways.
1) When there is no render signal available (due to a buffering
 issues, a zero block is inserted instead of the previous render 
 block. This avoids the same block being repeatedly inserted when
 there are many back-to-back calls.
2) The internal counters in the main adaptive filter gain are also
 reset when the filter is reset.
3) The internal counters in the shadow adaptive filter gain are
 reset when the filter is reset.

BUG=chromium:717920,webrtc:7559

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

[commit-bot: I haz the power, 2017-05-03 12:39:13]

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