Unit test for AudioFrame output from AcmReceiver::GetAudio

Unit test for AudioFrame output from AcmReceiver::GetAudio

This new unit test verifies the parameter fields (not the audio data
itself) written to the AudioFrame output by AcmReceiver::GetAudio.

Also corrected a few comments reflecting recent changes in the code.

BUG=webrtc:5669
Committed: https://crrev.com/7dc68897d3614be0dce3ad565e4ceb14fc8f53d3
Cr-Commit-Position: refs/heads/master@{#12253}

[hlundin-webrtc, 2016-04-05 09:20:17]

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

[hlundin-webrtc, 2016-04-05 09:20:18]

Minyue,

Please, review this new unit tests.

Thanks!

[minyue-webrtc, 2016-04-05 13:00:22]

LG with some questions for my curiosity.

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc (right):

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:321: const int
num_10ms_frames = codec.inst.pacsize / (10 * samples_per_ms);
no CheckedDivExact on this?

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:325: uint32_t
expected_output_ts = last_packet_send_timestamp_ - 30u;
what would it be if |last_packet_send_timestamp_| is smaller than 30u?

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:338:
EXPECT_EQ(AudioFrame::kVadActive, frame.vad_activity_);
why is VadActive when packet is silent?

[hlundin-webrtc, 2016-04-05 13:23:37]

Fixing Minyue's comments

[hlundin-webrtc, 2016-04-05 13:24:15]

Updated. PTAL again.

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc (right):

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:321: const int
num_10ms_frames = codec.inst.pacsize / (10 * samples_per_ms);
On 2016/04/05 13:00:22, minyue-webrtc wrote:
> no CheckedDivExact on this?

Done.

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:325: uint32_t
expected_output_ts = last_packet_send_timestamp_ - 30u;
On 2016/04/05 13:00:22, minyue-webrtc wrote:
> what would it be if |last_packet_send_timestamp_| is smaller than 30u?

In fact it is; last_packet_send_timestamp_ = 0 from the test setup. The
subtraction wraps to the high end of uint32_t as expected.

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:338:
EXPECT_EQ(AudioFrame::kVadActive, frame.vad_activity_);
On 2016/04/05 13:00:22, minyue-webrtc wrote:
> why is VadActive when packet is silent?

There is a restriction on the post-decode VAD in NetEq that makes it always say
"active speech" for sample rates > 16000 Hz. Legacy...

[minyue-webrtc, 2016-04-05 13:30:04]

LGTM + one request

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
File webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc (right):

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:325: uint32_t
expected_output_ts = last_packet_send_timestamp_ - 30u;
On 2016/04/05 13:24:14, hlundin-webrtc wrote:
> On 2016/04/05 13:00:22, minyue-webrtc wrote:
> > what would it be if |last_packet_send_timestamp_| is smaller than 30u?
> 
> In fact it is; last_packet_send_timestamp_ = 0 from the test setup. The
> subtraction wraps to the high end of uint32_t as expected.

Acknowledged.

https://codereview.webrtc.org/1859953002/diff/1/webrtc/modules/audio_coding/a...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:338:
EXPECT_EQ(AudioFrame::kVadActive, frame.vad_activity_);
On 2016/04/05 13:24:14, hlundin-webrtc wrote:
> On 2016/04/05 13:00:22, minyue-webrtc wrote:
> > why is VadActive when packet is silent?
> 
> There is a restriction on the post-decode VAD in NetEq that makes it always
say
> "active speech" for sample rates > 16000 Hz. Legacy...

so if codec_id equals to a 16kHz codec, this line will fail?

Would you add a comment? or even drop this line.

[hlundin-webrtc, 2016-04-05 13:51:46]

Generalizing with respect to the codec

[hlundin-webrtc, 2016-04-05 14:04:00]

Generalizing and adding tests for different codecs

[minyue-webrtc, 2016-04-05 14:08:59]

still LGTM

https://codereview.webrtc.org/1859953002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc (right):

https://codereview.webrtc.org/1859953002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:332:
last_packet_send_timestamp_ -
better moving this line up

[hlundin-webrtc, 2016-04-05 14:23:21]

Fixing a formatting nit

[hlundin-webrtc, 2016-04-05 14:24:15]

Thanks. This CL will be committed pending try jobs.

https://codereview.webrtc.org/1859953002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc (right):

https://codereview.webrtc.org/1859953002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/acm2/acm_receiver_unittest_oldapi.cc:332:
last_packet_send_timestamp_ -
On 2016/04/05 14:08:59, minyue-webrtc wrote:
> better moving this line up

Done.

[hlundin-webrtc, 2016-04-06 07:01:14]

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

[hlundin-webrtc, 2016-04-06 07:01:14]

The patchset sent to the CQ was uploaded after l-g-t-m from minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1859953002/#ps80001 (title:
"Fixing a formatting nit")

[commit-bot: I haz the power, 2016-04-06 07:01:24]

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

[commit-bot: I haz the power, 2016-04-06 08:03:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-06 08:03:10]

Description was changed from

==========
Unit test for AudioFrame output from AcmReceiver::GetAudio

This new unit test verifies the parameter fields (not the audio data
itself) written to the AudioFrame output by AcmReceiver::GetAudio.

Also corrected a few comments reflecting recent changes in the code.

BUG=webrtc:5669
==========

to

==========
Unit test for AudioFrame output from AcmReceiver::GetAudio

This new unit test verifies the parameter fields (not the audio data
itself) written to the AudioFrame output by AcmReceiver::GetAudio.

Also corrected a few comments reflecting recent changes in the code.

BUG=webrtc:5669

Committed: https://crrev.com/7dc68897d3614be0dce3ad565e4ceb14fc8f53d3
Cr-Commit-Position: refs/heads/master@{#12253}
==========

[commit-bot: I haz the power, 2016-04-06 08:03:13]

Patchset 5 (id:??) landed as
https://crrev.com/7dc68897d3614be0dce3ad565e4ceb14fc8f53d3
Cr-Commit-Position: refs/heads/master@{#12253}