Dont always downsample to 16kHz in the reverse stream in APM

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 56 matching lines @@
     case AudioProcessing::kStereo:
       return false;
     case AudioProcessing::kMonoAndKeyboard:
     case AudioProcessing::kStereoAndKeyboard:
       return true;
   }
 
   assert(false);
   return false;
 }
+
+bool is_multi_band(int sample_rate_hz) {

[peah-webrtc, 2016/03/09 13:25:43]

I think the style guide warrants a different name
https://google.github.io/styleguide/cppguide.html#Function_Names.

What about MultiBandSignal()?

[aluebs-webrtc, 2016/03/09 14:27:48]

I thought 2 "==" and 1 "&&" could be considered "cheap". But I accept your
suggestion.

[peah-webrtc, 2016/03/10 06:59:24]

You are of course right about the cheapness. And having seen the new name in
action I actually preferred the former name. Please change back if you want.
Sorry for the mess!

[aluebs-webrtc, 2016/03/10 15:34:32]

Done.

+  return sample_rate_hz == AudioProcessing::kSampleRate32kHz ||
+         sample_rate_hz == AudioProcessing::kSampleRate48kHz;
+}
+
+int get_proc_rate(int input_rate, int output_rate) {

[peah-webrtc, 2016/03/09 13:25:43]

The term proc is currently quite ambiguous in APM. Inside APM there are two proc
formats  (fwd_proc_format and rev_proc_format). But then "proc" in the
num_proc_channels() refers only to the number of output channels.

I think it is good to at this stage as far as possible avoid using "proc" in
variable and function names as it is currently an ambiguous term in APM which
could lead to confusion.

[peah-webrtc, 2016/03/09 13:25:44]

I think the style guide warrants a different name (right?)
https://google.github.io/styleguide/cppguide.html#Function_Names.

Furthermore, I think the purpose of the method would be clearer if the min
operation was moved to the actual function call. That would also simplify the
naming.

E.g. (with a method name change to ClosestNativeRate())
  capture_nonlocked_.fwd_proc_format = 
StreamConfig(ClosestNativeRate(std::min(formats_.api_format.input_stream().sample_rate_hz(),
formats_.api_format.output_stream().sample_rate_hz());


A problem I have with the name proc_rate is that it is very unambiguous to me. I
have no intuitive feeling for what it does and always need to check it up.

[aluebs-webrtc, 2016/03/09 14:27:48]

"proc" always means "for processing" and both "fwd_proc_format" and
"num_proc_channels" mean that. I don't find it ambiguous, but yes slightly
confusing, since it depends on a few factors. But I am accepting your suggestion
below, so this should not be a problem.

[aluebs-webrtc, 2016/03/09 14:27:48]

In this case I agree that the definition of "cheap" was slightly stretched. And
I agree that moving the min out of the function might make things more
intuitive. Accepting your suggestion.

[peah-webrtc, 2016/03/10 06:59:24]

Nice!

Acknowledged.

+  // We process at the closest native rate >= min(input rate, output rate)...
+  const int min_proc_rate = std::min(input_rate, output_rate);
+  int proc_rate;
+  for (size_t i = 0; i < AudioProcessing::kNumNativeSampleRates; ++i) {

[hlundin-webrtc, 2016/03/09 12:10:57]

I suggest a re-write of the for loop:
for (int rate : AudioProcessing::kNativeSampleRatesHz) {
  if (rate >= min_proc_rate) {
    return rate;
  }
}
// Handle the case of ending the loop without finding a candidate
// either as an error or by returning 
// AudioProcessing::kMaxNativeSampleRateHz

[aluebs-webrtc, 2016/03/09 13:16:02]

I see that I am more sensitive than most when writing methods with more than one
return statement. But there is no excuse for not using the range for-loop
(except maybe that the definition of kNativeSampleRatesHz needed to be moved
above this). Anyway, I accepted your suggestion. And the correct thing to do on
end of loop is returning kMaxNativeSampleRateHz, which also is the same behavior
as originally, since it will downsample any higher sampling rates to the highest
supported one and still do the right thing.

[peah-webrtc, 2016/03/09 13:25:44]

I think this should be an example where an ArrayView could be used to simplify
the loop even further

for (auto rate : rtc::ArrayView<int>(AudioProcessing::kNativeSampleRatesHz)) {
if (rate > min_proc_rate) {
  return rate;
}

[peah-webrtc, 2016/03/09 13:31:18]

My error, Hlundins example is actually much better than the ArrayView example I
proposed as ArrayView is not needed at all. So I take back my comment.

+    proc_rate = AudioProcessing::kNativeSampleRatesHz[i];
+    if (proc_rate >= min_proc_rate) {
+      break;
+    }
+  }
+  return proc_rate;
+}
+
 }  // namespace
 
 // Throughout webrtc, it's assumed that success is represented by zero.
 static_assert(AudioProcessing::kNoError == 0, "kNoError must be zero");
 
 struct AudioProcessingImpl::ApmPublicSubmodules {
   ApmPublicSubmodules()
       : echo_cancellation(nullptr),
         echo_control_mobile(nullptr),
         gain_control(nullptr) {}
@@ skipping 270 matching lines @@
     return kBadNumberChannelsError;
   }
 
   if (capture_nonlocked_.beamformer_enabled &&
       num_in_channels != capture_.array_geometry.size()) {
     return kBadNumberChannelsError;
   }
 
   formats_.api_format = config;
 
-  // We process at the closest native rate >= min(input rate, output rate).
-  const int min_proc_rate =
-      std::min(formats_.api_format.input_stream().sample_rate_hz(),
-               formats_.api_format.output_stream().sample_rate_hz());
-  int fwd_proc_rate;
-  for (size_t i = 0; i < kNumNativeSampleRates; ++i) {
-    fwd_proc_rate = kNativeSampleRatesHz[i];
-    if (fwd_proc_rate >= min_proc_rate) {
-      break;
-    }
-  }
+  capture_nonlocked_.fwd_proc_format = StreamConfig(get_proc_rate(
+      formats_.api_format.input_stream().sample_rate_hz(),
+      formats_.api_format.output_stream().sample_rate_hz()));
 
-  capture_nonlocked_.fwd_proc_format = StreamConfig(fwd_proc_rate);
-
-  // We normally process the reverse stream at 16 kHz. Unless...
-  int rev_proc_rate = kSampleRate16kHz;
+  int rev_proc_rate = get_proc_rate(
+      formats_.api_format.reverse_input_stream().sample_rate_hz(),
+      formats_.api_format.reverse_output_stream().sample_rate_hz());
+  // If the forward sample rate is 8 kHz, the reverse stream is also processed
+  // at this rate.
   if (capture_nonlocked_.fwd_proc_format.sample_rate_hz() == kSampleRate8kHz) {
-    // ...the forward stream is at 8 kHz.
     rev_proc_rate = kSampleRate8kHz;
-  } else {
-    if (formats_.api_format.reverse_input_stream().sample_rate_hz() ==
-        kSampleRate32kHz) {
-      // ...or the input is at 32 kHz, in which case we use the splitting
-      // filter rather than the resampler.
-      rev_proc_rate = kSampleRate32kHz;
-    }
   }
 
   // Always downmix the reverse stream to mono for analysis. This has been
   // demonstrated to work well for AEC in most practical scenarios.
   formats_.rev_proc_format = StreamConfig(rev_proc_rate, 1);
 
   if (capture_nonlocked_.fwd_proc_format.sample_rate_hz() == kSampleRate32kHz ||
       capture_nonlocked_.fwd_proc_format.sample_rate_hz() == kSampleRate48kHz) {
     capture_nonlocked_.split_rate = kSampleRate16kHz;
   } else {
@@ skipping 238 matching lines @@
     debug_dump_.capture.event_msg->set_type(audioproc::Event::STREAM);
     audioproc::Stream* msg = debug_dump_.capture.event_msg->mutable_stream();
     const size_t data_size =
         sizeof(int16_t) * frame->samples_per_channel_ * frame->num_channels_;
     msg->set_input_data(frame->data_, data_size);
   }
 #endif
 
   capture_.capture_audio->DeinterleaveFrom(frame);
   RETURN_ON_ERR(ProcessStreamLocked());
-  capture_.capture_audio->InterleaveTo(frame,
-                                       output_copy_needed(is_data_processed()));
+  capture_.capture_audio->InterleaveTo(frame, output_copy_needed());
 
 #ifdef WEBRTC_AUDIOPROC_DEBUG_DUMP
   if (debug_dump_.debug_file->Open()) {
     audioproc::Stream* msg = debug_dump_.capture.event_msg->mutable_stream();
     const size_t data_size =
         sizeof(int16_t) * frame->samples_per_channel_ * frame->num_channels_;
     msg->set_output_data(frame->data_, data_size);
     RETURN_ON_ERR(WriteMessageToDebugFile(debug_dump_.debug_file.get(),
                                           &debug_dump_.num_bytes_left_for_log_,
                                           &crit_debug_, &debug_dump_.capture));
@@ skipping 19 matching lines @@
 
   AudioBuffer* ca = capture_.capture_audio.get();  // For brevity.
 
   if (constants_.use_experimental_agc &&
       public_submodules_->gain_control->is_enabled()) {
     private_submodules_->agc_manager->AnalyzePreProcess(
         ca->channels()[0], ca->num_channels(),
         capture_nonlocked_.fwd_proc_format.num_frames());
   }
 
-  bool data_processed = is_data_processed();
-  if (analysis_needed(data_processed)) {
+  if (analysis_needed()) {
     ca->SplitIntoFrequencyBands();
   }
 
   if (capture_nonlocked_.beamformer_enabled) {
     private_submodules_->beamformer->ProcessChunk(*ca->split_data_f(),
                                                   ca->split_data_f());
     ca->set_num_channels(1);
   }
 
   public_submodules_->high_pass_filter->ProcessCaptureAudio(ca);
@@ skipping 18 matching lines @@
   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);
   }
   RETURN_ON_ERR(public_submodules_->gain_control->ProcessCaptureAudio(ca));
 
-  if (synthesis_needed(data_processed)) {
+  if (synthesis_needed()) {
     ca->MergeFrequencyBands();
   }
 
   // TODO(aluebs): Investigate if the transient suppression placement should be
   // before or after the AGC.
   if (capture_.transient_suppressor_enabled) {
     float voice_probability =
         private_submodules_->agc_manager.get()
             ? private_submodules_->agc_manager->voice_probability()
             : 1.f;
@@ skipping 154 matching lines @@
                                           &debug_dump_.num_bytes_left_for_log_,
                                           &crit_debug_, &debug_dump_.render));
   }
 #endif
   render_.render_audio->DeinterleaveFrom(frame);
   return ProcessReverseStreamLocked();
 }
 
 int AudioProcessingImpl::ProcessReverseStreamLocked() {
   AudioBuffer* ra = render_.render_audio.get();  // For brevity.
-  if (formats_.rev_proc_format.sample_rate_hz() == kSampleRate32kHz) {
+  if (rev_analysis_needed()) {
     ra->SplitIntoFrequencyBands();
   }
 
   if (constants_.intelligibility_enabled) {
     // Currently run in single-threaded mode when the intelligibility
     // enhancer is activated.
     // TODO(peah): Fix to be properly multi-threaded.
     rtc::CritScope cs(&crit_capture_);
     public_submodules_->intelligibility_enhancer->ProcessRenderAudio(
         ra->split_channels_f(kBand0To8kHz), capture_nonlocked_.split_rate,
         ra->num_channels());
   }
 
   RETURN_ON_ERR(public_submodules_->echo_cancellation->ProcessRenderAudio(ra));
   RETURN_ON_ERR(
       public_submodules_->echo_control_mobile->ProcessRenderAudio(ra));
   if (!constants_.use_experimental_agc) {
     RETURN_ON_ERR(public_submodules_->gain_control->ProcessRenderAudio(ra));
   }
 
-  if (formats_.rev_proc_format.sample_rate_hz() == kSampleRate32kHz &&
-      is_rev_processed()) {
+  if (rev_synthesis_needed()) {
     ra->MergeFrequencyBands();
   }
 
   return kNoError;
 }
 
 int AudioProcessingImpl::set_stream_delay_ms(int delay) {
   rtc::CritScope cs(&crit_capture_);
   Error retval = kNoError;
   capture_.was_stream_delay_set = true;
@@ skipping 170 matching lines @@
   // from the returned pointer.
   return public_submodules_->noise_suppression.get();
 }
 
 VoiceDetection* AudioProcessingImpl::voice_detection() const {
   // Adding a lock here has no effect as it allows any access to the submodule
   // from the returned pointer.
   return public_submodules_->voice_detection.get();
 }
 
 bool AudioProcessingImpl::is_data_processed() const {

[peah-webrtc, 2016/03/09 13:25:43]

This is the counterpart to output_copy_needed(). If changing the name of that,
it would be good to change the name of this method as well at the same time.
Currently it is not possible to tell from the name that it only refers to
whether the CaptureStream has been modified.

[aluebs-webrtc, 2016/03/09 14:27:48]

Might make sense, but in another CL.

   // The beamformer, noise suppressor and highpass filter
   // modify the data.
   if (capture_nonlocked_.beamformer_enabled ||
       public_submodules_->high_pass_filter->is_enabled() ||
       public_submodules_->noise_suppression->is_enabled() ||
       public_submodules_->echo_cancellation->is_enabled()) {
     return true;
   }
 
   // All of the private submodules modify the data.
   for (auto item : private_submodules_->component_list) {
     if (item->is_component_enabled()) {
       return true;
     }
   }
 
   // The capture data is otherwise unchanged.
   return false;
 }
 
-bool AudioProcessingImpl::output_copy_needed(bool is_data_processed) const {
+bool AudioProcessingImpl::output_copy_needed() const {

[peah-webrtc, 2016/03/09 13:25:43]

The naming rule https://google.github.io/styleguide/cppguide.html#Function_Names
should apply here I think. 

Furthermore, the output in this method is only used on the render side to
determine if the render audio stream has been modified, right?

I think it would be good if the name reflects where it is used and what it is
asked. What about  ReverseStreamModified() or IsReverseStreamModified()?

[aluebs-webrtc, 2016/03/09 14:27:48]

I am pretty sure this naming was chosen assuming it is a "cheap" method.
Changing it might make sense, but in another CL.

[peah-webrtc, 2016/03/10 06:59:24]

Acknowledged.

   // Check if we've upmixed or downmixed the audio.
   return ((formats_.api_format.output_stream().num_channels() !=
            formats_.api_format.input_stream().num_channels()) ||
-          is_data_processed || capture_.transient_suppressor_enabled);
+          is_data_processed() || capture_.transient_suppressor_enabled);
 }
 
-bool AudioProcessingImpl::synthesis_needed(bool is_data_processed) const {
-  return (is_data_processed &&
-          (capture_nonlocked_.fwd_proc_format.sample_rate_hz() ==
-               kSampleRate32kHz ||
-           capture_nonlocked_.fwd_proc_format.sample_rate_hz() ==
-               kSampleRate48kHz));
+bool AudioProcessingImpl::synthesis_needed() const {

[peah-webrtc, 2016/03/09 13:25:43]

The naming rule https://google.github.io/styleguide/cppguide.html#Function_Names
should apply here I think. 

Furthermore, this is a bit misleading since rev_synthesis_needed refers to the
reverse stream. It would be good to specify that this method only refers to the
forward stream to avoid confusion.

What about ForwardStreamBandSynthesisNeeded() or ForwardBandSynthesisNeeded()

[aluebs-webrtc, 2016/03/09 14:27:48]

Again, I am pretty sure all these names were chosen because of the "cheap"
nature of the functions. And if we think they should change, we should change
all of them together in another CL. On the other hand, I will accept that adding
a "fwd_" to the name makes sense, since this CL introduces the "rev_" version of
it.

[peah-webrtc, 2016/03/10 06:59:24]

Acknowledged.

+  return (is_data_processed() &&
+          is_multi_band(capture_nonlocked_.fwd_proc_format.sample_rate_hz()));
 }
 
-bool AudioProcessingImpl::analysis_needed(bool is_data_processed) const {
-  if (!is_data_processed &&
+bool AudioProcessingImpl::analysis_needed() const {

[peah-webrtc, 2016/03/09 13:25:43]

As above, what about
What about ForwardStreamBandAnalysisNeeded() or ForwardBandAnalysisNeeded()

[aluebs-webrtc, 2016/03/09 14:27:48]

As above, if we change the naming of these functions I think we should do it all
in one CL, but I agree that it makes sense to add a "fwd_" to it.

[peah-webrtc, 2016/03/10 06:59:24]

Acknowledged.

+  if (!is_data_processed() &&
       !public_submodules_->voice_detection->is_enabled() &&
       !capture_.transient_suppressor_enabled) {
     // Only public_submodules_->level_estimator is enabled.
     return false;
-  } else if (capture_nonlocked_.fwd_proc_format.sample_rate_hz() ==
-                 kSampleRate32kHz ||
-             capture_nonlocked_.fwd_proc_format.sample_rate_hz() ==
-                 kSampleRate48kHz) {
+  } else if (is_multi_band(
+                 capture_nonlocked_.fwd_proc_format.sample_rate_hz())) {
     // Something besides public_submodules_->level_estimator is enabled, and we
     // have super-wb.
     return true;
   }
   return false;
 }
 
 bool AudioProcessingImpl::is_rev_processed() const {
   return constants_.intelligibility_enabled;
 }
 
+bool AudioProcessingImpl::rev_synthesis_needed() const {
+  return (is_rev_processed() &&
+          is_multi_band(formats_.rev_proc_format.sample_rate_hz()));

[peah-webrtc, 2016/03/09 13:25:43]

As above, what about
What about ReverseStreamBandSynthesisNeeded() or ReverseBandSynthesisNeeded()

[aluebs-webrtc, 2016/03/09 14:27:48]

I would want these to have names that are consistent with the ones already in
place. If we think we should rename, we can do this for all of them together in
another CL.

[peah-webrtc, 2016/03/10 06:59:24]

Acknowledged.

+}
+
+bool AudioProcessingImpl::rev_analysis_needed() const {

[peah-webrtc, 2016/03/09 13:25:43]

As above, what about
What about ReverseStreamBandAnalysisNeeded() or ReverseBandAnalysisNeeded()?

[aluebs-webrtc, 2016/03/09 14:27:48]

I would want these to have names that are consistent with the ones already in
place. If we think we should rename, we can do this for all of them together in
another CL.

[peah-webrtc, 2016/03/10 06:59:24]

Acknowledged.

+  return is_multi_band(formats_.rev_proc_format.sample_rate_hz());
+}
+
 bool AudioProcessingImpl::render_check_rev_conversion_needed() const {
   return rev_conversion_needed();
 }
 
 bool AudioProcessingImpl::rev_conversion_needed() const {
   return (formats_.api_format.reverse_input_stream() !=
           formats_.api_format.reverse_output_stream());
 }
 
 void AudioProcessingImpl::InitializeExperimentalAgc() {
@@ skipping 257 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