Redesign: Implement RTX PT convention and enhance video codec support - Implemented conventional payload type assignment for RTX (Primary PT + 1) in CodecVendor. - Enabled FlexFEC PT assignment by checking both WebRTC-FlexFEC-03 and WebRTC-FlexFEC-03-Advertised field trials. - Restored strict error handling in CodecVendor for invalid MID recycling (media type mismatch). - Expanded CodecVendorRedesignTest with coverage for MID recycling failure, FEC PT assignment, and RTX convention. - Improved FakePayloadTypeSuggester conflict detection and fixed PayloadTypeRecorder mapping retrieval. - Updated redesign status and clarified MID recycling constraints in g3doc. Bug: webrtc:360058654 Change-Id: I1f139e4e82094cc16db00ed54b1d682567e24e61 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/473660 Reviewed-by: Tomas Gunnarsson <tommi@webrtc.org> Commit-Queue: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47741}
diff --git a/call/fake_payload_type_suggester.h b/call/fake_payload_type_suggester.h index ca4dc73..7e2731c 100644 --- a/call/fake_payload_type_suggester.h +++ b/call/fake_payload_type_suggester.h
@@ -102,8 +102,9 @@ bool IsPayloadTypeConflict(absl::string_view mid, PayloadType payload_type, const Codec& codec) const { - for (const auto& kv : recorders_) { - RTCErrorOr<Codec> existing = kv.second->LookupCodec(payload_type); + auto it = recorders_.find(mid); + if (it != recorders_.end()) { + RTCErrorOr<Codec> existing = it->second->LookupCodec(payload_type); if (existing.ok()) { if (!MatchesWithReferenceAttributes(existing.value(), codec)) { return true;
diff --git a/call/payload_type_picker.cc b/call/payload_type_picker.cc index 7669df2..c89965e 100644 --- a/call/payload_type_picker.cc +++ b/call/payload_type_picker.cc
@@ -317,7 +317,8 @@ std::vector<std::pair<PayloadType, Codec>> PayloadTypeRecorder::GetMappings() const { - return std::vector<std::pair<PayloadType, Codec>>{}; + return std::vector<std::pair<PayloadType, Codec>>( + payload_type_to_codec_.begin(), payload_type_to_codec_.end()); } RTCErrorOr<PayloadType> PayloadTypeRecorder::LookupPayloadType(
diff --git a/g3doc/todo/payload_type_redesign.md b/g3doc/todo/payload_type_redesign.md index d1304e6..0042e41 100644 --- a/g3doc/todo/payload_type_redesign.md +++ b/g3doc/todo/payload_type_redesign.md
@@ -118,21 +118,25 @@ ## Current implementation status -The new strategy is implemented for audio codecs. Several issues that caused -test failures when enabling the `WebRTC-PayloadTypesInTransport` field trial -have been identified and fixed: +The new strategy is implemented for audio codecs and is being enabled for video +codecs. Several issues that caused test failures when enabling the +`WebRTC-PayloadTypesInTransport` field trial have been identified and fixed: - **Audio/Video RED Collision:** RED codecs of different media types were incorrectly matching, leading to payload type conflicts. `MatchesWithCodecRules` now enforces media type equality. -- **MID Recycling:** When a MID was recycled for a different media type (e.g., - Audio -> Video), `CodecVendor` was incorrectly merging codecs from the old - description. This has been fixed by validating the media type before merging. +- **MID Recycling:** When a MID is recycled, it must preserve its media type + (e.g., Audio stays Audio). `CodecVendor` now correctly identifies and returns + an `INTERNAL_ERROR` if a MID is reused for a different media type, preventing + invalid codec merging. - **RED Matching Logic:** Relaxed the matching rules for RED to allow negotiation to proceed even when parameters (linking RED to primary codecs) are not yet populated, as this linking now happens late in the `CodecVendor`. +- **RTX PT Convention:** RTX payload types now follow the conventional + `Primary_PT + 1` rule where possible. -The new strategy is not yet implemented for video codecs. +The new strategy is now mostly implemented for video codecs, including support +for RTX, RED, ULPFEC, and FlexFEC late assignment. ## Unified Implementation Strategy for Audio and Video @@ -200,7 +204,7 @@ - **Stable PT Tests:** Add coverage to ensure that payload types remain stable across renegotiations, even when the order of codecs in the transceiver preferences changes. -- **MID Recycling:** Verify that MID recycling for video-to-audio and vice-versa +- **MID Recycling:** Verify that MID recycling (within the same media type) works correctly without PT collisions or crashes. ## Testing Strategy
diff --git a/pc/codec_vendor.cc b/pc/codec_vendor.cc index bba1723..4512b97 100644 --- a/pc/codec_vendor.cc +++ b/pc/codec_vendor.cc
@@ -196,6 +196,12 @@ config.codec.channels}) : CreateVideoCodec(PayloadType::NotSet(), kRtxCodecName); rtx.SetParam(kCodecParamAssociatedPayloadType, primary_codec.id.value()); + // Convention: RTX PT = primary PT + 1. + // Suggester will ignore this if it is already in use. + int preferred_id = primary_codec.id.value() + 1; + if (preferred_id <= 127) { + rtx.id = PayloadType(preferred_id); + } RTCErrorOr<PayloadType> result = pt_suggester.SuggestPayloadType(mid, rtx, pick_from_top_of_range); if (!result.ok()) { @@ -283,7 +289,8 @@ 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-Advertised") && + !trials.IsEnabled("WebRTC-FlexFEC-03"))) { return RTCError::OK(); } auto fec_it = absl::c_find_if(offered_codecs, [&](const Codec& c) {
diff --git a/pc/codec_vendor_redesign_unittest.cc b/pc/codec_vendor_redesign_unittest.cc index b58aa38..05f7141 100644 --- a/pc/codec_vendor_redesign_unittest.cc +++ b/pc/codec_vendor_redesign_unittest.cc
@@ -22,6 +22,7 @@ #include "api/field_trials.h" #include "api/media_types.h" #include "api/payload_type.h" +#include "api/rtc_error.h" #include "api/rtp_parameters.h" #include "api/rtp_transceiver_direction.h" #include "call/fake_payload_type_suggester.h" @@ -112,6 +113,9 @@ RtpTransceiverDirection::kSendRecv, /*stopped=*/false); + // Force VP8 to PT 120 so that RTX can get 121. + pt_suggester_.SetSuggestion("video", "vp8", PayloadType(120)); + auto result = vendor_->GetNegotiatedCodecsForOffer( options, MediaSessionOptions(), /*current_content=*/nullptr, pt_suggester_); @@ -137,6 +141,9 @@ std::string apt; EXPECT_TRUE(rtx_it->GetParam(kCodecParamAssociatedPayloadType, &apt)); EXPECT_EQ(apt, absl::StrCat(vp8_it->id)); + + // Verify conventional assignment: RTX_PT = primary_PT + 1 + EXPECT_EQ(rtx_it->id.value(), vp8_it->id.value() + 1); } TEST_F(CodecVendorRedesignTest, @@ -342,5 +349,72 @@ EXPECT_EQ(codecs[1].name, "opus"); } +TEST_F(CodecVendorRedesignTest, MidRecyclingToDifferentTypeFails) { + // 1. Generate a video offer for MID "0" + MediaDescriptionOptions video_options(MediaType::VIDEO, "0", + RtpTransceiverDirection::kSendRecv, + /*stopped=*/false); + auto video_result = vendor_->GetNegotiatedCodecsForOffer( + video_options, MediaSessionOptions(), /*current_content=*/nullptr, + pt_suggester_); + ASSERT_TRUE(video_result.ok()); + + // 2. Generate an audio offer for the same MID "0" (invalid recycling) + MediaDescriptionOptions audio_options(MediaType::AUDIO, "0", + RtpTransceiverDirection::kSendRecv, + /*stopped=*/false); + + // We need to provide current_content to simulate recycling + auto video_description = std::make_unique<VideoContentDescription>(); + video_description->set_codecs(video_result.value()); + ContentInfo current_content(MediaProtocolType::kRtp, "0", + std::move(video_description)); + + auto audio_result = vendor_->GetNegotiatedCodecsForOffer( + audio_options, MediaSessionOptions(), ¤t_content, pt_suggester_); + + // Verify that changing media type for the same MID is an error. + ASSERT_FALSE(audio_result.ok()); + EXPECT_EQ(audio_result.error().type(), RTCErrorType::INTERNAL_ERROR); +} + +TEST_F(CodecVendorRedesignTest, VideoOfferIncludesFecAndAssignsIds) { + // Explicitly enable FlexFEC field trial + FieldTrials flexfec_trials( + CreateTestFieldTrials("WebRTC-FlexFEC-03-Advertised/Enabled/" + "WebRTC-PayloadTypesInTransport/Enabled/")); + + std::vector<Codec> video_codecs({ + CreateVideoCodec(97, "vp8"), + CreateVideoCodec(100, "ulpfec"), + CreateVideoCodec(101, "flexfec-03"), + }); + media_engine_.SetVideoSendCodecs(video_codecs); + media_engine_.SetVideoRecvCodecs(video_codecs); + + auto flexfec_vendor = std::make_unique<CodecVendor>( + &media_engine_, /*rtx_enabled=*/true, flexfec_trials); + + MediaDescriptionOptions options(MediaType::VIDEO, "video", + RtpTransceiverDirection::kSendRecv, + /*stopped=*/false); + + auto result = flexfec_vendor->GetNegotiatedCodecsForOffer( + options, MediaSessionOptions(), /*current_content=*/nullptr, + pt_suggester_); + + ASSERT_TRUE(result.ok()); + const auto& codecs = result.value(); + + EXPECT_THAT(codecs, Contains(Field(&Codec::name, "vp8"))); + EXPECT_THAT(codecs, Contains(Field(&Codec::name, "ulpfec"))); + EXPECT_THAT(codecs, Contains(Field(&Codec::name, "flexfec-03"))); + + // Verify IDs are assigned + for (const auto& codec : codecs) { + EXPECT_TRUE(codec.id.IsSet()); + } +} + } // namespace } // namespace webrtc