Pull out the PostFilter to its own NonlinearBeamformer API

webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc

Index: webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc
diff --git a/webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc b/webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc
index f5bdd6a3c2d0308bea77074301f6d8f688c89ead..b5d5197edc0e6218c548368e49d27e3cddaabbe4 100644
--- a/webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc
+++ b/webrtc/modules/audio_processing/beamformer/nonlinear_beamformer.cc
@@ -183,6 +183,30 @@ const float NonlinearBeamformer::kHalfBeamWidthRadians = DegreesToRadians(20.f);
 // static

[peah-webrtc, 2016/05/22 21:06:48]

Please correct this comment as well while you are at it.

[aluebs-webrtc, 2016/05/26 01:04:45]

What is wrong with it?

[peah-webrtc, 2016/05/26 08:48:52]

It is not a proper sentence, and not terminated by a period.  But maybe this is
a standard way of commenting statics definitions?

[aluebs-webrtc, 2016/05/28 03:00:00]

I think the static definition is clearer like this than in a sentence. Also, I
haven't touched this in this CL, so it would add one more unrelated change. So I
will leave it as is.

[peah-webrtc, 2016/05/30 11:49:25]

I think the guidelines should be applied regardless of that. But I won't insist
any further on this.

[aluebs-webrtc, 2016/06/01 00:16:34]

Acknowledged.

 const size_t NonlinearBeamformer::kNumFreqBins;
 
+class PostFilterTransform : public LappedTransform::Callback {

[peah-webrtc, 2016/05/22 21:06:48]

This class is a way to be able to use the callback functionality of the
LappedTransform for two callbacks in the same class, right?

It makes the code more complicated than it needs to, I think. 
Why not skip the friend class approach, and instead create a new class that
implements the Callback interface? Then you could call that to apply the
postfilter effect in such a way that the kCompensationGain and  final_mask_
variables are passed as input to that call.
That would achieve a separation of concerns, data and functionality
encapsulation and would avoid the need of the friend class approach.

[aluebs-webrtc, 2016/05/26 01:04:45]

I don't see how this simplifies the code, but I agree that it avoids the friend
class which is not desired. Anyway, I implemented it.

[peah-webrtc, 2016/05/26 08:48:52]

The simplification is separation of concerns and data and functionality
encapsulation, as well as avoiding the use of an interface.

[aluebs-webrtc, 2016/05/28 03:00:00]

I think the separation/encapsulation is almost the same, but the call order is
more complicated since you have to pass in the needed arguments. But I see that
having friend classes is not desirable, so I implemented it already.

[peah-webrtc, 2016/05/30 11:49:25]

Acknowledged.

+ public:
+  explicit PostFilterTransform(NonlinearBeamformer* beamformer)
+      : beamformer_(beamformer) {}
+
+ protected:
+  // Process one frequency-domain block of audio. This is where the fun

[peah-webrtc, 2016/05/22 21:06:48]

Please describe this more thoroughly. I'm not sure the comment is needed but if
there is to be a comment it should be more descriptive, e.g., what kind of
processing is done, how does the callback work, etc. 

[aluebs-webrtc, 2016/05/26 01:04:45]

Removed comment. It was just to be consistent with the other callback.

[peah-webrtc, 2016/05/30 11:49:25]

Acknowledged.

+  // happens. Implements LappedTransform::Callback.
+  void ProcessAudioBlock(const complex<float>* const* input,
+                         size_t num_input_channels,
+                         size_t num_freq_bins,
+                         size_t num_output_channels,
+                         complex<float>* const* output) override {
+    RTC_CHECK_EQ(NonlinearBeamformer::kNumFreqBins, num_freq_bins);
+    RTC_CHECK_EQ(1u, num_input_channels);
+    RTC_CHECK_EQ(1u, num_output_channels);
+
+    beamformer_->ApplyPostFilter(input[0], output[0]);
+  }
+
+ private:
+  NonlinearBeamformer* beamformer_;
+};
+
 NonlinearBeamformer::NonlinearBeamformer(
     const std::vector<Point>& array_geometry,
     SphericalPointf target_direction)
@@ -208,13 +232,16 @@ void NonlinearBeamformer::Initialize(int chunk_size_ms, int sample_rate_hz) {
   hold_target_blocks_ = kHoldTargetSeconds * 2 * sample_rate_hz / kFftSize;
   interference_blocks_count_ = hold_target_blocks_;
 
-  lapped_transform_.reset(new LappedTransform(num_input_channels_,
-                                              1,
-                                              chunk_length_,
-                                              window_,
-                                              kFftSize,
-                                              kFftSize / 2,
-                                              this));
+  process_transform_.reset(new LappedTransform(num_input_channels_,
+                                               1u,
+                                               chunk_length_,
+                                               window_,
+                                               kFftSize,
+                                               kFftSize / 2,
+                                               this));
+  postfilter_transform_.reset(new LappedTransform(
+      1u, 1u, chunk_length_, window_, kFftSize, kFftSize / 2,
+      new PostFilterTransform(this)));
   for (size_t i = 0; i < kNumFreqBins; ++i) {
     time_smooth_mask_[i] = 1.f;
     final_mask_[i] = 1.f;
@@ -371,18 +398,31 @@ void NonlinearBeamformer::ProcessChunk(const ChannelBuffer<float>& input,
   RTC_DCHECK_EQ(input.num_channels(), num_input_channels_);
   RTC_DCHECK_EQ(input.num_frames_per_band(), chunk_length_);
 
-  float old_high_pass_mask = high_pass_postfilter_mask_;
-  lapped_transform_->ProcessChunk(input.channels(0), output->channels(0));
+  old_high_pass_mask_ = high_pass_postfilter_mask_;
+  process_transform_->ProcessChunk(input.channels(0), output->channels(0));
+  // Copy over only the first channel of each band.

[peah-webrtc, 2016/05/22 21:06:48]

Have you checked the impact on the signal when this is only done in the lowest
band. In theory the perfect reconstruction property of the signal could be
destroyed through this , causing aliasing distortions. Furthermore,  this could
in theory cause an sharp increase in the signal power at the first band-split
frequency.

Why don't you perform the linear beamforming in the time-domain instead, as you
anyway do not pre-steer the signals before applying that step (right?)? 
That would have the benefits
1) Be cheaper as you don't need to do the IFFT to go back to the time-domain.
2) Have lower delay, as the filter-bank delay is avoided. 
3) Have zero impact on the perfect-reconstruction property of the splitting
filter.
4) Have no edge effect at the first band-split frequency.
 

[aluebs-webrtc, 2016/05/26 01:04:45]

This is no longer relevant, since we decided offline to not do any linear
beamforming because it is negligible compared to the postfilter. But a few
points just to be clear:
* The perfect reconstruction of the filter bank is lost with any processing
done.
* The worst case 3dB increase of power in the higher bands is probably not
perceivable. And I am not sure what the problem with a "sharp" edge here is.

[peah-webrtc, 2016/05/26 08:48:52]

I think the perfect reconstruction may actually be preserved if you average two
PR filterbanks in the time-domain (which the linear beamforming is identical to
if no pre-steering is performed).

A 3 dB "edge" is perceivable, but I doubt as well if it will matter in general
for most noise types (at least the one with tilted spectral shapes). The sharp
edge is dependent on the sharpness of the splitting filter, as the subsequent
merge will merge two bands where the noise levels differ by 3 dB after the
beamforming.

[aluebs-webrtc, 2016/05/28 03:00:00]

I meant that other components are already non-linear, losing the perfect
reconstruction. I don't think the 3dB (in the worst case) would be perceivable
in the higher bands at all. But in any case it would sound slightly highpass,
but the sharpness is not something that would matter too much. Anyways, now I am
only taking one channel, so this is no longer relevant.

[peah-webrtc, 2016/05/30 11:49:25]

Acknowledged.

+  // This can be done because the effect of the linear beamformer is negligible
+  // compared to the post-filter.
+  for (size_t i = 1; i < input.num_bands(); ++i) {
+    memcpy(output->channels(i)[0],

[peah-webrtc, 2016/05/22 21:06:48]

what happens if the output is dual channel? Since the ProcessChunk method locks
the content of the output to be single-channel, it should either assert that the
number of output channels allready is one, or explicitly set the number of
channels to 1 in the output.

[aluebs-webrtc, 2016/05/26 01:04:45]

This can't be done as is, since the input and output can be the same
ChannelBuffer and the ChannelBuffer has a fixed number of channels.

[peah-webrtc, 2016/05/30 11:49:25]

So this means that the input could have 2 channels, and then the output gets
only 1 channel, but the meta-information (about the number of channels) in the
output does still not reflect that, which means that the caller needs to know
this and explicitly set the number of channels in the meta-information to 1!
That sounds error prone and more complicated than necessary.

It would be better then to replace this ProcessChunk call by two other calls,
One similar to this one, but that does not allow in-place calls, and another
that is explicitly in-place and takes only one ChannelBuffer pointer.
That would allow the ProcessChunk method to change the number of channels.

[aluebs-webrtc, 2016/06/01 00:16:34]

I don't think that adding an additional interface is a good idea. It only
increases code complexity. But, modifying the ChannelBuffer behavior, we can set
the number of output channels to 1.

[peah-webrtc, 2016/06/01 14:51:01]

It actually does not add code complexity, as it seems that only one method is
needed by the current beamformer. And I don't think we should keep any code only
to support (possible) future beamformers. That code could instead then be added
when those are introduced.

[aluebs-webrtc, 2016/06/01 22:13:20]

Interface removed and changed to what input-only.

+           input.channels(i)[0],
+           input.num_frames_per_band() * sizeof(output->channels(i)[0][0]));
+  }
+}
+
+void NonlinearBeamformer::PostFilter(const ChannelBuffer<float>& input,
+                                     ChannelBuffer<float>* output) {
+  RTC_DCHECK_EQ(input.num_frames_per_band(), chunk_length_);
+
+  postfilter_transform_->ProcessChunk(input.channels(0), output->channels(0));
+
   // Ramp up/down for smoothing. 1 mask per 10ms results in audible

[peah-webrtc, 2016/05/22 21:06:48]

I guess, what you mean is that smoothing is needed in order to avoid
discontinuities in the transitions between 10 ms frames, right?. I.e., the
problem is not 1 mask per 10 ms, but rather that the transition between two
masks cannot be done instantaneously.

If the above holds, this comment is a bit misleading.

[aluebs-webrtc, 2016/05/26 01:04:45]

Adds unrelated changes to the CL, but if you think that is ok.
I find the comment to be easy to understand, having "1 [constant] mask per 10ms
results in audible discontinuities [in the transitions]". But if you find it
confusing, I am happy to change it. WDYT of the current one?

[peah-webrtc, 2016/05/30 11:49:25]

Sounds awesome!

   // discontinuities.
   const float ramp_increment =
-      (high_pass_postfilter_mask_ - old_high_pass_mask) /
+      (high_pass_postfilter_mask_ - old_high_pass_mask_) /
       input.num_frames_per_band();
-  // Apply the smoothed high-pass mask to the first channel of each band.
-  // This can be done because the effect of the linear beamformer is negligible
-  // compared to the post-filter.
   for (size_t i = 1; i < input.num_bands(); ++i) {
-    float smoothed_mask = old_high_pass_mask;
+    float smoothed_mask = old_high_pass_mask_;
     for (size_t j = 0; j < input.num_frames_per_band(); ++j) {
       smoothed_mask += ramp_increment;
       output->channels(i)[0][j] = input.channels(i)[0][j] * smoothed_mask;
@@ -456,7 +496,7 @@ void NonlinearBeamformer::ProcessAudioBlock(const complex_f* const* input,
   ApplyLowFrequencyCorrection();
   ApplyHighFrequencyCorrection();
   ApplyMaskFrequencySmoothing();
-  ApplyMasks(input, output);
+  ApplyDelayAndSum(input, output);
 }
 
 float NonlinearBeamformer::CalculatePostfilterMask(
@@ -484,8 +524,8 @@ float NonlinearBeamformer::CalculatePostfilterMask(
   return numerator / denominator;
 }
 
-void NonlinearBeamformer::ApplyMasks(const complex_f* const* input,
-                                     complex_f* const* output) {
+void NonlinearBeamformer::ApplyDelayAndSum(const complex_f* const* input,
+                                           complex_f* const* output) {
   complex_f* output_channel = output[0];
   for (size_t f_ix = 0; f_ix < kNumFreqBins; ++f_ix) {
     output_channel[f_ix] = complex_f(0.f, 0.f);
@@ -495,8 +535,13 @@ void NonlinearBeamformer::ApplyMasks(const complex_f* const* input,
     for (size_t c_ix = 0; c_ix < num_input_channels_; ++c_ix) {
       output_channel[f_ix] += input[c_ix][f_ix] * delay_sum_mask_els[c_ix];
     }
+  }
+}
 
-    output_channel[f_ix] *= kCompensationGain * final_mask_[f_ix];
+void NonlinearBeamformer::ApplyPostFilter(const complex_f* input,
+                                          complex_f* output) {
+  for (size_t f_ix = 0; f_ix < kNumFreqBins; ++f_ix) {
+    output[f_ix] = kCompensationGain * final_mask_[f_ix] * input[f_ix];
   }
 }