Using ring buffer for AudioVector in NetEq.

Using ring buffer for AudioVector in NetEq.

AudioVector used NetEq was based on a shift buffer, which has a high complexity, and the complexity is very much dependent on the capacity of the buffer.

This CL changes the shift buffer to a ring buffer.

Reduction in the CPU usages of NetEq is expected.

BUG=608644
R=henrik.lundin@webrtc.org

Committed: https://crrev.com/79553cb66e4c2d0651a0236e25ad06dd0ee2e59b
Cr-Commit-Position: refs/heads/master@{#12676}

[minyue-webrtc, 2016-05-07 23:00:53]

Patchset #1 (id:1) has been deleted

[minyue-webrtc, 2016-05-07 23:01:01]

Patchset #1 (id:20001) has been deleted

[minyue-webrtc, 2016-05-07 23:01:08]

Patchset #1 (id:40001) has been deleted

[minyue-webrtc, 2016-05-07 23:01:15]

Patchset #1 (id:60001) has been deleted

[minyue-webrtc, 2016-05-07 23:01:37]

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

[commit-bot: I haz the power, 2016-05-07 23:01:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948483002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948483002/80001

[commit-bot: I haz the power, 2016-05-07 23:09:18]

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

[commit-bot: I haz the power, 2016-05-07 23:09:19]

Dry run: Try jobs failed on following builders:
  mac_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/14919)

[minyue-webrtc, 2016-05-07 23:15:03]

Patchset #1 (id:80001) has been deleted

[minyue-webrtc, 2016-05-07 23:15:12]

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

[commit-bot: I haz the power, 2016-05-07 23:15:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948483002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948483002/100001

[commit-bot: I haz the power, 2016-05-07 23:21:28]

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

[commit-bot: I haz the power, 2016-05-07 23:21:29]

Dry run: Try jobs failed on following builders:
  linux_ubsan_vptr on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/linux_ubsan_vptr/builds...)
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/14514)

[minyue-webrtc, 2016-05-07 23:38:35]

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

[commit-bot: I haz the power, 2016-05-07 23:38:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1948483002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1948483002/120001

[minyue-webrtc, 2016-05-07 23:38:47]

Patchset #1 (id:100001) has been deleted

[commit-bot: I haz the power, 2016-05-08 01:22:48]

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

[commit-bot: I haz the power, 2016-05-08 01:22:49]

Dry run: This issue passed the CQ dry run.

[minyue-webrtc, 2016-05-08 06:01:08]

Patchset #2 (id:140001) has been deleted

[minyue-webrtc, 2016-05-08 06:04:41]

Description was changed from

==========
towards using ring buffer for audio_vector


Change aggregation window of aecDivergentFilterFraction to 1 second.

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

to

==========
Using ring buffer for AudioVector in NetEq.

AudioVector used NetEq was based on a shift buffer, which has a high complexity,
and the complexity is very much dependent on the capacity of the buffer.

This CL changes the shift buffer to a ring buffer.

Reduction in the CPU usages of NetEq is expected.

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

[minyue-webrtc, 2016-05-08 06:04:41]

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

[minyue-webrtc, 2016-05-08 06:13:53]

Hi Henrik,

I changed the shift buffer in AudioVector to ring buffer. The CPU usage in
reduced by 17% on my Macbook. That is, the CPU usage on the chrome bug 608644
will likely become even lower than before 120 ms support was introduced.

Would you help review this?

https://codereview.webrtc.org/1948483002/diff/120001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/audio_vector.cc (right):

https://codereview.webrtc.org/1948483002/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:25:
begin_index_(kDefaultInitialSize),
initialize like gives a better test of loop back. This will be changes to normal
in the next patch set.

https://codereview.webrtc.org/1948483002/diff/120001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/merge.cc (right):

https://codereview.webrtc.org/1948483002/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/merge.cc:67: std::unique_ptr<int16_t[]>
input_channel(
will more the definition out of the loop.

https://codereview.webrtc.org/1948483002/diff/120001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/normal.cc (right):

https://codereview.webrtc.org/1948483002/diff/120001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/normal.cc:73: std::unique_ptr<int16_t[]>
signal(new int16_t[length_per_channel]);
will remove definition out of loop.

[minyue-webrtc, 2016-05-08 06:14:12]

Description was changed from

==========
Using ring buffer for AudioVector in NetEq.

AudioVector used NetEq was based on a shift buffer, which has a high complexity,
and the complexity is very much dependent on the capacity of the buffer.

This CL changes the shift buffer to a ring buffer.

Reduction in the CPU usages of NetEq is expected.

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

to

==========
Using ring buffer for AudioVector in NetEq.

AudioVector used NetEq was based on a shift buffer, which has a high complexity,
and the complexity is very much dependent on the capacity of the buffer.

This CL changes the shift buffer to a ring buffer.

Reduction in the CPU usages of NetEq is expected.

BUG=608644
==========

[hlundin-webrtc, 2016-05-10 07:53:19]

Impressive work! I just have a few minor comments and questions.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/audio_vector.cc (right):

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:23: : array_(new
int16_t[kDefaultInitialSize + 1]),
You can just delegate this to the other ctor:
AudioVector::AudioVector() : AudioVector(kDefaultInitialSize) {}

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:44: if (!copy_to)
Simply RTC_DCHECK(copy_to). I think the old implementation is a bit too nice
regarding invalid pointer parameters.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:109: position =
std::min(append_this.Size(), position);
Is it valid to call with a position larger than Size(), or should we just
DCHECK?

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:110: length = std::min(length,
append_this.Size() - position);
Same with length...

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:173: if (position <= Size() -
position) {
Please, add a comment about what this if-statement is doing, and why.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:187: if (position <= Size() -
position) {
Comment again.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:209: position +
first_chunk_length);
What if position + first_chunk_length > Size()? Then position will be capped in
OverwriteAt(const int16_t*...), won't it?

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:292: const size_t
first_chunk_length =
Why not use the CopyTo method?

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:313: temp_array.reset(new
int16_t[move_chunk_length]);
It seems inefficient to me to store the move-chunk in a temp array. Can you do
it like this instead?

1. Reserve new size.
2. memmove the move-chunk within the array.
3. insert the new data.

Or is it still more efficient to do like this?

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/comfort_noise.cc (right):

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/comfort_noise.cc:70: // The expression
&(*output)[0][0] is a pointer to the first element in
This comment needs updating.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/dsp_helper.h (right):

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/dsp_helper.h:76: // Same as above, but
processes |length| samples from |signal|, starting at
"Same as above, but for an AudioMultiVector."

[minyue-webrtc, 2016-05-10 12:47:49]

Patchset #3 (id:180001) has been deleted

[minyue-webrtc, 2016-05-10 12:48:07]

PTAL

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/audio_vector.cc (right):

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:23: : array_(new
int16_t[kDefaultInitialSize + 1]),
On 2016/05/10 07:53:18, hlundin-webrtc wrote:
> You can just delegate this to the other ctor:
> AudioVector::AudioVector() : AudioVector(kDefaultInitialSize) {}

Done.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:44: if (!copy_to)
On 2016/05/10 07:53:17, hlundin-webrtc wrote:
> Simply RTC_DCHECK(copy_to). I think the old implementation is a bit too nice
> regarding invalid pointer parameters.

Done.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:109: position =
std::min(append_this.Size(), position);
On 2016/05/10 07:53:18, hlundin-webrtc wrote:
> Is it valid to call with a position larger than Size(), or should we just
> DCHECK?

good point. since |length| and |position| refers the input vector, it is surely
better to prevent the value to become irregular.

whereas, in other places, like 171, when |position| refers to this->array_, we
can "Cap the position" to keep up with the old version.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:110: length = std::min(length,
append_this.Size() - position);
On 2016/05/10 07:53:17, hlundin-webrtc wrote:
> Same with length...

Done.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:173: if (position <= Size() -
position) {
On 2016/05/10 07:53:18, hlundin-webrtc wrote:
> Please, add a comment about what this if-statement is doing, and why.

Done.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:187: if (position <= Size() -
position) {
On 2016/05/10 07:53:18, hlundin-webrtc wrote:
> Comment again.

Done.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:209: position +
first_chunk_length);
On 2016/05/10 07:53:18, hlundin-webrtc wrote:
> What if position + first_chunk_length > Size()? Then position will be capped
in
> OverwriteAt(const int16_t*...), won't it?

OverwriteAt(const int16_t* insert_this will reserve the space and it will not
happen. But I found that it is better to reserve space here also. see a
following patch.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:292: const size_t
first_chunk_length =
On 2016/05/10 07:53:18, hlundin-webrtc wrote:
> Why not use the CopyTo method?

very good point, I should have used it. Thanks!

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/audio_vector.cc:313: temp_array.reset(new
int16_t[move_chunk_length]);
On 2016/05/10 07:53:18, hlundin-webrtc wrote:
> It seems inefficient to me to store the move-chunk in a temp array. Can you do
> it like this instead?
> 
> 1. Reserve new size.
> 2. memmove the move-chunk within the array.
> 3. insert the new data.
> 
> Or is it still more efficient to do like this?

I thought about it, but it can be a bit complicated. quite a few cases to deal
with (copy_from wrap/no warp + copy_to wrap/no warp). I think it might be better
to revisit this later. I added a todo.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/comfort_noise.cc (right):

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/comfort_noise.cc:70: // The expression
&(*output)[0][0] is a pointer to the first element in
On 2016/05/10 07:53:18, hlundin-webrtc wrote:
> This comment needs updating.

thanks. I missed it.

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
File webrtc/modules/audio_coding/neteq/dsp_helper.h (right):

https://codereview.webrtc.org/1948483002/diff/160001/webrtc/modules/audio_cod...
webrtc/modules/audio_coding/neteq/dsp_helper.h:76: // Same as above, but
processes |length| samples from |signal|, starting at
On 2016/05/10 07:53:19, hlundin-webrtc wrote:
> "Same as above, but for an AudioMultiVector."

Done.

[hlundin-webrtc, 2016-05-10 13:16:26]

lgtm

[minyue-webrtc, 2016-05-10 13:35:50]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2016-05-10 13:35:58]

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

[commit-bot: I haz the power, 2016-05-10 13:43:10]

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

[commit-bot: I haz the power, 2016-05-10 13:43:11]

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

[minyue-webrtc, 2016-05-10 14:53:02]

Hi Henrik,

since we becomes stricter to nullptr as input to CopyTo, we need to remove one
unittest. I guess you won't mind, and will commit right away. Let me know if you
want to change anything.

[minyue-webrtc, 2016-05-10 14:54:05]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-05-10 14:54:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1948483002/#ps220001 (title:
"removing a unittest")

[commit-bot: I haz the power, 2016-05-10 14:54:11]

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

[commit-bot: I haz the power, 2016-05-10 15:00:21]

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

[commit-bot: I haz the power, 2016-05-10 15:00:22]

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

[minyue-webrtc, 2016-05-10 17:56:16]

Description was changed from

==========
Using ring buffer for AudioVector in NetEq.

AudioVector used NetEq was based on a shift buffer, which has a high complexity,
and the complexity is very much dependent on the capacity of the buffer.

This CL changes the shift buffer to a ring buffer.

Reduction in the CPU usages of NetEq is expected.

BUG=608644
==========

to

==========
Using ring buffer for AudioVector in NetEq.

AudioVector used NetEq was based on a shift buffer, which has a high complexity,
and the complexity is very much dependent on the capacity of the buffer.

This CL changes the shift buffer to a ring buffer.

Reduction in the CPU usages of NetEq is expected.

BUG=608644
R=henrik.lundin@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/79553cb66e4c2d0651a0236e2...
==========

[minyue-webrtc, 2016-05-10 17:56:18]

Committed patchset #4 (id:220001) manually as
79553cb66e4c2d0651a0236e25ad06dd0ee2e59b (presubmit successful).

[commit-bot: I haz the power, 2016-05-10 17:56:20]

Description was changed from

==========
Using ring buffer for AudioVector in NetEq.

AudioVector used NetEq was based on a shift buffer, which has a high complexity,
and the complexity is very much dependent on the capacity of the buffer.

This CL changes the shift buffer to a ring buffer.

Reduction in the CPU usages of NetEq is expected.

BUG=608644
R=henrik.lundin@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/79553cb66e4c2d0651a0236e2...
==========

to

==========
Using ring buffer for AudioVector in NetEq.

AudioVector used NetEq was based on a shift buffer, which has a high complexity,
and the complexity is very much dependent on the capacity of the buffer.

This CL changes the shift buffer to a ring buffer.

Reduction in the CPU usages of NetEq is expected.

BUG=608644
R=henrik.lundin@webrtc.org

Committed: https://crrev.com/79553cb66e4c2d0651a0236e25ad06dd0ee2e59b
Cr-Commit-Position: refs/heads/master@{#12676}
==========