WebRtcVoiceMediaChannel::AddRecvStream: Don't call SetRecPayloadType

webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc

 /*
  *  Copyright (c) 2013 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.
  */
 
 #include "webrtc/modules/rtp_rtcp/include/rtp_payload_registry.h"
 
 #include <algorithm>
 
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/stringutils.h"
 #include "webrtc/common_types.h"
+#include "webrtc/modules/audio_coding/codecs/audio_format_conversion.h"
 #include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 
 namespace webrtc {
 
 namespace {
 
 bool PayloadIsCompatible(const RtpUtility::Payload& payload,
                          const CodecInst& audio_codec) {
   if (!payload.audio)
     return false;
@@ skipping 83 matching lines @@
 
 RTPPayloadRegistry::RTPPayloadRegistry()
     : incoming_payload_type_(-1),
       last_received_payload_type_(-1),
       last_received_media_payload_type_(-1),
       rtx_(false),
       ssrc_rtx_(0) {}
 
 RTPPayloadRegistry::~RTPPayloadRegistry() = default;
 
+bool RTPPayloadRegistry::SetAudioReceivePayloads(

[the sun, 2017/03/14 20:07:01]

Like with DecoderDatabase, we should be able to trust the upper layer (WVoMC) to
provide us a valid set of mappings. Here, we should DCHECK that the set is good,
since that is in the method's contract, but we needn't work around.

[kwiberg-webrtc, 2017/03/16 10:24:12]

Done.

+    std::map<int, SdpAudioFormat> codecs) {
+  for (const auto& kv : codecs) {
+    if (!IsPayloadTypeValid(kv.first)) {
+      return false;
+    }
+  }
+
+  // Make sure we have no collisions.
+  auto existing_it = payload_type_map_.cbegin();
+  auto new_it = codecs.cbegin();
+  while (existing_it != payload_type_map_.cend() && new_it != codecs.cend()) {
+    if (existing_it->second.audio) {
+      ++existing_it;
+    } else {
+      if (existing_it->first == new_it->first) {
+        return false;  // Trying to overwrite a video PT with audio.

[the sun, 2017/03/14 20:07:01]

This should never happen, since we have an RTPPayloadRegistry instance per
voe::Channel.

[kwiberg-webrtc, 2017/03/16 10:24:12]

Acknowledged. I'll try adding DCHECKs that verify this.

+      }
+      if (existing_it->first < new_it->first) {
+        ++existing_it;
+      } else {
+        ++new_it;
+      }
+    }
+  }
+
+  // Remove existing audio payload types.

[the sun, 2017/03/14 20:07:01]

This shouldn't be allowed.

[kwiberg-webrtc, 2017/03/16 10:24:12]

In principle, yes. See other comments...

+  for (auto it = payload_type_map_.begin(); it != payload_type_map_.end();) {
+    if (it->second.audio) {
+      it = payload_type_map_.erase(it);
+    } else {
+      ++it;
+    }
+  }
+
+  // Insert the new payload types.
+  for (const auto& kv : codecs) {
+    const int& rtp_payload_type = kv.first;
+    const SdpAudioFormat& audio_format = kv.second;

[the sun, 2017/03/14 20:07:01]

I suggest you do this conversion in voe::Channel, to avoid the dependency on
audio_codec_api in the rtp_rtcp_module. Then this function could just take a
vector of CodecInst.

[kwiberg-webrtc, 2017/03/16 10:24:12]

Well, yes... but I'm trying to reduce the use of CodecInst. Adding another use
of it in an interface seems like a bad idea.

I think that the rtp_rtcp module needs only a subset of the information in
CodecInst/SdpAudioFormat, but actually switching to a type that's appropriate
may be more churn than we want in this CL, so a follow-up CL seems called for.

+    const CodecInst ci = SdpToCodecInst(rtp_payload_type, audio_format);
+    payload_type_map_[rtp_payload_type] = CreatePayloadType(ci);
+  }
+
+  // Clear the value of last received payload type since it might mean
+  // something else now.
+  last_received_payload_type_ = -1;
+  last_received_media_payload_type_ = -1;
+  return true;

[the sun, 2017/03/14 20:07:01]

If you address the above I don't think we even need a a return value.

[kwiberg-webrtc, 2017/03/16 10:24:12]

Acknowledged.

+}
+
 int32_t RTPPayloadRegistry::RegisterReceivePayload(const CodecInst& audio_codec,
                                                    bool* created_new_payload) {
   *created_new_payload = false;
   if (!IsPayloadTypeValid(audio_codec.pltype))
     return -1;
 
   rtc::CritScope cs(&crit_sect_);
 
   auto it = payload_type_map_.find(audio_codec.pltype);
   if (it != payload_type_map_.end()) {
@@ skipping 254 matching lines @@
     const char* payload_name) const {
   rtc::CritScope cs(&crit_sect_);
   for (const auto& it : payload_type_map_) {
     if (_stricmp(it.second.name, payload_name) == 0)
       return it.first;
   }
   return -1;
 }
 
 }  // namespace webrtc