Pull out the PostFilter to its own NonlinearBeamformer API

webrtc/modules/audio_processing/audio_processing_impl.cc

 /*
  *  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 662 matching lines @@
     private_submodules_->agc_manager->AnalyzePreProcess(
         ca->channels()[0], ca->num_channels(),
         capture_nonlocked_.fwd_proc_format.num_frames());
   }
 
   if (fwd_analysis_needed()) {
     ca->SplitIntoFrequencyBands();
   }
 
   if (capture_nonlocked_.beamformer_enabled) {
     private_submodules_->beamformer->ProcessChunk(*ca->split_data_f(),

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

If the ProcessChunk is made input-only,  you should be able to pass
ca->split_bands_const_f here instead, right?

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

Because other Beamformers might do some processing (even all of it) in
ProcessChunk(), I think we should leave it as is.

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

Are there other beamformer implementations, apart from the one reviewed in this
CL, that could be called here?

I don't think it is a good idea to keep it like this, because it gives the
impression that the beamformer is actually changing the signal, which it is not.
Furthermore, it unnecessarily makes the APM sensitive to any bugs in the
beamformer implementation that accidentally changes the signal.

If there are other beamformer implementations that could actually change the
signal, they should not be allowed to be used together with the AEC, so in that
case it would make sense to place a DCHECK that the AEC is not activated if they
are active. 

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

There are no other beamformers in WebRTC at the moment, but there were plans to
support multiple different ones if there is a use case for them. If going from
stereo to mono is to modify the signal, it actually does modify it. And also,
other linear beamformers that actually change the signal here should totally be
allowed to be used with the AEC.

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

I don't think we should write/keep code to support possibly upcoming beamformer
solutions. That code can instead be (re-)added at that point.

You are correct that going from mono to stereo is a signal change, but that I
would then prefer to keep outside of the beamformer as it is more explicit. 
To me, downmixing from stereo to mono is not something that is that is the task
of a beamformer, in particular when this is the only signal modification done by
a beamformer.
Another drawback with that solution is that the downmixing unnecessarily forces
the rest of the audio pipeline to be mono. This is not at all needed and if this
restriction is imposed by the beamformer, the beamformer will need to be turned
off whenever a stereo signal is to be processed by the rest of the components.

I'm not sure what you mean with the linear beamformer and the AEC? Do you mean a
fixed linear beamformer? That I definitely agree should be fine, but until such
a beamformer is developed, I don't think it makes sense to keep/add code that
supports that. Adding that code is very easily done when that beamformer
solution is developed.

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

This whole CL is only to support "possibly upcoming AEC", since the current
setup seems to be working perfectly fine with today's overly aggressive AEC. I
also don't find it hard to follow your comments that are contradicting: should I
or should I not force the output ChannelBuffer to be mono?

                                                   ca->split_data_f());
     ca->set_num_channels(1);

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

Please add a comment that this operation discards all channels by the leftmost
one.

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

Done.

   }
 
   public_submodules_->high_pass_filter->ProcessCaptureAudio(ca);
   RETURN_ON_ERR(public_submodules_->gain_control->AnalyzeCaptureAudio(ca));
   public_submodules_->noise_suppression->AnalyzeCaptureAudio(ca);
 
   // Ensure that the stream delay was set before the call to the
   // AEC ProcessCaptureAudio function.
   if (public_submodules_->echo_cancellation->is_enabled() &&
       !was_stream_delay_set()) {
@@ skipping 19 matching lines @@
   // Ensure that the stream delay was set before the call to the
   // AECM ProcessCaptureAudio function.
   if (public_submodules_->echo_control_mobile->is_enabled() &&
       !was_stream_delay_set()) {
     return AudioProcessing::kStreamParameterNotSetError;
   }
 
   RETURN_ON_ERR(public_submodules_->echo_control_mobile->ProcessCaptureAudio(
       ca, stream_delay_ms()));
 
+  if (capture_nonlocked_.beamformer_enabled) {
+    private_submodules_->beamformer->PostFilter(ca->split_data_f());
+  }
+
   public_submodules_->voice_detection->ProcessCaptureAudio(ca);
 
   if (constants_.use_experimental_agc &&
       public_submodules_->gain_control->is_enabled() &&
       (!capture_nonlocked_.beamformer_enabled ||
        private_submodules_->beamformer->is_target_present())) {
     private_submodules_->agc_manager->Process(
         ca->split_bands_const(0)[kBand0To8kHz], ca->num_frames_per_band(),
         capture_nonlocked_.split_rate);
   }
@@ skipping 726 matching lines @@
   debug_dump_.capture.event_msg->mutable_config()->CopyFrom(config);
 
   RETURN_ON_ERR(WriteMessageToDebugFile(debug_dump_.debug_file.get(),
                                         &debug_dump_.num_bytes_left_for_log_,
                                         &crit_debug_, &debug_dump_.capture));
   return kNoError;
 }
 #endif  // WEBRTC_AUDIOPROC_DEBUG_DUMP
 
 }  // namespace webrtc