Surface the noise estimate of the NS to be used by other components

Surface the noise estimate of the NS to be used by other components

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

Committed: https://crrev.com/fa639f0bb31febce6ebf7f21edbb4d69f4885f13
Cr-Commit-Position: refs/heads/master@{#11541}

[aluebs-webrtc, 2016-01-30 04:03:39]

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

[turaj, 2016-02-05 01:47:58]

Sorry for the late review.

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

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/include/audio_processing.h:918: // Returns the
noise estimate per frequency bin of current chunk averaged
Isn't it some sort of noise power averaged over some time interval. The comment
says "current chunk" sounds like instantaneous power, is it really it?

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

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/noise_suppression_impl.cc:196:
static_cast<float>(noise[i] >> 17) / suppressors_.size();
Is nois[] is Q17? maybe a comment helps. And if it is in Q17 by shifting >> 17
we lose precision, I don't know how important it is, perhaps not much, but just
to be fair we can do the division in float.

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/ns/noise_suppression.c (right):

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/ns/noise_suppression.c:64: return NULL;
Is NULL preferred over nullptr in WebRTC?

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/ns/noise_suppression.h (right):

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/ns/noise_suppression.h:121: float*
WebRtcNs_noise_estimate(NsHandle* handle);
shouldn't be const float*?

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/ns/noise_suppression.h:128: size_t
WebRtcNs_num_freq();
Just a suggestion: these two can be combined 

float* WebRtcNs_noise_estimate(NsHandle* handle, size_t* num_freq) where
num_freq == 0 means uninitialized. Doesn't need to return nullptr

[aluebs-webrtc, 2016-02-06 00:12:28]

hlundin, could you please take a look whenever you have a minute?

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

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/include/audio_processing.h:918: // Returns the
noise estimate per frequency bin of current chunk averaged
On 2016/02/05 01:47:58, turaj wrote:
> Isn't it some sort of noise power averaged over some time interval. The
comment
> says "current chunk" sounds like instantaneous power, is it really it?

No, it actually estimates it smoothly over time. Good point, I removed the
"current chunk" comment.

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

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/noise_suppression_impl.cc:196:
static_cast<float>(noise[i] >> 17) / suppressors_.size();
On 2016/02/05 01:47:58, turaj wrote:
> Is nois[] is Q17? maybe a comment helps. And if it is in Q17 by shifting >> 17
> we lose precision, I don't know how important it is, perhaps not much, but
just
> to be fair we can do the division in float.

It is an unsigned 32-bit integer, and we want to get it to the level of a signed
16-bit integer, so it needs 17 shifts. I thought about making the division in
float to not lose precision, but I thought it was not going to affect and this
was easier to reed. But I guess you are right. Added comment and did division in
float.

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/ns/noise_suppression.c (right):

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/ns/noise_suppression.c:64: return NULL;
On 2016/02/05 01:47:58, turaj wrote:
> Is NULL preferred over nullptr in WebRTC?

No, but nullptr is C++11 and this is a C file :)

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
File webrtc/modules/audio_processing/ns/noise_suppression.h (right):

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/ns/noise_suppression.h:121: float*
WebRtcNs_noise_estimate(NsHandle* handle);
On 2016/02/05 01:47:58, turaj wrote:
> shouldn't be const float*?

Nice catch! Done.

https://codereview.webrtc.org/1654443004/diff/1/webrtc/modules/audio_processi...
webrtc/modules/audio_processing/ns/noise_suppression.h:128: size_t
WebRtcNs_num_freq();
On 2016/02/05 01:47:58, turaj wrote:
> Just a suggestion: these two can be combined 
> 
> float* WebRtcNs_noise_estimate(NsHandle* handle, size_t* num_freq) where
> num_freq == 0 means uninitialized. Doesn't need to return nullptr

I think I prefer to keep them separate, since the num_freq is a constant that
that doesn't depend on the handle and this makes it simple to just re-size the
container once and then average the noise estimation for each channel in
noise_suppression_impl.cc. But if you have a strong opinion, I am happy to
combine them.

[hlundin-webrtc, 2016-02-08 10:12:32]

Please, see comments inline.

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

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:179: const
std::vector<float>& NoiseSuppressionImpl::noise_estimate() {
I think I would prefer the following changes to this method:

1. Use a local temp std::vector<float> noise_estimate instead of the class
member noise_estimate_.

2. Return it by value, and let the compiler do return-value optimization. That
is:
std::vector<float> NoiseSuppressionImpl::noise_estimate();

Then you can get rid of the member variable noise_estimate_, which you seem to
have created just to be able to return a reference.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:181: if
(noise_estimate_updated_) {
What if the noise estimate is not updated. How valid is the return value? What
will it contain? Empty vector?

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/ns/noise_suppression.c (right):

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression.c:61: const float*
WebRtcNs_noise_estimate(NsHandle* handle) {
You can make the input a const pointer.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression.c:62: NoiseSuppressionC*
self = (NoiseSuppressionC*)handle;
const

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression.c:70: return
HALF_ANAL_BLOCKL;
Is this a medical condition? :)

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/ns/noise_suppression.h (right):

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression.h:119: *                   
    NULL pointer - NULL pointer or uninitialized instance.
"NULL pointer - NULL pointer"?
Suggest: "Returns NULL if the input is a NULL pointer or an uninitialized
instance.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/ns/noise_suppression_x.c (right):

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression_x.c:48: const uint32_t*
WebRtcNsx_noise_estimate(NsxHandle* nsxInst) {
const input.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression_x.c:49:
NoiseSuppressionFixedC* self = (NoiseSuppressionFixedC*)nsxInst;
const

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/ns/noise_suppression_x.h (right):

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression_x.h:87: * frequency bins
can be get using WebRtcNsx_num_freq().
"can be get" -> "can be provided" or something else.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression_x.h:93: *                  
     NULL pointer - NULL pointer or uninitialized instance.
Same as before.

[aluebs-webrtc, 2016-02-09 00:01:43]

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

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:179: const
std::vector<float>& NoiseSuppressionImpl::noise_estimate() {
On 2016/02/08 10:12:31, hlundin-webrtc wrote:
> I think I would prefer the following changes to this method:
> 
> 1. Use a local temp std::vector<float> noise_estimate instead of the class
> member noise_estimate_.
> 
> 2. Return it by value, and let the compiler do return-value optimization. That
> is:
> std::vector<float> NoiseSuppressionImpl::noise_estimate();
> 
> Then you can get rid of the member variable noise_estimate_, which you seem to
> have created just to be able to return a reference.

I thought about doing that, but then each call to noise_estimate() would re-do
the average, cast, normalization, etc instead of caching the noise_estimate
until it changes. This is only a problem if it is called more than once per
chunk. But I could imagine this to be the case if some components (maybe IE and
AEC) start using this noise estimate instead. Of course, we can always only call
it once in the APM and pass in the same copy to all components but that puts the
burden on the APM instead of the NS.
In any case, I implemented your suggestions, because I also see the pros and the
simplicity of this approach :)

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:181: if
(noise_estimate_updated_) {
On 2016/02/08 10:12:31, hlundin-webrtc wrote:
> What if the noise estimate is not updated. How valid is the return value? What
> will it contain? Empty vector?

Since the vector is calculated every time, I removed the flag that checked if it
was updated. So now a zero vector will be returned, which I think makes sense.
Or do you think I should keep the flag and return an empty vector?

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/ns/noise_suppression.c (right):

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression.c:61: const float*
WebRtcNs_noise_estimate(NsHandle* handle) {
On 2016/02/08 10:12:32, hlundin-webrtc wrote:
> You can make the input a const pointer.

Done.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression.c:62: NoiseSuppressionC*
self = (NoiseSuppressionC*)handle;
On 2016/02/08 10:12:32, hlundin-webrtc wrote:
> const

Done.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression.c:70: return
HALF_ANAL_BLOCKL;
On 2016/02/08 10:12:32, hlundin-webrtc wrote:
> Is this a medical condition? :)

I didn't come up with that naming :)

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/ns/noise_suppression.h (right):

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression.h:119: *                   
    NULL pointer - NULL pointer or uninitialized instance.
On 2016/02/08 10:12:32, hlundin-webrtc wrote:
> "NULL pointer - NULL pointer"?
> Suggest: "Returns NULL if the input is a NULL pointer or an uninitialized
> instance.

Done.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/ns/noise_suppression_x.c (right):

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression_x.c:48: const uint32_t*
WebRtcNsx_noise_estimate(NsxHandle* nsxInst) {
On 2016/02/08 10:12:32, hlundin-webrtc wrote:
> const input.

Done.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression_x.c:49:
NoiseSuppressionFixedC* self = (NoiseSuppressionFixedC*)nsxInst;
On 2016/02/08 10:12:32, hlundin-webrtc wrote:
> const

Done.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/ns/noise_suppression_x.h (right):

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression_x.h:87: * frequency bins
can be get using WebRtcNsx_num_freq().
On 2016/02/08 10:12:32, hlundin-webrtc wrote:
> "can be get" -> "can be provided" or something else.

Done.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression_x.h:93: *                  
     NULL pointer - NULL pointer or uninitialized instance.
On 2016/02/08 10:12:32, hlundin-webrtc wrote:
> Same as before.

Done.

[hlundin-webrtc, 2016-02-09 15:33:52]

LGTM with one nit.

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
File webrtc/modules/audio_processing/ns/noise_suppression.c (right):

https://codereview.webrtc.org/1654443004/diff/20001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/ns/noise_suppression.c:70: return
HALF_ANAL_BLOCKL;
On 2016/02/09 00:01:42, aluebs-webrtc wrote:
> On 2016/02/08 10:12:32, hlundin-webrtc wrote:
> > Is this a medical condition? :)
> 
> I didn't come up with that naming :)

Acknowledged.

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

https://codereview.webrtc.org/1654443004/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:176:
std::vector<float> NoiseSuppressionImpl::noise_estimate() {
Rename function to NoiseEstimate, to reflect the fact that it is not a simple
getter; some calculations will happen that may lead to performance impact.

[turaj, 2016-02-09 15:56:11]

lgtm

[aluebs-webrtc, 2016-02-09 17:48:49]

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

https://codereview.webrtc.org/1654443004/diff/40001/webrtc/modules/audio_proc...
webrtc/modules/audio_processing/noise_suppression_impl.cc:176:
std::vector<float> NoiseSuppressionImpl::noise_estimate() {
On 2016/02/09 15:33:51, hlundin-webrtc wrote:
> Rename function to NoiseEstimate, to reflect the fact that it is not a simple
> getter; some calculations will happen that may lead to performance impact.

Good idea. Done.

[aluebs-webrtc, 2016-02-09 18:15:09]

The CQ bit was checked by aluebs@webrtc.org

[aluebs-webrtc, 2016-02-09 18:15:09]

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/1654443004/#ps80001 (title:
"Fix normalization factor")

[commit-bot: I haz the power, 2016-02-09 18:15:18]

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

[commit-bot: I haz the power, 2016-02-09 18:49:54]

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

[commit-bot: I haz the power, 2016-02-09 18:49:55]

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

[aluebs-webrtc, 2016-02-09 18:54:47]

The CQ bit was checked by aluebs@webrtc.org

[commit-bot: I haz the power, 2016-02-09 18:54:50]

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

[commit-bot: I haz the power, 2016-02-09 19:21:14]

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

[commit-bot: I haz the power, 2016-02-09 19:21:14]

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

[aluebs-webrtc, 2016-02-09 19:24:56]

Description was changed from

==========
Surface the noise estimate of the NS to be used by other components
==========

to

==========
Surface the noise estimate of the NS to be used by other components

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

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

[aluebs-webrtc, 2016-02-09 19:24:58]

Committed patchset #5 (id:80001) manually as
fa639f0bb31febce6ebf7f21edbb4d69f4885f13 (presubmit successful).

[commit-bot: I haz the power, 2016-02-09 19:24:58]

Description was changed from

==========
Surface the noise estimate of the NS to be used by other components

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

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

to

==========
Surface the noise estimate of the NS to be used by other components

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

Committed: https://crrev.com/fa639f0bb31febce6ebf7f21edbb4d69f4885f13
Cr-Commit-Position: refs/heads/master@{#11541}
==========

[commit-bot: I haz the power, 2016-02-09 19:24:59]

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