Surface the IntelligibilityEnhancer on MediaConstraints

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 d89cc339315c5bf6b76cda4334eff5dbe8137121..c9fec0f577be91999449c9f8213e83ce8d4dc025 100644
--- a/webrtc/modules/audio_processing/audio_processing_impl.cc
+++ b/webrtc/modules/audio_processing/audio_processing_impl.cc
@@ -163,12 +163,10 @@ AudioProcessingImpl::AudioProcessingImpl(const Config& config,
       private_submodules_(new ApmPrivateSubmodules(beamformer)),
       constants_(config.Get<ExperimentalAgc>().startup_min_volume,
 #if defined(WEBRTC_ANDROID) || defined(WEBRTC_IOS)
-                 false,
+                 false),
 #else
-                 config.Get<ExperimentalAgc>().enabled,
+                 config.Get<ExperimentalAgc>().enabled),
 #endif
-                 config.Get<Intelligibility>().enabled),
-
 #if defined(WEBRTC_ANDROID) || defined(WEBRTC_IOS)
       capture_(false,
 #else
@@ -176,7 +174,8 @@ AudioProcessingImpl::AudioProcessingImpl(const Config& config,
 #endif
                config.Get<Beamforming>().array_geometry,
                config.Get<Beamforming>().target_direction),
-      capture_nonlocked_(config.Get<Beamforming>().enabled)
+      capture_nonlocked_(config.Get<Beamforming>().enabled,
+                         config.Get<Intelligibility>().enabled)
 {
   {
     rtc::CritScope cs_render(&crit_render_);
@@ -411,6 +410,13 @@ void AudioProcessingImpl::SetExtraOptions(const Config& config) {
     InitializeTransient();
   }
 
+  if(capture_nonlocked_.intelligibility_enabled !=
+     config.Get<Intelligibility>().enabled) {
+    capture_nonlocked_.intelligibility_enabled =
+        config.Get<Intelligibility>().enabled;
+    InitializeIntelligibility();

[tommi, 2016/05/11 06:06:48]

Don't you need to check first if the value is true?

Also, is the above copy/pasted code possibly buggy?

Is the initialization thread safe? 

Is it global or will it only affect the current instance of AudioProcessingImpl?

[aluebs-webrtc, 2016/05/11 21:22:05]

The check if the value is true is done inside InitializeIntelligibility().

I am familiar with this code and I am quite sure it is not buggy, but please let
me know if you see something wrong with it.

The initialization is thread safe since SetExtraOptions takes both cs_render and
cs_capture.

This only affects the current instance of AudioProcessingImpl, like all the
settings in it.

[tommi, 2016/05/12 07:06:38]

OK. It is confusing that if the value is false, we call Initialize and _then_
check the value. I would rather expect that there be only a dcheck inside
Initialize that confirms that the feature is enabled and force the caller to
only call the method when it should be called. 

[aluebs-webrtc, 2016/05/13 02:04:38]

The older components (that use to inherit from ProcessingComponent) were forced
to have a Initialize method that would re-allocate all the internal memory and
re-initialize the internal state. This was bug prone, since it had to maintain
this method on top of the constructor. In the newer components we decided to
just re-create the whole component itself since it is simpler and more scalable.
Older components are always created at construction time, whether they are
enabled or not. And when Initializing them they re-allocate their internal
memory whether they are enabled or not. On the other hand, newer components are
not necessarily created at construction time, but only if/when enabled. But all
Initialize*() methods in APM (there are 11, one for each component) are called
without checking if the component is enabled. I think there is definitively room
for improvement, but I think we should do it in a consistent way and this is not
the right CL for it. WDYT?

+  }
+
 #ifdef WEBRTC_ANDROID_PLATFORM_BUILD
   if (capture_nonlocked_.beamformer_enabled !=
           config.Get<Beamforming>().enabled) {
@@ -704,7 +710,7 @@ int AudioProcessingImpl::ProcessStreamLocked() {
     ca->CopyLowPassToReference();
   }
   public_submodules_->noise_suppression->ProcessCaptureAudio(ca);
-  if (constants_.intelligibility_enabled) {
+  if (capture_nonlocked_.intelligibility_enabled) {
     RTC_DCHECK(public_submodules_->noise_suppression->is_enabled());
     RTC_DCHECK(public_submodules_->gain_control->is_enabled());
     public_submodules_->intelligibility_enhancer->SetCaptureNoiseEstimate(
@@ -902,7 +908,7 @@ int AudioProcessingImpl::ProcessReverseStreamLocked() {
     ra->SplitIntoFrequencyBands();
   }
 
-  if (constants_.intelligibility_enabled) {
+  if (capture_nonlocked_.intelligibility_enabled) {
     public_submodules_->intelligibility_enhancer->ProcessRenderAudio(
         ra->split_channels_f(kBand0To8kHz), capture_nonlocked_.split_rate,
         ra->num_channels());
@@ -1150,7 +1156,7 @@ bool AudioProcessingImpl::fwd_analysis_needed() const {
 }
 
 bool AudioProcessingImpl::is_rev_processed() const {
-  return constants_.intelligibility_enabled;
+  return capture_nonlocked_.intelligibility_enabled;
 }
 
 bool AudioProcessingImpl::rev_synthesis_needed() const {
@@ -1215,7 +1221,7 @@ void AudioProcessingImpl::InitializeBeamformer() {
 }
 
 void AudioProcessingImpl::InitializeIntelligibility() {
-  if (constants_.intelligibility_enabled) {

[tommi, 2016/05/12 07:06:38]

This should be a dcheck and the function should do as the functions below do. 

[aluebs-webrtc, 2016/05/13 02:04:38]

Please see explanation above.

+  if (capture_nonlocked_.intelligibility_enabled) {
     public_submodules_->intelligibility_enhancer.reset(
         new IntelligibilityEnhancer(capture_nonlocked_.split_rate,
                                     render_.render_audio->num_channels(),