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,
-                               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) {
+      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 = 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());
+    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 {
     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 88 matching lines @@
 
   if (!ValidateRtpExtensions(params.extensions)) {
     return false;
   }
   std::vector<webrtc::RtpExtension> filtered_extensions =
       FilterRtpExtensions(params.extensions,
                           webrtc::RtpExtension::IsSupportedForAudio, false);
   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;
 }
 
 webrtc::RtpParameters WebRtcVoiceMediaChannel::GetRtpSendParameters(
     uint32_t ssrc) const {
   RTC_DCHECK(worker_thread_checker_.CalledOnValidThread());
   auto it = send_streams_.find(ssrc);
   if (it == send_streams_.end()) {
@@ skipping 161 matching lines @@
     // never removed.
     return true;
   }
 
   if (playout_) {
     // Receive codecs can not be changed while playing. So we temporarily
     // pause playout.
     ChangePlayout(false);
   }
 
+  std::map<int, webrtc::SdpAudioFormat> decoder_map;
+  for (const AudioCodec& codec : codecs) {
+    LOG(LS_INFO) << ToString(codec);
+    decoder_map.insert({codec.id, AudioCodecToSdp(codec)});
+  }
   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;
-    }
+  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;
   }
-  if (result) {
-    recv_codecs_ = codecs;
-  }
+  recv_codecs_ = codecs;
 
   if (desired_playout_ && !playout_) {
     ChangePlayout(desired_playout_);
   }
   return result;
 }
 
 // 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.
@@ skipping 768 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