Injectable audio encoders: WebRtcVoiceEngine and company

webrtc/voice_engine/channel.cc

Index: webrtc/voice_engine/channel.cc
diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc
index 006e3db69bf55d754eec187c52793b4a5f66e54c..15c487aa095836cad87e209303040a7ae1ffae63 100644
--- a/webrtc/voice_engine/channel.cc
+++ b/webrtc/voice_engine/channel.cc
@@ -1104,6 +1104,10 @@ Channel::GetAudioDecoderFactory() const {
   return decoder_factory_;
 }
 
+AudioCodingModule& Channel::GetAudioCodingModule() {
+  return *audio_coding_;
+}
+
 int32_t Channel::StartPlayout() {
   WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId, _channelId),
                "Channel::StartPlayout()");
@@ -1202,6 +1206,48 @@ int32_t Channel::StopSend() {
   return 0;
 }
 
+bool Channel::SetSendFormat(int payload_type,
+                            const SdpAudioFormat& format,
+                            AudioEncoderFactory* factory) {
+  WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId, _channelId),
+               "Channel::SetSendFormat()");
+
+  auto encoder = factory->MakeAudioEncoder(payload_type, format);

[ossu, 2017/02/22 10:24:23]

I put the MakeAudioEncoder call here, rather than webrtcvoiceengine or
audio_send_stream, because we need to call RegisterSendPayload with stuff in
format, so we'll need to pass format along anyway. This could be changed, i.e.
by being able to ask the encoder about its format, or maybe RegisterSendPayload
could be redesigned?

[kwiberg-webrtc, 2017/02/22 10:42:06]

We want RegisterSendPayload to not take a CodecInst, at any rate. If we can
figure out what data it should actually take, it'll hopefully become clearer
whether AudioEncoder instances should reasonably carry it.

+  if (!encoder) {
+    WEBRTC_TRACE(kTraceError, kTraceVoice, VoEId(_instanceId, _channelId),
+                 "SetSendFormat() failed to create encoder");
+    return false;
+  }
+
+  // TODO(ossu): Make a CodecInst up for now.
+  CodecInst lies;
+  lies.pltype = payload_type;
+  strncpy(lies.plname, format.name.c_str(), RTP_PAYLOAD_NAME_SIZE);

[kwiberg-webrtc, 2017/02/21 23:35:04]

The length limiter should also involve sizeof(lies.plname).

[ossu, 2017/02/22 10:24:23]

But sizeof(lies.plname) _is_ RTP_PAYLOAD_NAME_SIZE.

[kwiberg-webrtc, 2017/02/22 10:42:06]

You know that, and I know that (obviously!), but I have a friend who didn't know
this for certain and would have had to look it up, but was too lazy.

So I'd still suggest you use sizeof(lies.plname). If you want to use
RTP_PAYLOAD_NAME_SIZE, please static_assert that they're the same.

[ossu, 2017/02/22 10:47:07]

You're right. I'll use the sizeof instead. I think I just copied this phrasing
from somewhere else.

+  lies.plname[RTP_PAYLOAD_NAME_SIZE - 1] = 0;
+  // Seems unclear if it should be clock rate or sample rate. CodecInst used to
+  // carry sample rate, but only clock rate seems sensible to send to the

[kwiberg-webrtc, 2017/02/21 23:35:04]

"CodecInst supposedly carries the sample rate,"

[ossu, 2017/02/22 10:24:23]

Alright. I think this comment is more for the review, rather than being left in.
But maybe nobody really knows which one should be in there? It SeemsToWork(tm)
regardless of the value I choose. If that's the case, then the comment should
remain and I'll rephrase it.

[kwiberg-webrtc, 2017/02/22 10:42:06]

Acknowledged.

+  // RTP/RTCP module.
+  // lies.plfreq = encoder->SampleRateHz();
+  lies.plfreq = format.clockrate_hz;
+  lies.pacsize = 0;
+  lies.channels = encoder->NumChannels();
+  lies.rate = 0;
+
+  if (_rtpRtcpModule->RegisterSendPayload(lies) != 0) {
+    _rtpRtcpModule->DeRegisterSendPayload(payload_type);
+    if (_rtpRtcpModule->RegisterSendPayload(lies) != 0) {
+      WEBRTC_TRACE(
+          kTraceError, kTraceVoice, VoEId(_instanceId, _channelId),
+          "SetSendFormat() failed to register codec to RTP/RTCP module");
+      return false;
+    }
+  }
+
+  audio_coding_->SetEncoder(std::move(encoder));
+
+  return true;
+}
+
 int32_t Channel::RegisterVoiceEngineObserver(VoiceEngineObserver& observer) {
   WEBRTC_TRACE(kTraceInfo, kTraceVoice, VoEId(_instanceId, _channelId),
                "Channel::RegisterVoiceEngineObserver()");