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

webrtc/modules/audio_processing/audio_processing_impl.cc

Index: webrtc/modules/audio_processing/audio_processing_impl.cc
diff --git a/webrtc/modules/audio_processing/audio_processing_impl.cc b/webrtc/modules/audio_processing/audio_processing_impl.cc
index 66123962c3a87468357b4b006665bd8b6ea6d02d..e4f62de906a3adc230103ea4badfb317294ec1bf 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -74,6 +74,25 @@ static bool LayoutHasKeyboard(AudioProcessing::ChannelLayout layout) {
   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.
@@ -364,32 +383,17 @@ int AudioProcessingImpl::InitializeLocked(const ProcessingConfig& config) {
 
   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(fwd_proc_rate);
+  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()));
 
-  // 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
@@ -648,8 +652,7 @@ int AudioProcessingImpl::ProcessStream(AudioFrame* frame) {
 
   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()) {
@@ -689,8 +692,7 @@ int AudioProcessingImpl::ProcessStreamLocked() {
         capture_nonlocked_.fwd_proc_format.num_frames());
   }
 
-  bool data_processed = is_data_processed();
-  if (analysis_needed(data_processed)) {
+  if (analysis_needed()) {
     ca->SplitIntoFrequencyBands();
   }
 
@@ -729,7 +731,7 @@ int AudioProcessingImpl::ProcessStreamLocked() {
   }
   RETURN_ON_ERR(public_submodules_->gain_control->ProcessCaptureAudio(ca));
 
-  if (synthesis_needed(data_processed)) {
+  if (synthesis_needed()) {
     ca->MergeFrequencyBands();
   }
 
@@ -904,7 +906,7 @@ int AudioProcessingImpl::AnalyzeReverseStream(AudioFrame* frame) {
 
 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();
   }
 
@@ -925,8 +927,7 @@ int AudioProcessingImpl::ProcessReverseStreamLocked() {
     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();
   }
 
@@ -1138,31 +1139,26 @@ bool AudioProcessingImpl::is_data_processed() const {
   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;
@@ -1174,6 +1170,15 @@ 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();
 }