WebRtcVoiceMediaChannel::AddRecvStream: Don't call SetRecPayloadType

webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc

Index: webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc
index 3d2aad42a0160170b3e6be6cfe55dc05834bb213..9bbb1d273cc8d7d442e5217cf48ea8ec8287000e 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc
@@ -16,6 +16,7 @@
 #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 {
@@ -119,6 +120,56 @@ RTPPayloadRegistry::RTPPayloadRegistry()
 
 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;