Making NetEq bitexactness test independent on reference files.

Making NetEq bitexactness test independent on reference files.

NetEq bitexactness test depended on reference files which differs from platform to platform. This makes it very hard to update Neteq.

New method maintains the ability to save output into files. But it verifies the checksum only. With this, when bitexactness test fails, we can still check closely to the output file if need, but the test becomes much easier to modify.

BUG=

Committed: https://crrev.com/4f90677527d8577d310ef718d3bba089bc7b36b9
Cr-Commit-Position: refs/heads/master@{#12567}

[minyue-webrtc, 2016-04-28 13:34:25]

Patchset #1 (id:1) has been deleted

[minyue-webrtc, 2016-04-28 15:11:34]

Description was changed from

==========
making bitexactness test independent on reference files

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

to

==========
Making NetEq bitexactness test independent on reference files.

NetEq bitexactness test depended on reference files which differs from platform
to platform. This makes it very hard to update Neteq. 

New method maintains the ability to save output into files. But it verifies the
checksum only. With this, when bitexactness test fails, we can still check
closely to the output file if need, but the test becomes much easier to modify.

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

[minyue-webrtc, 2016-04-28 15:11:35]

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

[minyue-webrtc, 2016-04-28 15:21:36]

Hi Henrik,

I updated the NetEq bit exactness tests in a way that it checks only the SHA1.

To make the review easier (perhaps), I made it into three steps.

patch set 1. Let the test check xxx.xxx.sha1, so as to make sure that we are
compatible legacy.

patch set 2. Copy the  xxx.xxx.sha1's content to the test and deprecate sha1
files.

patch set 3. Refactor class and methods that do not make sense any longer, and
more cleaning.

~~~~
Notes:

patch set 1: There are a couple of Android bot failing. I think it is due to
Android's poor support on ifstream. It seems that the .sha1 file was not read
properly. I do not think it is necessary to fix it. Since in patch set 2, we get
rid of ifstream anyway.

Thanks for reviewing!

[hlundin-webrtc, 2016-04-29 12:26:29]

Very nice. Please, see my comments.

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

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:51: if
(!checksum_android.empty())
I think I would rather have the strings always populated with something, that
is, skip the check for empty.

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:119: if (size <= 0)
Can you omit this early return? I mean, the length() of a string should never be
< 0, and the case == 0 should work in the code below, too, right?

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:153: : output_fp_(NULL),
nullptr

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:423:
ref_files.VerifyChecksum(output_checksum);
I think you should add SCOPED_TRACE("ref_files") or simply SCOPED_TRACE("")
before each call to VerifyChecksum. This will give you a context on which call
that failed. See
https://codereview.webrtc.org/1861303002/diff/60001/webrtc/modules/audio_codi....

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:468: "",
Follow my suggestion in PlatformChecksum, and explicitly reiterate the checksums
that are identical across platforms.

[minyue-webrtc, 2016-04-29 13:11:27]

thanks for comments! now addressed.

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

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:51: if
(!checksum_android.empty())
On 2016/04/29 12:26:28, hlundin-webrtc wrote:
> I think I would rather have the strings always populated with something, that
> is, skip the check for empty.

Sounds good to me. I was thinking that 1) the reference files were designed to
always be able to fall back to a default, we can do the same. 2) we can tell
that some systems do not differ from the default, but of course, reading the
checksum does the same.

So I will go for your suggestion.

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:112: void WriteMessage(FILE*
file, rtc::MessageDigest* digest,
I also like to rename this guy

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:119: if (size <= 0)
On 2016/04/29 12:26:28, hlundin-webrtc wrote:
> Can you omit this early return? I mean, the length() of a string should never
be
> < 0, and the case == 0 should work in the code below, too, right?

Yes. When I read this part, I did not like it myself, either. Good suggestion.

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:153: : output_fp_(NULL),
On 2016/04/29 12:26:28, hlundin-webrtc wrote:
> nullptr

Done.

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:386: ResultSink
ref_files(ref_out_file);
I'd rename ref_files as well.

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:423:
ref_files.VerifyChecksum(output_checksum);
On 2016/04/29 12:26:28, hlundin-webrtc wrote:
> I think you should add SCOPED_TRACE("ref_files") or simply SCOPED_TRACE("")
> before each call to VerifyChecksum. This will give you a context on which call
> that failed. See
>
https://codereview.webrtc.org/1861303002/diff/60001/webrtc/modules/audio_codi....

Done.

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:468: "",
On 2016/04/29 12:26:28, hlundin-webrtc wrote:
> Follow my suggestion in PlatformChecksum, and explicitly reiterate the
checksums
> that are identical across platforms.

Done.

[hlundin-webrtc, 2016-04-29 13:19:10]

Awesome! LGTM

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

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:112: void WriteMessage(FILE*
file, rtc::MessageDigest* digest,
On 2016/04/29 13:11:27, minyue-webrtc wrote:
> I also like to rename this guy

Acknowledged.

https://codereview.webrtc.org/1928923002/diff/50001/webrtc/modules/audio_codi...
webrtc/modules/audio_coding/neteq/neteq_unittest.cc:386: ResultSink
ref_files(ref_out_file);
On 2016/04/29 13:11:27, minyue-webrtc wrote:
> I'd rename ref_files as well.

Acknowledged.

[minyue-webrtc, 2016-04-29 13:38:39]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2016-04-29 13:38:46]

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

[commit-bot: I haz the power, 2016-04-29 13:41:56]

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

[commit-bot: I haz the power, 2016-04-29 13:41:57]

Try jobs failed on following builders:
  win_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_rel/builds/14651)
  win_x64_rel on tryserver.webrtc (JOB_FAILED,
http://build.chromium.org/p/tryserver.webrtc/builders/win_x64_rel/builds/14325)

[minyue-webrtc, 2016-04-29 14:07:44]

The CQ bit was checked by minyue@webrtc.org

[minyue-webrtc, 2016-04-29 14:07:45]

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/1928923002/#ps90001 (title:
"fixing an compiling error")

[commit-bot: I haz the power, 2016-04-29 14:07:48]

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

[commit-bot: I haz the power, 2016-04-29 16:08:23]

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

[commit-bot: I haz the power, 2016-04-29 16:08:24]

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

[minyue-webrtc, 2016-04-29 17:23:48]

The CQ bit was checked by minyue@webrtc.org

[commit-bot: I haz the power, 2016-04-29 17:23:58]

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

[commit-bot: I haz the power, 2016-04-29 18:05:17]

Description was changed from

==========
Making NetEq bitexactness test independent on reference files.

NetEq bitexactness test depended on reference files which differs from platform
to platform. This makes it very hard to update Neteq. 

New method maintains the ability to save output into files. But it verifies the
checksum only. With this, when bitexactness test fails, we can still check
closely to the output file if need, but the test becomes much easier to modify.

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

to

==========
Making NetEq bitexactness test independent on reference files.

NetEq bitexactness test depended on reference files which differs from platform
to platform. This makes it very hard to update Neteq. 

New method maintains the ability to save output into files. But it verifies the
checksum only. With this, when bitexactness test fails, we can still check
closely to the output file if need, but the test becomes much easier to modify.

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

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

Committed patchset #5 (id:90001)

[commit-bot: I haz the power, 2016-05-01 22:02:04]

Description was changed from

==========
Making NetEq bitexactness test independent on reference files.

NetEq bitexactness test depended on reference files which differs from platform
to platform. This makes it very hard to update Neteq. 

New method maintains the ability to save output into files. But it verifies the
checksum only. With this, when bitexactness test fails, we can still check
closely to the output file if need, but the test becomes much easier to modify.

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

to

==========
Making NetEq bitexactness test independent on reference files.

NetEq bitexactness test depended on reference files which differs from platform
to platform. This makes it very hard to update Neteq.

New method maintains the ability to save output into files. But it verifies the
checksum only. With this, when bitexactness test fails, we can still check
closely to the output file if need, but the test becomes much easier to modify.

BUG=

Committed: https://crrev.com/4f90677527d8577d310ef718d3bba089bc7b36b9
Cr-Commit-Position: refs/heads/master@{#12567}
==========

[commit-bot: I haz the power, 2016-05-01 22:02:06]

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