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 438d1cc78a5aca5d7657b6368bfbac03fa5aed8e..476191a9281276cf3b91766e8ec55134f0c164dc 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,
@@ -52,6 +45,7 @@ AudioSendStream::AudioSendStream(
     : worker_queue_(worker_queue),
       config_(config),
       audio_state_(audio_state),
+      event_log_(event_log),
       bitrate_allocator_(bitrate_allocator),
       congestion_controller_(congestion_controller) {
   LOG(LS_INFO) << "AudioSendStream: " << config_.ToString();
@@ -61,7 +55,7 @@ AudioSendStream::AudioSendStream(
 
   VoiceEngineImpl* voe_impl = static_cast<VoiceEngineImpl*>(voice_engine());
   channel_proxy_ = voe_impl->GetChannelProxy(config_.voe_channel_id);
-  channel_proxy_->SetRtcEventLog(event_log);
+  channel_proxy_->SetRtcEventLog(event_log_);
   channel_proxy_->SetRtcpRttStats(rtcp_rtt_stats);
   channel_proxy_->SetRTCPStatus(true);
   channel_proxy_->SetLocalSSRC(config.rtp.ssrc);
@@ -88,8 +82,8 @@ AudioSendStream::AudioSendStream(
   channel_proxy_->RegisterSenderCongestionControlObjects(
       congestion_controller->pacer(), congestion_controller, packet_router,
       bandwidth_observer_.get());
-  if (!SetupSendCodec()) {
-    LOG(LS_ERROR) << "Failed to set up send codec state.";
+  if (config_.send_codec_spec && !SetupSendCodec()) {
+      LOG(LS_ERROR) << "Failed to set up send codec state.";

[the sun, 2017/03/20 20:17:25]

nit: indent off

[ossu, 2017/03/21 14:53:25]

Acknowledged.

   }
 }
 
@@ -169,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)) {
-    RTC_DCHECK_NE(codec_inst.pltype, -1);
-    stats.codec_name = codec_inst.plname;
-    stats.codec_payload_type = rtc::Optional<int>(codec_inst.pltype);
+  if (config_.send_codec_spec) {
+    const auto& spec = *config_.send_codec_spec;
+    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()) {
@@ -182,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;
       }
@@ -268,113 +261,46 @@ 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();
-
-  const auto& send_codec_spec = config_.send_codec_spec;
-
-  // 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;
-    }
+  RTC_DCHECK(config_.send_codec_spec);
+  const auto& spec = *config_.send_codec_spec;
+  std::unique_ptr<AudioEncoder> encoder =
+      config_.encoder_factory->MakeAudioEncoder(spec.payload_type, spec.format);
+
+  if (!encoder) {
+    LOG(LS_ERROR) << "Unable to create encoder for " << 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;
-    }
+  // If a bitrate has been specified for the codec, use it over the
+  // codec's default.
+  if (spec.target_bitrate_bps) {
+    encoder->OnReceivedTargetAudioBitrate(*spec.target_bitrate_bps);
   }
 
-  // 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);
+  // Enable ANA if configured (currently only used by Opus).
+  if (config_.audio_network_adaptor_config) {
+    if (encoder->EnableAudioNetworkAdaptor(
+            *config_.audio_network_adaptor_config, event_log_,
+            Clock::GetRealTimeClock())) {
       LOG(LS_INFO) << "Audio network adaptor enabled on SSRC "
                    << config_.rtp.ssrc;
+    } else {
+      RTC_NOTREACHED();
     }
   }
 
-  // Set the CN payloadtype and the VAD status.
-  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.
-      }
-    }
-
-    // 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;
-      }
-    }
+  // Wrap the encoder in a an AudioEncoderCNG, if VAD is enabled.
+  if (spec.cng_payload_type != -1) {

[the sun, 2017/03/20 20:17:25]

Is it time to make cng_payload_type an Optional?

[ossu, 2017/03/21 14:53:25]

Yeah, why not? It's clearer imo. -1 is just HowItUsedToBe(tm).

[kwiberg-webrtc, 2017/03/22 00:00:44]

+1

+    AudioEncoderCng::Config config;
+    config.num_channels = encoder->NumChannels();
+    config.payload_type = spec.cng_payload_type;
+    config.speech_encoder = std::move(encoder);
+    config.vad_mode = Vad::kVadNormal;
+    encoder.reset(new AudioEncoderCng(std::move(config)));
   }
+
+  channel_proxy_->SetEncoder(spec.payload_type, std::move(encoder));

[the sun, 2017/03/20 20:17:25]

This is so great - we won't have the half-setup states anymore, but only set the
new codec once we know ALL configuration was successful!

But, previously we would only reconfigure the channel's AudioEncoder object,
whereas now we're creating a complete new one for each new AudioSendStream. Are
we losing state? How will that work with hardware encoders? I think this may
have been the reason we originally envisioned WVoMC creating the encoder and
passing it down to the stream.

[ossu, 2017/03/21 14:53:25]

Hmm, I see what you mean. I'll have a look at moving creation into
WebRtcAudioSendStream or possibly stop reconstructing AudioSendStreams.

Regardless of where we create it, with the ACM taking ownership of the
AudioEncoder object, we still can't reliably hang on to it outside of the ACM.
Maybe we should add some checks to not reconstruct the encoder object unless the
SendCodecSpec has actually changed. Perhaps even allow unwrapping and rewrapping
the encoder with CNG, if that's the only thing that changed.

I have a couple of TODOs about not reconstructing the stream when we change the
target bitrate. I should probably address those as well.

[ossu, 2017/03/22 13:28:32]

I have now looked at it and I cannot move the construction of the AudioEncoder
higher without breaking at least one design constraint:
- From reading the design docs on VoE2 and the Call API, it's been explicitly
designed to recreate the internal streams, rather than reconfigure them. The
internal implementation of AudioSendStream is explicitly hidden from view by
having call return its base class webrtc::AudioSendStream.
- VoiceEngine as it looks today takes ownership of the encoder sent to it.

Looking at the video side of Call, it seems designed and implemented much the
same, with streams getting reconstructed and encoders reconstructed with the
streams. What they do differently, though, is to gather all the changes caused
by SetSendParameters before reconstructing the stream. We don't, causing our
encoder to get reconstructed a couple of times in quick succession.

So without major redesign, which I'd consider outside the scope of this CL, I
believe encoder construction is where it should be.

I will, however, take a look at restructuring SetSendParameters a bit, so we
don't end up reconstructing the stream more than once during that call.

+
   return true;
 }