Use ProcessReverseStream in VoiceEngines OutputMixer

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 424 matching lines @@
     capture_nonlocked_.beamformer_enabled = config.Get<Beamforming>().enabled;
     if (config.Get<Beamforming>().array_geometry.size() > 1) {
       capture_.array_geometry = config.Get<Beamforming>().array_geometry;
     }
     capture_.target_direction = config.Get<Beamforming>().target_direction;
     InitializeBeamformer();
   }
 #endif  // WEBRTC_ANDROID_PLATFORM_BUILD
 }
 
-int AudioProcessingImpl::input_sample_rate_hz() const {
-  // Accessed from outside APM, hence a lock is needed.
-  rtc::CritScope cs(&crit_capture_);
-  return formats_.api_format.input_stream().sample_rate_hz();
-}
-
 int AudioProcessingImpl::proc_sample_rate_hz() const {
   // Used as callback from submodules, hence locking is not allowed.
   return capture_nonlocked_.fwd_proc_format.sample_rate_hz();
 }
 
 int AudioProcessingImpl::proc_split_sample_rate_hz() const {
   // Used as callback from submodules, hence locking is not allowed.
   return capture_nonlocked_.split_rate;
 }
 
@@ skipping 314 matching lines @@
 
 int AudioProcessingImpl::ProcessReverseStream(
     const float* const* src,
     const StreamConfig& reverse_input_config,
     const StreamConfig& reverse_output_config,
     float* const* dest) {
   TRACE_EVENT0("webrtc", "AudioProcessing::ProcessReverseStream_StreamConfig");
   rtc::CritScope cs(&crit_render_);
   RETURN_ON_ERR(AnalyzeReverseStreamLocked(src, reverse_input_config,
                                            reverse_output_config));
   if (is_rev_processed()) {

[the sun, 2016/03/10 12:49:22]

Remove this function:
- The name uses an ambiguous abbreviation.
- The plain "constants_.intelligibility_enabled" is used in more places than
this function, for the same purpose.

[aluebs-webrtc, 2016/03/10 16:33:17]

I still would argue that having a method would make it easier to refactor when
we add other components in the render stream that modify the signal. Also, I
can't find any place where the plain constant is used for the purpose to check
if the signal has been processed. We could argue about the naming, but if you
still think it should be removed I am happy to do so in another CL.

     render_.render_audio->CopyTo(formats_.api_format.reverse_output_stream(),
                                  dest);
   } else if (render_check_rev_conversion_needed()) {

[the sun, 2016/03/10 12:49:22]

Remove this function:
- It does the exact same thing as rev_conversion_needed()
- It is used in just one place.

Rename rev_conversion_needed() while you're at it.

[aluebs-webrtc, 2016/03/10 16:33:17]

Here you have a great point, but I don't think I should do the change in the CL,
since it has nothing to do with it. But happy to do so in a follow-up CL.

     render_.render_converter->Convert(src, reverse_input_config.num_samples(),
                                       dest,
                                       reverse_output_config.num_samples());
   } else {
     CopyAudioIfNeeded(src, reverse_input_config.num_frames(),
                       reverse_input_config.num_channels(), dest);
   }
 
   return kNoError;
 }
@@ skipping 29 matching lines @@
          i < formats_.api_format.reverse_input_stream().num_channels(); ++i)
       msg->add_channel(src[i], channel_size);
     RETURN_ON_ERR(WriteMessageToDebugFile(debug_dump_.debug_file.get(),
                                           &debug_dump_.num_bytes_left_for_log_,
                                           &crit_debug_, &debug_dump_.render));
   }
 #endif
 
   render_.render_audio->CopyFrom(src,
                                  formats_.api_format.reverse_input_stream());
   return ProcessReverseStreamLocked();

[the sun, 2016/03/10 12:49:22]

Wow! So AnalyzeReverseStreamLocked() can have side effects? It isn't purely for
analyzing?

[aluebs-webrtc, 2016/03/10 16:33:17]

As discussed offline, it doesn't, because it doesn't copy the result into the
AudioFrame as ProcessReverseStream does. But I agree it can be confusing.

 }
 
 int AudioProcessingImpl::ProcessReverseStream(AudioFrame* frame) {
   TRACE_EVENT0("webrtc", "AudioProcessing::ProcessReverseStream_AudioFrame");
   RETURN_ON_ERR(AnalyzeReverseStream(frame));
   rtc::CritScope cs(&crit_render_);

[the sun, 2016/03/11 09:49:19]

You should take this lock before calling AnalyzeReverseStream(), otherwise you
don't know that the data you're copying out hasn't been touched by someone else.

That is, IF you need to take a lock instead of using a thread checker in the
"reverse" methods.

[aluebs-webrtc, 2016/03/11 13:31:03]

But AnalyzeReverseStream is taking this lock as well, so that would be a
problem. I see the issue here, but that will be fixed in my next CL where I
deprecate AnalyzeReverseStream and consolidate this in one method.

   if (is_rev_processed()) {
     render_.render_audio->InterleaveTo(frame, true);
   }
 
   return kNoError;
 }
 
 int AudioProcessingImpl::AnalyzeReverseStream(AudioFrame* frame) {
   TRACE_EVENT0("webrtc", "AudioProcessing::AnalyzeReverseStream_AudioFrame");
   rtc::CritScope cs(&crit_render_);
@@ skipping 587 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