Refactor RMSLevel and give it new functionality

Refactor RMSLevel and give it new functionality

This change rewrites RMSLevel, making it accept an ArrayView as input,
and modify the implementation somewhat. It also makes the class keep
track of the peak RMS in addition to the average RMS over the
measurement period.

New tests are added to cover the new functionality.

BUG=webrtc:6622
Committed: https://crrev.com/5049942219776f857b20991ad61a55f06aebae79
Cr-Commit-Position: refs/heads/master@{#15294}

[hlundin-webrtc, 2016-11-25 13:58:44]

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

[hlundin-webrtc, 2016-11-25 13:58:44]

Per, PTAL

[peah-webrtc, 2016-11-29 08:57:13]

Nice!

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/rms_level.cc (right):

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:24: static constexpr float
kMinLevel = 1.995262314968883e-13f;
Can this be computed from kMinLevelDb using a constexpr?

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:28: : sum_square_(0),
0.f

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:30: max_mean_square_(0) {}
0.f

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:30: max_mean_square_(0) {}
Would it make sense to do a call to Reset() inside the constructor instead of
the initializer lists?

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:35: sum_square_ = 0;
0.f;

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:37: max_mean_square_ = 0;
0.f;

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:52: max_mean_square_ =
std::max(max_mean_square_, sum_square / data.size());
Why not do the division only at the call of AverageAndPeak. That is cheaper,
harmonizes with how sum_square is computed, and does not require the comment on
line 90.

In order to really save computations, you could then compute the division once
by precomputing 1/sample_count_ and apply it both to sum_square_ and
max_sum_square_.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:59: namespace {
Is there a special reason for having two anonymous code sections? Would it make
sense to combine them?

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:65: int CalcRms(float mean_square)
{
I would suggest avoiding the abbreviation Calc since this method name is already
so short, it already contains an abbreviation, and since the style guide
suggests that
(https://google.github.io/styleguide/cppguide.html#General_Naming_Rules).

What about CalculateRms or ComputeRms?

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:74: const float rms = 10 *
log10(mean_square_norm);
10.f

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:75: RTC_DCHECK_LE(rms, 0);
0.f

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:78: return static_cast<int>(-rms +
0.5);
0.5f

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/rms_level.h (right):

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.h:42: void
Process(rtc::ArrayView<const int16_t> data);
While you are anyway changing this: Would it make sense to change Process to
Analyze?

Elsewhere inside APM the convention seems to be that if a method changes the
signal, it is called Process, otherwise it is called Analyze.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.h:52: int RMS();
Suggestion: I think it would make sense to rename this method while you are at
it.

The class is called RMSLevel and having a method called RMS is then kind of
double bookeeping.

For instance, I envision a typical usage being as
{
RMSLevel rms_level_;

...
rms_value = rms_level_.RMS();
}

To me it looks better if it instead would be
{
RMSLevel rms_level_;

...
rms_value = rms_level_.Average();
}

or 

{
RMSLevel rms_level_;

...
rms_value = rms_level_.Value();
}


This is even more the case when you add the AverageAndPeak() method.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2770: rtc::ArrayView<const
int16_t>(_audioFrame.data_, length));
You should probably add the include array_view.h. Now it is somehow implictly
included via another include as it is neither in channel.cc or channel.h.

[hlundin-webrtc, 2016-11-29 09:43:18]

The easy fixes

[hlundin-webrtc, 2016-11-29 10:24:55]

PTAL again. I uploaded a series of patch sets, to make reviewing easier, I
hope...

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/rms_level.cc (right):

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:24: static constexpr float
kMinLevel = 1.995262314968883e-13f;
On 2016/11/29 08:57:13, peah-webrtc wrote:
> Can this be computed from kMinLevelDb using a constexpr?

Already tried, but failed. Cannot use static_assert either, since neither
std::pow, nor std::log10 can be used in a constexpr.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:30: max_mean_square_(0) {}
On 2016/11/29 08:57:13, peah-webrtc wrote:
> Would it make sense to do a call to Reset() inside the constructor instead of
> the initializer lists?

Done.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:35: sum_square_ = 0;
On 2016/11/29 08:57:13, peah-webrtc wrote:
> 0.f;

Done.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:37: max_mean_square_ = 0;
On 2016/11/29 08:57:13, peah-webrtc wrote:
> 0.f;

Done.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:52: max_mean_square_ =
std::max(max_mean_square_, sum_square / data.size());
On 2016/11/29 08:57:13, peah-webrtc wrote:
> Why not do the division only at the call of AverageAndPeak. That is cheaper,
> harmonizes with how sum_square is computed, and does not require the comment
on
> line 90.
> 
> In order to really save computations, you could then compute the division once
> by precomputing 1/sample_count_ and apply it both to sum_square_ and
> max_sum_square_.

Changed the implementation to cope with variable block sizes (by resetting).

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:59: namespace {
On 2016/11/29 08:57:13, peah-webrtc wrote:
> Is there a special reason for having two anonymous code sections? Would it
make
> sense to combine them?

I thought it made sense to have the code close to where it was called from. Now
I consolidated the namespaces.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:65: int CalcRms(float mean_square)
{
On 2016/11/29 08:57:12, peah-webrtc wrote:
> I would suggest avoiding the abbreviation Calc since this method name is
already
> so short, it already contains an abbreviation, and since the style guide
> suggests that
> (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules).
> 
> What about CalculateRms or ComputeRms?

Done.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:74: const float rms = 10 *
log10(mean_square_norm);
On 2016/11/29 08:57:12, peah-webrtc wrote:
> 10.f

Done.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:75: RTC_DCHECK_LE(rms, 0);
On 2016/11/29 08:57:12, peah-webrtc wrote:
> 0.f

Done.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.cc:78: return static_cast<int>(-rms +
0.5);
On 2016/11/29 08:57:13, peah-webrtc wrote:
> 0.5f

Done.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/rms_level.h (right):

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.h:42: void
Process(rtc::ArrayView<const int16_t> data);
On 2016/11/29 08:57:13, peah-webrtc wrote:
> While you are anyway changing this: Would it make sense to change Process to
> Analyze?
> 
> Elsewhere inside APM the convention seems to be that if a method changes the
> signal, it is called Process, otherwise it is called Analyze.

Done.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/rms_level.h:52: int RMS();
On 2016/11/29 08:57:13, peah-webrtc wrote:
> Suggestion: I think it would make sense to rename this method while you are at
> it.
> 
> The class is called RMSLevel and having a method called RMS is then kind of
> double bookeeping.
> 
> For instance, I envision a typical usage being as
> {
> RMSLevel rms_level_;
> 
> ...
> rms_value = rms_level_.RMS();
> }
> 
> To me it looks better if it instead would be
> {
> RMSLevel rms_level_;
> 
> ...
> rms_value = rms_level_.Average();
> }
> 
> or 
> 
> {
> RMSLevel rms_level_;
> 
> ...
> rms_value = rms_level_.Value();
> }
> 
> 
> This is even more the case when you add the AverageAndPeak() method.
> 
> 
> 
> 
> 
> 

Done.

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/voice_engine/chann...
File webrtc/voice_engine/channel.cc (right):

https://codereview.webrtc.org/2535523002/diff/40001/webrtc/voice_engine/chann...
webrtc/voice_engine/channel.cc:2770: rtc::ArrayView<const
int16_t>(_audioFrame.data_, length));
On 2016/11/29 08:57:13, peah-webrtc wrote:
> You should probably add the include array_view.h. Now it is somehow implictly
> included via another include as it is neither in channel.cc or channel.h.

Done.

[peah-webrtc, 2016-11-29 11:55:17]

On 2016/11/29 10:24:55, hlundin-webrtc wrote:
> PTAL again. I uploaded a series of patch sets, to make reviewing easier, I
> hope...
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/rms_level.cc (right):
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:24: static constexpr float
> kMinLevel = 1.995262314968883e-13f;
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > Can this be computed from kMinLevelDb using a constexpr?
> 
> Already tried, but failed. Cannot use static_assert either, since neither
> std::pow, nor std::log10 can be used in a constexpr.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:30: max_mean_square_(0) {}
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > Would it make sense to do a call to Reset() inside the constructor instead
of
> > the initializer lists?
> 
> Done.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:35: sum_square_ = 0;
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > 0.f;
> 
> Done.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:37: max_mean_square_ = 0;
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > 0.f;
> 
> Done.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:52: max_mean_square_ =
> std::max(max_mean_square_, sum_square / data.size());
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > Why not do the division only at the call of AverageAndPeak. That is cheaper,
> > harmonizes with how sum_square is computed, and does not require the comment
> on
> > line 90.
> > 
> > In order to really save computations, you could then compute the division
once
> > by precomputing 1/sample_count_ and apply it both to sum_square_ and
> > max_sum_square_.
> 
> Changed the implementation to cope with variable block sizes (by resetting).
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:59: namespace {
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > Is there a special reason for having two anonymous code sections? Would it
> make
> > sense to combine them?
> 
> I thought it made sense to have the code close to where it was called from.
Now
> I consolidated the namespaces.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:65: int CalcRms(float
mean_square)
> {
> On 2016/11/29 08:57:12, peah-webrtc wrote:
> > I would suggest avoiding the abbreviation Calc since this method name is
> already
> > so short, it already contains an abbreviation, and since the style guide
> > suggests that
> > (https://google.github.io/styleguide/cppguide.html#General_Naming_Rules).
> > 
> > What about CalculateRms or ComputeRms?
> 
> Done.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:74: const float rms = 10 *
> log10(mean_square_norm);
> On 2016/11/29 08:57:12, peah-webrtc wrote:
> > 10.f
> 
> Done.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:75: RTC_DCHECK_LE(rms, 0);
> On 2016/11/29 08:57:12, peah-webrtc wrote:
> > 0.f
> 
> Done.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.cc:78: return static_cast<int>(-rms
+
> 0.5);
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > 0.5f
> 
> Done.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> File webrtc/modules/audio_processing/rms_level.h (right):
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.h:42: void
> Process(rtc::ArrayView<const int16_t> data);
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > While you are anyway changing this: Would it make sense to change Process to
> > Analyze?
> > 
> > Elsewhere inside APM the convention seems to be that if a method changes the
> > signal, it is called Process, otherwise it is called Analyze.
> 
> Done.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/modules/audio_proc...
> webrtc/modules/audio_processing/rms_level.h:52: int RMS();
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > Suggestion: I think it would make sense to rename this method while you are
at
> > it.
> > 
> > The class is called RMSLevel and having a method called RMS is then kind of
> > double bookeeping.
> > 
> > For instance, I envision a typical usage being as
> > {
> > RMSLevel rms_level_;
> > 
> > ...
> > rms_value = rms_level_.RMS();
> > }
> > 
> > To me it looks better if it instead would be
> > {
> > RMSLevel rms_level_;
> > 
> > ...
> > rms_value = rms_level_.Average();
> > }
> > 
> > or 
> > 
> > {
> > RMSLevel rms_level_;
> > 
> > ...
> > rms_value = rms_level_.Value();
> > }
> > 
> > 
> > This is even more the case when you add the AverageAndPeak() method.
> > 
> > 
> > 
> > 
> > 
> > 
> 
> Done.
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/voice_engine/chann...
> File webrtc/voice_engine/channel.cc (right):
> 
>
https://codereview.webrtc.org/2535523002/diff/40001/webrtc/voice_engine/chann...
> webrtc/voice_engine/channel.cc:2770: rtc::ArrayView<const
> int16_t>(_audioFrame.data_, length));
> On 2016/11/29 08:57:13, peah-webrtc wrote:
> > You should probably add the include array_view.h. Now it is somehow
implictly
> > included via another include as it is neither in channel.cc or channel.h.
> 
> Done.

Great!
lgtm

[hlundin-webrtc, 2016-11-29 12:08:44]

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

[commit-bot: I haz the power, 2016-11-29 12:08:49]

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-11-29 12:19:38]

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

[commit-bot: I haz the power, 2016-11-29 12:19:39]

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

[henrika_webrtc, 2016-11-29 12:24:39]

You need a real owner ;-)

LGTM

[henrika_webrtc, 2016-11-29 12:24:45]

The CQ bit was checked by henrika@webrtc.org

[commit-bot: I haz the power, 2016-11-29 12:24:48]

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-11-29 12:26:24]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1480422285740670,
"parent_rev": "733c7bdc4908a320260b689601a06e29a910e600", "commit_rev":
"0d8de37ef9eb36c1fad677749f9b2f74f4ed7519"}

[commit-bot: I haz the power, 2016-11-29 12:26:27]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-11-29 12:26:41]

Description was changed from

==========
Refactor RMSLevel and give it new functionality

This change rewrites RMSLevel, making it accept an ArrayView as input,
and modify the implementation somewhat. It also makes the class keep
track of the peak RMS in addition to the average RMS over the
measurement period.

New tests are added to cover the new functionality.

BUG=webrtc:6622
==========

to

==========
Refactor RMSLevel and give it new functionality

This change rewrites RMSLevel, making it accept an ArrayView as input,
and modify the implementation somewhat. It also makes the class keep
track of the peak RMS in addition to the average RMS over the
measurement period.

New tests are added to cover the new functionality.

BUG=webrtc:6622

Committed: https://crrev.com/5049942219776f857b20991ad61a55f06aebae79
Cr-Commit-Position: refs/heads/master@{#15294}
==========

[commit-bot: I haz the power, 2016-11-29 12:26:42]

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