Fix normalization of noise estimate in NoiseSuppressor

webrtc/modules/audio_processing/ns/noise_suppression_x.c

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 27 matching lines @@
 }
 
 void WebRtcNsx_Process(NsxHandle* nsxInst,
                       const short* const* speechFrame,
                       int num_bands,
                       short* const* outFrame) {
   WebRtcNsx_ProcessCore((NoiseSuppressionFixedC*)nsxInst, speechFrame,
                         num_bands, outFrame);
 }
 
-const uint32_t* WebRtcNsx_noise_estimate(const NsxHandle* nsxInst) {
-  const NoiseSuppressionFixedC* self = (const NoiseSuppressionFixedC*)nsxInst;
+const uint16_t* WebRtcNsx_noise_estimate(NsxHandle* nsxInst) {
+  NoiseSuppressionFixedC* self = (NoiseSuppressionFixedC*)nsxInst;
   if (nsxInst == NULL || self->initFlag == 0) {
     return NULL;
   }
-  return self->prevNoiseU32;
+  for (size_t i = 0; i < HALF_ANAL_BLOCKL; ++i) {
+    self->prevNoiseU16[i] = self->prevNoiseU32[i] >> (self->prevQNoise + 11);

[hlundin-webrtc, 2016/03/30 07:34:59]

Can you update prevNoiseU16 where prevNoiseU32 is updated, and retain the
const-ness (w.r.t. nsxInst) of this function?

[peah-webrtc, 2016/03/30 13:44:46]

This change transitions from a noise level of 32 bits with adaptive Q value to a
16 bit value with a fixed Q value of 11, right?

How has it been verified that this value is sufficient to avoid saturation/
ensure a proper level?

An alternative would be to convert the spectrum to floating point (Q0) as it is
anyway only used in floating point format.

[peah-webrtc, 2016/03/30 13:44:46]

I don't see why the prevNoiseU16 is used here. It is only used as a static
buffer now, right? And this buffer will remain on the state of the noise
suppressor.

I think a better solution is to have the WebRtcNsx_noise_estimate and
NoiseEstimate() methods accept an array/vector that is owned by the outside
which it copies the noise estimate to. In that way:
1) There is no need for a contract in which the noise suppressor guarantees that
this internal buffer is not accessed meanwhile an external user is reading it.
2) There is no space wasted on the noise supressor state for doing this.

[turaj, 2016/03/30 14:51:55]

Is the new Q value of noise level 11 or -11? I suppose you want to keep this
interface in fixed-point because it is fixed-point implementation of the noise
suppressor, however, currently the only module using it is IE which is in
floating point. So I really don't see of going through some scaling here that is
not meaningful for the caller. 

I suppose there is a top level API that calls either fixed-point or
floating-point NS, depending on the build. I suggest that a conversion from
fixed-point (with adaptive Q number) to float has to happen in that top-level
API. Then, this API would return  the noise level as it is with the associated Q
value, and let the top level API do a safe fixed-point to floating-point
conversion.

This way, the method will be const, as henrik suggested, and we do not need
extra buffer, as Per concerned. 

IMPORTANT: there should be a agreement that whether the noise level is computed
for int16 range of input signal or [-1 1] range.

[aluebs-webrtc, 2016/03/31 00:26:32]

I agree with turaj, that we want to keep the interface fixed-point, since this
is the fixed-point implementation. So as suggested by turaj I moved the
normalization to the noise_suppression_impl. I tried initially to avoid that
because it means adding an API to get the dynamical domain and have the caller
handle something that should be implementation details of the component. But the
bright side is that now the normalization is done safely after converting to
float and no additional buffer is necessary. And of course, then the method can
be const, as turaj pointed out.

+  }
+  return self->prevNoiseU16;
 }
 
 size_t WebRtcNsx_num_freq() {
   return HALF_ANAL_BLOCKL;
 }