NetEq: New test for muted state during CNG

NetEq: New test for muted state during CNG

Verifies that NetEq doesn't enter muted state when CNG mode is active
and the packet stream is suspended for a long time.

BUG=webrtc:5608
Committed: https://crrev.com/42feb51f1579dc8ea9dae6ab78404b16f805a5cd
Cr-Commit-Position: refs/heads/master@{#14308}

[hlundin-webrtc, 2016-09-16 13:07:48]

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

[hlundin-webrtc, 2016-09-16 13:07:49]

Alex,
Please, take a look at this CL.

Thanks!

[aleloi, 2016-09-19 11:55:07]

Have some comments! And sorry for the delay :)

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

https://codereview.webrtc.org/2335343011/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1450: for (int i = 0; i <
1000; ++i) {
Does it take much time to do 1000 iterations? I.e., does NetEq wait for 10ms
before GetAudio returns or does it happen faster? 10 seconds seems a very long
time to wait for a test.

https://codereview.webrtc.org/2335343011/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1454: ASSERT_FALSE(muted);
I recently read go/unit-test-practices and I think one of the tips for more
resilient tests applies here (link below).

It seems to me that this test checks for multiple behaviors at the same time. It
checks that 
 (A) NetEq outputs un-muted frames with speech type CNG after being fed a CNG
packet and 
 (B) the speech type goes back to un-muted normal speech.

I think that the test could check for these two behaviors separately and e.g.
just have GetAudio with no checks in the loop.

Then there would be less repeated assertions and there is a better separation
between set-up and checks.

Link:
https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b...

[hlundin-webrtc, 2016-09-20 08:09:02]

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

https://codereview.webrtc.org/2335343011/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1450: for (int i = 0; i <
1000; ++i) {
On 2016/09/19 11:55:07, aleloi wrote:
> Does it take much time to do 1000 iterations? I.e., does NetEq wait for 10ms
> before GetAudio returns or does it happen faster? 10 seconds seems a very long
> time to wait for a test.

The test completes in 22 ms on my machine. It runs at "full speed", and does not
have to wait 10 ms between each lap. I'll modify the comment to clarify.

https://codereview.webrtc.org/2335343011/diff/1/webrtc/modules/audio_coding/n...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:1454: ASSERT_FALSE(muted);
On 2016/09/19 11:55:07, aleloi wrote:
> I recently read go/unit-test-practices and I think one of the tips for more
> resilient tests applies here (link below).
> 
> It seems to me that this test checks for multiple behaviors at the same time.
It
> checks that 
>  (A) NetEq outputs un-muted frames with speech type CNG after being fed a CNG
> packet and 
>  (B) the speech type goes back to un-muted normal speech.
> 
> I think that the test could check for these two behaviors separately and e.g.
> just have GetAudio with no checks in the loop.
> 
> Then there would be less repeated assertions and there is a better separation
> between set-up and checks.
> 
> Link:
>
https://engdoc.corp.google.com/eng/doc/devguide/coding/testing/unit-testing-b...

How about this?

[aleloi, 2016-09-20 08:16:30]

LGTM!

[hlundin-webrtc, 2016-09-20 09:42:37]

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

[commit-bot: I haz the power, 2016-09-20 09:42:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-20 11:43:22]

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

[commit-bot: I haz the power, 2016-09-20 11:43:23]

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

[hlundin-webrtc, 2016-09-20 13:21:37]

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

[commit-bot: I haz the power, 2016-09-20 13:21:40]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...

[commit-bot: I haz the power, 2016-09-20 13:51:44]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-20 13:51:53]

Description was changed from

==========
NetEq: New test for muted state during CNG

Verifies that NetEq doesn't enter muted state when CNG mode is active
and the packet stream is suspended for a long time.

BUG=webrtc:5608
==========

to

==========
NetEq: New test for muted state during CNG

Verifies that NetEq doesn't enter muted state when CNG mode is active
and the packet stream is suspended for a long time.

BUG=webrtc:5608

Committed: https://crrev.com/42feb51f1579dc8ea9dae6ab78404b16f805a5cd
Cr-Commit-Position: refs/heads/master@{#14308}
==========

[commit-bot: I haz the power, 2016-09-20 13:51:54]

Patchset 2 (id:??) landed as
https://crrev.com/42feb51f1579dc8ea9dae6ab78404b16f805a5cd
Cr-Commit-Position: refs/heads/master@{#14308}