Fix and simplify the power estimation in the IntelligibilityEnhancer

Fix and simplify the power estimation in the IntelligibilityEnhancer

R=henrik.lundin@webrtc.org, turaj@webrtc.org

Committed: https://crrev.com/32348192cc67a33eee1c85baadc2f9efe0930482
Cr-Commit-Position: refs/heads/master@{#11663}

[aluebs-webrtc, 2016-02-10 18:51:04]

aluebs@webrtc.org changed reviewers:
+ henrik.lundin@webrtc.org, turaj@webrtc.org

[turaj, 2016-02-12 23:12:35]

LGTM with minor comments.

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:173:
if (true) {
Can we get rid off this "if(true)"?

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h
(right):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:38:
decay_rate(0.9f),
Maybe give names to 0.9f and 60.

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
(right):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:16:
#include <algorithm>
I guess <algorithm> should come first, now.

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:74:
factor = 1.f;
Please keep 1.0f for consistency, as in lines 64 and 65 it is 1.0f

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h
(left):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:24:
namespace intelligibility {
Do you think it was a bad idea to protect this utility in a namespace?

[aluebs-webrtc, 2016-02-13 01:47:50]

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:173:
if (true) {
On 2016/02/12 23:12:35, turaj wrote:
> Can we get rid off this "if(true)"?

I did that in the next CL: https://codereview.webrtc.org/1693823004/

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h
(right):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.h:38:
decay_rate(0.9f),
On 2016/02/12 23:12:35, turaj wrote:
> Maybe give names to 0.9f and 60.

In the next CL I am deleting this Config and moving to using named constants for
these values: https://codereview.webrtc.org/1693823004/

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
(right):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:16:
#include <algorithm>
On 2016/02/12 23:12:35, turaj wrote:
> I guess <algorithm> should come first, now.

Well the styleguide says C includes should come before C++ ones, does this also
apply when using <c*> instead of <*.h>. I am not sure.
https://engdoc.corp.google.com/eng/doc/devguide/cpp/styleguide.shtml?cl=head#...

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:74:
factor = 1.f;
On 2016/02/12 23:12:35, turaj wrote:
> Please keep 1.0f for consistency, as in lines 64 and 65 it is 1.0f 

I changed all occurrences in this file to .f.

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h
(left):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:24:
namespace intelligibility {
On 2016/02/12 23:12:35, turaj wrote:
> Do you think it was a bad idea to protect this utility in a namespace? 

I think it adds too much verbosity, but if you think it helps I am happy to add
the namespace back.

[hlundin-webrtc, 2016-02-15 13:48:57]

LGTM with question.

https://codereview.webrtc.org/1685703004/diff/50001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
(right):

https://codereview.webrtc.org/1685703004/diff/50001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:13:
#include <cmath>
Why did you change these?

[aluebs-webrtc, 2016-02-17 03:11:59]

Ok, I am landing this CL, but please let me know if you think I should change
anything and I will address it in a follow up CL.

https://codereview.webrtc.org/1685703004/diff/50001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
(right):

https://codereview.webrtc.org/1685703004/diff/50001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:13:
#include <cmath>
On 2016/02/15 13:48:57, hlundin-webrtc wrote:
> Why did you change these?

I realize it has nothing to do with this change and should have probably been in
its own CL, but I thought it was the desired format, right?

[aluebs-webrtc, 2016-02-17 03:12:02]

The CQ bit was checked by aluebs@webrtc.org

[aluebs-webrtc, 2016-02-17 03:12:02]

The patchset sent to the CQ was uploaded after l-g-t-m from turaj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1685703004/#ps50001 (title:
"Address turajs comments")

[commit-bot: I haz the power, 2016-02-17 03:12:13]

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

[commit-bot: I haz the power, 2016-02-17 03:22:52]

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

[commit-bot: I haz the power, 2016-02-17 03:22:53]

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

[hlundin-webrtc, 2016-02-17 07:45:21]

https://codereview.webrtc.org/1685703004/diff/50001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
(right):

https://codereview.webrtc.org/1685703004/diff/50001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:13:
#include <cmath>
On 2016/02/17 03:11:59, aluebs-webrtc wrote:
> On 2016/02/15 13:48:57, hlundin-webrtc wrote:
> > Why did you change these?
> 
> I realize it has nothing to do with this change and should have probably been
in
> its own CL, but I thought it was the desired format, right?

I am not sure we have an explicit policy, but I do know that pbos unified all c
includes to use the c version (math.h, etc) instead of the c++ version (cmath,
etc), where the two provide the same functionality.

In any case, you messed up the sorting, so some kind of follow-up is needed. :)

[turaj, 2016-02-17 16:36:30]

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h
(left):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:24:
namespace intelligibility {
On 2016/02/13 01:47:50, aluebs-webrtc wrote:
> On 2016/02/12 23:12:35, turaj wrote:
> > Do you think it was a bad idea to protect this utility in a namespace? 
> 
> I think it adds too much verbosity, but if you think it helps I am happy to
add
> the namespace back.

I thought, in general, limiting the scope is recommended, obviously one should
not over do it, otherwise we will end up with many layers of namespace. In this
case, I thought maybe it makes sense to have utility namespace, especially that
the class names are so generic. That said, really don't have strong opinion,
just curious why to change something that is potentially good.

[aluebs-webrtc, 2016-02-17 23:37:59]

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h
(left):

https://codereview.webrtc.org/1685703004/diff/30001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.h:24:
namespace intelligibility {
On 2016/02/17 16:36:30, turaj wrote:
> On 2016/02/13 01:47:50, aluebs-webrtc wrote:
> > On 2016/02/12 23:12:35, turaj wrote:
> > > Do you think it was a bad idea to protect this utility in a namespace? 
> > 
> > I think it adds too much verbosity, but if you think it helps I am happy to
> add
> > the namespace back.
> 
> I thought, in general, limiting the scope is recommended, obviously one should
> not over do it, otherwise we will end up with many layers of namespace. In
this
> case, I thought maybe it makes sense to have utility namespace, especially
that
> the class names are so generic. That said, really don't have strong opinion,
> just curious why to change something that is potentially good.

I completely agree that in general limiting the scope is good, but I felt in
this case it was overly verbose with not much gain, but you are right, the names
of the utility classes are really generic. Added the namespace back.

https://codereview.webrtc.org/1685703004/diff/50001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc
(right):

https://codereview.webrtc.org/1685703004/diff/50001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_utils.cc:13:
#include <cmath>
On 2016/02/17 07:45:21, hlundin-webrtc wrote:
> On 2016/02/17 03:11:59, aluebs-webrtc wrote:
> > On 2016/02/15 13:48:57, hlundin-webrtc wrote:
> > > Why did you change these?
> > 
> > I realize it has nothing to do with this change and should have probably
been
> in
> > its own CL, but I thought it was the desired format, right?
> 
> I am not sure we have an explicit policy, but I do know that pbos unified all
c
> includes to use the c version (math.h, etc) instead of the c++ version (cmath,
> etc), where the two provide the same functionality.
> 
> In any case, you messed up the sorting, so some kind of follow-up is needed.
:)

I was not aware there was an effort to unify them in c version and thought the
c++ version was preferred. Sorry about that confusion. Changed them back :)

[aluebs-webrtc, 2016-02-17 23:38:37]

The CQ bit was checked by aluebs@webrtc.org

[aluebs-webrtc, 2016-02-17 23:38:37]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, turaj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1685703004/#ps70001 (title:
"Re-add the intelligibility namespace")

[commit-bot: I haz the power, 2016-02-17 23:38:43]

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

[commit-bot: I haz the power, 2016-02-18 01:38:59]

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

[commit-bot: I haz the power, 2016-02-18 01:39:00]

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

[aluebs-webrtc, 2016-02-18 04:04:28]

Description was changed from

==========
Fix and simplify the power estimation in the IntelligibilityEnhancer
==========

to

==========
Fix and simplify the power estimation in the IntelligibilityEnhancer

R=henrik.lundin@webrtc.org, turaj@webrtc.org

Committed:
https://chromium.googlesource.com/external/webrtc/+/32348192cc67a33eee1c85baa...
==========

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

Description was changed from

==========
Fix and simplify the power estimation in the IntelligibilityEnhancer
==========

to

==========
Fix and simplify the power estimation in the IntelligibilityEnhancer

R=henrik.lundin@webrtc.org, turaj@webrtc.org

Committed: https://crrev.com/32348192cc67a33eee1c85baadc2f9efe0930482
Cr-Commit-Position: refs/heads/master@{#11663}
==========

[aluebs-webrtc, 2016-02-18 04:04:29]

Committed patchset #5 (id:70001) manually as
32348192cc67a33eee1c85baadc2f9efe0930482 (presubmit successful).

[commit-bot: I haz the power, 2016-02-18 04:04:30]

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