NetEq: Fixing a corner case with depleted sync buffer

NetEq: Fixing a corner case with depleted sync buffer

In some cases, the number of samples (per channel) in NetEq's sync
buffer could fall below the allowed minimum (5 samples for narrowband,
scaling for other rates). If the number of samples extracted from the
buffer was smaller than the desired number, an error is
returned. However, if the decoder returns fewer samples than expected,
it could happen that the sync buffer level falls under the minimum,
but enough samples are extracted. This triggered an assert. With this
change, the minimum level of the sync buffer is always enforced.

A test is implemented to trigger the problem. It made the assert fire
without this fix, but it now passes.

BUG=webrtc:4840
R=minyue@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/05f71fcb61df680ab0e0dab06ed6578ce062fa21

[hlundin-webrtc, 2015-08-28 07:22:44]

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

[hlundin-webrtc, 2015-08-28 07:22:44]

Minyue,

Please, take a look at this CL.

Thanks!

[minyue-webrtc, 2015-08-30 21:50:27]

https://codereview.chromium.org/1324453002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.chromium.org/1324453002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:845: missing_lookahead_samples);
How do you know that next_index_ >= missing_lookahead_samples

[hlundin-webrtc, 2015-08-31 08:03:09]

Adding a DCHECK

[hlundin-webrtc, 2015-08-31 08:08:25]

https://codereview.webrtc.org/1324453002/diff/1/webrtc/modules/audio_coding/n...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.webrtc.org/1324453002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:845: missing_lookahead_samples);
On 2015/08/30 21:50:27, minyue-webrtc wrote:
> How do you know that next_index_ >= missing_lookahead_samples

next_index_ is by definition FutureLength() from the end of sync_buffer, that
is, next_index_ = sync_buffer_->Size() - sync_buffer_->FutureLength().
Meanwhile, missing_lookahead_samples = expand_->overlap_length() -
sync_buffer_->FutureLength().

So, next_index_ >= missing_lookahead_samples holds true if sync_buffer_->Size()
>= expand_->overlap_length().

sync_buffer_->Size() is 120 ms and overlap_length() is 0.625 ms (5 samples in
8000 Hz). So, it holds true. But I'll add a DCHECK.

[minyue-webrtc, 2015-09-01 07:36:07]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2015-09-01 07:36:07]

lgtm

https://codereview.chromium.org/1324453002/diff/1/webrtc/modules/audio_coding...
File webrtc/modules/audio_coding/neteq/neteq_impl.cc (right):

https://codereview.chromium.org/1324453002/diff/1/webrtc/modules/audio_coding...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:845: missing_lookahead_samples);
On 2015/08/31 08:08:25, hlundin-webrtc wrote:
> On 2015/08/30 21:50:27, minyue-webrtc wrote:
> > How do you know that next_index_ >= missing_lookahead_samples
> 
> next_index_ is by definition FutureLength() from the end of sync_buffer, that
> is, next_index_ = sync_buffer_->Size() - sync_buffer_->FutureLength().
> Meanwhile, missing_lookahead_samples = expand_->overlap_length() -
> sync_buffer_->FutureLength().
> 
> So, next_index_ >= missing_lookahead_samples holds true if
sync_buffer_->Size()
> >= expand_->overlap_length().
> 
> sync_buffer_->Size() is 120 ms and overlap_length() is 0.625 ms (5 samples in
> 8000 Hz). So, it holds true. But I'll add a DCHECK.

Acknowledged.

[commit-bot: I haz the power, 2015-09-01 07:36:16]

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

[commit-bot: I haz the power, 2015-09-01 07:40:36]

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

[commit-bot: I haz the power, 2015-09-01 07:40:37]

Try jobs failed on following builders:
  mac_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_x64_rel/builds/8980)

[hlundin-webrtc, 2015-09-01 09:05:19]

The CQ bit was checked by henrik.lundin@webrtc.org

[commit-bot: I haz the power, 2015-09-01 09:05:31]

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

[commit-bot: I haz the power, 2015-09-01 09:36:25]

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

[commit-bot: I haz the power, 2015-09-01 09:36:26]

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

[hlundin-webrtc, 2015-09-01 09:52:12]

Committed patchset #2 (id:20001) manually as
05f71fcb61df680ab0e0dab06ed6578ce062fa21 (presubmit successful).