Surface the IntelligibilityEnhancer on MediaConstraints

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 145 matching lines @@
 
 AudioProcessingImpl::AudioProcessingImpl(const Config& config)
     : AudioProcessingImpl(config, nullptr) {}
 
 AudioProcessingImpl::AudioProcessingImpl(const Config& config,
                                          Beamformer<float>* beamformer)
     : public_submodules_(new ApmPublicSubmodules()),
       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
       capture_(config.Get<ExperimentalNs>().enabled,
 #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_);
     rtc::CritScope cs_capture(&crit_capture_);
 
     public_submodules_->echo_cancellation.reset(
         new EchoCancellationImpl(&crit_render_, &crit_capture_));
     public_submodules_->echo_control_mobile.reset(
         new EchoControlMobileImpl(&crit_render_, &crit_capture_));
     public_submodules_->gain_control.reset(
@@ skipping 214 matching lines @@
 
   public_submodules_->echo_cancellation->SetExtraOptions(config);
 
   if (capture_.transient_suppressor_enabled !=
       config.Get<ExperimentalNs>().enabled) {
     capture_.transient_suppressor_enabled =
         config.Get<ExperimentalNs>().enabled;
     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) {
     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();
   }
@@ skipping 273 matching lines @@
   }
 
   RETURN_ON_ERR(public_submodules_->echo_cancellation->ProcessCaptureAudio(
       ca, stream_delay_ms()));
 
   if (public_submodules_->echo_control_mobile->is_enabled() &&
       public_submodules_->noise_suppression->is_enabled()) {
     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(
         public_submodules_->noise_suppression->NoiseEstimate(),
         public_submodules_->gain_control->compression_gain_db());
   }
 
   // Ensure that the stream delay was set before the call to the
   // AECM ProcessCaptureAudio function.
   if (public_submodules_->echo_control_mobile->is_enabled() &&
@@ skipping 177 matching lines @@
   }
   return kNoError;
 }
 
 int AudioProcessingImpl::ProcessReverseStreamLocked() {
   AudioBuffer* ra = render_.render_audio.get();  // For brevity.
   if (rev_analysis_needed()) {
     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());
   }
 
   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));
@@ skipping 227 matching lines @@
   } 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;
+  return capture_nonlocked_.intelligibility_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()) &&
          (is_rev_processed() ||
@@ skipping 44 matching lines @@
     if (!private_submodules_->beamformer) {
       private_submodules_->beamformer.reset(new NonlinearBeamformer(
           capture_.array_geometry, capture_.target_direction));
     }
     private_submodules_->beamformer->Initialize(kChunkSizeMs,
                                                 capture_nonlocked_.split_rate);
   }
 }
 
 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(),
                                     NoiseSuppressionImpl::num_noise_bins()));
   }
 }
 
 void AudioProcessingImpl::InitializeHighPassFilter() {
   public_submodules_->high_pass_filter->Initialize(num_proc_channels(),
                                                    proc_sample_rate_hz());
@@ skipping 232 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