Tweak kDecayRate in the IntelligibilityEnhancer

Tweak kDecayRate in the IntelligibilityEnhancer

This makes the addaptation of the IntelligibilityEnhancer slower, which makes it take more time to kick in or when the background noise changes drastically. But on the other hand, it reduces the risk of clipping and makes the changing in coloring less noticeable.

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

Committed: https://crrev.com/2d66cf9d8d5326029aaac1863acb8ccb9ae91db0
Cr-Commit-Position: refs/heads/master@{#12202}

[aluebs-webrtc, 2016-03-31 23:22:56]

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

[peah-webrtc, 2016-04-01 04:43:40]

On 2016/03/31 23:22:56, aluebs-webrtc wrote:

lgtm but please look into my comment.

[peah-webrtc, 2016-04-01 04:43:51]

https://codereview.webrtc.org/1848123002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1848123002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:37:
const float kDecayRate = 0.994f;              // Power estimation decay rate.
I cannot really say that I yet understand the underlying algorithm but it seems
a bit strange to lock the map the relative_change_limit in GainApplier to the
decay rate.

I'm not saying it is wrong to do so, but they seem quite independent, and are
used in different places. Furthermore, this is a change from how it was before.

[hlundin-webrtc, 2016-04-01 08:36:39]

LGTM with the same concerns as Per.

https://codereview.webrtc.org/1848123002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1848123002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:37:
const float kDecayRate = 0.994f;              // Power estimation decay rate.
On 2016/04/01 04:43:50, peah-webrtc wrote:
> I cannot really say that I yet understand the underlying algorithm but it
seems
> a bit strange to lock the map the relative_change_limit in GainApplier to the
> decay rate.
> 
> I'm not saying it is wrong to do so, but they seem quite independent, and are
> used in different places. Furthermore, this is a change from how it was
before.

Acknowledged.

[aluebs-webrtc, 2016-04-01 17:33:35]

turaj, any comments?

https://codereview.webrtc.org/1848123002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc
(right):

https://codereview.webrtc.org/1848123002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:37:
const float kDecayRate = 0.994f;              // Power estimation decay rate.
On 2016/04/01 08:36:38, hlundin-webrtc wrote:
> On 2016/04/01 04:43:50, peah-webrtc wrote:
> > I cannot really say that I yet understand the underlying algorithm but it
> seems
> > a bit strange to lock the map the relative_change_limit in GainApplier to
the
> > decay rate.
> > 
> > I'm not saying it is wrong to do so, but they seem quite independent, and
are
> > used in different places. Furthermore, this is a change from how it was
> before.
> 
> Acknowledged.

Well, the decay rate of the PSD estimations doesn't necessarily have to be
linked to the maximum gain change, but it kind of makes sense, since both are
linked to the speed the algorithm can adapt. And I find it elegant to only have
one constant to tweak the adaptation speed. But of course, I can go back to
having 2 different constants if you think it adds value.

[turaj, 2016-04-01 17:45:37]

I don't have further comment, than the one Per and Henrik brought up. 

I see your point of view, that how both constant affect the speed of adaptation,
and that if one wants to have a fast adapting PSD, perhaps they also want fast
changing gains and vice versa, otherwise it doesn't make much of sense.

But I see them fundamentally/philosophically different. The other argument is
that it will confuse people who look at the code for the first time that why the
memory in PSD computation should be so tight to how much gain change is allowed.


LGTM.

[aluebs-webrtc, 2016-04-01 18:27:19]

Because of popular demand, I brought back kMaxRelativeGainChange.

[aluebs-webrtc, 2016-04-01 18:27:25]

The CQ bit was checked by aluebs@webrtc.org

[aluebs-webrtc, 2016-04-01 18:27:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
henrik.lundin@webrtc.org, peah@webrtc.org, turaj@webrtc.org
Link to the patchset: https://codereview.webrtc.org/1848123002/#ps20001 (title:
"Separate variables")

[commit-bot: I haz the power, 2016-04-01 18:27:37]

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

[commit-bot: I haz the power, 2016-04-01 18:32:52]

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

[commit-bot: I haz the power, 2016-04-01 18:32:53]

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

[aluebs-webrtc, 2016-04-01 20:59:48]

Description was changed from

==========
Tweak kDecayRate in the IntelligibilityEnhancer

This makes the addaptation of the IntelligibilityEnhancer slower, which makes it
take more time to kick in or when the background noise changes drastically. But
on the other hand, it reduces the risk of clipping and makes the changing in
coloring less noticeable.
==========

to

==========
Tweak kDecayRate in the IntelligibilityEnhancer

This makes the addaptation of the IntelligibilityEnhancer slower, which makes it
take more time to kick in or when the background noise changes drastically. But
on the other hand, it reduces the risk of clipping and makes the changing in
coloring less noticeable.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/2d66cf9d8d5326029aaac1863...
==========

[aluebs-webrtc, 2016-04-01 20:59:50]

Committed patchset #3 (id:40001) manually as
2d66cf9d8d5326029aaac1863acb8ccb9ae91db0 (presubmit successful).

[commit-bot: I haz the power, 2016-04-01 20:59:50]

Description was changed from

==========
Tweak kDecayRate in the IntelligibilityEnhancer

This makes the addaptation of the IntelligibilityEnhancer slower, which makes it
take more time to kick in or when the background noise changes drastically. But
on the other hand, it reduces the risk of clipping and makes the changing in
coloring less noticeable.

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

Committed:
https://chromium.googlesource.com/external/webrtc/+/2d66cf9d8d5326029aaac1863...
==========

to

==========
Tweak kDecayRate in the IntelligibilityEnhancer

This makes the addaptation of the IntelligibilityEnhancer slower, which makes it
take more time to kick in or when the background noise changes drastically. But
on the other hand, it reduces the risk of clipping and makes the changing in
coloring less noticeable.

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

Committed: https://crrev.com/2d66cf9d8d5326029aaac1863acb8ccb9ae91db0
Cr-Commit-Position: refs/heads/master@{#12202}
==========

[commit-bot: I haz the power, 2016-04-01 20:59:52]

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