Injectable audio encoders: WebRtcVoiceEngine and company

webrtc/audio/audio_send_stream.cc

 /*
  *  Copyright (c) 2015 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.
  */
 
 #include "webrtc/audio/audio_send_stream.h"
 
 #include <string>
+#include <utility>
 
 #include "webrtc/audio/audio_state.h"
 #include "webrtc/audio/conversion.h"
 #include "webrtc/audio/scoped_voe_interface.h"
 #include "webrtc/base/checks.h"
 #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"
 #include "webrtc/modules/rtp_rtcp/include/rtp_rtcp_defines.h"
 #include "webrtc/voice_engine/channel_proxy.h"
 #include "webrtc/voice_engine/include/voe_base.h"
 #include "webrtc/voice_engine/transmit_mixer.h"
 #include "webrtc/voice_engine/voice_engine_impl.h"
 
 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,
     const rtc::scoped_refptr<webrtc::AudioState>& audio_state,
     rtc::TaskQueue* worker_queue,
     PacketRouter* packet_router,
     CongestionController* congestion_controller,
     BitrateAllocator* bitrate_allocator,
     RtcEventLog* event_log,
     RtcpRttStats* rtcp_rtt_stats)
     : 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();
   RTC_DCHECK_NE(config_.voe_channel_id, -1);
   RTC_DCHECK(audio_state_.get());
   RTC_DCHECK(congestion_controller);
 
   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);
   channel_proxy_->SetRTCP_CNAME(config.rtp.c_name);
   // TODO(solenberg): Config NACK history window (which is a packet count),
   // using the actual packet size for the configured codec.
   channel_proxy_->SetNACKStatus(config_.rtp.nack.rtp_history_ms != 0,
                                 config_.rtp.nack.rtp_history_ms / 20);
 
   channel_proxy_->RegisterExternalTransport(config.send_transport);
 
   for (const auto& extension : config.rtp.extensions) {
     if (extension.uri == RtpExtension::kAudioLevelUri) {
       channel_proxy_->SetSendAudioLevelIndicationStatus(true, extension.id);
     } else if (extension.uri == RtpExtension::kTransportSequenceNumberUri) {
       channel_proxy_->EnableSendTransportSequenceNumber(extension.id);
       congestion_controller->EnablePeriodicAlrProbing(true);
       bandwidth_observer_.reset(congestion_controller->GetBitrateController()
                                     ->CreateRtcpBandwidthObserver());
     } else {
       RTC_NOTREACHED() << "Registering unsupported RTP extension.";
     }
   }
   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.

   }
 }
 
 AudioSendStream::~AudioSendStream() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   LOG(LS_INFO) << "~AudioSendStream: " << config_.ToString();
   channel_proxy_->DeRegisterExternalTransport();
   channel_proxy_->ResetCongestionControlObjects();
   channel_proxy_->SetRtcEventLog(nullptr);
   channel_proxy_->SetRtcpRttStats(nullptr);
@@ skipping 59 matching lines @@
   stats.packets_sent = call_stats.packetsSent;
   // RTT isn't known until a RTCP report is received. Until then, VoiceEngine
   // returns 0 to indicate an error value.
   if (call_stats.rttMs > 0) {
     stats.rtt_ms = call_stats.rttMs;
   }
   // TODO(solenberg): [was ajm]: Re-enable this metric once we have a reliable
   //                  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()) {
       // Lookup report for send ssrc only.
       if (block.source_SSRC == stats.local_ssrc) {
         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;
       }
     }
   }
 
   ScopedVoEInterface<VoEBase> base(voice_engine());
   RTC_DCHECK(base->transmit_mixer());
   stats.audio_level = base->transmit_mixer()->AudioLevelFullRange();
   RTC_DCHECK_LE(0, stats.audio_level);
@@ skipping 62 matching lines @@
 VoiceEngine* AudioSendStream::voice_engine() const {
   internal::AudioState* audio_state =
       static_cast<internal::AudioState*>(audio_state_.get());
   VoiceEngine* voice_engine = audio_state->voice_engine();
   RTC_DCHECK(voice_engine);
   return voice_engine;
 }
 
 // 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);
+  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);
 
-  // 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();
+  if (!encoder) {
+    LOG(LS_ERROR) << "Unable to create encoder for " << spec.format;
+    return false;
+  }
 
-  const auto& send_codec_spec = config_.send_codec_spec;
+  // 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);
+  }
 
-  // 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;
+  // 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();
     }
   }
 
-  // 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;
-    }
+  // 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)));
   }
 
-  // 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;
-    }
+  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.

 
-    // 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;
-    }
-  }
-
-  // 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;
-      }
-    }
-  }
   return true;
 }
 
 }  // namespace internal
 }  // namespace webrtc