Compensate for the compression gain in the IntelligibilityEnhancer

Compensate for the compression gain in the IntelligibilityEnhancer

The render signal that reaches the IE is already normalized and compressed by an AGC on the far-end capture processing. Because the noise estimation is done before the AGC compression, we need to compensate for this gain in the IE to be able to compare these 2 powers.

Committed: https://crrev.com/11d4a42ce5aecae3885498a22254b709687c07e8
Cr-Commit-Position: refs/heads/master@{#12549}

[aluebs-webrtc, 2016-04-22 21:35:58]

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

[peah-webrtc, 2016-04-25 07:23:20]

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

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:711:
public_submodules_->gain_control->compression_gain_db());
Is there some action in place to ensure that the AGC is active in the
appropriate mode.
An option could be to add a DCHECK for that in the activation of the IE similar
to what was done for the noise suppressor.

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

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:115:
for (size_t i = 0; i < noise.size(); ++i) {
Suggestion: You could use a foreach loop here.

[hlundin-webrtc, 2016-04-25 08:00:35]

Nice and short. But I wonder if you should add some unit testing for the new
functionality. The only test you have sets the gain to 0.

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/gain_control_impl.h (right):

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/gain_control_impl.h:54: int
compression_gain_db() const override;
Nit: you should reorder the methods in the .cc file to match the ordering in the
.h file.

[turaj, 2016-04-25 13:51:13]

LGTM, maybe add a few line to the CL's description why this is needed, or a link
to the section of document that justifies gain compensation.

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

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:114:
float gain = std::pow(10.f, gain_db / 20.f);
suggestion: const float

[aluebs-webrtc, 2016-04-27 22:52:26]

Description was changed from

==========
Compensate for the compression gain in the IntelligibilityEnhancer
==========

to

==========
Compensate for the compression gain in the IntelligibilityEnhancer

The render signal that reaches the IE is already normalized and compressed by an
AGC on the far-end capture processing. Because the noise estimation is done
before the AGC compression, we need to compensate for this gain in the IE to be
able to compare these 2 powers.
==========

[aluebs-webrtc, 2016-04-27 22:52:58]

Addressed comments, added unittest and expanded description. PTAL.

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

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/audio_processing_impl.cc:711:
public_submodules_->gain_control->compression_gain_db());
On 2016/04/25 07:23:20, peah-webrtc wrote:
> Is there some action in place to ensure that the AGC is active in the
> appropriate mode.
> An option could be to add a DCHECK for that in the activation of the IE
similar
> to what was done for the noise suppressor.

Good point. Done.

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/gain_control_impl.h (right):

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/gain_control_impl.h:54: int
compression_gain_db() const override;
On 2016/04/25 08:00:35, hlundin-webrtc wrote:
> Nit: you should reorder the methods in the .cc file to match the ordering in
the
> .h file.

That doesn't hold for this file at all (they are in a complete other order
already), but I moved this particular method to be somewhat more ordered. I
don't want to move the other methods around though.

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

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:114:
float gain = std::pow(10.f, gain_db / 20.f);
On 2016/04/25 13:51:13, turaj wrote:
> suggestion: const float

Totally!

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer.cc:115:
for (size_t i = 0; i < noise.size(); ++i) {
On 2016/04/25 07:23:20, peah-webrtc wrote:
> Suggestion: You could use a foreach loop here.

Done.

[hlundin-webrtc, 2016-04-28 06:19:01]

lgtm

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/gain_control_impl.h (right):

https://codereview.webrtc.org/1913603002/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/gain_control_impl.h:54: int
compression_gain_db() const override;
On 2016/04/27 22:52:58, aluebs-webrtc wrote:
> On 2016/04/25 08:00:35, hlundin-webrtc wrote:
> > Nit: you should reorder the methods in the .cc file to match the ordering in
> the
> > .h file.
> 
> That doesn't hold for this file at all (they are in a complete other order
> already), but I moved this particular method to be somewhat more ordered. I
> don't want to move the other methods around though.

Acknowledged.

https://codereview.webrtc.org/1913603002/diff/40001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc
(right):

https://codereview.webrtc.org/1913603002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:411:
const float kGainTimes = std::pow(10.f, kGainDB / 20.f);
Suggest kGainFactor or kGainMult(iplier).

[aluebs-webrtc, 2016-04-28 17:30:09]

peah, any additional comments?

https://codereview.webrtc.org/1913603002/diff/40001/webrtc/modules/audio_proc...
File
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc
(right):

https://codereview.webrtc.org/1913603002/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:411:
const float kGainTimes = std::pow(10.f, kGainDB / 20.f);
On 2016/04/28 06:19:01, hlundin-webrtc wrote:
> Suggest kGainFactor or kGainMult(iplier).

Done.

[peah-webrtc, 2016-04-28 18:53:24]

On 2016/04/28 17:30:09, aluebs-webrtc wrote:
> peah, any additional comments?
> 
>
https://codereview.webrtc.org/1913603002/diff/40001/webrtc/modules/audio_proc...
> File
>
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc
> (right):
> 
>
https://codereview.webrtc.org/1913603002/diff/40001/webrtc/modules/audio_proc...
>
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:411:
> const float kGainTimes = std::pow(10.f, kGainDB / 20.f);
> On 2016/04/28 06:19:01, hlundin-webrtc wrote:
> > Suggest kGainFactor or kGainMult(iplier).
> 
> Done.

lgtm for the code change. Good with the update of the description of the CL with
the background to the change but I still don't see the rationale by the change
so I cannot lgtm the algorithm aspect.

Also, please doublecheck the gain application. I have a feeling that there is a
square root that should not be there.

[aluebs-webrtc, 2016-04-28 19:58:55]

On 2016/04/28 18:53:24, peah-webrtc wrote:
> On 2016/04/28 17:30:09, aluebs-webrtc wrote:
> > peah, any additional comments?
> > 
> >
>
https://codereview.webrtc.org/1913603002/diff/40001/webrtc/modules/audio_proc...
> > File
> >
>
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc
> > (right):
> > 
> >
>
https://codereview.webrtc.org/1913603002/diff/40001/webrtc/modules/audio_proc...
> >
>
webrtc/modules/audio_processing/intelligibility/intelligibility_enhancer_unittest.cc:411:
> > const float kGainTimes = std::pow(10.f, kGainDB / 20.f);
> > On 2016/04/28 06:19:01, hlundin-webrtc wrote:
> > > Suggest kGainFactor or kGainMult(iplier).
> > 
> > Done.
> 
> lgtm for the code change. Good with the update of the description of the CL
with
> the background to the change but I still don't see the rationale by the change
> so I cannot lgtm the algorithm aspect.
> 
> Also, please doublecheck the gain application. I have a feeling that there is
a
> square root that should not be there.

The square root there is correct, because it is applied on the magnitude. As you
can see in the unittest I added, actually the square of this factor is finally
applied to the PSD.

[aluebs-webrtc, 2016-04-28 19:58:59]

The CQ bit was checked by aluebs@webrtc.org

[aluebs-webrtc, 2016-04-28 19:59:00]

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

[commit-bot: I haz the power, 2016-04-28 19:59:05]

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

[commit-bot: I haz the power, 2016-04-28 21:58:35]

Description was changed from

==========
Compensate for the compression gain in the IntelligibilityEnhancer

The render signal that reaches the IE is already normalized and compressed by an
AGC on the far-end capture processing. Because the noise estimation is done
before the AGC compression, we need to compensate for this gain in the IE to be
able to compare these 2 powers.
==========

to

==========
Compensate for the compression gain in the IntelligibilityEnhancer

The render signal that reaches the IE is already normalized and compressed by an
AGC on the far-end capture processing. Because the noise estimation is done
before the AGC compression, we need to compensate for this gain in the IE to be
able to compare these 2 powers.
==========

[commit-bot: I haz the power, 2016-04-28 21:58:36]

Committed patchset #4 (id:60001)

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

Description was changed from

==========
Compensate for the compression gain in the IntelligibilityEnhancer

The render signal that reaches the IE is already normalized and compressed by an
AGC on the far-end capture processing. Because the noise estimation is done
before the AGC compression, we need to compensate for this gain in the IE to be
able to compare these 2 powers.
==========

to

==========
Compensate for the compression gain in the IntelligibilityEnhancer

The render signal that reaches the IE is already normalized and compressed by an
AGC on the far-end capture processing. Because the noise estimation is done
before the AGC compression, we need to compensate for this gain in the IE to be
able to compare these 2 powers.

Committed: https://crrev.com/11d4a42ce5aecae3885498a22254b709687c07e8
Cr-Commit-Position: refs/heads/master@{#12549}
==========

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

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