Associate RTX with preceding codec in redesign In the payload type redesign path, associate RTX codecs with the immediately preceding codec in the supported formats list. - pc/typed_codec_vendor.cc: Update CollectVideoCodecConfigurations to associate RTX with the preceding codec. Add validation to reject RTX preceded by a resiliency codec. Skip generic RTX at the end of the formats list if auxiliary codecs are added anyway. - pc/codec_vendor.cc: Use MatchesWithCodecRules and negotiate packetization when merging codecs. - pc/media_session_unittest.cc: Update AddRtxCodec helper to insert RTX after its primary codec in tests. Adjust test expectations to match negotiated codec order (which appends new RTX). Bug: webrtc:360058654 Change-Id: I667089b8eef502b1da10e4ad811f919dadef2e6d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/479180 Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47915}
diff --git a/call/fake_payload_type_suggester.h b/call/fake_payload_type_suggester.h index 25d356b..5887bed 100644 --- a/call/fake_payload_type_suggester.h +++ b/call/fake_payload_type_suggester.h
@@ -52,12 +52,6 @@ return it->second; } - if (codec.id.IsSet() && !IsPayloadTypeConflict(mid, codec.id, codec)) { - pt_picker_.AddMapping(codec.id, codec); - recorder.AddMapping(codec.id, codec); - return codec.id; - } - // There's only one PT picker, but multiple recorders. RTCErrorOr<PayloadType> suggested_result = pt_picker_.SuggestMapping(codec, &recorder, pick_from_top_of_range); @@ -84,8 +78,12 @@ bundle_groups_ = bundle_groups; } - bool HasMapping(PayloadType payload_type) const { - return pt_picker_.LookupCodec(payload_type).has_value(); + bool HasMapping(absl::string_view mid, PayloadType payload_type) const { + const PayloadTypeRecorder* recorder = LookupRecorderConst(mid); + if (recorder) { + return recorder->LookupCodec(payload_type).ok(); + } + return false; } RTCError AddLocalMapping(absl::string_view mid, @@ -95,22 +93,6 @@ return pt_picker_.AddMapping(payload_type, codec); } - RTCErrorOr<RtpHeaderExtensionId> SuggestRtpHeaderExtensionId( - absl::string_view mid, - const RtpExtension& extension, - RtpTransceiverIdDomain id_domain) override { - return rtp_extension_picker_.SuggestMapping( - extension.uri, extension.encrypt, extension.id, id_domain, nullptr); - } - [[nodiscard]] RTCError AddRtpHeaderExtensionMapping( - absl::string_view mid, - const RtpExtension& extension, - bool local) override { - return rtp_extension_picker_.AddMapping(extension.id, extension.uri, - extension.encrypt); - } - - private: bool IsPayloadTypeConflict(absl::string_view mid, PayloadType payload_type, const Codec& codec) const { @@ -132,6 +114,39 @@ return false; } + RTCErrorOr<RtpHeaderExtensionId> SuggestRtpHeaderExtensionId( + absl::string_view mid, + const RtpExtension& extension, + RtpTransceiverIdDomain id_domain) override { + return rtp_extension_picker_.SuggestMapping( + extension.uri, extension.encrypt, extension.id, id_domain, nullptr); + } + [[nodiscard]] RTCError AddRtpHeaderExtensionMapping( + absl::string_view mid, + const RtpExtension& extension, + bool local) override { + return rtp_extension_picker_.AddMapping(extension.id, extension.uri, + extension.encrypt); + } + + private: + const PayloadTypeRecorder* LookupRecorderConst(absl::string_view mid) const { + if (mid.empty()) + return nullptr; + std::string transport_mapped_name = std::string(mid); + for (const std::vector<std::string>& group : bundle_groups_) { + if (std::find(group.begin(), group.end(), mid) != group.end()) { + transport_mapped_name = group[0]; + break; + } + } + auto it = recorders_.find(transport_mapped_name); + if (it == recorders_.end()) { + return nullptr; + } + return it->second.get(); + } + PayloadTypeRecorder& LookupRecorder(absl::string_view mid) { RTC_CHECK(!mid.empty()); std::string transport_mapped_name = std::string(mid);
diff --git a/pc/BUILD.gn b/pc/BUILD.gn index 4445dfd..ae23689 100644 --- a/pc/BUILD.gn +++ b/pc/BUILD.gn
@@ -466,6 +466,8 @@ "../media:media_constants", "../media:media_engine", "../rtc_base:checks", + "../rtc_base:logging", + "../rtc_base/containers:flat_map", "../rtc_base/containers:flat_set", "//third_party/abseil-cpp/absl/base:nullability", "//third_party/abseil-cpp/absl/strings",
diff --git a/pc/codec_vendor.cc b/pc/codec_vendor.cc index 4562e4f..8fd3922 100644 --- a/pc/codec_vendor.cc +++ b/pc/codec_vendor.cc
@@ -312,9 +312,7 @@ PayloadTypeSuggester& pt_suggester, const FieldTrialsView& trials, bool pick_from_top_of_range) { - if (!config.resiliency.flexfec || config.codec.type != Codec::Type::kVideo || - (!trials.IsEnabled("WebRTC-FlexFEC-03-Advertised") && - !trials.IsEnabled("WebRTC-FlexFEC-03"))) { + if (!config.resiliency.flexfec || config.codec.type != Codec::Type::kVideo) { return RTCError::OK(); } auto fec_it = absl::c_find_if(offered_codecs, [&](const Codec& c) { @@ -917,7 +915,6 @@ const std::vector<CodecConfiguration>& recv_configs = (type == MediaType::AUDIO) ? audio_recv_codecs_.configurations() : video_recv_codecs_.configurations(); - switch (direction) { case RtpTransceiverDirection::kSendRecv: case RtpTransceiverDirection::kStopped: @@ -933,10 +930,36 @@ std::vector<CodecConfiguration> intersected; for (const CodecConfiguration& send_config : send_configs) { for (const CodecConfiguration& recv_config : recv_configs) { - if (absl::EqualsIgnoreCase(send_config.codec.name, - recv_config.codec.name) && - send_config.codec.clockrate == recv_config.codec.clockrate) { - intersected.push_back(send_config); + if (MatchesWithCodecRules(send_config.codec, recv_config.codec)) { + CodecConfiguration merged_config = recv_config; + merged_config.codec.IntersectFeedbackParams(send_config.codec); + NegotiatePacketization(send_config.codec, recv_config.codec, + &merged_config.codec); + merged_config.resiliency.red = + send_config.resiliency.red && recv_config.resiliency.red; + merged_config.resiliency.ulpfec = + send_config.resiliency.ulpfec && recv_config.resiliency.ulpfec; + merged_config.resiliency.flexfec = send_config.resiliency.flexfec && + recv_config.resiliency.flexfec; + merged_config.resiliency.rtx = + send_config.resiliency.rtx && recv_config.resiliency.rtx; + if (absl::EqualsIgnoreCase(send_config.codec.name, + kH264CodecName)) { + H264GenerateProfileLevelIdForAnswer(send_config.codec.params, + recv_config.codec.params, + &merged_config.codec.params); + } +#ifdef RTC_ENABLE_H265 + if (absl::EqualsIgnoreCase(send_config.codec.name, + kH265CodecName)) { + H265GenerateProfileTierLevelForAnswer( + send_config.codec.params, recv_config.codec.params, + &merged_config.codec.params); + NegotiateTxMode(send_config.codec, recv_config.codec, + &merged_config.codec); + } +#endif + intersected.push_back(merged_config); break; } }
diff --git a/pc/codec_vendor_redesign_unittest.cc b/pc/codec_vendor_redesign_unittest.cc index 7abca4b..9b47441 100644 --- a/pc/codec_vendor_redesign_unittest.cc +++ b/pc/codec_vendor_redesign_unittest.cc
@@ -551,5 +551,72 @@ EXPECT_THAT(answer_codecs, Contains(Field(&Codec::name, "codec1"))); } +TEST_F(CodecVendorRedesignTest, AddSecondRtxInNewOffer) { + // 1. Configure engine with H264-SVC, H264, and RTX for H264. + std::vector<Codec> codecs1({ + CreateVideoCodec(96, "H264-SVC"), CreateVideoCodec(97, "H264"), + CreateVideoRtxCodec(98, 97), // RTX for H264 + }); + media_engine_.SetVideoSendCodecs(codecs1); + media_engine_.SetVideoRecvCodecs(codecs1); + vendor_ = std::make_unique<CodecVendor>(&media_engine_, /*rtx_enabled=*/true, + trials_); + + MediaDescriptionOptions options(MediaType::VIDEO, "video", + RtpTransceiverDirection::kSendRecv, + /*stopped=*/false); + + // First offer + auto result1 = vendor_->GetNegotiatedCodecsForOffer( + options, MediaSessionOptions(), /*current_content=*/nullptr, + pt_suggester_); + ASSERT_TRUE(result1.ok()); + + // We expect H264-SVC, H264, and RTX for H264. + // Order: [H264-SVC, H264, RTX] + ASSERT_EQ(result1.value().size(), 3u); + EXPECT_EQ(result1.value()[0].name, "H264-SVC"); + EXPECT_EQ(result1.value()[1].name, "H264"); + EXPECT_EQ(result1.value()[2].name, "rtx"); + + // Create current_content from first offer result. + auto video_description = std::make_unique<VideoContentDescription>(); + video_description->set_codecs(result1.value()); + ContentInfo current_content(MediaProtocolType::kRtp, "video", + std::move(video_description)); + + // 2. Configure engine to add RTX for H264-SVC. + // We insert RTX for H264-SVC (96) after H264-SVC. + std::vector<Codec> codecs2 = codecs1; + codecs2.insert(codecs2.begin() + 1, CreateVideoRtxCodec(125, 96)); + media_engine_.SetVideoSendCodecs(codecs2); + media_engine_.SetVideoRecvCodecs(codecs2); + vendor_ = std::make_unique<CodecVendor>(&media_engine_, /*rtx_enabled=*/true, + trials_); + + // Second offer (passing current_content) + auto result2 = vendor_->GetNegotiatedCodecsForOffer( + options, MediaSessionOptions(), ¤t_content, pt_suggester_); + ASSERT_TRUE(result2.ok()); + + // We expect: + // - H264-SVC, H264, RTX(for H264) from current_content (preserved order). + // - New RTX(for H264-SVC) appended at the end. + const auto& codecs = result2.value(); + ASSERT_EQ(codecs.size(), 4u); + EXPECT_EQ(codecs[0].name, "H264-SVC"); + EXPECT_EQ(codecs[1].name, "H264"); + + EXPECT_EQ(codecs[2].name, "rtx"); + std::string apt2; + EXPECT_TRUE(codecs[2].GetParam(kCodecParamAssociatedPayloadType, &apt2)); + EXPECT_EQ(apt2, "97"); + + EXPECT_EQ(codecs[3].name, "rtx"); + std::string apt3; + EXPECT_TRUE(codecs[3].GetParam(kCodecParamAssociatedPayloadType, &apt3)); + EXPECT_EQ(apt3, "96"); +} + } // namespace } // namespace webrtc
diff --git a/pc/media_session_unittest.cc b/pc/media_session_unittest.cc index 593dc61..064122a 100644 --- a/pc/media_session_unittest.cc +++ b/pc/media_session_unittest.cc
@@ -121,7 +121,8 @@ void RegisterExpectations(absl::string_view mid, std::span<const Codec> codecs) { for (const Codec& c : codecs) { - if (c.id.IsSet() && !payload_type_suggester_.HasMapping(c.id)) { + if (c.id.IsSet() && !payload_type_suggester_.HasMapping(mid, c.id) && + !payload_type_suggester_.IsPayloadTypeConflict(mid, c.id, c)) { RTC_CHECK(payload_type_suggester_.AddLocalMapping(mid, c.id, c).ok()); } } @@ -569,6 +570,16 @@ void AddRtxCodec(const Codec& rtx_codec, std::vector<Codec>* codecs) { RTC_LOG(LS_VERBOSE) << "Adding RTX codec " << FullMimeType(rtx_codec); ASSERT_FALSE(FindCodecById(*codecs, rtx_codec.id)); + auto apt_it = rtx_codec.params.find(kCodecParamAssociatedPayloadType); + if (apt_it != rtx_codec.params.end()) { + int apt = FromString<int>(apt_it->second); + auto it = + absl::c_find_if(*codecs, [apt](const Codec& c) { return c.id == apt; }); + if (it != codecs->end()) { + codecs->insert(it + 1, rtx_codec); + return; + } + } codecs->push_back(rtx_codec); } @@ -3696,8 +3707,15 @@ GetFirstVideoContentDescription(updated_offer.get()); // 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); + if (env_.field_trials().IsEnabled("WebRTC-PayloadTypesInTransport")) { + AddRtxCodec(CreateVideoRtxCodec(125, kVideoCodecs1[1].id), + &expected_codecs); + expected_codecs.push_back(kVideoCodecs2[1]); + } else { + expected_codecs.push_back(kVideoCodecs2[1]); + AddRtxCodec(CreateVideoRtxCodec(125, kVideoCodecs1[1].id), + &expected_codecs); + } EXPECT_THAT(updated_vcd->codecs(), CodecListsMatch(expected_codecs, &env_.field_trials())); } @@ -3846,7 +3864,7 @@ ASSERT_TRUE(updated_offer); vcd = GetFirstVideoContentDescription(updated_offer.get()); - AddRtxCodec(CreateVideoRtxCodec(125, kVideoCodecs1[0].id), &expected_codecs); + expected_codecs.push_back(CreateVideoRtxCodec(125, kVideoCodecs1[0].id)); EXPECT_THAT(vcd->codecs(), CodecListsMatch(expected_codecs, &env_.field_trials())); }
diff --git a/pc/typed_codec_vendor.cc b/pc/typed_codec_vendor.cc index 6152e95..6620699 100644 --- a/pc/typed_codec_vendor.cc +++ b/pc/typed_codec_vendor.cc
@@ -10,6 +10,7 @@ #include "pc/typed_codec_vendor.h" +#include <cstddef> #include <functional> #include <map> #include <string> @@ -30,7 +31,9 @@ #include "media/base/media_engine.h" #include "pc/codec_configuration.h" #include "rtc_base/checks.h" +#include "rtc_base/containers/flat_map.h" #include "rtc_base/containers/flat_set.h" +#include "rtc_base/logging.h" namespace webrtc { @@ -125,12 +128,17 @@ if (formats.empty()) { return {}; } + if (absl::EqualsIgnoreCase(formats[0].name, kRtxCodecName)) { + RTC_LOG(LS_ERROR) << "RTX codec at index 0 is invalid."; + return {}; + } bool has_red = false; bool has_ulpfec = false; bool has_flexfec = false; - bool has_rtx = false; + // Note whether the resiliency methods that apply to all + // media codecs are present. for (const SdpVideoFormat& format : formats) { if (absl::EqualsIgnoreCase(format.name, kRedCodecName)) { has_red = true; @@ -138,37 +146,54 @@ has_ulpfec = true; } else if (absl::EqualsIgnoreCase(format.name, kFlexfecCodecName)) { has_flexfec = true; - } else if (absl::EqualsIgnoreCase(format.name, kRtxCodecName)) { - has_rtx = true; } } + // Convert list to CodecConfiguration format. + // An Rtx codec in the list indicates that the previous codec should + // have a resiliency mechanism of Rtx. std::vector<CodecConfiguration> out; - for (const SdpVideoFormat& format : formats) { - Codec codec = CreateVideoCodec(format); - if (codec.IsResiliencyCodec()) { + bool previous_was_media_codec = false; + for (size_t i = 0; i < formats.size(); ++i) { + const SdpVideoFormat& format = formats[i]; + if (absl::EqualsIgnoreCase(format.name, kRtxCodecName)) { + if (add_auxiliary_codecs) { + // If we add auxiliary codecs, we will enable RTX for all primary codecs + // anyway, so we don't need to associate this RTX format. + continue; + } + if (!previous_was_media_codec) { + RTC_LOG(LS_ERROR) << "RTX codec preceded by non-media codec is invalid"; + return {}; + } + if (rtx_enabled) { + if (!out.empty()) { + out.back().resiliency.rtx = true; + } else { + RTC_DCHECK(false) << "Preceding codec was not added to out?"; + } + } continue; } - + Codec codec = CreateVideoCodec(format); + if (codec.IsResiliencyCodec()) { + previous_was_media_codec = false; + continue; + } + previous_was_media_codec = true; AddDefaultFeedbackParams(&codec, trials); CodecConfiguration config; config.codec = codec; - if (rtx_enabled && (has_rtx || add_auxiliary_codecs)) { - Codec::ResiliencyType resiliency_type = codec.GetResiliencyType(); - if (resiliency_type != Codec::ResiliencyType::kFlexfec && - resiliency_type != Codec::ResiliencyType::kUlpfec) { - config.resiliency.rtx = true; - } - } config.resiliency.red = has_red; config.resiliency.ulpfec = has_ulpfec; - if (trials.IsEnabled("WebRTC-FlexFEC-03-Advertised") || - trials.IsEnabled("WebRTC-FlexFEC-03")) { - config.resiliency.flexfec = has_flexfec; + config.resiliency.flexfec = has_flexfec; + if (rtx_enabled && add_auxiliary_codecs) { + config.resiliency.rtx = true; } out.push_back(config); } + return out; }