Pass SdpAudioFormat through Channel, without converting to CodecInst

webrtc/media/engine/webrtcvoiceengine.cc

Index: webrtc/media/engine/webrtcvoiceengine.cc
diff --git a/webrtc/media/engine/webrtcvoiceengine.cc b/webrtc/media/engine/webrtcvoiceengine.cc
index eec9340640493e4b90afa6dac79108267062563f..b8a0db6ac6cf3bb1a3b774b7928b389be3d5fc7d 100644
--- a/webrtc/media/engine/webrtcvoiceengine.cc
+++ b/webrtc/media/engine/webrtcvoiceengine.cc
@@ -1467,14 +1467,15 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream {
     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,
-                               use_transport_cc,
-                               use_nack,
-                               extensions);
+    RecreateAudioReceiveStream();
   }
 
   ~WebRtcAudioReceiveStream() {
@@ -1484,27 +1485,38 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream {
 
   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) {
+      std::vector<webrtc::RtpExtension>&& extensions) {
+    RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
+    config_.rtp.extensions = std::move(extensions);
+    RecreateAudioReceiveStream();
+  }
+
+  // Set a new payload type -> decoder map. The new map must be a superset of
+  // the old one.
+  bool RecreateAudioReceiveStream(
+      std::map<int, webrtc::SdpAudioFormat>&& decoder_map) {
     RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
-    RecreateAudioReceiveStream(config_.rtp.local_ssrc,
-                               config_.rtp.transport_cc,
-                               config_.rtp.nack.rtp_history_ms != 0,
-                               extensions);
+    for (const auto& item : config_.decoder_map) {

[the sun, 2016/12/08 10:53:52]

This is a good check, but it has (or *should* have) already been done in
WVoMC::SetRecvCodecs(). I think we should put it as a lambda inside a DCHECK so
we strip it out in release builds.

[kwiberg-webrtc, 2016/12/09 02:39:01]

Done. Also changed return type to void.

+      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.
+      }
+    }
+    config_.decoder_map = std::move(decoder_map);
+    RecreateAudioReceiveStream();
+    return true;
   }
 
   webrtc::AudioReceiveStream::Stats GetStats() const {
@@ -1542,21 +1554,11 @@ class WebRtcVoiceMediaChannel::WebRtcAudioReceiveStream {
   }
 
  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_);
@@ -1665,7 +1667,8 @@ bool WebRtcVoiceMediaChannel::SetRecvParameters(
   if (recv_rtp_extensions_ != filtered_extensions) {
     recv_rtp_extensions_.swap(filtered_extensions);
     for (auto& it : recv_streams_) {
-      it.second->RecreateAudioReceiveStream(recv_rtp_extensions_);
+      auto ext = recv_rtp_extensions_;  // Make a copy.

[the sun, 2016/12/08 10:53:52]

To me, the fact that you need to write a comment indicates this practise is more
obscure than simply pushing the extensions as a const& to
RecreateAudioReceiveStream(). Is there really any benefit except for consistency
here?

[kwiberg-webrtc, 2016/12/09 02:39:01]

I declared RecreateAudioReceiveStreamn to take an rvalue reference, so the copy
needs to be explicit. (This is on purpose, since copying a std::map isn't
cheap.) Do you think this is a bad idea?

[the sun, 2016/12/09 12:29:49]

I think it's a little unnecessary. Sometimes we need to make copies. Yes,
copying a map is more expensive than copying a string, but in this case it
happens relatively rarely. Making the code easy on the eye is also important. 

[ossu, 2016/12/09 13:23:39]

I think I agree with solenberg here. This map isn't very large, and we're not
recreating streams that often. Also, we're doing the copy anyway, so we're not
saving anything at this call site. Are we at any other?

[kwiberg-webrtc, 2016/12/11 11:24:55]

Not sure. We may be, but I guess it doesn't matter that much.

The idea is that for expensive-to-copy but copyable types like this, you get the
optimal behavior by always taking arguments of the form T&& if the callee will
always keep a copy, since that way, if a copy is going to be made it has to be
an explicit copy at the call site. (Ideally, there'd be a utility function for
this, such as std::copy, analogously to std::move. But since there isn't such a
function, you get the extra auto statement instead.) The same could be done for
move-only types, and it would be more efficient for them too since passing
references around is at least as cheap as moving. (But for the overwhelmingly
most important move-only type, std::unique_ptr, the benefit would be negligible
at best since a move is just a copying a pointer and zeroing the source
pointer.)

This is analogous with the idiom of passing arguments as const T& whenever the
callee will not keep a copy, or if the caller is expected to want to keep a
copy.

So I'll take your advice and pass const T& instead. I still think the
pass-by-T&& idiom makes sense, in this particular case and in general, but the
performance benefits may not be great enough in this case to justify it given
that the pass-by-T&& idiom isn't familiar to people.

+      it.second->RecreateAudioReceiveStream(std::move(ext));
     }
   }
   return true;
@@ -1847,29 +1850,17 @@ bool WebRtcVoiceMediaChannel::SetRecvCodecs(
     ChangePlayout(false);
   }
 
-  bool result = true;
-  for (const AudioCodec& codec : new_codecs) {
-    webrtc::CodecInst voe_codec = {0};
-    if (WebRtcVoiceEngine::ToCodecInst(codec, &voe_codec)) {
-      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;
-    }
+  std::map<int, webrtc::SdpAudioFormat> decoder_map;
+  for (const AudioCodec& codec : codecs) {
+    LOG(LS_INFO) << ToString(codec);
+    decoder_map.insert({codec.id, AudioCodecToSdp(codec)});
   }
-  if (result) {
-    recv_codecs_ = codecs;
+  bool result = true;
+  for (auto& it : recv_streams_) {

[the sun, 2016/12/08 10:53:52]

nit: it -> kv

[kwiberg-webrtc, 2016/12/09 02:39:01]

Done.

+    auto dm = decoder_map;  // Make a copy.

[the sun, 2016/12/08 10:53:52]

Same comment as on line 1670

+    result = it.second->RecreateAudioReceiveStream(std::move(dm)) && result;
   }
+  recv_codecs_ = codecs;
 
   if (desired_playout_ && !playout_) {
     ChangePlayout(desired_playout_);