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 !=

[peah-webrtc, 2016/05/05 18:49:41]

Could you please explain more why it is desired to move the activation of IE
from the constructor to SetExtraOptions? I was not part of the discussions
mentioned so I don't know the background.

In general I think SetExtraOptions is something we should deprecate ASAP and
instead do all the configuration at creation-time so I'd definitely prefer to
have the activation of IE at the APM creation rather than using SetExtraOptions.

[aluebs-webrtc, 2016/05/05 21:53:05]

It was not moved from the constructor. Just added the possibility to enable it
through SetExtraOptions. And I agree we should eventually deprecate this method
and do all configuration at creation-time, but unfortunately this is not how the
VoiceEngine works today. Probably solenberg can comment on this. The behavior of
the IE would be similar to the possibility to ask for a pointer to certain
component (AEC for example) and call the Enable method on it dynamically.

[peah-webrtc, 2016/05/05 22:13:18]

Yes, you are right, it is still activated in the constructor since the
constructor calls SetExtraOptions. The ability to activate this outside of the
constructor was my main concern though. I know that it is allowed to activate
other components this way, via the enable methods and the SetExtraOptions
method. But that I think is something we'll move away from as soon as we are
able to and therefore I think we should try hard not to include the activation
of anything else this way.

Regarding that, could you please explain why it is not possible to activate the
IE at construction time. Is the reason VoE?

[the sun, 2016/05/06 09:00:16]

Different processing can be enabled/disabled depending on options
selected/negotiated after the VoE has been constructed (the APM is constructed
at same time by VoE). The VoE could potentially recreate the APM instead when
options change and we wouldn't need this.

[aluebs-webrtc, 2016/05/06 16:27:10]

It would be awesome to move to recreating the APM when the options change, but I
don't think this CL is the right place to do it. And I think that having the
possibility of enabling IE in SetExtraOptions (as other components already are)
doesn't make it harder to move away from it.

[peah-webrtc, 2016/05/12 08:05:12]

What I meant was that we should be able to activate the IE at creation-time,
similarly to how the beamformer is activated, and similarly to how IE was
created before.

Does this CL really require the activation of IE to be done during runtime
instead of at creation?

To clarify, with this I don't mean that the APM should be recreated every time
an option change (that is for sure something for another CL). What I mean is
that I'm concerned that this CL changes the way that IE is activated/created
from being done at creation-time to being done at runtime. This is something
that we should go at lengths not do do.

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

The Beamformer can be activated only at creation time, because it is only used
in Chrome, which creates their own APM (doesn't use the VoiceEngine's one) and
sets all configs at creation time. The IE could only be activated at creation
time, because it was not activated anywhere. But because the main use case for
the IE is mobile we require it to be activated from the VoiceEngine, which today
activates components (calling Enable methods of other components for example)
during run time. Once the VoiceEngine migrates to re-creating the APM with new
options I totally agree we should drop SetExtraOptions and the possibility of
accessing the components directly for that matter.

[peah-webrtc, 2016/05/13 12:39:49]

Thanks, then I see the problem. 

+     config.Get<Intelligibility>().enabled) {
+    capture_nonlocked_.intelligibility_enabled =
+        config.Get<Intelligibility>().enabled;
+    InitializeIntelligibility();
+  }
+
 #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) {
+  if (capture_nonlocked_.intelligibility_enabled) {
     public_submodules_->intelligibility_enhancer.reset(
         new IntelligibilityEnhancer(capture_nonlocked_.split_rate,
                                     render_.render_audio->num_channels(),