From 882b32d00f6330da948d8e578d259dec0078e7ed Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 12 Dec 2024 22:15:39 +0000 Subject: [PATCH] Reland "Use PayloadTypePicker for video PT assignment" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit e046787a5a80a9d292b3aec7e946644e025a2b95. Reason for revert: Revised codec matching to fix issue. Changes also back out some changes that should not have been included (using PayloadTypePicker for codec list merging). Original change's description: > Revert "Use PayloadTypePicker for video PT assignment" > > This reverts commit e5048949b0fcc275264e24f3b2a4c658fcc84aa3. > > Reason for revert: Broke internal tests. > > Original change's description: > > Use PayloadTypePicker for video PT assignment > > > > This includes changes that change the order of codecs. > > It is preparatory to doing late assignment of video PTs. > > > > Bug: webrtc:360058654 > > Change-Id: Id5ddaf94d4b9557c0502a373e42635108d8fdf26 > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/366400 > > Reviewed-by: Henrik Boström > > Commit-Queue: Harald Alvestrand > > Cr-Commit-Position: refs/heads/main@{#43489} > > Bug: webrtc:360058654 > Change-Id: I5c94a7bafa49bdf17f665480398707155e458d26 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/370240 > Bot-Commit: rubber-stamper@appspot.gserviceaccount.com > Commit-Queue: Harald Alvestrand > Cr-Commit-Position: refs/heads/main@{#43490} Bug: webrtc:360058654 Change-Id: I66b3b6bd657c66f8860c5e67a504266d7707f48d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/370380 Commit-Queue: Harald Alvestrand Reviewed-by: Artem Titov Cr-Commit-Position: refs/heads/main@{#43554} --- api/video_codecs/h264_profile_level_id.cc | 12 ++ api/video_codecs/h264_profile_level_id.h | 4 + call/BUILD.gn | 11 ++ call/DEPS | 3 + call/fake_payload_type_suggester.h | 45 ++++++ call/payload_type.h | 14 ++ call/payload_type_picker.cc | 86 ++++++++-- call/payload_type_picker_unittest.cc | 32 ++++ media/BUILD.gn | 2 + media/DEPS | 3 + media/base/codec_comparators.cc | 5 + media/base/codec_comparators.h | 9 +- media/engine/fake_webrtc_call.h | 26 +-- media/engine/webrtc_video_engine.cc | 184 +++++++++++----------- pc/BUILD.gn | 1 + pc/media_session_unittest.cc | 96 +++++++---- pc/sdp_offer_answer.cc | 4 +- pc/sdp_offer_answer_unittest.cc | 41 +++-- pc/test/integration_test_helpers.h | 4 +- 19 files changed, 398 insertions(+), 184 deletions(-) create mode 100644 call/fake_payload_type_suggester.h diff --git a/api/video_codecs/h264_profile_level_id.cc b/api/video_codecs/h264_profile_level_id.cc index 9a2dca98d1..0bdaed7fd3 100644 --- a/api/video_codecs/h264_profile_level_id.cc +++ b/api/video_codecs/h264_profile_level_id.cc @@ -257,4 +257,16 @@ bool H264IsSameProfile(const CodecParameterMap& params1, profile_level_id->profile == other_profile_level_id->profile; } +bool H264IsSameProfileAndLevel(const CodecParameterMap& params1, + const CodecParameterMap& params2) { + const std::optional profile_level_id = + ParseSdpForH264ProfileLevelId(params1); + const std::optional other_profile_level_id = + ParseSdpForH264ProfileLevelId(params2); + // Compare H264 profiles, but not levels. + return profile_level_id && other_profile_level_id && + profile_level_id->profile == other_profile_level_id->profile && + profile_level_id->level == other_profile_level_id->level; +} + } // namespace webrtc diff --git a/api/video_codecs/h264_profile_level_id.h b/api/video_codecs/h264_profile_level_id.h index 593b8e901e..1b21f18834 100644 --- a/api/video_codecs/h264_profile_level_id.h +++ b/api/video_codecs/h264_profile_level_id.h @@ -86,6 +86,10 @@ RTC_EXPORT std::optional H264ProfileLevelIdToString( // etc). RTC_EXPORT bool H264IsSameProfile(const CodecParameterMap& params1, const CodecParameterMap& params2); +// Returns true if the parameters have the same H264 profile (Baseline, High, +// etc) and same level. +RTC_EXPORT bool H264IsSameProfileAndLevel(const CodecParameterMap& params1, + const CodecParameterMap& params2); } // namespace webrtc diff --git a/call/BUILD.gn b/call/BUILD.gn index 7637f4f969..8f37af0be1 100644 --- a/call/BUILD.gn +++ b/call/BUILD.gn @@ -773,6 +773,17 @@ if (rtc_include_tests) { ] } + rtc_source_set("fake_payload_type_suggester") { + testonly = true + sources = [ "fake_payload_type_suggester.h" ] + deps = [ + ":payload_type", + ":payload_type_picker", + "../api:rtc_error", + "../media:codec", + ] + } + rtc_library("fake_network_pipe_unittests") { testonly = true diff --git a/call/DEPS b/call/DEPS index e74b22f677..98a8a4b68d 100644 --- a/call/DEPS +++ b/call/DEPS @@ -70,5 +70,8 @@ specific_include_rules = { ], "call\.cc": [ "+media/base/codec.h", + ], + "fake_payload_type_suggester": [ + "+media/base/codec.h", ] } diff --git a/call/fake_payload_type_suggester.h b/call/fake_payload_type_suggester.h new file mode 100644 index 0000000000..c2aa6ebd01 --- /dev/null +++ b/call/fake_payload_type_suggester.h @@ -0,0 +1,45 @@ +/* + * Copyright (c) 2015 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. + */ + +#ifndef CALL_FAKE_PAYLOAD_TYPE_SUGGESTER_H_ +#define CALL_FAKE_PAYLOAD_TYPE_SUGGESTER_H_ + +#include + +#include "api/rtc_error.h" +#include "call/payload_type.h" +#include "call/payload_type_picker.h" +#include "media/base/codec.h" + +namespace webrtc { +// Fake payload type suggester, for use in tests. +// It uses a real PayloadTypePicker in order to do consistent PT +// assignment. +class FakePayloadTypeSuggester : public webrtc::PayloadTypeSuggester { + public: + webrtc::RTCErrorOr SuggestPayloadType( + const std::string& mid, + cricket::Codec codec) override { + // Ignores mid argument. + return pt_picker_.SuggestMapping(codec, nullptr); + } + webrtc::RTCError AddLocalMapping(const std::string& mid, + webrtc::PayloadType payload_type, + const cricket::Codec& codec) override { + return webrtc::RTCError::OK(); + } + + private: + webrtc::PayloadTypePicker pt_picker_; +}; + +} // namespace webrtc + +#endif // CALL_FAKE_PAYLOAD_TYPE_SUGGESTER_H_ diff --git a/call/payload_type.h b/call/payload_type.h index c0520e8f4c..672c489fc6 100644 --- a/call/payload_type.h +++ b/call/payload_type.h @@ -26,11 +26,25 @@ class PayloadType : public StrongAlias { // removed once calling code is upgraded. PayloadType(uint8_t pt) { value_ = pt; } // NOLINT: explicit constexpr operator uint8_t() const& { return value_; } // NOLINT: Explicit + static bool IsValid(PayloadType id, bool rtcp_mux) { + // A payload type is a 7-bit value in the RTP header, so max = 127. + // If RTCP multiplexing is used, the numbers from 64 to 95 are reserved + // for RTCP packets. + if (rtcp_mux && (id > 63 && id < 96)) { + return false; + } + return id >= 0 && id <= 127; + } + template + friend void AbslStringify(Sink& sink, const PayloadType pt) { + absl::Format(&sink, "%d", pt.value_); + } }; class PayloadTypeSuggester { public: virtual ~PayloadTypeSuggester() = default; + // Suggest a payload type for a given codec on a given media section. // Media section is indicated by MID. // The function will either return a PT already in use on the connection diff --git a/call/payload_type_picker.cc b/call/payload_type_picker.cc index 3e75db4c37..d26abb754d 100644 --- a/call/payload_type_picker.cc +++ b/call/payload_type_picker.cc @@ -12,6 +12,7 @@ #include #include +#include #include #include @@ -47,21 +48,78 @@ struct MapTableEntry { int payload_type; }; -RTCErrorOr FindFreePayloadType(std::set seen_pt) { +// Helper function to determine whether a codec should use the [35, 63] range. +// Should be used when adding new codecs (or variants). +bool CodecPrefersLowerRange(const cricket::Codec& codec) { + // All audio codecs prefer upper range. + if (codec.type == cricket::Codec::Type::kAudio) { + return false; + } + if (absl::EqualsIgnoreCase(codec.name, cricket::kFlexfecCodecName) || + absl::EqualsIgnoreCase(codec.name, cricket::kAv1CodecName)) { + return true; + } else if (absl::EqualsIgnoreCase(codec.name, cricket::kH264CodecName)) { + std::string profile_level_id; + std::string packetization_mode; + + if (codec.GetParam(cricket::kH264FmtpProfileLevelId, &profile_level_id)) { + if (absl::StartsWithIgnoreCase(profile_level_id, "4d00")) { + if (codec.GetParam(cricket::kH264FmtpPacketizationMode, + &packetization_mode)) { + return packetization_mode == "0"; + } + } + // H264 with YUV444. + return absl::StartsWithIgnoreCase(profile_level_id, "f400"); + } + } else if (absl::EqualsIgnoreCase(codec.name, cricket::kVp9CodecName)) { + std::string profile_id; + + if (codec.GetParam(cricket::kVP9ProfileId, &profile_id)) { + if (profile_id.compare("1") == 0 || profile_id.compare("3") == 0) { + return true; + } + } + } + return false; +} + +RTCErrorOr FindFreePayloadType(const cricket::Codec& codec, + std::set seen_pt) { + // Prefer to use lower range for codecs that can handle it. + bool prefer_lower_range = CodecPrefersLowerRange(codec); + if (prefer_lower_range) { + for (auto i = kFirstDynamicPayloadTypeLowerRange; + i <= kLastDynamicPayloadTypeLowerRange; i++) { + if (seen_pt.count(PayloadType(i)) == 0) { + return PayloadType(i); + } + } + } for (auto i = kFirstDynamicPayloadTypeUpperRange; - i < kLastDynamicPayloadTypeUpperRange; i++) { + 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); + // If the upper range is full, we do lower range also for codecs + // that prefer the upper range. + if (!prefer_lower_range) { + 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"); + if (prefer_lower_range) { + return RTCError(RTCErrorType::RESOURCE_EXHAUSTED, + "All available dynamic PTs have been assigned"); + } else { + return RTCError( + RTCErrorType::RESOURCE_EXHAUSTED, + "All available dynamic PTs have been assigned, codec preferred upper"); + } } } // namespace @@ -141,10 +199,11 @@ RTCErrorOr PayloadTypePicker::SuggestMapping( // The first matching entry is returned, unless excluder // maps it to something different. for (auto entry : entries_) { - if (MatchesWithCodecRules(entry.codec(), codec)) { + if (MatchesWithReferenceAttributes(entry.codec(), codec)) { if (excluder) { auto result = excluder->LookupCodec(entry.payload_type()); - if (result.ok() && !MatchesWithCodecRules(result.value(), codec)) { + if (result.ok() && + !MatchesWithReferenceAttributes(result.value(), codec)) { continue; } } @@ -152,7 +211,8 @@ RTCErrorOr PayloadTypePicker::SuggestMapping( } } // Assign the first free payload type. - RTCErrorOr found_pt = FindFreePayloadType(seen_payload_types_); + RTCErrorOr found_pt = + FindFreePayloadType(codec, seen_payload_types_); if (found_pt.ok()) { AddMapping(found_pt.value(), codec); } @@ -165,7 +225,7 @@ RTCError PayloadTypePicker::AddMapping(PayloadType payload_type, // Multiple mappings for the same codec and the same PT are legal; for (auto entry : entries_) { if (payload_type == entry.payload_type() && - MatchesWithCodecRules(codec, entry.codec())) { + MatchesWithReferenceAttributes(codec, entry.codec())) { return RTCError::OK(); } } @@ -225,7 +285,7 @@ RTCErrorOr PayloadTypeRecorder::LookupPayloadType( auto result = std::find_if(payload_type_to_codec_.begin(), payload_type_to_codec_.end(), [codec](const auto& iter) { - return MatchesWithCodecRules(iter.second, codec); + return MatchesWithReferenceAttributes(iter.second, codec); }); if (result == payload_type_to_codec_.end()) { return RTCError(RTCErrorType::INVALID_PARAMETER, diff --git a/call/payload_type_picker_unittest.cc b/call/payload_type_picker_unittest.cc index 6e4666ed8a..fd64923b84 100644 --- a/call/payload_type_picker_unittest.cc +++ b/call/payload_type_picker_unittest.cc @@ -19,6 +19,8 @@ namespace webrtc { using testing::Eq; +using testing::Ge; +using testing::Lt; using testing::Ne; TEST(PayloadTypePicker, PayloadTypeAssignmentWorks) { @@ -178,6 +180,36 @@ TEST(PayloadTypePicker, RecordedValueExcluded) { EXPECT_NE(47, result.value()); } +TEST(PayloadTypePicker, AudioGetsHigherRange) { + PayloadTypePicker picker; + cricket::Codec an_audio_codec = + cricket::CreateAudioCodec(-1, "lyra", 8000, 1); + auto result = picker.SuggestMapping(an_audio_codec, nullptr).value(); + EXPECT_THAT(result, Ge(96)); +} + +TEST(PayloadTypePicker, VideoGetsTreatedSpecially) { + PayloadTypePicker picker; + cricket::Codec h264_constrained = cricket::CreateVideoCodec(SdpVideoFormat( + cricket::kH264CodecName, {{cricket::kH264FmtpProfileLevelId, "42e01f"}, + {cricket::kH264FmtpLevelAsymmetryAllowed, "1"}, + {cricket::kH264FmtpPacketizationMode, "1"}})); + cricket::Codec h264_yuv444 = cricket::CreateVideoCodec(SdpVideoFormat( + cricket::kH264CodecName, {{cricket::kH264FmtpProfileLevelId, "f4001f"}, + {cricket::kH264FmtpLevelAsymmetryAllowed, "1"}, + {cricket::kH264FmtpPacketizationMode, "1"}})); + cricket::Codec vp9_profile_2 = cricket::CreateVideoCodec(SdpVideoFormat( + {cricket::kVp9CodecName, {{cricket::kVP9ProfileId, "2"}}})); + cricket::Codec vp9_profile_3 = cricket::CreateVideoCodec(SdpVideoFormat( + {cricket::kVp9CodecName, {{cricket::kVP9ProfileId, "3"}}})); + // Valid for high range only. + EXPECT_THAT(picker.SuggestMapping(h264_constrained, nullptr).value(), Ge(96)); + EXPECT_THAT(picker.SuggestMapping(vp9_profile_2, nullptr).value(), Ge(96)); + // Valud for lower range. + EXPECT_THAT(picker.SuggestMapping(h264_yuv444, nullptr).value(), Lt(63)); + EXPECT_THAT(picker.SuggestMapping(vp9_profile_3, nullptr).value(), Lt(63)); +} + TEST(PayloadTypePicker, ChoosingH264Profiles) { // No opinion on whether these are right or wrong, just that their // behavior is consistent. diff --git a/media/BUILD.gn b/media/BUILD.gn index a931c6c489..69de926516 100644 --- a/media/BUILD.gn +++ b/media/BUILD.gn @@ -658,6 +658,7 @@ rtc_library("rtc_audio_video") { "../video/config:encoder_config", "//third_party/abseil-cpp/absl/algorithm", "//third_party/abseil-cpp/absl/algorithm:container", + "//third_party/abseil-cpp/absl/container:flat_hash_map", "//third_party/abseil-cpp/absl/container:inlined_vector", "//third_party/abseil-cpp/absl/functional:any_invocable", "//third_party/abseil-cpp/absl/functional:bind_front", @@ -829,6 +830,7 @@ if (rtc_include_tests) { "../api/video:video_rtp_headers", "../api/video_codecs:video_codecs_api", "../call:call_interfaces", + "../call:fake_payload_type_suggester", "../call:mock_rtp_interfaces", "../call:payload_type", "../call:payload_type_picker", diff --git a/media/DEPS b/media/DEPS index 663bf7feef..10636cc5af 100644 --- a/media/DEPS +++ b/media/DEPS @@ -25,6 +25,9 @@ specific_include_rules = { ".*webrtc_video_engine\.h": [ "+video/config", ], + ".*webrtc_video_engine\.cc": [ + "+video/config", + ], ".*media_channel\.h": [ "+video/config", ], diff --git a/media/base/codec_comparators.cc b/media/base/codec_comparators.cc index 49525f48f4..45a1b9b8cf 100644 --- a/media/base/codec_comparators.cc +++ b/media/base/codec_comparators.cc @@ -342,6 +342,11 @@ bool MatchesWithCodecRules(const Codec& left_codec, const Codec& right_codec) { return matches_id && matches_type_specific(); } +bool MatchesWithReferenceAttributes(const Codec& codec1, const Codec& codec2) { + return MatchesWithReferenceAttributesAndComparator( + codec1, codec2, [](int a, int b) { return a == b; }); +} + // Finds a codec in `codecs2` that matches `codec_to_match`, which is // a member of `codecs1`. If `codec_to_match` is an RED or RTX codec, both // the codecs themselves and their associated codecs must match. diff --git a/media/base/codec_comparators.h b/media/base/codec_comparators.h index a797dc8375..64423c7346 100644 --- a/media/base/codec_comparators.h +++ b/media/base/codec_comparators.h @@ -19,14 +19,15 @@ namespace webrtc { -// Comparison used in the PayloadTypePicker -bool MatchesForSdp(const cricket::Codec& codec_1, - const cricket::Codec& codec_2); - // Comparison used for the Codec::Matches function bool MatchesWithCodecRules(const cricket::Codec& left_codec, const cricket::Codec& codec); +// Comparison that also checks on codecs referenced by PT in the +// fmtp line, as used with RED and RTX "codecs". +bool MatchesWithReferenceAttributes(const cricket::Codec& left_codec, + const cricket::Codec& right_codec); + // Finds a codec in `codecs2` that matches `codec_to_match`, which is // a member of `codecs1`. If `codec_to_match` is an RED or RTX codec, both // the codecs themselves and their associated codecs must match. diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index 04724d831f..28dbe294ca 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -37,7 +37,6 @@ #include "api/environment/environment.h" #include "api/frame_transformer_interface.h" #include "api/media_types.h" -#include "api/rtc_error.h" #include "api/rtp_headers.h" #include "api/rtp_parameters.h" #include "api/rtp_sender_interface.h" @@ -54,15 +53,14 @@ #include "call/audio_receive_stream.h" #include "call/audio_send_stream.h" #include "call/call.h" +#include "call/fake_payload_type_suggester.h" #include "call/flexfec_receive_stream.h" #include "call/packet_receiver.h" #include "call/payload_type.h" -#include "call/payload_type_picker.h" #include "call/rtp_transport_controller_send_interface.h" #include "call/test/mock_rtp_transport_controller_send.h" #include "call/video_receive_stream.h" #include "call/video_send_stream.h" -#include "media/base/codec.h" #include "modules/rtp_rtcp/include/receive_statistics.h" #include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "rtc_base/buffer.h" @@ -397,26 +395,6 @@ class FakeFlexfecReceiveStream final : public webrtc::FlexfecReceiveStream { webrtc::FlexfecReceiveStream::Config config_; }; -// Fake payload type suggester. -// This is injected into FakeCall at initialization. -class FakePayloadTypeSuggester : public webrtc::PayloadTypeSuggester { - public: - webrtc::RTCErrorOr SuggestPayloadType( - const std::string& /* mid */, - cricket::Codec codec) override { - // Ignores mid argument. - return pt_picker_.SuggestMapping(codec, nullptr); - } - webrtc::RTCError AddLocalMapping(const std::string& /* mid */, - webrtc::PayloadType /* payload_type */, - const cricket::Codec& /* codec */) override { - return webrtc::RTCError::OK(); - } - - private: - webrtc::PayloadTypePicker pt_picker_; -}; - class FakeCall final : public webrtc::Call, public webrtc::PacketReceiver { public: explicit FakeCall(const webrtc::Environment& env); @@ -563,7 +541,7 @@ class FakeCall final : public webrtc::Call, public webrtc::PacketReceiver { int num_created_send_streams_; int num_created_receive_streams_; - FakePayloadTypeSuggester pt_suggester_; + webrtc::FakePayloadTypeSuggester pt_suggester_; }; } // namespace cricket diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 8cce555a96..8822022103 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -14,28 +14,46 @@ #include #include +#include #include +#include +#include #include #include #include -#include #include +#include #include "absl/algorithm/container.h" -#include "absl/container/inlined_vector.h" #include "absl/functional/bind_front.h" #include "absl/strings/match.h" +#include "absl/strings/string_view.h" +#include "api/array_view.h" +#include "api/crypto/crypto_options.h" +#include "api/crypto/frame_decryptor_interface.h" +#include "api/field_trials_view.h" +#include "api/frame_transformer_interface.h" #include "api/make_ref_counted.h" #include "api/media_stream_interface.h" #include "api/media_types.h" #include "api/priority.h" #include "api/rtc_error.h" +#include "api/rtp_headers.h" #include "api/rtp_parameters.h" +#include "api/rtp_sender_interface.h" #include "api/rtp_transceiver_direction.h" +#include "api/scoped_refptr.h" +#include "api/sequence_checker.h" +#include "api/task_queue/pending_task_safety_flag.h" +#include "api/task_queue/task_queue_base.h" +#include "api/transport/rtp/rtp_source.h" #include "api/units/time_delta.h" #include "api/units/timestamp.h" -#include "api/video/resolution.h" +#include "api/video/recordable_encoded_frame.h" +#include "api/video/video_bitrate_allocator_factory.h" #include "api/video/video_codec_type.h" +#include "api/video/video_sink_interface.h" +#include "api/video/video_source_interface.h" #include "api/video_codecs/scalability_mode.h" #include "api/video_codecs/sdp_video_format.h" #include "api/video_codecs/video_codec.h" @@ -43,24 +61,31 @@ #include "api/video_codecs/video_encoder.h" #include "api/video_codecs/video_encoder_factory.h" #include "call/call.h" +#include "call/flexfec_receive_stream.h" #include "call/packet_receiver.h" +#include "call/payload_type_picker.h" #include "call/receive_stream.h" #include "call/rtp_config.h" #include "call/rtp_transport_controller_send_interface.h" +#include "call/video_receive_stream.h" #include "call/video_send_stream.h" #include "common_video/frame_counts.h" -#include "common_video/include/quality_limitation_reason.h" #include "media/base/codec.h" #include "media/base/codec_comparators.h" #include "media/base/media_channel.h" +#include "media/base/media_channel_impl.h" +#include "media/base/media_config.h" #include "media/base/media_constants.h" +#include "media/base/media_engine.h" #include "media/base/rid_description.h" #include "media/base/rtp_utils.h" +#include "media/base/stream_params.h" #include "media/engine/webrtc_media_engine.h" #include "modules/rtp_rtcp/include/receive_statistics.h" -#include "modules/rtp_rtcp/include/report_block_data.h" #include "modules/rtp_rtcp/include/rtcp_statistics.h" +#include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" +#include "modules/rtp_rtcp/source/rtp_packet_received.h" #include "modules/rtp_rtcp/source/rtp_util.h" #include "modules/video_coding/svc/scalability_mode_util.h" #include "rtc_base/checks.h" @@ -69,8 +94,10 @@ #include "rtc_base/logging.h" #include "rtc_base/socket.h" #include "rtc_base/strings/string_builder.h" +#include "rtc_base/synchronization/mutex.h" #include "rtc_base/time_utils.h" #include "rtc_base/trace_event.h" +#include "video/config/video_encoder_config.h" namespace cricket { @@ -81,10 +108,6 @@ using ::webrtc::ParseRtpSsrc; constexpr int64_t kUnsignaledSsrcCooldownMs = rtc::kNumMillisecsPerSec / 2; -// TODO(bugs.webrtc.org/13166): Remove AV1X when backwards compatibility is not -// needed. -constexpr char kAv1xCodecName[] = "AV1X"; - // This constant is really an on/off, lower-level configurable NACK history // duration hasn't been implemented. const int kNackHistoryMs = 1000; @@ -135,38 +158,6 @@ void AddDefaultFeedbackParams(Codec* codec, } } -// Helper function to determine whether a codec should use the [35, 63] range. -// Should be used when adding new codecs (or variants). -bool IsCodecValidForLowerRange(const Codec& codec) { - if (absl::EqualsIgnoreCase(codec.name, kFlexfecCodecName) || - absl::EqualsIgnoreCase(codec.name, kAv1CodecName) || - absl::EqualsIgnoreCase(codec.name, kAv1xCodecName)) { - return true; - } else if (absl::EqualsIgnoreCase(codec.name, kH264CodecName)) { - std::string profile_level_id; - std::string packetization_mode; - - if (codec.GetParam(kH264FmtpProfileLevelId, &profile_level_id)) { - if (absl::StartsWithIgnoreCase(profile_level_id, "4d00")) { - if (codec.GetParam(kH264FmtpPacketizationMode, &packetization_mode)) { - return packetization_mode == "0"; - } - } - // H264 with YUV444. - return absl::StartsWithIgnoreCase(profile_level_id, "f400"); - } - } else if (absl::EqualsIgnoreCase(codec.name, kVp9CodecName)) { - std::string profile_id; - - if (codec.GetParam(kVP9ProfileId, &profile_id)) { - if (profile_id.compare("1") == 0 || profile_id.compare("3") == 0) { - return true; - } - } - } - return false; -} - // Get the default set of supported codecs. // is_decoder_factory is needed to keep track of the implict assumption that any // H264 decoder also supports constrained base line profile. @@ -208,70 +199,48 @@ std::vector GetDefaultSupportedFormats( // This function will assign dynamic payload types (in the range [96, 127] // and then [35, 63]) to the input codecs, and also add ULPFEC, RED, FlexFEC, -// and associated RTX codecs for recognized codecs (VP8, VP9, H264, and RED). // It will also add default feedback params to the codecs. -std::vector AssignPayloadTypesAndAddRtx( +webrtc::RTCErrorOr> AssignPayloadTypes( const std::vector& supported_formats, - bool include_rtx, + webrtc::PayloadTypePicker& pt_mapper, const webrtc::FieldTrialsView& trials) { - // 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; - int payload_type_upper = kFirstDynamicPayloadTypeUpperRange; - int payload_type_lower = kFirstDynamicPayloadTypeLowerRange; - std::vector output_codecs; for (const webrtc::SdpVideoFormat& format : supported_formats) { Codec codec = cricket::CreateVideoCodec(format); - bool isFecCodec = absl::EqualsIgnoreCase(codec.name, kUlpfecCodecName) || - absl::EqualsIgnoreCase(codec.name, kFlexfecCodecName); - - // Check if we ran out of payload types. - if (payload_type_lower > kLastDynamicPayloadTypeLowerRange) { - // TODO(https://bugs.chromium.org/p/webrtc/issues/detail?id=12248): - // return an error. - RTC_LOG(LS_ERROR) << "Out of dynamic payload types [35,63] after " - "fallback from [96, 127], skipping the rest."; - RTC_DCHECK_EQ(payload_type_upper, kLastDynamicPayloadTypeUpperRange); - break; - } - - // Lower range gets used for "new" codecs or when running out of payload - // types in the upper range. - if (IsCodecValidForLowerRange(codec) || - payload_type_upper >= kLastDynamicPayloadTypeUpperRange) { - codec.id = payload_type_lower++; - } else { - codec.id = payload_type_upper++; - } + codec.id = pt_mapper.SuggestMapping(codec, /* excluder= */ nullptr).value(); + // TODO: https://issues.webrtc.org/360058654 - Handle running out of IDs. AddDefaultFeedbackParams(&codec, trials); output_codecs.push_back(codec); + } + return output_codecs; +} +// This function will add associated RTX codecs for recognized codecs (VP8, VP9, +// H264, and RED). +webrtc::RTCErrorOr> AddRtx( + const std::vector& input_codecs, + webrtc::PayloadTypePicker& pt_mapper) { + std::vector output_codecs; + for (const Codec& codec : input_codecs) { + // We want to interleave the input codecs with the output codecs. + output_codecs.push_back(codec); + bool isFecCodec = absl::EqualsIgnoreCase(codec.name, kUlpfecCodecName) || + absl::EqualsIgnoreCase(codec.name, kFlexfecCodecName); // Add associated RTX codec for non-FEC codecs. - if (include_rtx) { - if (!isFecCodec) { - // Check if we ran out of payload types. - if (payload_type_lower > kLastDynamicPayloadTypeLowerRange) { - // TODO(https://bugs.chromium.org/p/webrtc/issues/detail?id=12248): - // return an error. - RTC_LOG(LS_ERROR) << "Out of dynamic payload types [35,63] after " - "fallback from [96, 127], skipping the rest."; - RTC_DCHECK_EQ(payload_type_upper, kLastDynamicPayloadTypeUpperRange); + if (!isFecCodec) { + Codec rtx_codec = + cricket::CreateVideoRtxCodec(Codec::kIdNotSet, codec.id); + webrtc::RTCErrorOr result = + pt_mapper.SuggestMapping(rtx_codec, /* excluder= */ nullptr); + if (!result.ok()) { + if (result.error().type() == webrtc::RTCErrorType::RESOURCE_EXHAUSTED) { + // Out of payload types. Stop adding RTX codecs. break; } - if (IsCodecValidForLowerRange(codec) || - payload_type_upper >= kLastDynamicPayloadTypeUpperRange) { - output_codecs.push_back( - cricket::CreateVideoRtxCodec(payload_type_lower++, codec.id)); - } else { - output_codecs.push_back( - cricket::CreateVideoRtxCodec(payload_type_upper++, codec.id)); - } + return result.MoveError(); } + rtx_codec.id = result.value(); + output_codecs.push_back(rtx_codec); } } return output_codecs; @@ -288,7 +257,21 @@ std::vector GetPayloadTypesAndDefaultCodecs( auto supported_formats = GetDefaultSupportedFormats(factory, is_decoder_factory, trials); - return AssignPayloadTypesAndAddRtx(supported_formats, include_rtx, trials); + // Temporary: Use PayloadTypePicker for assignments. + // TODO: https://issues.webrtc.org/360058654 - stop assigning PTs here. + webrtc::PayloadTypePicker pt_mapper; + std::vector output_codecs; + webrtc::RTCErrorOr> result = + AssignPayloadTypes(supported_formats, pt_mapper, trials); + RTC_DCHECK(result.ok()); + output_codecs = result.MoveValue(); + if (include_rtx) { + webrtc::RTCErrorOr> result = + AddRtx(output_codecs, pt_mapper); + RTC_DCHECK(result.ok()); + return result.MoveValue(); + } + return output_codecs; } static std::string CodecVectorToString(const std::vector& codecs) { @@ -584,6 +567,7 @@ webrtc::RTCErrorOr> MapCodecs( // `rtx_mapping` maps video payload type to rtx payload type. std::map rtx_mapping; std::map rtx_time_mapping; + std::map defined_codecs; webrtc::UlpfecConfig ulpfec_config; std::optional flexfec_payload_type; @@ -592,11 +576,19 @@ webrtc::RTCErrorOr> MapCodecs( const int payload_type = in_codec.id; if (payload_codec_type.find(payload_type) != payload_codec_type.end()) { - RTC_LOG(LS_ERROR) << "Payload type already registered: " - << in_codec.ToString(); + if (webrtc::MatchesWithCodecRules(defined_codecs.at(in_codec.id), + in_codec)) { + // Ignore second occurence of the same codec. + // This can happen with multiple H.264 profiles. + continue; + } + RTC_LOG(LS_ERROR) << "Duplicate codec ID, rejecting " << in_codec + << " because " << defined_codecs.at(in_codec.id) + << " is earlier in the list."; return webrtc::RTCError(webrtc::RTCErrorType::INVALID_PARAMETER, - "Duplicate payload type"); + "Duplicate codec ID with non-matching codecs"); } + defined_codecs.insert({in_codec.id, in_codec}); payload_codec_type[payload_type] = in_codec.GetResiliencyType(); switch (in_codec.GetResiliencyType()) { diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 12ca1bf189..a82032a15d 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn @@ -2015,6 +2015,7 @@ if (rtc_include_tests && !build_with_chromium) { "../api/video:builtin_video_bitrate_allocator_factory", "../api/video:recordable_encoded_frame", "../api/video/test:mock_recordable_encoded_frame", + "../call:fake_payload_type_suggester", "../call:payload_type_picker", "../call:rtp_interfaces", "../call:rtp_receiver", diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index fa71feec0c..3a157520a3 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc @@ -23,11 +23,13 @@ #include "absl/algorithm/container.h" #include "absl/strings/match.h" #include "absl/strings/string_view.h" +#include "api/array_view.h" #include "api/audio_codecs/audio_format.h" #include "api/candidate.h" #include "api/media_types.h" #include "api/rtp_parameters.h" #include "api/rtp_transceiver_direction.h" +#include "call/fake_payload_type_suggester.h" #include "media/base/codec.h" #include "media/base/media_constants.h" #include "media/base/rid_description.h" @@ -164,6 +166,25 @@ const Codec kVideoCodecsH265Level52[] = { CreateVideoCodec(96, kH265MainProfileLevel52Sdp)}; const Codec kVideoCodecsH265Level6[] = { CreateVideoCodec(96, kH265MainProfileLevel6Sdp)}; +// Match two codec lists for content, but ignore the ID. +bool CodecListsMatch(rtc::ArrayView list1, + rtc::ArrayView list2) { + if (list1.size() != list2.size()) { + return false; + } + for (size_t i = 0; i < list1.size(); ++i) { + Codec codec1 = list1[i]; + Codec codec2 = list2[i]; + codec1.id = Codec::kIdNotSet; + codec2.id = Codec::kIdNotSet; + if (codec1 != codec2) { + RTC_LOG(LS_ERROR) << "Mismatch at position " << i << " between " << codec1 + << " and " << codec2; + return false; + } + } + return true; +} const RtpExtension kAudioRtpExtension1[] = { RtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 8), @@ -486,8 +507,8 @@ class MediaSessionDescriptionFactoryTest : public testing::Test { MediaSessionDescriptionFactoryTest() : tdf1_(field_trials), tdf2_(field_trials), - f1_(nullptr, false, &ssrc_generator1, &tdf1_, nullptr), - f2_(nullptr, false, &ssrc_generator2, &tdf2_, nullptr) { + f1_(nullptr, false, &ssrc_generator1, &tdf1_, &pt_suggester_1_), + f2_(nullptr, false, &ssrc_generator2, &tdf2_, &pt_suggester_2_) { f1_.set_audio_codecs(MAKE_VECTOR(kAudioCodecs1), MAKE_VECTOR(kAudioCodecs1)); f1_.set_video_codecs(MAKE_VECTOR(kVideoCodecs1), @@ -736,6 +757,8 @@ class MediaSessionDescriptionFactoryTest : public testing::Test { UniqueRandomIdGenerator ssrc_generator2; TransportDescriptionFactory tdf1_; TransportDescriptionFactory tdf2_; + webrtc::FakePayloadTypeSuggester pt_suggester_1_; + webrtc::FakePayloadTypeSuggester pt_suggester_2_; MediaSessionDescriptionFactory f1_; MediaSessionDescriptionFactory f2_; }; @@ -879,7 +902,6 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateOfferWithCustomCodecs) { // Fields in codec are set during the gen process, so simple compare // does not work. EXPECT_EQ(acd->codecs()[0].name, custom_audio_codec.name); - RTC_LOG(LS_ERROR) << "DEBUG: audio PT assigned is " << acd->codecs()[0].id; EXPECT_EQ(MEDIA_TYPE_VIDEO, vcd->type()); ASSERT_EQ(vcd->codecs().size(), 1U); @@ -2995,11 +3017,11 @@ TEST_F(MediaSessionDescriptionFactoryTest, const AudioContentDescription* updated_acd = GetFirstAudioContentDescription(updated_offer.get()); - EXPECT_THAT(updated_acd->codecs(), ElementsAreArray(kUpdatedAudioCodecOffer)); + EXPECT_TRUE(CodecListsMatch(updated_acd->codecs(), kUpdatedAudioCodecOffer)); const VideoContentDescription* updated_vcd = GetFirstVideoContentDescription(updated_offer.get()); - EXPECT_THAT(updated_vcd->codecs(), ElementsAreArray(kUpdatedVideoCodecOffer)); + EXPECT_TRUE(CodecListsMatch(updated_vcd->codecs(), kUpdatedVideoCodecOffer)); } // Test that a reoffer does not reuse audio codecs from a previous media section @@ -3027,7 +3049,13 @@ TEST_F(MediaSessionDescriptionFactoryTest, // section was not recycled the payload types would match the initial offerer. const AudioContentDescription* acd = GetFirstAudioContentDescription(reoffer.get()); - EXPECT_THAT(acd->codecs(), ElementsAreArray(kAudioCodecs2)); + // EXPECT_THAT(acd->codecs(), ElementsAreArray(kAudioCodecs2)), + // except that we don't want to check the PT numbers. + EXPECT_EQ(acd->codecs().size(), + sizeof(kAudioCodecs2) / sizeof(kAudioCodecs2[0])); + for (size_t i = 0; i < acd->codecs().size(); ++i) { + EXPECT_EQ(acd->codecs()[i].name, kAudioCodecs2[i].name); + } } // Test that a reoffer does not reuse video codecs from a previous media section @@ -3054,7 +3082,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, // section was not recycled the payload types would match the initial offerer. const VideoContentDescription* vcd = GetFirstVideoContentDescription(reoffer.get()); - EXPECT_THAT(vcd->codecs(), ElementsAreArray(kVideoCodecs2)); + EXPECT_TRUE(CodecListsMatch(vcd->codecs(), kVideoCodecs2)); } // Test that a reanswer does not reuse audio codecs from a previous media @@ -3152,7 +3180,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, std::vector expected_codecs = MAKE_VECTOR(kVideoCodecsAnswer); AddRtxCodec(CreateVideoRtxCodec(126, kVideoCodecs1[1].id), &expected_codecs); - EXPECT_EQ(expected_codecs, vcd->codecs()); + EXPECT_TRUE(CodecListsMatch(expected_codecs, vcd->codecs())); // Now, make sure we get same result (except for the order) if `f2_` creates // an updated offer even though the default payload types between `f1_` and @@ -3167,7 +3195,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, const VideoContentDescription* updated_vcd = GetFirstVideoContentDescription(updated_answer.get()); - EXPECT_EQ(expected_codecs, updated_vcd->codecs()); + EXPECT_TRUE(CodecListsMatch(expected_codecs, updated_vcd->codecs())); } // Regression test for: @@ -3322,7 +3350,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, // New offer should attempt to add H263, and RTX for H264. expected_codecs.push_back(kVideoCodecs2[1]); AddRtxCodec(CreateVideoRtxCodec(125, kVideoCodecs1[1].id), &expected_codecs); - EXPECT_EQ(expected_codecs, updated_vcd->codecs()); + EXPECT_TRUE(CodecListsMatch(expected_codecs, updated_vcd->codecs())); } // Test that RTX is ignored when there is no associated payload type parameter. @@ -3431,7 +3459,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, std::vector expected_codecs = MAKE_VECTOR(kVideoCodecsAnswer); AddRtxCodec(CreateVideoRtxCodec(126, kVideoCodecs1[1].id), &expected_codecs); - EXPECT_EQ(expected_codecs, vcd->codecs()); + EXPECT_TRUE(CodecListsMatch(expected_codecs, vcd->codecs())); } // Test that after one RTX codec has been negotiated, a new offer can attempt @@ -3454,7 +3482,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, AddSecondRtxInNewOffer) { std::vector expected_codecs = MAKE_VECTOR(kVideoCodecs1); AddRtxCodec(CreateVideoRtxCodec(126, kVideoCodecs1[1].id), &expected_codecs); - EXPECT_EQ(expected_codecs, vcd->codecs()); + EXPECT_TRUE(CodecListsMatch(expected_codecs, vcd->codecs())); // Now, attempt to add RTX for H264-SVC. AddRtxCodec(CreateVideoRtxCodec(125, kVideoCodecs1[0].id), &f1_codecs); @@ -3466,7 +3494,7 @@ TEST_F(MediaSessionDescriptionFactoryTest, AddSecondRtxInNewOffer) { vcd = GetFirstVideoContentDescription(updated_offer.get()); AddRtxCodec(CreateVideoRtxCodec(125, kVideoCodecs1[0].id), &expected_codecs); - EXPECT_EQ(expected_codecs, vcd->codecs()); + EXPECT_TRUE(CodecListsMatch(expected_codecs, vcd->codecs())); } // Test that when RTX is used in conjunction with simulcast, an RTX ssrc is @@ -4555,8 +4583,8 @@ class MediaProtocolTest : public testing::TestWithParam { MediaProtocolTest() : tdf1_(field_trials_), tdf2_(field_trials_), - f1_(nullptr, false, &ssrc_generator1, &tdf1_, nullptr), - f2_(nullptr, false, &ssrc_generator2, &tdf2_, nullptr) { + f1_(nullptr, false, &ssrc_generator1, &tdf1_, &pt_suggester_1_), + f2_(nullptr, false, &ssrc_generator2, &tdf2_, &pt_suggester_2_) { f1_.set_audio_codecs(MAKE_VECTOR(kAudioCodecs1), MAKE_VECTOR(kAudioCodecs1)); f1_.set_video_codecs(MAKE_VECTOR(kVideoCodecs1), @@ -4575,6 +4603,8 @@ class MediaProtocolTest : public testing::TestWithParam { webrtc::test::ScopedKeyValueConfig field_trials_; TransportDescriptionFactory tdf1_; TransportDescriptionFactory tdf2_; + webrtc::FakePayloadTypeSuggester pt_suggester_1_; + webrtc::FakePayloadTypeSuggester pt_suggester_2_; MediaSessionDescriptionFactory f1_; MediaSessionDescriptionFactory f2_; UniqueRandomIdGenerator ssrc_generator1; @@ -4619,8 +4649,9 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestSetAudioCodecs) { std::unique_ptr(new rtc::FakeSSLIdentity("id")))); UniqueRandomIdGenerator ssrc_generator; + webrtc::FakePayloadTypeSuggester pt_suggester; MediaSessionDescriptionFactory sf(nullptr, false, &ssrc_generator, &tdf, - nullptr); + &pt_suggester); std::vector send_codecs = MAKE_VECTOR(kAudioCodecs1); std::vector recv_codecs = MAKE_VECTOR(kAudioCodecs2); @@ -4691,8 +4722,9 @@ void TestAudioCodecsOffer(RtpTransceiverDirection direction) { std::unique_ptr(new rtc::FakeSSLIdentity("id")))); UniqueRandomIdGenerator ssrc_generator; + webrtc::FakePayloadTypeSuggester pt_suggester; MediaSessionDescriptionFactory sf(nullptr, false, &ssrc_generator, &tdf, - nullptr); + &pt_suggester); const std::vector send_codecs = MAKE_VECTOR(kAudioCodecs1); const std::vector recv_codecs = MAKE_VECTOR(kAudioCodecs2); const std::vector sendrecv_codecs = MAKE_VECTOR(kAudioCodecsAnswer); @@ -4733,13 +4765,15 @@ void TestAudioCodecsOffer(RtpTransceiverDirection direction) { } } -const Codec kOfferAnswerCodecs[] = {CreateAudioCodec(0, "codec0", 16000, 1), - CreateAudioCodec(1, "codec1", 8000, 1), - CreateAudioCodec(2, "codec2", 8000, 1), - CreateAudioCodec(3, "codec3", 8000, 1), - CreateAudioCodec(4, "codec4", 8000, 2), - CreateAudioCodec(5, "codec5", 32000, 1), - CreateAudioCodec(6, "codec6", 48000, 1)}; +// Since the PT suggester reserves the static range for specific codecs, +// PT numbers from the 36-63 range are used. +const Codec kOfferAnswerCodecs[] = {CreateAudioCodec(40, "codec0", 16000, 1), + CreateAudioCodec(41, "codec1", 8000, 1), + CreateAudioCodec(42, "codec2", 8000, 1), + CreateAudioCodec(43, "codec3", 8000, 1), + CreateAudioCodec(44, "codec4", 8000, 2), + CreateAudioCodec(45, "codec5", 32000, 1), + CreateAudioCodec(46, "codec6", 48000, 1)}; /* The codecs groups below are chosen as per the matrix below. The objective * is to have different sets of codecs in the inputs, to get unique sets of @@ -4795,10 +4829,12 @@ void TestAudioCodecsAnswer(RtpTransceiverDirection offer_direction, rtc::RTCCertificate::Create(std::unique_ptr( new rtc::FakeSSLIdentity("answer_id")))); UniqueRandomIdGenerator ssrc_generator1, ssrc_generator2; + webrtc::FakePayloadTypeSuggester offer_pt_suggester; MediaSessionDescriptionFactory offer_factory(nullptr, false, &ssrc_generator1, - &offer_tdf, nullptr); + &offer_tdf, &offer_pt_suggester); + webrtc::FakePayloadTypeSuggester answer_pt_suggester; MediaSessionDescriptionFactory answer_factory( - nullptr, false, &ssrc_generator2, &answer_tdf, nullptr); + nullptr, false, &ssrc_generator2, &answer_tdf, &answer_pt_suggester); offer_factory.set_audio_codecs( VectorFromIndices(kOfferAnswerCodecs, kOfferSendCodecs), @@ -4881,7 +4917,7 @@ void TestAudioCodecsAnswer(RtpTransceiverDirection offer_direction, bool first = true; os << "{"; for (const auto& c : codecs) { - os << (first ? " " : ", ") << c.id; + os << (first ? " " : ", ") << c.id << ":" << c.name; first = false; } os << " }"; @@ -4946,12 +4982,12 @@ class VideoCodecsOfferH265LevelIdTest : public testing::Test { false, &ssrc_generator_offerer_, &tdf_offerer_, - nullptr), + &pt_suggester_offerer_), sf_answerer_(nullptr, false, &ssrc_generator_answerer_, &tdf_answerer_, - nullptr) { + &pt_suggester_answerer_) { tdf_offerer_.set_certificate( rtc::RTCCertificate::Create(std::unique_ptr( new rtc::FakeSSLIdentity("offer_id")))); @@ -4979,6 +5015,8 @@ class VideoCodecsOfferH265LevelIdTest : public testing::Test { UniqueRandomIdGenerator ssrc_generator_answerer_; MediaSessionDescriptionFactory sf_offerer_; MediaSessionDescriptionFactory sf_answerer_; + webrtc::FakePayloadTypeSuggester pt_suggester_offerer_; + webrtc::FakePayloadTypeSuggester pt_suggester_answerer_; }; // Both sides support H.265 level 5.2 for encoding and decoding. diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 3f7e74bb1e..c39b14e629 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -84,7 +84,6 @@ #include "pc/stream_collection.h" #include "pc/transceiver_list.h" #include "pc/usage_pattern.h" -#include "pc/used_ids.h" #include "pc/webrtc_session_description_factory.h" #include "rtc_base/checks.h" #include "rtc_base/crypto_random.h" @@ -592,8 +591,7 @@ RTCError ValidatePayloadTypes(const cricket::SessionDescription& description) { if (type == cricket::MEDIA_TYPE_AUDIO || type == cricket::MEDIA_TYPE_VIDEO) { for (const auto& codec : media_description->codecs()) { - if (!cricket::UsedPayloadTypes::IsIdValid( - codec, media_description->rtcp_mux())) { + if (!PayloadType::IsValid(codec.id, media_description->rtcp_mux())) { LOG_AND_RETURN_ERROR( RTCErrorType::INVALID_PARAMETER, "The media section with MID='" + content.mid() + diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index 029878ff77..5e8ae14816 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -8,19 +8,25 @@ * be found in the AUTHORS file in the root of the source tree. */ +#include +#include #include +#include +#include #include #include +#include "absl/strings/match.h" #include "absl/strings/str_replace.h" -#include "api/audio/audio_device.h" -#include "api/audio/audio_mixer.h" -#include "api/audio/audio_processing.h" #include "api/audio_codecs/builtin_audio_decoder_factory.h" #include "api/audio_codecs/builtin_audio_encoder_factory.h" #include "api/create_peerconnection_factory.h" +#include "api/jsep.h" #include "api/media_types.h" #include "api/peer_connection_interface.h" +#include "api/rtc_error.h" +#include "api/rtp_parameters.h" +#include "api/rtp_transceiver_direction.h" #include "api/rtp_transceiver_interface.h" #include "api/scoped_refptr.h" #include "api/video_codecs/video_decoder_factory_template.h" @@ -33,12 +39,14 @@ #include "api/video_codecs/video_encoder_factory_template_libvpx_vp8_adapter.h" #include "api/video_codecs/video_encoder_factory_template_libvpx_vp9_adapter.h" #include "api/video_codecs/video_encoder_factory_template_open_h264_adapter.h" -#include "p2p/base/port_allocator.h" +#include "media/base/codec.h" +#include "media/base/media_constants.h" +#include "media/base/stream_params.h" #include "pc/peer_connection_wrapper.h" #include "pc/session_description.h" #include "pc/test/fake_audio_capture_module.h" #include "pc/test/mock_peer_connection_observers.h" -#include "rtc_base/rtc_certificate_generator.h" +#include "rtc_base/string_encode.h" #include "rtc_base/thread.h" #include "system_wrappers/include/metrics.h" #include "test/gmock.h" @@ -658,19 +666,28 @@ TEST_F(SdpOfferAnswerTest, SimulcastOfferWithMixedCodec) { // Verify that the serialized SDP includes pt=. std::string sdp; offer->ToString(&sdp); - EXPECT_THAT(sdp, testing::HasSubstr("a=rid:1 send pt=" + - std::to_string(send_codecs[0].id))); - EXPECT_THAT(sdp, testing::HasSubstr("a=rid:2 send pt=" + - std::to_string(send_codecs[1].id))); + int vp8_pt = cricket::Codec::kIdNotSet; + int vp9_pt = cricket::Codec::kIdNotSet; + for (auto& codec : send_codecs) { + if (codec.name == vp8_codec->name && vp8_pt == cricket::Codec::kIdNotSet) { + vp8_pt = codec.id; + } + if (codec.name == vp9_codec->name && vp9_pt == cricket::Codec::kIdNotSet) { + vp9_pt = codec.id; + } + } + EXPECT_THAT(sdp, + testing::HasSubstr("a=rid:1 send pt=" + std::to_string(vp8_pt))); + EXPECT_THAT(sdp, + testing::HasSubstr("a=rid:2 send pt=" + std::to_string(vp9_pt))); // Verify that SDP containing pt= can be parsed correctly. auto offer2 = CreateSessionDescription(SdpType::kOffer, sdp); auto& offer_contents2 = offer2->description()->contents(); auto send_rids2 = offer_contents2[0].media_description()->streams()[0].rids(); - auto send_codecs2 = offer_contents2[0].media_description()->codecs(); EXPECT_EQ(send_rids2[0].payload_types.size(), 1u); - EXPECT_EQ(send_rids2[0].payload_types[0], send_codecs2[0].id); + EXPECT_EQ(send_rids2[0].payload_types[0], vp8_pt); EXPECT_EQ(send_rids2[1].payload_types.size(), 1u); - EXPECT_EQ(send_rids2[1].payload_types[0], send_codecs2[1].id); + EXPECT_EQ(send_rids2[1].payload_types[0], vp9_pt); } TEST_F(SdpOfferAnswerTest, SimulcastAnswerWithPayloadType) { diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h index c6e438a948..121a0953f7 100644 --- a/pc/test/integration_test_helpers.h +++ b/pc/test/integration_test_helpers.h @@ -686,9 +686,7 @@ class PeerConnectionIntegrationWrapper : public PeerConnectionObserver, bool SetRemoteDescription(std::unique_ptr desc) { auto observer = rtc::make_ref_counted(); - std::string str; - desc->ToString(&str); - RTC_LOG(LS_INFO) << debug_name_ << ": SetRemoteDescription SDP:\n" << str; + RTC_LOG(LS_INFO) << debug_name_ << ": SetRemoteDescription SDP:" << desc; pc()->SetRemoteDescription(std::move(desc), observer); // desc.release()); RemoveUnusedVideoRenderers(); EXPECT_TRUE_WAIT(observer->called(), kDefaultTimeout);