audio_processing/aec: make delay estimator aware of starving farend buffer

audio_processing/aec: make delay estimator aware of starving farend buffer

We've seen that if we get a buffer underrun followed by a sudden buffer build up the DA-AEC can't really catch up even though it should be possible to estimate the upcoming difference. We have a feature for this already, but that is only used in the regular AEC. This CL turns that feature on also for DA-AEC.

- Adds a helper function MoveFarReadPtrWithoutSystemDelayUpdate()
- Only apply conservative correction for positive delays, where we can put the AEC into a non-causal state
- Stuff the farend buffer if we don't have enough data to process w.r.t. to current nearend buffer.
- Always run delay estimation based on reported delays to catch buffer starvation.


BUG=
R=henrik.lundin@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/9002cc426dab7a576f5247f45ba888cd081a39f0

[bjornv1, 2015-06-15 09:08:35]

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

[bjornv1, 2015-06-16 12:50:21]

Henrik, I'd really appreciate if you could take a look at this before I go to
STO tomorrow morning.

[hlundin-webrtc, 2015-06-16 20:14:38]

lgtm with two comments.

https://codereview.webrtc.org/1180423006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1180423006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:861:
WebRtc_MoveReadPtr(self->far_buf_windowed, elements);
If I understand it correctly, WebRtc_MoveReadPtr returns the number of elements
the pointer was moved, which may differ from the input |elements|. Is there any
point in verifying that all returned the same value?

https://codereview.webrtc.org/1180423006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:1819: // can since the delay
estimation can be wrong.  We therefore stuff the
Missing word? "... we can [what?]"

[bjornv1, 2015-06-16 20:30:01]

Committed patchset #2 (id:20001) manually as
9002cc426dab7a576f5247f45ba888cd081a39f0 (presubmit successful).

[bjornv1, 2015-06-16 20:30:06]

https://codereview.webrtc.org/1180423006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1180423006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:861:
WebRtc_MoveReadPtr(self->far_buf_windowed, elements);
On 2015/06/16 20:14:38, hlundin-webrtc wrote:
> If I understand it correctly, WebRtc_MoveReadPtr returns the number of
elements
> the pointer was moved, which may differ from the input |elements|. Is there
any
> point in verifying that all returned the same value?

Yes, you're right, though all these buffers are constructed with the same size,
hence if one of them "breaks", they all do. We could add asserts for sure to
catch such an event. I'll commit without assert and add it in a second CL if you
insist.

https://codereview.webrtc.org/1180423006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:1819: // can since the delay
estimation can be wrong.  We therefore stuff the
On 2015/06/16 20:14:38, hlundin-webrtc wrote:
> Missing word? "... we can [what?]"

Done.

[hlundin-webrtc, 2015-06-16 20:32:30]

https://codereview.webrtc.org/1180423006/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/aec/aec_core.c (right):

https://codereview.webrtc.org/1180423006/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/aec/aec_core.c:861:
WebRtc_MoveReadPtr(self->far_buf_windowed, elements);
On 2015/06/16 20:30:05, bjornv1 wrote:
> On 2015/06/16 20:14:38, hlundin-webrtc wrote:
> > If I understand it correctly, WebRtc_MoveReadPtr returns the number of
> elements
> > the pointer was moved, which may differ from the input |elements|. Is there
> any
> > point in verifying that all returned the same value?
> 
> Yes, you're right, though all these buffers are constructed with the same
size,
> hence if one of them "breaks", they all do. We could add asserts for sure to
> catch such an event. I'll commit without assert and add it in a second CL if
you
> insist.

I think it is fine the way it is. Don't bother with the asserts.