diff --git a/pc/payload_type_picker.cc b/pc/payload_type_picker.cc index cebc7eb4cc..236b91438e 100644 --- a/pc/payload_type_picker.cc +++ b/pc/payload_type_picker.cc @@ -22,14 +22,6 @@ namespace webrtc { namespace { -// Due to interoperability issues with old Chrome/WebRTC versions that -// ignore the [35, 63] range prefer the lower range for new codecs. -static const int kFirstDynamicPayloadTypeLowerRange = 35; -static const int kLastDynamicPayloadTypeLowerRange = 63; - -static const int kFirstDynamicPayloadTypeUpperRange = 96; -static const int kLastDynamicPayloadTypeUpperRange = 127; - // Note: The only fields we need from a Codec are the type (audio/video), // the subtype (vp8/h264/....), the clock rate, the channel count, and the // fmtp parameters. The use of cricket::Codec, which contains more fields, @@ -43,127 +35,16 @@ bool MatchesForSdp(const cricket::Codec& codec_1, codec_1.params == codec_2.params; } -struct MapTableEntry { - webrtc::SdpAudioFormat format; - int payload_type; -}; - -RTCErrorOr FindFreePayloadType(std::set seen_pt) { - for (auto i = kFirstDynamicPayloadTypeUpperRange; - i < kLastDynamicPayloadTypeUpperRange; i++) { - if (seen_pt.count(PayloadType(i)) == 0) { - return PayloadType(i); - } - } - for (auto i = kFirstDynamicPayloadTypeLowerRange; - i < kLastDynamicPayloadTypeLowerRange; i++) { - if (seen_pt.count(PayloadType(i)) == 0) { - return PayloadType(i); - } - } - return RTCError(RTCErrorType::RESOURCE_EXHAUSTED, - "All available dynamic PTs have been assigned"); -} - } // namespace -PayloadTypePicker::PayloadTypePicker() { - // Default audio codecs. Duplicates media/engine/payload_type_mapper.cc - const MapTableEntry default_audio_mappings[] = { - // Static payload type assignments according to RFC 3551. - {{cricket::kPcmuCodecName, 8000, 1}, 0}, - {{"GSM", 8000, 1}, 3}, - {{"G723", 8000, 1}, 4}, - {{"DVI4", 8000, 1}, 5}, - {{"DVI4", 16000, 1}, 6}, - {{"LPC", 8000, 1}, 7}, - {{cricket::kPcmaCodecName, 8000, 1}, 8}, - {{cricket::kG722CodecName, 8000, 1}, 9}, - {{cricket::kL16CodecName, 44100, 2}, 10}, - {{cricket::kL16CodecName, 44100, 1}, 11}, - {{"QCELP", 8000, 1}, 12}, - {{cricket::kCnCodecName, 8000, 1}, 13}, - // RFC 4566 is a bit ambiguous on the contents of the "encoding - // parameters" field, which, for audio, encodes the number of - // channels. It is "optional and may be omitted if the number of - // channels is one". Does that necessarily imply that an omitted - // encoding parameter means one channel? Since RFC 3551 doesn't - // specify a value for this parameter for MPA, I've included both 0 - // and 1 here, to increase the chances it will be correctly used if - // someone implements an MPEG audio encoder/decoder. - {{"MPA", 90000, 0}, 14}, - {{"MPA", 90000, 1}, 14}, - {{"G728", 8000, 1}, 15}, - {{"DVI4", 11025, 1}, 16}, - {{"DVI4", 22050, 1}, 17}, - {{"G729", 8000, 1}, 18}, - - // Payload type assignments currently used by WebRTC. - // Includes data to reduce collisions (and thus reassignments) - {{cricket::kIlbcCodecName, 8000, 1}, 102}, - {{cricket::kCnCodecName, 16000, 1}, 105}, - {{cricket::kCnCodecName, 32000, 1}, 106}, - {{cricket::kOpusCodecName, - 48000, - 2, - {{cricket::kCodecParamMinPTime, "10"}, - {cricket::kCodecParamUseInbandFec, cricket::kParamValueTrue}}}, - 111}, - // RED for opus is assigned in the lower range, starting at the top. - // Note that the FMTP refers to the opus payload type. - {{cricket::kRedCodecName, - 48000, - 2, - {{cricket::kCodecParamNotInNameValueFormat, "111/111"}}}, - 63}, - // TODO(solenberg): Remove the hard coded 16k,32k,48k DTMF once we - // assign payload types dynamically for send side as well. - {{cricket::kDtmfCodecName, 48000, 1}, 110}, - {{cricket::kDtmfCodecName, 32000, 1}, 112}, - {{cricket::kDtmfCodecName, 16000, 1}, 113}, - {{cricket::kDtmfCodecName, 8000, 1}, 126}}; - for (auto entry : default_audio_mappings) { - AddMapping(PayloadType(entry.payload_type), - cricket::CreateAudioCodec(entry.format)); - } -} - RTCErrorOr PayloadTypePicker::SuggestMapping( - cricket::Codec codec, - PayloadTypeRecorder* excluder) { - // The first matching entry is returned, unless excluder - // maps it to something different. - for (auto entry : entries_) { - if (MatchesForSdp(entry.codec(), codec)) { - if (excluder) { - auto result = excluder->LookupCodec(entry.payload_type()); - if (result.ok() && !MatchesForSdp(result.value(), codec)) { - continue; - } - } - return entry.payload_type(); - } - } - RTCErrorOr found_pt = FindFreePayloadType(seen_payload_types_); - if (found_pt.ok()) { - AddMapping(found_pt.value(), codec); - } - return found_pt; + cricket::Codec codec) { + return RTCError(RTCErrorType::UNSUPPORTED_OPERATION, "Not implemented yet"); } RTCError PayloadTypePicker::AddMapping(PayloadType payload_type, cricket::Codec codec) { - // Completely duplicate mappings are ignored. - // Multiple mappings for the same codec and the same PT are legal; - for (auto entry : entries_) { - if (payload_type == entry.payload_type() && - MatchesForSdp(codec, entry.codec())) { - return RTCError::OK(); - } - } - entries_.emplace_back(MapEntry(payload_type, codec)); - seen_payload_types_.emplace(payload_type); - return RTCError::OK(); + return RTCError(RTCErrorType::UNSUPPORTED_OPERATION, "Not implemented yet"); } RTCError PayloadTypeRecorder::AddMapping(PayloadType payload_type, @@ -171,6 +52,9 @@ RTCError PayloadTypeRecorder::AddMapping(PayloadType payload_type, auto existing_codec_it = payload_type_to_codec_.find(payload_type); if (existing_codec_it != payload_type_to_codec_.end() && !MatchesForSdp(codec, existing_codec_it->second)) { + RTC_LOG(LS_ERROR) << "DEBUG: AddMapping invalid, PT " << int(payload_type) + << " new codec " << codec.ToString() << " old codec " + << existing_codec_it->second.ToString(); return RTCError(RTCErrorType::INVALID_PARAMETER, "Attempt to insert duplicate mapping for PT"); } diff --git a/pc/payload_type_picker.h b/pc/payload_type_picker.h index f29979759b..a1dd946fc6 100644 --- a/pc/payload_type_picker.h +++ b/pc/payload_type_picker.h @@ -12,7 +12,6 @@ #define PC_PAYLOAD_TYPE_PICKER_H_ #include -#include #include #include @@ -30,35 +29,16 @@ class PayloadType : public StrongAlias { constexpr operator uint8_t() const& { return value_; } // NOLINT: Explicit }; -class PayloadTypeRecorder; - class PayloadTypePicker { public: - PayloadTypePicker(); + PayloadTypePicker() = default; PayloadTypePicker(const PayloadTypePicker&) = delete; PayloadTypePicker& operator=(const PayloadTypePicker&) = delete; PayloadTypePicker(PayloadTypePicker&&) = delete; PayloadTypePicker& operator=(PayloadTypePicker&&) = delete; - // Suggest a payload type for the codec. - // If the excluder maps it to something different, don't suggest it. - RTCErrorOr SuggestMapping(cricket::Codec codec, - PayloadTypeRecorder* excluder); + + RTCErrorOr SuggestMapping(cricket::Codec codec); RTCError AddMapping(PayloadType payload_type, cricket::Codec codec); - - private: - class MapEntry { - public: - MapEntry(PayloadType payload_type, cricket::Codec codec) - : payload_type_(payload_type), codec_(codec) {} - PayloadType payload_type() const { return payload_type_; } - cricket::Codec codec() const { return codec_; } - - private: - PayloadType payload_type_; - cricket::Codec codec_; - }; - std::vector entries_; - std::set seen_payload_types_; }; class PayloadTypeRecorder { diff --git a/pc/payload_type_picker_unittest.cc b/pc/payload_type_picker_unittest.cc index 628a06d346..0e486bf506 100644 --- a/pc/payload_type_picker_unittest.cc +++ b/pc/payload_type_picker_unittest.cc @@ -91,48 +91,4 @@ TEST(PayloadTypePicker, RollbackAndCommit) { } } -TEST(PayloadTypePicker, StaticValueIsGood) { - PayloadTypePicker picker; - cricket::Codec a_codec = - cricket::CreateAudioCodec(-1, cricket::kPcmuCodecName, 8000, 1); - auto result = picker.SuggestMapping(a_codec, nullptr); - // In the absence of existing mappings, PCMU always has 0 as PT. - ASSERT_TRUE(result.ok()); - EXPECT_EQ(result.value(), PayloadType(0)); -} - -TEST(PayloadTypePicker, DynamicValueIsGood) { - PayloadTypePicker picker; - cricket::Codec a_codec = cricket::CreateAudioCodec(-1, "lyra", 8000, 1); - auto result = picker.SuggestMapping(a_codec, nullptr); - // This should result in a value from the dynamic range; since this is the - // first assignment, it should be in the upper range. - ASSERT_TRUE(result.ok()); - EXPECT_GE(result.value(), PayloadType(96)); - EXPECT_LE(result.value(), PayloadType(127)); -} - -TEST(PayloadTypePicker, RecordedValueReturned) { - PayloadTypePicker picker; - PayloadTypeRecorder recorder(picker); - cricket::Codec a_codec = cricket::CreateAudioCodec(-1, "lyra", 8000, 1); - recorder.AddMapping(47, a_codec); - auto result = picker.SuggestMapping(a_codec, &recorder); - ASSERT_TRUE(result.ok()); - EXPECT_EQ(47, result.value()); -} - -TEST(PayloadTypePicker, RecordedValueExcluded) { - PayloadTypePicker picker; - PayloadTypeRecorder recorder1(picker); - PayloadTypeRecorder recorder2(picker); - cricket::Codec a_codec = cricket::CreateAudioCodec(-1, "lyra", 8000, 1); - cricket::Codec b_codec = cricket::CreateAudioCodec(-1, "mlcodec", 8000, 1); - recorder1.AddMapping(47, a_codec); - recorder2.AddMapping(47, b_codec); - auto result = picker.SuggestMapping(b_codec, &recorder1); - ASSERT_TRUE(result.ok()); - EXPECT_NE(47, result.value()); -} - } // namespace webrtc