Injectable audio encoders: WebRtcVoiceEngine and company

webrtc/audio/audio_send_stream.cc

Index: webrtc/audio/audio_send_stream.cc
diff --git a/webrtc/audio/audio_send_stream.cc b/webrtc/audio/audio_send_stream.cc
index ec18125427282d4624543bc45b274d2b1a42692e..24507c53a803769333e7ecbfef2c0031bffba80c 100644
--- a/webrtc/audio/audio_send_stream.cc
+++ b/webrtc/audio/audio_send_stream.cc
@@ -11,6 +11,7 @@
 #include "webrtc/audio/audio_send_stream.h"
 
 #include <string>
+#include <utility>
 
 #include "webrtc/audio/audio_state.h"
 #include "webrtc/audio/conversion.h"
@@ -19,6 +20,7 @@
 #include "webrtc/base/event.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/task_queue.h"
+#include "webrtc/modules/audio_coding/codecs/cng/audio_encoder_cng.h"
 #include "webrtc/modules/bitrate_controller/include/bitrate_controller.h"
 #include "webrtc/modules/congestion_controller/include/congestion_controller.h"
 #include "webrtc/modules/pacing/paced_sender.h"
@@ -30,15 +32,6 @@
 
 namespace webrtc {
 
-namespace {
-
-constexpr char kOpusCodecName[] = "opus";
-
-bool IsCodec(const webrtc::CodecInst& codec, const char* ref_name) {
-  return (STR_CASE_CMP(codec.plname, ref_name) == 0);
-}
-}  // namespace
-
 namespace internal {
 AudioSendStream::AudioSendStream(
     const webrtc::AudioSendStream::Config& config,
@@ -170,11 +163,10 @@ webrtc::AudioSendStream::Stats AudioSendStream::GetStats() const {
   //                  implementation.
   stats.aec_quality_min = -1;
 
-  webrtc::CodecInst codec_inst = {0};
-  if (channel_proxy_->GetSendCodec(&codec_inst)) {

[the sun, 2017/03/16 08:48:19]

Do you have a separate CL lined up to clean out the unnecessary calls from
ChannelProxy/voe::Channel?

-    RTC_DCHECK_NE(codec_inst.pltype, -1);
-    stats.codec_name = codec_inst.plname;
-    stats.codec_payload_type = rtc::Optional<int>(codec_inst.pltype);
+  const auto& spec = config_.send_codec_spec;
+  if (config_.send_codec_spec.format.name != "") {

[kwiberg-webrtc, 2017/03/01 12:26:47]

Is this a test for whether the format is "empty"? In that case, shouldn't it be
Optional?

[the sun, 2017/03/02 20:37:24]

nit: use spec.format.name

[ossu, 2017/03/02 20:43:27]

Sure thing!

+    stats.codec_name = spec.format.name;
+    stats.codec_payload_type = rtc::Optional<int>(spec.payload_type);
 
     // Get data from the last remote RTCP report.
     for (const auto& block : channel_proxy_->GetRemoteRTCPReportBlocks()) {
@@ -183,10 +175,10 @@ webrtc::AudioSendStream::Stats AudioSendStream::GetStats() const {
         stats.packets_lost = block.cumulative_num_packets_lost;
         stats.fraction_lost = Q8ToFloat(block.fraction_lost);
         stats.ext_seqnum = block.extended_highest_sequence_number;
-        // Convert samples to milliseconds.
-        if (codec_inst.plfreq / 1000 > 0) {
+        // Convert timestamps to milliseconds.
+        if (spec.format.clockrate_hz / 1000 > 0) {
           stats.jitter_ms =
-              block.interarrival_jitter / (codec_inst.plfreq / 1000);
+              block.interarrival_jitter / (spec.format.clockrate_hz / 1000);
         }
         break;
       }
@@ -274,113 +266,50 @@ VoiceEngine* AudioSendStream::voice_engine() const {
 
 // Apply current codec settings to a single voe::Channel used for sending.
 bool AudioSendStream::SetupSendCodec() {
-  // Disable VAD and FEC unless we know the other side wants them.
-  channel_proxy_->SetVADStatus(false);
-  channel_proxy_->SetCodecFECStatus(false);
-
-  // We disable audio network adaptor here. This will on one hand make sure that
-  // audio network adaptor is disabled by default, and on the other allow audio
-  // network adaptor to be reconfigured, since SetReceiverFrameLengthRange can
-  // be only called when audio network adaptor is disabled.
-  channel_proxy_->DisableAudioNetworkAdaptor();
+  // TODO(ossu): This check is due to some Call tests creating AudioSendStreams
+  // without a reasonable config.

[the sun, 2017/03/16 08:48:19]

Is that because they're explicitly setting up the send codec via VoECodec? Can
we update the test instead?

[ossu, 2017/03/20 18:19:48]

After working through it, I realized streams getting created without a proper
SendCodecSpec is completely valid: they get created from a call to
SetLocalContent but the codec to use isn't decided until negotiation is done and
SetRemoteContent gets called. I've made SendCodecSpec optional in lots of
places, and we no longer try to setup codecs unless it's properly set, unlike
before this CL, where we'd just error out with a warning.

+  if (!config_.encoder_factory || config_.send_codec_spec.format.name == "")
+    return false;
 
   const auto& send_codec_spec = config_.send_codec_spec;
+  std::unique_ptr<AudioEncoder> encoder =
+      config_.encoder_factory->MakeAudioEncoder(send_codec_spec.payload_type,
+                                                send_codec_spec.format);
 
-  // We set the codec first, since the below extra configuration is only applied
-  // to the "current" codec.
-
-  // If codec is already configured, we do not it again.
-  // TODO(minyue): check if this check is really needed, or can we move it into
-  // |codec->SetSendCodec|.
-  webrtc::CodecInst current_codec = {0};
-  if (!channel_proxy_->GetSendCodec(&current_codec) ||
-      (send_codec_spec.codec_inst != current_codec)) {
-    if (!channel_proxy_->SetSendCodec(send_codec_spec.codec_inst)) {
-      LOG(LS_WARNING) << "SetSendCodec() failed.";
-      return false;
-    }
+  if (!encoder) {

[the sun, 2017/03/16 08:48:19]

Any chance we could make this a CHECK or DCHECK going forward? WVoMC should
already have verified the format is supported, right?

[ossu, 2017/03/20 18:19:48]

I've rephrased it and made it log an error instead. There might be a case where
actually constructing the encoder fails, even though the input is alright. We
probably don't want to take the whole app down with us then. Or do we?

+    LOG(LS_WARNING) << "Unknown format " << send_codec_spec.format;
+    return false;
   }
 
-  // Codec internal FEC. Treat any failure as fatal internal error.
-  if (send_codec_spec.enable_codec_fec) {
-    if (!channel_proxy_->SetCodecFECStatus(true)) {
-      LOG(LS_WARNING) << "SetCodecFECStatus() failed.";
-      return false;
-    }
-  }
-
-  // DTX and maxplaybackrate are only set if current codec is Opus.
-  if (IsCodec(send_codec_spec.codec_inst, kOpusCodecName)) {
-    if (!channel_proxy_->SetOpusDtx(send_codec_spec.enable_opus_dtx)) {
-      LOG(LS_WARNING) << "SetOpusDtx() failed.";
-      return false;
-    }
-
-    // If opus_max_playback_rate <= 0, the default maximum playback rate
-    // (48 kHz) will be used.
-    if (send_codec_spec.opus_max_playback_rate > 0) {
-      if (!channel_proxy_->SetOpusMaxPlaybackRate(
-              send_codec_spec.opus_max_playback_rate)) {
-        LOG(LS_WARNING) << "SetOpusMaxPlaybackRate() failed.";
-        return false;
-      }
-    }
-
-    if (config_.audio_network_adaptor_config) {
-      // Audio network adaptor is only allowed for Opus currently.
-      // |SetReceiverFrameLengthRange| needs to be called before
-      // |EnableAudioNetworkAdaptor|.
-      channel_proxy_->SetReceiverFrameLengthRange(send_codec_spec.min_ptime_ms,
-                                                  send_codec_spec.max_ptime_ms);
-      channel_proxy_->EnableAudioNetworkAdaptor(
-          *config_.audio_network_adaptor_config);
-      LOG(LS_INFO) << "Audio network adaptor enabled on SSRC "
-                   << config_.rtp.ssrc;
-    }
+  // If a bitrate has been specified for the codec, use it over the
+  // codec's default.
+  if (send_codec_spec.target_bitrate_bps) {
+    encoder->OnReceivedTargetAudioBitrate(*send_codec_spec.target_bitrate_bps);
   }
 
   // Set the CN payloadtype and the VAD status.

[the sun, 2017/03/16 08:48:19]

Comment is a bit off.

[ossu, 2017/03/20 18:19:48]

Acknowledged.

   if (send_codec_spec.cng_payload_type != -1) {
-    // The CN payload type for 8000 Hz clockrate is fixed at 13.
-    if (send_codec_spec.cng_plfreq != 8000) {
-      webrtc::PayloadFrequencies cn_freq;
-      switch (send_codec_spec.cng_plfreq) {
-        case 16000:
-          cn_freq = webrtc::kFreq16000Hz;
-          break;
-        case 32000:
-          cn_freq = webrtc::kFreq32000Hz;
-          break;
-        default:
-          RTC_NOTREACHED();
-          return false;
-      }
-      if (!channel_proxy_->SetSendCNPayloadType(
-          send_codec_spec.cng_payload_type, cn_freq)) {
-        LOG(LS_WARNING) << "SetSendCNPayloadType() failed.";
-        // TODO(ajm): This failure condition will be removed from VoE.
-        // Restore the return here when we update to a new enough webrtc.
-        //
-        // Not returning false because the SetSendCNPayloadType will fail if
-        // the channel is already sending.
-        // This can happen if the remote description is applied twice, for
-        // example in the case of ROAP on top of JSEP, where both side will
-        // send the offer.
-      }
-    }
+    AudioEncoderCng::Config config;
+    config.num_channels = encoder->NumChannels();
+    config.payload_type = send_codec_spec.cng_payload_type;
+    config.speech_encoder = std::move(encoder);
+    config.vad_mode = Vad::kVadNormal;
+    encoder =
+        std::unique_ptr<AudioEncoder>(new AudioEncoderCng(std::move(config)));

[kwiberg-webrtc, 2017/03/01 12:26:47]

encoder.reset(...) would be shorter here.

[ossu, 2017/03/02 01:30:28]

Yupp! Will change.

+  }
 
-    // Only turn on VAD if we have a CN payload type that matches the
-    // clockrate for the codec we are going to use.
-    if (send_codec_spec.cng_plfreq == send_codec_spec.codec_inst.plfreq &&
-        send_codec_spec.codec_inst.channels == 1) {
-      // TODO(minyue): If CN frequency == 48000 Hz is allowed, consider the
-      // interaction between VAD and Opus FEC.
-      if (!channel_proxy_->SetVADStatus(true)) {
-        LOG(LS_WARNING) << "SetVADStatus() failed.";
-        return false;
-      }
-    }
+  channel_proxy_->SetEncoder(send_codec_spec.payload_type, std::move(encoder));
+
+  // TODO(ossu): The encoder interface wants an RtcEventLogProxy and a Clock,

[the sun, 2017/03/16 08:48:19]

We've decided to use the webrtc/base/timeutils.h API and rely on a global clock.
You could either change the encoder(s) to use that, or add an extra method on
Channel to return the Clock*...

+  // both of which are in Channel. Solve this!
+  if (config_.audio_network_adaptor_config) {
+    // Audio network adaptor is only allowed for Opus currently.

[kwiberg-webrtc, 2017/03/01 12:26:47]

This comment doesn't appear to have any corresponding code.

[ossu, 2017/03/02 01:30:28]

Hmm... I agree that it doesn't describe the code that follows, but it does
indicate in what circumstance audio_network_adaptor_config could be not false.

+    channel_proxy_->EnableAudioNetworkAdaptor(

[the sun, 2017/03/02 20:37:24]

Is it possible to avoid this call to voe::Channel and apply the setting directly
to the encoder object?

[ossu, 2017/03/02 20:43:27]

I'll look into it. The AudioEncoder version of this call requires a couple of
objects that live within Channel, but I'll see if I can get to them and avoid
Channel altogether.

[ossu, 2017/03/20 18:19:48]

Turns out avoiding the call through ChannelProxy here was a piece of cake!

+        *config_.audio_network_adaptor_config);
+    LOG(LS_INFO) << "Audio network adaptor enabled on SSRC "
+                 << config_.rtp.ssrc;
   }
+
   return true;
 }