Only split into bands when the reverse stream is analyzed in the 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 a09c1c9acf3362f53cbd9f968ad5a4b6a5cf0932..c816aaa1d415322c98905760b60b5196247c6891 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -1146,13 +1146,21 @@ bool AudioProcessingImpl::is_rev_processed() const {
   return constants_.intelligibility_enabled;
 }
 
+bool AudioProcessingImpl::is_rev_analyzed() const {
+  return is_rev_processed() ||

[peah-webrtc, 2016/03/30 13:01:11]

As I see it, the purpose of this method is to avoid having to do reverse stream
synthesis when it is not necessary, right? That is a great change indeed.

I think this is an excellent time to simplify this scheme.

Currently we have:
-fwd_analysis_needed()
-fwd_synthesis_needed()
-rev_synthesis_needed()
-rev_analysis_needed()
-is_data_processed()
-output_copy_needed()
-is_rev_processed() 
-is_rev_analyzed() 
Which basically all have the purpose of determining whether 
-an output copy is needed for the stream, 
-whether a synthesis needs to be done.
-whether an analysis needs to be done.

It should be possible to simplify this quite a lot. 
For instance:
-It would make sense to have matching name to their reverse stream counterparts
for the methods:
--is_data_processed()
--output_copy_needed() 
-The methods rev_analysis_needed() and is_rev_analyzed() do different things,
but have names which indicate that they are exactly the same thing (there should
be no case where the reverse stream has not been analyzed even though it has
been flagged that it needs to be analyzed).
-is_rev_processed indicates that there is only reverse side processing being
done if that is true. That is, however, not really the case as the
band-splitting filter merge needed for the other components also modifies the
stream. Therefore it would make sense to bundle these methods into one method.

[aluebs-webrtc, 2016/03/31 01:00:18]

No, this change is to avoid doing reverse stream analysis when it is not
necessary.
I think that rev_analysis_needed and is_rev_analyzed were easier to read in
different functions, but the naming was probably chosen poorly by me. Anyway, I
unified them, since the new function was only used in one place.
I definitively agree that we can simplify these functions, but I am hesitant to
mix that kind of refactoring into a CL that is supposed to change the behavior,
like this one. Although renaming I think is ok, there is no reverse counterpart
to output_copy_needed, because it is not as simple as you describe it.
Unfortunately, because the AnalyzeReverseStream API is not deprecated yet, it we
need to keep track manually of the output_copy and conversions needed.
is_rev_processed indicates there is reverse processing done, but not necessarily
only processing or not only in the reverse stream. And the merging would not
happen if no processing was done, so it still holds up.

+         public_submodules_->echo_cancellation->is_enabled() ||
+         public_submodules_->echo_control_mobile->is_enabled() ||
+         public_submodules_->gain_control->is_enabled();
+}
+
 bool AudioProcessingImpl::rev_synthesis_needed() const {
   return (is_rev_processed() &&
           is_multi_band(formats_.rev_proc_format.sample_rate_hz()));
 }
 
 bool AudioProcessingImpl::rev_analysis_needed() const {
-  return is_multi_band(formats_.rev_proc_format.sample_rate_hz());
+  return is_rev_analyzed() &&
+         is_multi_band(formats_.rev_proc_format.sample_rate_hz());
 }
 
 bool AudioProcessingImpl::render_check_rev_conversion_needed() const {