Reenables several NetEq unittests on android.

Reenables several NetEq unittests on android.

Several unittests were disabled on android, this CL will reenable them. One of
the tests was accidentally disabled on all platforms, and now no longer gives a
bitexact result.

BUG=webrtc:3343, webrtc:5349
Committed: https://crrev.com/72c08edcedd67355b6d454dddad4580348205ee0
Cr-Commit-Position: refs/heads/master@{#11323}

[ivoc, 2015-12-17 13:22:00]

Description was changed from

==========
Reenables several NetEq unittests on android.

Several unittests were disabled on android, this CL will reenable them. One of
the tests was accidentally disabled on all platforms, and now no longer gives a
bitexact result.

BUG=webrtc:3343,5349
==========

to

==========
Reenables several NetEq unittests on android.

Several unittests were disabled on android, this CL will reenable them. One of
the tests was accidentally disabled on all platforms, and now no longer gives a
bitexact result.

BUG=webrtc:3343,webrtc:5349
==========

[ivoc, 2015-12-17 13:22:26]

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

[ivoc, 2015-12-17 13:28:25]

ivoc@webrtc.org changed reviewers:
+ phoglund@webrtc.org

[ivoc, 2015-12-17 13:32:40]

Hi guys, please have a look at this delayed fixit-week CL.

[ivoc, 2015-12-17 13:34:14]

Patchset #1 (id:1) has been deleted

[phoglund, 2015-12-17 13:40:41]

lgtm

[hlundin-webrtc, 2015-12-17 14:05:12]

lgtm % a comment nit (and of course that you make the bots green).

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

https://codereview.webrtc.org/1532903002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:412: // Payload type 104 is
not supported on android.
104 is iSAC-swb (for this particular input file); please, mention that too.

[minyue-webrtc, 2015-12-17 14:32:52]

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

[minyue-webrtc, 2015-12-17 14:32:58]

Nice! see my few comments in line.

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

https://codereview.webrtc.org/1532903002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:412: // Payload type 104 is
not supported on android.
I prefer WEBRTC_CODEC_ISAC to WEBRTC_ANDROID, because line 379 reads that way.

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_stereo_unittest.cc (right):

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_stereo_unittest.cc:367: 200)
should it be as large as 200?

And if only Android needs this margin, I prefer

#ifdef WEBRTC_ANDROID
const int MARGIN = 200;
#else
const int MARGIN = 0;
#endif

...
EXPECT_NEAR(output_[i], output_multi_channel_[i * num_channels_ + j], MARGIN);

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:909:
TEST_F(NetEqDecodingTest, IF_ISAC(DecoderError)) {
may you change this to MAYBE_?

#if defined(WEBRTC_CODEC_ISAC) || defined(WEBRTC_CODEC_ISACFX)
#define MAYBE_DecoderError DecoderError
#else
#define MAYBE_DecoderError DISABLED_DecoderError
#endif

TEST_F(NetEqDecodingTest, DISABLED_ON_ANDROID(IF_ISAC(DecoderError))) {

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:931: EXPECT_EQ(6730,
neteq_->LastDecoderError());
would you use enum (see below) or const for these two values

enum {
ISAC_LENGTH_MISMATCH = 6730,
ISAC_RANGE_ERROR_DECODE_FRAME_LENGTH = 6640
}

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:932: #else
maybe use
#elif defined(WEBRTC_CODEC_ISACFX)

[ivoc, 2015-12-17 15:05:26]

Patchset #3 (id:50001) has been deleted

[ivoc, 2015-12-17 15:06:28]

Thanks for the quick reviews!

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

https://codereview.webrtc.org/1532903002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:412: // Payload type 104 is
not supported on android.
On 2015/12/17 14:05:12, hlundin-webrtc wrote:
> 104 is iSAC-swb (for this particular input file); please, mention that too.

Done.

https://codereview.webrtc.org/1532903002/diff/20001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:412: // Payload type 104 is
not supported on android.
On 2015/12/17 14:32:58, minyue-webrtc wrote:
> I prefer WEBRTC_CODEC_ISAC to WEBRTC_ANDROID, because line 379 reads that way.

Done.

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_stereo_unittest.cc (right):

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_stereo_unittest.cc:367: 200)
On 2015/12/17 14:32:58, minyue-webrtc wrote:
> should it be as large as 200?
> 
> And if only Android needs this margin, I prefer
> 
> #ifdef WEBRTC_ANDROID
> const int MARGIN = 200;
> #else
> const int MARGIN = 0;
> #endif
> 
> ...
> EXPECT_NEAR(output_[i], output_multi_channel_[i * num_channels_ + j], MARGIN);

This test was previously accidentally disabled on all platforms for the past 2+
years, so this is needed on all platforms for now. There is a bug to track this
issue, see: https://bugs.chromium.org/p/webrtc/issues/detail?id=5349 . I will
change the constant to a const int like you suggested.

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_unittest.cc (right):

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:909:
TEST_F(NetEqDecodingTest, IF_ISAC(DecoderError)) {
On 2015/12/17 14:32:58, minyue-webrtc wrote:
> may you change this to MAYBE_?
> 
> #if defined(WEBRTC_CODEC_ISAC) || defined(WEBRTC_CODEC_ISACFX)
> #define MAYBE_DecoderError DecoderError
> #else
> #define MAYBE_DecoderError DISABLED_DecoderError
> #endif
> 
> TEST_F(NetEqDecodingTest, DISABLED_ON_ANDROID(IF_ISAC(DecoderError))) {

Good idea, done.

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:931: EXPECT_EQ(6730,
neteq_->LastDecoderError());
On 2015/12/17 14:32:58, minyue-webrtc wrote:
> would you use enum (see below) or const for these two values
> 
> enum {
> ISAC_LENGTH_MISMATCH = 6730,
> ISAC_RANGE_ERROR_DECODE_FRAME_LENGTH = 6640
> }

Thanks for the suggestion, looks more clear.

https://codereview.webrtc.org/1532903002/diff/40001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:932: #else
On 2015/12/17 14:32:58, minyue-webrtc wrote:
> maybe use
> #elif defined(WEBRTC_CODEC_ISACFX)

Done.

[minyue-webrtc, 2015-12-18 15:32:30]

LGTM + nit

https://codereview.webrtc.org/1532903002/diff/70001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_stereo_unittest.cc (right):

https://codereview.webrtc.org/1532903002/diff/70001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_stereo_unittest.cc:366: const int
error_margin = 200;
Nit: compiler time constant can name as kErrorMagin

[ivoc, 2016-01-18 15:22:09]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-01-18 15:22:09]

The patchset sent to the CQ was uploaded after l-g-t-m from phoglund@webrtc.org,
henrik.lundin@webrtc.org, minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1532903002/#ps110001 (title:
"Rebase and small fixes.")

[commit-bot: I haz the power, 2016-01-18 15:22:13]

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

[ivoc, 2016-01-18 15:33:24]

ivoc@webrtc.org changed reviewers:
+ kjellander@webrtc.org

[ivoc, 2016-01-18 15:36:46]

Hi kjellander, could you have a look at the .isolate file please? I forgot to
add an owner for that file.

Thanks!

https://codereview.webrtc.org/1532903002/diff/70001/webrtc/modules/audio_codi...
File webrtc/modules/audio_coding/neteq/neteq_stereo_unittest.cc (right):

https://codereview.webrtc.org/1532903002/diff/70001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_stereo_unittest.cc:366: const int
error_margin = 200;
On 2015/12/18 15:32:29, minyue-webrtc wrote:
> Nit: compiler time constant can name as kErrorMagin

Done.

[kjellander_webrtc, 2016-01-18 15:38:21]

lgtm

[commit-bot: I haz the power, 2016-01-18 16:29:12]

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

[commit-bot: I haz the power, 2016-01-18 16:29:13]

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

[ivoc, 2016-01-20 13:16:15]

The CQ bit was checked by ivoc@webrtc.org

[ivoc, 2016-01-20 13:16:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, phoglund@webrtc.org, kjellander@webrtc.org,
minyue@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1532903002/#ps130001 (title:
"Small change to disable bitexactness test on arm64.")

[commit-bot: I haz the power, 2016-01-20 13:16:23]

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

[commit-bot: I haz the power, 2016-01-20 15:26:28]

Description was changed from

==========
Reenables several NetEq unittests on android.

Several unittests were disabled on android, this CL will reenable them. One of
the tests was accidentally disabled on all platforms, and now no longer gives a
bitexact result.

BUG=webrtc:3343,webrtc:5349
==========

to

==========
Reenables several NetEq unittests on android.

Several unittests were disabled on android, this CL will reenable them. One of
the tests was accidentally disabled on all platforms, and now no longer gives a
bitexact result.

BUG=webrtc:3343,webrtc:5349
==========

[commit-bot: I haz the power, 2016-01-20 15:26:29]

Committed patchset #6 (id:130001)

[commit-bot: I haz the power, 2016-01-20 15:26:33]

Description was changed from

==========
Reenables several NetEq unittests on android.

Several unittests were disabled on android, this CL will reenable them. One of
the tests was accidentally disabled on all platforms, and now no longer gives a
bitexact result.

BUG=webrtc:3343,webrtc:5349
==========

to

==========
Reenables several NetEq unittests on android.

Several unittests were disabled on android, this CL will reenable them. One of
the tests was accidentally disabled on all platforms, and now no longer gives a
bitexact result.

BUG=webrtc:3343,webrtc:5349

Committed: https://crrev.com/72c08edcedd67355b6d454dddad4580348205ee0
Cr-Commit-Position: refs/heads/master@{#11323}
==========

[commit-bot: I haz the power, 2016-01-20 15:26:34]

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