Change NetEq::GetAudio to use AudioFrame

Change NetEq::GetAudio to use AudioFrame

With this change, NetEq now uses AudioFrame as output type, like the
surrounding functions in ACM and VoiceEngine already do.

The computational savings is probably slim, since one memcpy is
removed while another one is added (both in AcmReceiver::GetAudio).

More simplifications and clean-up will be done in
AcmReceiver::GetAudio in future CLs.

BUG=webrtc:5607
Committed: https://crrev.com/6d8e011b648900f24c3c7b4488d3694ce4deca70
Cr-Commit-Position: refs/heads/master@{#11874}

[hlundin-webrtc, 2016-03-01 14:16:20]

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

[hlundin-webrtc, 2016-03-01 14:16:20]

Ivo,

Please, review this CL. I'm sorry it is quite large, but as it turns out, quite
a number of tests were calling the modified method.

Thanks!

[ivoc, 2016-03-03 16:28:15]

Nice, I only have a few minor remarks, see below. LGTM

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_external_decoder_unittest.cc:299: for
(size_t i = 0; i < output.samples_per_channel_; ++i) {
Do we know that there is a single channel here?

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:866: const size_t
samples_from_sync = sync_buffer_->GetNextAudioInterleaved(
Does it make sense to rewrite this function to accept an AudioFrame as input?

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.h:98: 
Wouldn't it be good to keep some comments here?

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:855: const size_t
kExpectedOutputSize = 10 * kSampleRateHz / 1000 * kChannels;
Some brackets or reshuffling would be nice to make the order of operations more
clear.

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1069:
AudioFrame::kMaxDataSizeSamples);  // Set to non-zero.
Should be multiplied by sizeof(int16_t), right?

[hlundin-webrtc, 2016-03-04 09:52:49]

Taking care of review comments

[hlundin-webrtc, 2016-03-04 09:55:32]

Some non-trivial updates were made as a result of your comments. PTAL again.

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_external_decoder_unittest.cc:299: for
(size_t i = 0; i < output.samples_per_channel_; ++i) {
On 2016/03/03 16:28:14, ivoc wrote:
> Do we know that there is a single channel here?

Now we know. :)

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.cc:866: const size_t
samples_from_sync = sync_buffer_->GetNextAudioInterleaved(
On 2016/03/03 16:28:15, ivoc wrote:
> Does it make sense to rewrite this function to accept an AudioFrame as input?

Done. It took some more re-writing.

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl.h:98: 
On 2016/03/03 16:28:15, ivoc wrote:
> Wouldn't it be good to keep some comments here?

This was essentially a copy of the comment in neteq.h (where the interface is
declared). I've applied a more restrictive policy for function description
lately: keep the comments in one place only to avoid discrepancy over time.

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:855: const size_t
kExpectedOutputSize = 10 * kSampleRateHz / 1000 * kChannels;
On 2016/03/03 16:28:15, ivoc wrote:
> Some brackets or reshuffling would be nice to make the order of operations
more
> clear.

Done.

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1069:
AudioFrame::kMaxDataSizeSamples);  // Set to non-zero.
On 2016/03/03 16:28:15, ivoc wrote:
> Should be multiplied by sizeof(int16_t), right?

True. But even better to use sizeof the array itself, since it is statically
known.

https://codereview.webrtc.org/1750353002/diff/20001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/audio_multi_vector.cc (right):

https://codereview.webrtc.org/1750353002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/audio_multi_vector.cc:134:
RTC_DCHECK(destination);
This change is just to keep consistency with
SyncBuffer::GetNextAudioInterleaved.

[hlundin-webrtc, 2016-03-04 09:55:42]

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

[commit-bot: I haz the power, 2016-03-04 09:55:49]

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

[commit-bot: I haz the power, 2016-03-04 09:58:01]

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

[commit-bot: I haz the power, 2016-03-04 09:58:02]

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

[hlundin-webrtc, 2016-03-04 10:22:32]

Rebase

[hlundin-webrtc, 2016-03-04 10:23:19]

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

[commit-bot: I haz the power, 2016-03-04 10:23:32]

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

[ivoc, 2016-03-04 10:54:08]

Still LGTM :-)

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_impl_unittest.cc:855: const size_t
kExpectedOutputSize = 10 * kSampleRateHz / 1000 * kChannels;
On 2016/03/04 09:55:31, hlundin-webrtc wrote:
> On 2016/03/03 16:28:15, ivoc wrote:
> > Some brackets or reshuffling would be nice to make the order of operations
> more
> > clear.
> 
> Done.

Great, the comments make it much clearer.

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

https://codereview.webrtc.org/1750353002/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1069:
AudioFrame::kMaxDataSizeSamples);  // Set to non-zero.
On 2016/03/04 09:55:31, hlundin-webrtc wrote:
> On 2016/03/03 16:28:15, ivoc wrote:
> > Should be multiplied by sizeof(int16_t), right?
> 
> True. But even better to use sizeof the array itself, since it is statically
> known.

Yep, good idea.

[commit-bot: I haz the power, 2016-03-04 12:24:19]

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

[commit-bot: I haz the power, 2016-03-04 12:24:20]

Dry run: Try jobs failed on following builders:
  android_arm64_rel on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  android_dbg on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  linux_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  mac_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)
  win_baremetal on tryserver.webrtc (JOB_TIMED_OUT, no build URL)

[hlundin-webrtc, 2016-03-04 12:33:10]

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

[commit-bot: I haz the power, 2016-03-04 12:33:21]

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

[commit-bot: I haz the power, 2016-03-04 14:33:51]

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

[commit-bot: I haz the power, 2016-03-04 14:33:52]

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

[hlundin-webrtc, 2016-03-04 18:32:36]

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

[commit-bot: I haz the power, 2016-03-04 18:32:43]

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

[commit-bot: I haz the power, 2016-03-04 18:34:29]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-04 18:34:34]

Description was changed from

==========
Change NetEq::GetAudio to use AudioFrame

With this change, NetEq now uses AudioFrame as output type, like the
surrounding functions in ACM and VoiceEngine already do.

The computational savings is probably slim, since one memcpy is
removed while another one is added (both in AcmReceiver::GetAudio).

More simplifications and clean-up will be done in
AcmReceiver::GetAudio in future CLs.

BUG=webrtc:5607
==========

to

==========
Change NetEq::GetAudio to use AudioFrame

With this change, NetEq now uses AudioFrame as output type, like the
surrounding functions in ACM and VoiceEngine already do.

The computational savings is probably slim, since one memcpy is
removed while another one is added (both in AcmReceiver::GetAudio).

More simplifications and clean-up will be done in
AcmReceiver::GetAudio in future CLs.

BUG=webrtc:5607

Committed: https://crrev.com/6d8e011b648900f24c3c7b4488d3694ce4deca70
Cr-Commit-Position: refs/heads/master@{#11874}
==========

[commit-bot: I haz the power, 2016-03-04 18:34:35]

Patchset 3 (id:??) landed as
https://crrev.com/6d8e011b648900f24c3c7b4488d3694ce4deca70
Cr-Commit-Position: refs/heads/master@{#11874}