Remove usage of VoEAudioProcessing from WVoE/MC.

webrtc/media/engine/webrtcvoiceengine.cc

Index: webrtc/media/engine/webrtcvoiceengine.cc
diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc
index f96c8e741e04ad94075ad69add97fab351230e01..6cca08ee5c3c700056805d7f699397156bccfc31 100644
--- a/webrtc/media/engine/webrtcvoiceengine.cc
+++ b/webrtc/media/engine/webrtcvoiceengine.cc
@@ -40,6 +40,7 @@
 #include "webrtc/modules/audio_processing/include/audio_processing.h"
 #include "webrtc/system_wrappers/include/field_trial.h"
 #include "webrtc/system_wrappers/include/trace.h"
+#include "webrtc/voice_engine/transmit_mixer.h"
 
 namespace cricket {
 namespace {
@@ -559,7 +560,159 @@ rtc::Optional<int> ComputeSendBitrate(int max_send_bitrate_bps,
   return rtc::Optional<int>(codec_rate);
 }
 
-}  // namespace {
+} // namespace {

[hlundin-webrtc, 2017/02/13 21:03:57]

Why the extra '{'?
I think it fooled the automatic formatter to remove one space between } and '}'
'//'.

[the sun, 2017/02/13 23:34:47]

Done.

+
+namespace apm_helpers {
+
+webrtc::AgcConfig GetAgcConfig(webrtc::AudioProcessing* apm) {
+  RTC_DCHECK(apm);
+  webrtc::AgcConfig result;
+  result.targetLeveldBOv = apm->gain_control()->target_level_dbfs();
+  result.digitalCompressionGaindB = apm->gain_control()->compression_gain_db();
+  result.limiterEnable = apm->gain_control()->is_limiter_enabled();
+  return result;
+}
+
+void SetAgcConfig(webrtc::AudioProcessing* apm,

[hlundin-webrtc, 2017/02/13 21:03:57]

Swap order of parameters; input before input/output.

[the sun, 2017/02/13 23:34:47]

<bikeshed>
Hm.
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering
"This is not a hard and fast rule"

In this context the apm is the object we're operating on, a.k.a. "this", which
is why it feels more natural for me to let it be the first parameter. It's also
common among these helper functions, so keeping it the first parameter makes it
easy to see they belong together.
</bikeshed>

[hlundin-webrtc, 2017/02/14 07:50:43]

I accept that. But this bikeshedding increased my total comment-count on this CL
quite dramatically... :)

+                  const webrtc::AgcConfig& config) {
+  RTC_DCHECK(apm);
+  webrtc::GainControl* gc = apm->gain_control();
+  if (gc->set_target_level_dbfs(config.targetLeveldBOv) != 0) {
+    LOG(LS_ERROR) << "Failed to set target level: " << config.targetLeveldBOv;
+  }
+  if (gc->set_compression_gain_db(config.digitalCompressionGaindB) != 0) {
+    LOG(LS_ERROR) << "Failed to set compression gain: "
+                  << config.digitalCompressionGaindB;
+  }
+  if (gc->enable_limiter(config.limiterEnable) != 0) {
+    LOG(LS_ERROR) << "Failed to set limiter on/off: " << config.limiterEnable;
+  }
+}
+
+void SetEcStatus(webrtc::AudioProcessing* apm,

[hlundin-webrtc, 2017/02/13 21:03:57]

apm last

[the sun, 2017/02/13 23:34:46]

Acknowledged.

+                 bool enable,
+                 webrtc::EcModes mode) {
+  RTC_DCHECK(apm);
+  RTC_DCHECK(mode == webrtc::kEcConference || mode == webrtc::kEcAecm);
+  webrtc::EchoCancellation* ec = apm->echo_cancellation();
+  webrtc::EchoControlMobile* ecm = apm->echo_control_mobile();
+  if (mode == webrtc::kEcConference) {
+    // Disable the AECM before enable the AEC

[hlundin-webrtc, 2017/02/13 21:03:57]

enable -> enabling

[the sun, 2017/02/13 23:34:47]

Done.

+    if (enable && ecm->is_enabled() && ecm->Enable(false) != 0) {

[hlundin-webrtc, 2017/02/13 21:03:57]

If we set mode=webrtc::kEcConference and enable=false, we may end up with AEC
Mobile being (still) enabled. Is this desired?

[the sun, 2017/02/13 23:34:47]

I've tried to faithfully replicate how this method works (but only keeping the
logic we actually execute):
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/voice_engin...
It isn't entirely out of the question that I've got it wrong, but I think this
is what the code used to do.

Eventually this should be configurable through the APM::Config. How about we try
to maintain the current logic in this CL, then try and refine things in a
separate one (I know peah@ is itching to work this over :)?

+      LOG(LS_ERROR) << "Failed to disable AECM.";
+      return;
+    }
+    if (ec->Enable(enable) != 0) {
+      LOG(LS_ERROR) << "Failed to enable/disable AEC: " << enable;
+      return;
+    }
+    if (ec->set_suppression_level(webrtc::EchoCancellation::kHighSuppression)
+        != 0) {
+      LOG(LS_ERROR) << "Failed to set high AEC aggressiveness.";
+      return;
+    }
+  } else {
+    // Disable the AEC before enable the AECM

[hlundin-webrtc, 2017/02/13 21:03:57]

enable -> enabling

[the sun, 2017/02/13 23:34:47]

Done.

+    if (enable && ec->is_enabled() && ec->Enable(false) != 0) {

[hlundin-webrtc, 2017/02/13 21:03:57]

Similar snag here as above.

[the sun, 2017/02/13 23:34:47]

Acknowledged.

+      LOG(LS_ERROR) << "Failed to disable AEC.";
+      return;
+    }
+    if (ecm->Enable(enable) != 0) {
+      LOG(LS_ERROR) << "Failed to enable/disable AECM: " << enable;
+      return;
+    }
+  }
+  LOG(LS_INFO) << "Echo control set to " << enable << " with mode " << mode;
+}
+
+void SetEcMetricsStatus(webrtc::AudioProcessing* apm, bool enable) {

[hlundin-webrtc, 2017/02/13 21:03:57]

Swap parameter order.

[the sun, 2017/02/13 23:34:47]

Acknowledged.

+  RTC_DCHECK(apm);
+  if ((apm->echo_cancellation()->enable_metrics(enable) != 0) ||
+      (apm->echo_cancellation()->enable_delay_logging(enable) != 0)) {
+    LOG(LS_ERROR) << "Failed to enable/disable EC metrics: " << enable;
+  }
+}
+
+void SetAecmMode(webrtc::AudioProcessing* apm, bool enable_cng) {

[hlundin-webrtc, 2017/02/13 21:03:57]

Swap parameter order.

[the sun, 2017/02/13 23:34:47]

Acknowledged.

+  RTC_DCHECK(apm);
+  webrtc::EchoControlMobile* ecm = apm->echo_control_mobile();
+  if (ecm->set_routing_mode(webrtc::EchoControlMobile::kSpeakerphone) != 0) {
+    LOG(LS_ERROR) << "Failed to set AECM mode kSpeakerphone.";
+    return;
+  }
+  if (ecm->enable_comfort_noise(enable_cng) != 0) {
+      LOG(LS_ERROR) << "Failed to enable/disable CNG: " << enable_cng;
+    return;
+  }
+}
+
+void SetAgcStatus(webrtc::AudioProcessing* apm,

[hlundin-webrtc, 2017/02/13 21:03:57]

Input before output.

[the sun, 2017/02/13 23:34:47]

Acknowledged.

+                  webrtc::AudioDeviceModule* adm,
+                  bool enable,
+                  webrtc::AgcModes mode) {
+  RTC_DCHECK(apm);
+  RTC_DCHECK(adm);
+  RTC_DCHECK(mode == webrtc::kAgcFixedDigital ||
+             mode == webrtc::kAgcAdaptiveAnalog);
+#if defined(WEBRTC_IOS) || defined(ATA) || defined(WEBRTC_ANDROID)

[hlundin-webrtc, 2017/02/13 21:03:57]

Are these defined to either 1 or 0? If so, skip the defined.

[the sun, 2017/02/13 23:34:46]

defined(nn) is used in other parts of the code.

Do you have any idea what "ATA" is? I've just copied from
voe_audio_processing_impl.cc

+  RTC_DCHECK(mode == webrtc::kAgcFixedDigital);

[hlundin-webrtc, 2017/02/13 21:03:57]

RTC_DCHECK_EQ

[the sun, 2017/02/13 23:34:47]

Done.

+  webrtc::GainControl::Mode agc_mode = webrtc::GainControl::kFixedDigital;
+#else
+  RTC_DCHECK(mode == webrtc::kAgcAdaptiveAnalog);

[hlundin-webrtc, 2017/02/13 21:03:57]

RTC_DCHECK_EQ

[the sun, 2017/02/13 23:34:47]

Done.

+  webrtc::GainControl::Mode agc_mode = webrtc::GainControl::kAdaptiveAnalog;
+#endif
+  webrtc::GainControl* gc = apm->gain_control();
+  if (gc->set_mode(agc_mode) != 0) {
+    LOG(LS_ERROR) << "Failed to set AGC mode: " << agc_mode;
+    return;
+  }
+  if (gc->Enable(enable) != 0) {
+    LOG(LS_ERROR) << "Failed to enable/disable AGC: " << enable;
+    return;
+  }
+  if (agc_mode != webrtc::GainControl::kFixedDigital) {
+    // Set Agc state in the ADM when adaptive Agc mode has been selected.
+    // Note that we also enable the ADM Agc when Adaptive Digital mode is
+    // used since we want to be able to provide the APM with updated mic
+    // levels when the user modifies the mic level manually.
+    if (adm->SetAGC(enable) != 0) {
+      LOG(LS_ERROR) << "Failed to set AGC mode in ADM: " << enable;
+      return;
+    }
+  }
+  LOG(LS_INFO) << "AGC set to " << enable << " with mode " << mode;
+}
+
+void SetNsStatus(webrtc::AudioProcessing* apm, bool enable) {

[hlundin-webrtc, 2017/02/13 21:03:57]

Swap.

[the sun, 2017/02/13 23:34:46]

Acknowledged.

+  RTC_DCHECK(apm);
+  webrtc::NoiseSuppression* ns = apm->noise_suppression();
+  if (ns->set_level(webrtc::NoiseSuppression::kHigh) != 0) {
+    LOG(LS_ERROR) << "Failed to set high NS level.";
+    return;
+  }
+  if (ns->Enable(enable) != 0) {
+    LOG(LS_ERROR) << "Failed to enable/disable NS: " << enable;
+    return;
+  }
+  LOG(LS_INFO) << "NS set to " << enable;
+}
+
+void SetTypingDetectionStatus(webrtc::AudioProcessing* apm, bool enable) {

[hlundin-webrtc, 2017/02/13 21:03:57]

Swap.

[hlundin-webrtc, 2017/02/13 21:03:57]

This is confusing. The method name speaks of typing detection, while the code
mentions voice detection and VAD.

[the sun, 2017/02/13 23:34:47]

Acknowledged.

[the sun, 2017/02/13 23:34:47]

Yeah. See:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/voice_engin...

I've tried to maintain the current terminology and logic, but perhaps it's time
to draw a line somewhere...

[hlundin-webrtc, 2017/02/14 07:50:43]

I see. Could you snatch a few of the comments in the code you refer to? That
would make this code look less like a copy-paste error, and also document some
of the requirements for using the typing detection.

[the sun, 2017/02/14 19:25:57]

Not much commentary to copy, so I wrote my own essay. WDYT?

[hlundin-webrtc, 2017/02/15 13:29:04]

Good.

+  RTC_DCHECK(apm);
+#if WEBRTC_VOICE_ENGINE_TYPING_DETECTION
+  webrtc::VoiceDetection* vd = apm->voice_detection();
+  if (vd->Enable(enable)) {
+    LOG(LS_ERROR) << "Failed to enable/disable VAD: " << enable;
+    return;
+  }
+  if (vd->set_likelihood(webrtc::VoiceDetection::kVeryLowLikelihood)) {
+    LOG(LS_ERROR) << "Failed to set low VAD likelihood.";
+    return;
+  }
+  LOG(LS_INFO) << "VAD set to " << enable;
+#endif
+}
+}  // apm_helpers
 
 bool WebRtcVoiceEngine::ToCodecInst(const AudioCodec& in,
                                     webrtc::CodecInst* out) {
@@ -620,10 +773,12 @@ WebRtcVoiceEngine::WebRtcVoiceEngine(
   apm_ = voe_wrapper_->base()->audio_processing();
   RTC_DCHECK(apm_);
 
+  transmit_mixer_ = voe_wrapper_->base()->transmit_mixer();
+  RTC_DCHECK(transmit_mixer_);
+
   // Save the default AGC configuration settings. This must happen before
   // calling ApplyOptions or the default will be overwritten.
-  int error = voe_wrapper_->processing()->GetAgcConfig(default_agc_config_);
-  RTC_DCHECK_EQ(0, error);
+  default_agc_config_ = apm_helpers::GetAgcConfig(apm_);
 
   // Set default engine options.
   {
@@ -680,9 +835,7 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) {
 
   // kEcConference is AEC with high suppression.
   webrtc::EcModes ec_mode = webrtc::kEcConference;
-  webrtc::AecmModes aecm_mode = webrtc::kAecmSpeakerphone;
   webrtc::AgcModes agc_mode = webrtc::kAgcAdaptiveAnalog;
-  webrtc::NsModes ns_mode = webrtc::kNsHighSuppression;
   if (options.aecm_generate_comfort_noise) {
     LOG(LS_VERBOSE) << "Comfort noise explicitly set to "
                     << *options.aecm_generate_comfort_noise
@@ -729,8 +882,6 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) {
   options.intelligibility_enhancer = rtc::Optional<bool>(false);
 #endif
 
-  webrtc::VoEAudioProcessing* voep = voe_wrapper_->processing();
-
   if (options.echo_cancellation) {
     // Check if platform supports built-in EC. Currently only supported on
     // Android and in combination with Java based audio layer.
@@ -751,26 +902,13 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) {
         LOG(LS_INFO) << "Disabling EC since built-in EC will be used instead";
       }
     }
-    if (voep->SetEcStatus(*options.echo_cancellation, ec_mode) == -1) {
-      LOG_RTCERR2(SetEcStatus, *options.echo_cancellation, ec_mode);
-      return false;
-    } else {
-      LOG(LS_INFO) << "Echo control set to " << *options.echo_cancellation
-                   << " with mode " << ec_mode;
-    }
+    apm_helpers::SetEcStatus(apm(), *options.echo_cancellation, ec_mode);

[the sun, 2017/02/14 19:25:57]

In case you didn't notice, I feel I should point out that the control flow here
has changed slightly.

Instead of dropping out of ApplyOptions() whenever a VoE::AudioProcessing call
fails, the new helper function will log the condition and return, but then
ApplyOptions() will continue and try to configure the next thing.

This is why I brought in both of you guys on this CL - it's not abundantly clear
to me which combinations of processing are legal/sensible. Perhaps (since I
already changed the logic), the points Henrik raised about e.g. SetEcStatus()
*should* be addressed in this CL?

[hlundin-webrtc, 2017/02/15 13:29:04]

I'm not at all sure how this code is supposed to balk when given a bad config.
So, your design is as good as mine.

[the sun, 2017/02/15 15:13:30]

Acknowledged.

 #if !defined(ANDROID)
-    // TODO(ajm): Remove the error return on Android from webrtc.
-    if (voep->SetEcMetricsStatus(*options.echo_cancellation) == -1) {
-      LOG_RTCERR1(SetEcMetricsStatus, *options.echo_cancellation);
-      return false;
-    }
+    apm_helpers::SetEcMetricsStatus(apm(), *options.echo_cancellation);
 #endif
     if (ec_mode == webrtc::kEcAecm) {
       bool cn = options.aecm_generate_comfort_noise.value_or(false);
-      if (voep->SetAecmMode(aecm_mode, cn) != 0) {
-        LOG_RTCERR2(SetAecmMode, aecm_mode, cn);
-        return false;
-      }
+      apm_helpers::SetAecmMode(apm(), cn);
     }
   }
 
@@ -785,17 +923,12 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) {
         LOG(LS_INFO) << "Disabling AGC since built-in AGC will be used instead";
       }
     }
-    if (voep->SetAgcStatus(*options.auto_gain_control, agc_mode) == -1) {
-      LOG_RTCERR2(SetAgcStatus, *options.auto_gain_control, agc_mode);
-      return false;
-    } else {
-      LOG(LS_INFO) << "Auto gain set to " << *options.auto_gain_control
-                   << " with mode " << agc_mode;
-    }
+    apm_helpers::SetAgcStatus(
+        apm(), adm(), *options.auto_gain_control, agc_mode);
   }
 
   if (options.tx_agc_target_dbov || options.tx_agc_digital_compression_gain ||
-      options.tx_agc_limiter) {
+      options.tx_agc_limiter || options.adjust_agc_delta) {
     // Override default_agc_config_. Generally, an unset option means "leave
     // the VoE bits alone" in this function, so we want whatever is set to be
     // stored as the new "default". If we didn't, then setting e.g.
@@ -811,13 +944,15 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) {
             default_agc_config_.digitalCompressionGaindB);
     default_agc_config_.limiterEnable =
         options.tx_agc_limiter.value_or(default_agc_config_.limiterEnable);
-    if (voe_wrapper_->processing()->SetAgcConfig(default_agc_config_) == -1) {
-      LOG_RTCERR3(SetAgcConfig,
-                  default_agc_config_.targetLeveldBOv,
-                  default_agc_config_.digitalCompressionGaindB,
-                  default_agc_config_.limiterEnable);
-      return false;
+
+    webrtc::AgcConfig config = default_agc_config_;
+    if (options.adjust_agc_delta) {
+      config.targetLeveldBOv -= *options.adjust_agc_delta;
+      LOG(LS_INFO) << "Adjusting AGC level from default -"
+                   << default_agc_config_.targetLeveldBOv << "dB to -"
+                   << config.targetLeveldBOv << "dB";
     }
+    apm_helpers::SetAgcConfig(apm_, config);
   }
 
   if (options.intelligibility_enhancer) {
@@ -840,22 +975,12 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) {
         LOG(LS_INFO) << "Disabling NS since built-in NS will be used instead";
       }
     }
-    if (voep->SetNsStatus(*options.noise_suppression, ns_mode) == -1) {
-      LOG_RTCERR2(SetNsStatus, *options.noise_suppression, ns_mode);
-      return false;
-    } else {
-      LOG(LS_INFO) << "Noise suppression set to " << *options.noise_suppression
-                   << " with mode " << ns_mode;
-    }
+    apm_helpers::SetNsStatus(apm(), *options.noise_suppression);
   }
 
   if (options.stereo_swapping) {

[hlundin-webrtc, 2017/02/13 21:03:57]

Hmmm. When is this used? Just curious...

[the sun, 2017/02/13 23:34:46]

It can be set from a constraint:
https://chromium.googlesource.com/external/webrtc/+/master/webrtc/api/mediaco...

[hlundin-webrtc, 2017/02/14 07:50:43]

Acknowledged.

     LOG(LS_INFO) << "Stereo swapping enabled? " << *options.stereo_swapping;
-    voep->EnableStereoChannelSwapping(*options.stereo_swapping);
-    if (voep->IsStereoChannelSwappingEnabled() != *options.stereo_swapping) {
-      LOG_RTCERR1(EnableStereoChannelSwapping, *options.stereo_swapping);
-      return false;
-    }
+    transmit_mixer()->EnableStereoChannelSwapping(*options.stereo_swapping);
   }
 
   if (options.audio_jitter_buffer_max_packets) {
@@ -874,17 +999,7 @@ bool WebRtcVoiceEngine::ApplyOptions(const AudioOptions& options_in) {
   if (options.typing_detection) {
     LOG(LS_INFO) << "Typing detection is enabled? "
                  << *options.typing_detection;
-    if (voep->SetTypingDetectionStatus(*options.typing_detection) == -1) {
-      // In case of error, log the info and continue
-      LOG_RTCERR1(SetTypingDetectionStatus, *options.typing_detection);
-    }
-  }
-
-  if (options.adjust_agc_delta) {
-    LOG(LS_INFO) << "Adjust agc delta is " << *options.adjust_agc_delta;
-    if (!AdjustAgcLevel(*options.adjust_agc_delta)) {
-      return false;
-    }
+    apm_helpers::SetTypingDetectionStatus(apm(), *options.typing_detection);
   }
 
   webrtc::Config config;
@@ -1067,25 +1182,6 @@ void WebRtcVoiceEngine::UnregisterChannel(WebRtcVoiceMediaChannel* channel) {
   channels_.erase(it);
 }
 
-// Adjusts the default AGC target level by the specified delta.
-// NB: If we start messing with other config fields, we'll want
-// to save the current webrtc::AgcConfig as well.
-bool WebRtcVoiceEngine::AdjustAgcLevel(int delta) {
-  RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-  webrtc::AgcConfig config = default_agc_config_;
-  config.targetLeveldBOv -= delta;
-
-  LOG(LS_INFO) << "Adjusting AGC level from default -"
-               << default_agc_config_.targetLeveldBOv << "dB to -"
-               << config.targetLeveldBOv << "dB";
-
-  if (voe_wrapper_->processing()->SetAgcConfig(config) == -1) {
-    LOG_RTCERR1(SetAgcConfig, config.targetLeveldBOv);
-    return false;
-  }
-  return true;
-}
-
 bool WebRtcVoiceEngine::StartAecDump(rtc::PlatformFile file,
                                      int64_t max_size_bytes) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
@@ -1148,6 +1244,12 @@ webrtc::AudioProcessing* WebRtcVoiceEngine::apm() {
   return apm_;
 }
 
+webrtc::voe::TransmitMixer* WebRtcVoiceEngine::transmit_mixer() {
+  RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+  RTC_DCHECK(transmit_mixer_);
+  return transmit_mixer_;
+}
+
 AudioCodecs WebRtcVoiceEngine::CollectRecvCodecs() const {
   PayloadTypeMapper mapper;
   AudioCodecs out;
@@ -1876,7 +1978,7 @@ bool WebRtcVoiceMediaChannel::SetOptions(const AudioOptions& options) {
     it.second->RecreateAudioSendStream(audio_network_adatptor_config);
   }
 
-  LOG(LS_INFO) << "Set voice channel options.  Current options: "
+  LOG(LS_INFO) << "Set voice channel options. Current options: "
                << options_.ToString();
   return true;
 }