From 76d29f9bf8604a30e01f7cc0496297efbbeac51b Mon Sep 17 00:00:00 2001
From: ossu <ossu@webrtc.org>
Date: Fri, 9 Jun 2017 07:30:13 -0700
Subject: [PATCH] Fix Channel::GetSendCodec when used together with SetEncoder.

When using the SetEncoder interface, there's no actual CodecInst to return from Channel::GetSendCodec. Before this CL, this was done by calling the ACM, which has functionality for generating a CodecInst with the necessary values even when handed an external encoder. Unfortunately, this call takes a lock and does some extra processing which isn't strictly necessary in this case. Since GetSendCodec is called inside the audio input callback code, this can cause problems.

This CL instead generates a CodecInst in the SetEncoder call and has GetSendCodec simply return that one if it's available. If it isn't the value from codec_manager_ is returned instead, as was the case before injectable audio codec related changes were added to Channel.

BUG=b/38018041

Review-Url: https://codereview.webrtc.org/2924363004
Cr-Commit-Position: refs/heads/master@{#18515}
---
 webrtc/voice_engine/channel.cc | 47 +++++++++++++++++++++-------------
 webrtc/voice_engine/channel.h  |  2 ++
 2 files changed, 31 insertions(+), 18 deletions(-)

diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc
index b0bae4e4ce..0a9e9fce34 100644
--- a/webrtc/voice_engine/channel.cc
+++ b/webrtc/voice_engine/channel.cc
@@ -1278,23 +1278,34 @@ bool Channel::SetEncoder(int payload_type,
                          std::unique_ptr<AudioEncoder> encoder) {
   RTC_DCHECK_GE(payload_type, 0);
   RTC_DCHECK_LE(payload_type, 127);
-  // TODO(ossu): Make a CodecInst up for now. It seems like very little of this
-  // information is actually used, possibly only payload type and clock rate.
-  CodecInst lies;
-  lies.pltype = payload_type;
-  strncpy(lies.plname, "audio", sizeof(lies.plname));
-  lies.plname[sizeof(lies.plname) - 1] = 0;
+  // TODO(ossu): Make CodecInsts up, for now: one for the RTP/RTCP module and
+  // one for for us to keep track of sample rate and number of channels, etc.
+
+  // The RTP/RTCP module needs to know the RTP timestamp rate (i.e. clockrate)
+  // as well as some other things, so we collect this info and send it along.
+  CodecInst rtp_codec;
+  rtp_codec.pltype = payload_type;
+  strncpy(rtp_codec.plname, "audio", sizeof(rtp_codec.plname));
+  rtp_codec.plname[sizeof(rtp_codec.plname) - 1] = 0;
   // Seems unclear if it should be clock rate or sample rate. CodecInst
   // supposedly carries the sample rate, but only clock rate seems sensible to
   // send to the RTP/RTCP module.
-  lies.plfreq = encoder->RtpTimestampRateHz();
-  lies.pacsize = 0;
-  lies.channels = encoder->NumChannels();
-  lies.rate = 0;
+  rtp_codec.plfreq = encoder->RtpTimestampRateHz();
+  rtp_codec.pacsize = rtc::CheckedDivExact(
+      static_cast<int>(encoder->Max10MsFramesInAPacket() * rtp_codec.plfreq),
+      100);
+  rtp_codec.channels = encoder->NumChannels();
+  rtp_codec.rate = 0;
 
-  if (_rtpRtcpModule->RegisterSendPayload(lies) != 0) {
+  // For audio encoding we need, instead, the actual sample rate of the codec.
+  // The rest of the information should be the same.
+  CodecInst send_codec = rtp_codec;
+  send_codec.plfreq = encoder->SampleRateHz();
+  cached_send_codec_.emplace(send_codec);
+
+  if (_rtpRtcpModule->RegisterSendPayload(rtp_codec) != 0) {
     _rtpRtcpModule->DeRegisterSendPayload(payload_type);
-    if (_rtpRtcpModule->RegisterSendPayload(lies) != 0) {
+    if (_rtpRtcpModule->RegisterSendPayload(rtp_codec) != 0) {
       WEBRTC_TRACE(
           kTraceError, kTraceVoice, VoEId(_instanceId, _channelId),
           "SetEncoder() failed to register codec to RTP/RTCP module");
@@ -1343,18 +1354,16 @@ int32_t Channel::DeRegisterVoiceEngineObserver() {
 }
 
 int32_t Channel::GetSendCodec(CodecInst& codec) {
-  {
+  if (cached_send_codec_) {
+    codec = *cached_send_codec_;
+    return 0;
+  } else {
     const CodecInst* send_codec = codec_manager_.GetCodecInst();
     if (send_codec) {
       codec = *send_codec;
       return 0;
     }
   }
-  rtc::Optional<CodecInst> acm_send_codec = audio_coding_->SendCodec();
-  if (acm_send_codec) {
-    codec = *acm_send_codec;
-    return 0;
-  }
   return -1;
 }
 
@@ -1383,6 +1392,8 @@ int32_t Channel::SetSendCodec(const CodecInst& codec) {
     }
   }
 
+  cached_send_codec_.reset();
+
   return 0;
 }
 
diff --git a/webrtc/voice_engine/channel.h b/webrtc/voice_engine/channel.h
index 8c9d623b19..101de4060b 100644
--- a/webrtc/voice_engine/channel.h
+++ b/webrtc/voice_engine/channel.h
@@ -543,6 +543,8 @@ class Channel
   // TODO(ossu): Remove once GetAudioDecoderFactory() is no longer needed.
   rtc::scoped_refptr<AudioDecoderFactory> decoder_factory_;
 
+  rtc::Optional<CodecInst> cached_send_codec_;
+
   rtc::ThreadChecker construction_thread_;
 
   const bool use_twcc_plr_for_ana_;