Added a bitexactness test for the level estimator in the audio processing module

Added a bitexactness test for the level estimator in the audio
processing module.

BUG=webrtc:5338
Committed: https://crrev.com/19b7b665cc69ae5027826ef45f98cb72ed95605d
Cr-Commit-Position: refs/heads/master@{#12064}

[peah-webrtc, 2016-03-16 07:21:16]

Description was changed from

==========
Added a bitexactness test for the level estimator in the audio
processing module.


BUG=webrtc:5338
==========

to

==========
Added a bitexactness test for the level estimator in the audio
processing module.


BUG=webrtc:5338
==========

[peah-webrtc, 2016-03-16 07:21:27]

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

[minyue-webrtc, 2016-03-16 09:55:01]

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

[minyue-webrtc, 2016-03-16 09:55:01]

small drive-by comment

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc
(right):

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:79:
void RunBitexactnessTest(int sample_rate_hz,
Is |rms_reference| derivable from |signal_level|, or a predefined std::map?

[hlundin-webrtc, 2016-03-16 12:57:05]

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc
(right):

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:2: * 
Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
2016

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:30: if
(sample_rate_hz > AudioProcessing::kSampleRate16kHz) {
What is the purpose of splitting and merging bands without anything in between?
I'm sure these is some purpose, but please document it.

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:42:
void ConstructTestVector(int samples_per_channel,
This seems very similar to the function in the previous CL. Can you consolidate
them? If not, please, consider my comments in that CL and apply them here too.

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:108:
EXPECT_PRED_FORMAT2(test::AssertIntegersNotEqual, rms, rms_reference);
EXPECT_NEAR

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:114:
const int kRmsReference = 65;
You are using three different values for kRmsReference. Add them as distinct
constants in the unnamed namespace, (kLowRmsReference, ...), and don't define
them in each TEST.

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/bitexactness_tools.cc (right):

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/bitexactness_tools.cc:20:
::testing::AssertionResult AssertIntegersNotEqual(const char* m_expr,
Consider using EXPECT_NEAR instead of this custom predicate.

[peah-webrtc, 2016-03-17 14:18:06]

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc
(right):

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:2: * 
Copyright (c) 2015 The WebRTC project authors. All Rights Reserved.
On 2016/03/16 12:57:05, hlundin-webrtc wrote:
> 2016

Done.

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:30: if
(sample_rate_hz > AudioProcessing::kSampleRate16kHz) {
On 2016/03/16 12:57:05, hlundin-webrtc wrote:
> What is the purpose of splitting and merging bands without anything in
between?
> I'm sure these is some purpose, but please document it.
None, removed that code.
Done.

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:42:
void ConstructTestVector(int samples_per_channel,
On 2016/03/16 12:57:05, hlundin-webrtc wrote:
> This seems very similar to the function in the previous CL. Can you
consolidate
> them? If not, please, consider my comments in that CL and apply them here too.

Done.

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:79:
void RunBitexactnessTest(int sample_rate_hz,
On 2016/03/16 09:55:01, minyue-webrtc wrote:
> Is |rms_reference| derivable from |signal_level|, or a predefined std::map?

No, signal_level was just a scaling of the signal to try to test the
level-dependency of the level estimator. I removed that though since it mainly
complicates the test.

Done.

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:108:
EXPECT_PRED_FORMAT2(test::AssertIntegersNotEqual, rms, rms_reference);
On 2016/03/16 12:57:05, hlundin-webrtc wrote:
> EXPECT_NEAR
Changed to EXPECT_EQ instead.
Done.

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:114:
const int kRmsReference = 65;
On 2016/03/16 12:57:05, hlundin-webrtc wrote:
> You are using three different values for kRmsReference. Add them as distinct
> constants in the unnamed namespace, (kLowRmsReference, ...), and don't define
> them in each TEST.

I'm not too happy about that. It may be that they will differ for the different
tests. I'd rather have them gathered inside the actual test. I have rewrite it a
bit now though. Please let me know what you think!

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/test/bitexactness_tools.cc (right):

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/test/bitexactness_tools.cc:20:
::testing::AssertionResult AssertIntegersNotEqual(const char* m_expr,
On 2016/03/16 12:57:05, hlundin-webrtc wrote:
> Consider using EXPECT_NEAR instead of this custom predicate.

Done.

[hlundin-webrtc, 2016-03-17 15:20:58]

LGTM with a nit, and a lingering feeling that you've messed up your CL
dependencies...

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc
(right):

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:114:
const int kRmsReference = 65;
On 2016/03/17 14:18:06, peah-webrtc wrote:
> On 2016/03/16 12:57:05, hlundin-webrtc wrote:
> > You are using three different values for kRmsReference. Add them as distinct
> > constants in the unnamed namespace, (kLowRmsReference, ...), and don't
define
> > them in each TEST.
> 
> I'm not too happy about that. It may be that they will differ for the
different
> tests. I'd rather have them gathered inside the actual test. I have rewrite it
a
> bit now though. Please let me know what you think!

Fair enough, now that you have fewer test cases.

https://codereview.webrtc.org/1811443002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc
(right):

https://codereview.webrtc.org/1811443002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:55: int
rms = level_estimator.RMS();
Skip this line and simply do a one-liner:
EXPECT_EQ(rms_reference, level_estimator.RMS());

https://codereview.webrtc.org/1811443002/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/test/bitexactness_tools.h (right):

https://codereview.webrtc.org/1811443002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/bitexactness_tools.h:1: 
Nope.

https://codereview.webrtc.org/1811443002/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/test/bitexactness_tools.h:15: #include <string>
Aren't all these changes done in the previous CL? I wonder if you've messed up
your CL chain somehow.

[minyue-webrtc, 2016-03-18 08:33:50]

lgtm

[peah-webrtc, 2016-03-20 12:56:01]

The CQ bit was checked by peah@webrtc.org

[commit-bot: I haz the power, 2016-03-20 12:56:11]

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

[commit-bot: I haz the power, 2016-03-20 15:36:31]

Description was changed from

==========
Added a bitexactness test for the level estimator in the audio
processing module.


BUG=webrtc:5338
==========

to

==========
Added a bitexactness test for the level estimator in the audio
processing module.


BUG=webrtc:5338
==========

[commit-bot: I haz the power, 2016-03-20 15:36:32]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-20 15:36:41]

Description was changed from

==========
Added a bitexactness test for the level estimator in the audio
processing module.


BUG=webrtc:5338
==========

to

==========
Added a bitexactness test for the level estimator in the audio
processing module.

BUG=webrtc:5338

Committed: https://crrev.com/19b7b665cc69ae5027826ef45f98cb72ed95605d
Cr-Commit-Position: refs/heads/master@{#12064}
==========

[commit-bot: I haz the power, 2016-03-20 15:36:42]

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

[peah-webrtc, 2016-03-20 15:40:55]

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc
(right):

https://codereview.webrtc.org/1811443002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/level_estimator_bitexactness_unittest.cc:114:
const int kRmsReference = 65;
On 2016/03/17 15:20:57, hlundin-webrtc wrote:
> On 2016/03/17 14:18:06, peah-webrtc wrote:
> > On 2016/03/16 12:57:05, hlundin-webrtc wrote:
> > > You are using three different values for kRmsReference. Add them as
distinct
> > > constants in the unnamed namespace, (kLowRmsReference, ...), and don't
> define
> > > them in each TEST.
> > 
> > I'm not too happy about that. It may be that they will differ for the
> different
> > tests. I'd rather have them gathered inside the actual test. I have rewrite
it
> a
> > bit now though. Please let me know what you think!
> 
> Fair enough, now that you have fewer test cases.

Acknowledged.