Pass SdpAudioFormat through Channel, without converting to CodecInst

webrtc/media/engine/webrtcvoiceengine.cc

 /*
  *  Copyright (c) 2004 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.
  */
 
@@ skipping 1449 matching lines @@
       bool use_nack,
       const std::string& sync_group,
       const std::vector<webrtc::RtpExtension>& extensions,
       webrtc::Call* call,
       webrtc::Transport* rtcp_send_transport,
       const rtc::scoped_refptr<webrtc::AudioDecoderFactory>& decoder_factory)
       : call_(call), config_() {
     RTC_DCHECK_GE(ch, 0);
     RTC_DCHECK(call);
     config_.rtp.remote_ssrc = remote_ssrc;
+    config_.rtp.local_ssrc = local_ssrc;
+    config_.rtp.transport_cc = use_transport_cc;
+    config_.rtp.nack.rtp_history_ms = use_nack ? kNackRtpHistoryMs : 0;
+    config_.rtp.extensions = extensions;
     config_.rtcp_send_transport = rtcp_send_transport;
     config_.voe_channel_id = ch;
     config_.sync_group = sync_group;
     config_.decoder_factory = decoder_factory;
-    RecreateAudioReceiveStream(local_ssrc,

[the sun, 2016/12/14 08:58:24]

Thanks for fixing this btw!

-                               use_transport_cc,
-                               use_nack,
-                               extensions);
+    RecreateAudioReceiveStream();
   }
 
   ~WebRtcAudioReceiveStream() {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     call_->DestroyAudioReceiveStream(stream_);
   }
 
   void RecreateAudioReceiveStream(uint32_t local_ssrc) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    RecreateAudioReceiveStream(local_ssrc,
-                               config_.rtp.transport_cc,
-                               config_.rtp.nack.rtp_history_ms != 0,
-                               config_.rtp.extensions);
+    config_.rtp.local_ssrc = local_ssrc;
+    RecreateAudioReceiveStream();
   }
 
   void RecreateAudioReceiveStream(bool use_transport_cc, bool use_nack) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    RecreateAudioReceiveStream(config_.rtp.local_ssrc,
-                               use_transport_cc,
-                               use_nack,
-                               config_.rtp.extensions);
+    config_.rtp.transport_cc = use_transport_cc;
+    config_.rtp.nack.rtp_history_ms = use_nack ? kNackRtpHistoryMs : 0;
+    RecreateAudioReceiveStream();
   }
 
   void RecreateAudioReceiveStream(
       const std::vector<webrtc::RtpExtension>& extensions) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    RecreateAudioReceiveStream(config_.rtp.local_ssrc,
-                               config_.rtp.transport_cc,
-                               config_.rtp.nack.rtp_history_ms != 0,
-                               extensions);
+    config_.rtp.extensions = extensions;
+    RecreateAudioReceiveStream();
+  }
+
+  // Set a new payload type -> decoder map. The new map must be a superset of
+  // the old one.
+  void RecreateAudioReceiveStream(
+      const std::map<int, webrtc::SdpAudioFormat>& decoder_map) {
+    RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+    RTC_DCHECK([&] {
+      for (const auto& item : config_.decoder_map) {
+        auto it = decoder_map.find(item.first);
+        if (it == decoder_map.end() || *it != item) {
+          return false;  // The old map isn't a subset of the new map.
+        }
+      }
+      return true;
+    }());
+    config_.decoder_map = decoder_map;
+    RecreateAudioReceiveStream();
   }
 
   webrtc::AudioReceiveStream::Stats GetStats() const {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     RTC_DCHECK(stream_);
     return stream_->GetStats();
   }
 
   int channel() const {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
@@ skipping 17 matching lines @@
       LOG(LS_INFO) << "Starting playout for channel #" << channel();
       stream_->Start();
     } else {
       LOG(LS_INFO) << "Stopping playout for channel #" << channel();
       stream_->Stop();
     }
     playout_ = playout;
   }
 
  private:
-  void RecreateAudioReceiveStream(
-      uint32_t local_ssrc,
-      bool use_transport_cc,
-      bool use_nack,
-      const std::vector<webrtc::RtpExtension>& extensions) {
+  void RecreateAudioReceiveStream() {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
     if (stream_) {
       call_->DestroyAudioReceiveStream(stream_);
-      stream_ = nullptr;
     }
-    config_.rtp.local_ssrc = local_ssrc;
-    config_.rtp.transport_cc = use_transport_cc;
-    config_.rtp.nack.rtp_history_ms = use_nack ? kNackRtpHistoryMs : 0;
-    config_.rtp.extensions = extensions;
-    RTC_DCHECK(!stream_);
     stream_ = call_->CreateAudioReceiveStream(config_);
     RTC_CHECK(stream_);
     SetPlayout(playout_);
   }
 
   rtc::ThreadChecker worker_thread_checker_;
   webrtc::Call* call_ = nullptr;
   webrtc::AudioReceiveStream::Config config_;
   // The stream is owned by WebRtcAudioReceiveStream and may be reallocated if
   // configuration changes.
@@ skipping 264 matching lines @@
     } else {
       new_codecs.push_back(codec);
     }
   }
   if (new_codecs.empty()) {
     // There are no new codecs to configure. Already configured codecs are
     // never removed.
     return true;
   }
 
+  // Create a payload type -> SdpAudioFormat map with all the decoders. Fail
+  // unless the factory claims to support all decoders.
+  std::map<int, webrtc::SdpAudioFormat> decoder_map;
+  for (const AudioCodec& codec : codecs) {
+    auto saf = AudioCodecToSdpAudioFormat(codec);

[the sun, 2016/12/14 08:58:21]

How about "format" instead of "saf"?

[kwiberg-webrtc, 2016/12/14 13:09:36]

Done.

+    if (!IsCodec(codec, "cn") && !IsCodec(codec, "telephone-event") &&
+        !engine()->decoder_factory_->IsSupportedDecoder(saf)) {
+      LOG(LS_ERROR) << "Unsupported codec: " << saf;
+      return false;
+    }
+    decoder_map.insert({codec.id, std::move(saf)});
+  }
+
   if (playout_) {
     // Receive codecs can not be changed while playing. So we temporarily
     // pause playout.
     ChangePlayout(false);
   }
 
-  bool result = true;
-  for (const AudioCodec& codec : new_codecs) {
-    webrtc::CodecInst voe_codec = {0};
-    if (WebRtcVoiceEngine::ToCodecInst(codec, &voe_codec)) {

[the sun, 2016/12/14 08:58:21]

What about the other logic that ToCodecInst() has, such as setting ac.bitrate to
zero if the codec is "multi rate", and ci.rate to -1 for ISAC when ac.bitrate <=
0. Perhaps there's more, the function is rather subtle.

[kwiberg-webrtc, 2016/12/14 13:09:36]

There's a bunch of logic to handle G722's clock rate, but we handle that
elsewhere when doing the SdpAudioFormat -> AudioDecoder step. There's also a
bunch of logic for twiddling the bitrate, but (1) SdpAudioFormat doesn't have a
bitrate field, and (2) we don't need it when creating decoders (none of our
AudioDecoder constructors take a bitrate argument).

Other than that, I don't think there's anything.

-      LOG(LS_INFO) << ToString(codec);
-      voe_codec.pltype = codec.id;
-      for (const auto& ch : recv_streams_) {
-        if (engine()->voe()->codec()->SetRecPayloadType(
-                ch.second->channel(), voe_codec) == -1) {
-          LOG_RTCERR2(SetRecPayloadType, ch.second->channel(),
-                      ToString(voe_codec));
-          result = false;
-        }
-      }
-    } else {
-      LOG(LS_WARNING) << "Unknown codec " << ToString(codec);
-      result = false;
-      break;
-    }
+  for (auto& kv : recv_streams_) {
+    kv.second->RecreateAudioReceiveStream(decoder_map);
   }
-  if (result) {
-    recv_codecs_ = codecs;
-  }
+  recv_codecs_ = codecs;
 
   if (desired_playout_ && !playout_) {
     ChangePlayout(desired_playout_);
   }
-  return result;
+  return true;
 }
 
 // Utility function called from SetSendParameters() to extract current send
 // codec settings from the given list of codecs (originally from SDP). Both send
 // and receive streams may be reconfigured based on the new settings.
 bool WebRtcVoiceMediaChannel::SetSendCodecs(
     const std::vector<AudioCodec>& codecs) {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   dtmf_payload_type_ = rtc::Optional<int>();
   dtmf_payload_freq_ = -1;
@@ skipping 331 matching lines @@
   // Create a new channel for receiving audio data.
   const int channel = CreateVoEChannel();
   if (channel == -1) {
     return false;
   }
 
   // Turn off all supported codecs.
   // TODO(solenberg): Remove once "no codecs" is the default state of a stream.
   for (webrtc::CodecInst voe_codec : webrtc::acm2::RentACodec::Database()) {
     voe_codec.pltype = -1;
     if (engine()->voe()->codec()->SetRecPayloadType(channel, voe_codec) == -1) {

[the sun, 2016/12/14 10:37:38]

We should be able to remove these calls to SetRecPayloadType() as well?

[kwiberg-webrtc, 2016/12/14 13:09:36]

Maybe. But removing this for loop breaks a half-dozen tests, so I'd rather not
try to do it in this CL.

       LOG_RTCERR2(SetRecPayloadType, channel, ToString(voe_codec));
       DeleteVoEChannel(channel);
       return false;
     }
   }
 
   // Only enable those configured for this channel.
   for (const auto& codec : recv_codecs_) {
     webrtc::CodecInst voe_codec = {0};
     if (WebRtcVoiceEngine::ToCodecInst(codec, &voe_codec)) {
@@ skipping 411 matching lines @@
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   const auto it = send_streams_.find(ssrc);
   if (it != send_streams_.end()) {
     return it->second->channel();
   }
   return -1;
 }
 }  // namespace cricket
 
 #endif  // HAVE_WEBRTC_VOICE