Reject bundles with codec collisions. UMA shows this at 3% - the first expectation of this CL is to get proper error reports. Bug: webrtc:42224689 Change-Id: I6646192804d8be9f956b27efdd0fd22f40692d64 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/438741 Reviewed-by: Guido Urdaneta <guidou@webrtc.org> Commit-Queue: Guido Urdaneta <guidou@webrtc.org> Auto-Submit: Harald Alvestrand <hta@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47457}
diff --git a/experiments/field_trials.py b/experiments/field_trials.py index 84489a3..2b3c13b 100755 --- a/experiments/field_trials.py +++ b/experiments/field_trials.py
@@ -206,6 +206,9 @@ FieldTrial('WebRTC-Sctp-Snap', 426480601, date(2026, 1, 1)), + FieldTrial('WebRTC-SdpBundlePayloadTypeCollisionCheck', + 42224689, + date(2027, 1, 1)), FieldTrial('WebRTC-SimulcastEncoderAdapter-DropUnalignedResolution', 415329365, date(2025, 11, 2)),
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index b36b5b8..3e13b30 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc
@@ -3962,9 +3962,15 @@ // Validate that there are no collisions of bundled payload types. error = ValidateBundledPayloadTypes(*sdesc->description()); - // TODO(bugs.webrtc.org/14420): actually reject. RTC_HISTOGRAM_BOOLEAN("WebRTC.PeerConnection.ValidBundledPayloadTypes", error.ok()); + if (!error.ok()) { + RTC_LOG(LS_ERROR) << "Bundled payload type collision: " << error.message(); + if (env_.field_trials().IsEnabled( + "WebRTC-SdpBundlePayloadTypeCollisionCheck")) { + return error; + } + } // Validate that there are no collisions of bundled header extensions ids. error = ValidateBundledRtpHeaderExtensions(*sdesc->description());
diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index 239ee62..da39fa3 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc
@@ -188,8 +188,9 @@ transceiver->stopped(); } -TEST_F(SdpOfferAnswerTest, BundleRejectsCodecCollisionsAudioVideo) { - auto pc = CreatePeerConnection(); +TEST_F(SdpOfferAnswerTest, + BundleAcceptsCodecCollisionsAudioVideoWhenFieldTrialDisabled) { + auto pc = CreatePeerConnection(""); std::string sdp = "v=0\r\n" "o=- 0 3 IN IP4 127.0.0.1\r\n" @@ -221,7 +222,6 @@ ASSERT_NE(desc, nullptr); RTCError error; pc->SetRemoteDescription(std::move(desc), &error); - // There is no error yet but the metrics counter will increase. EXPECT_TRUE(error.ok()); EXPECT_METRIC_EQ( @@ -240,8 +240,52 @@ true)); } -TEST_F(SdpOfferAnswerTest, BundleRejectsCodecCollisionsVideoFmtp) { - auto pc = CreatePeerConnection(); +TEST_F(SdpOfferAnswerTest, + BundleRejectsCodecCollisionsAudioVideoWhenFieldTrialEnabled) { + auto pc = CreatePeerConnection( + "WebRTC-SdpBundlePayloadTypeCollisionCheck/Enabled/"); + std::string sdp = + "v=0\r\n" + "o=- 0 3 IN IP4 127.0.0.1\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=group:BUNDLE 0 1\r\n" + "a=fingerprint:sha-1 " + "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n" + "a=setup:actpass\r\n" + "a=ice-ufrag:ETEn\r\n" + "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n" + "m=audio 9 UDP/TLS/RTP/SAVPF 111\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:0\r\n" + "a=rtpmap:111 opus/48000/2\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 111\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:1\r\n" + "a=rtpmap:111 H264/90000\r\n" + "a=fmtp:111 " + "level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=" + "42e01f\r\n"; + + auto desc = CreateSessionDescription(SdpType::kOffer, sdp); + ASSERT_NE(desc, nullptr); + RTCError error; + pc->SetRemoteDescription(std::move(desc), &error); + EXPECT_FALSE(error.ok()); + EXPECT_THAT(error.type(), Eq(RTCErrorType::INVALID_PARAMETER)); + + EXPECT_METRIC_EQ( + 1, metrics::NumEvents("WebRTC.PeerConnection.ValidBundledPayloadTypes", + false)); +} + +TEST_F(SdpOfferAnswerTest, + BundleAcceptsCodecCollisionsVideoFmtpWhenFieldTrialDisabled) { + auto pc = CreatePeerConnection(""); std::string sdp = "v=0\r\n" "o=- 0 3 IN IP4 127.0.0.1\r\n" @@ -282,6 +326,51 @@ false)); } +TEST_F(SdpOfferAnswerTest, + BundleRejectsCodecCollisionsVideoFmtpWhenFieldTrialEnabled) { + auto pc = CreatePeerConnection( + "WebRTC-SdpBundlePayloadTypeCollisionCheck/Enabled/"); + std::string sdp = + "v=0\r\n" + "o=- 0 3 IN IP4 127.0.0.1\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=group:BUNDLE 0 1\r\n" + "a=fingerprint:sha-1 " + "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n" + "a=setup:actpass\r\n" + "a=ice-ufrag:ETEn\r\n" + "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 111\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:0\r\n" + "a=rtpmap:111 H264/90000\r\n" + "a=fmtp:111 " + "level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=" + "42e01f\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 111\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:1\r\n" + "a=rtpmap:111 H264/90000\r\n" + "a=fmtp:111 " + "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=" + "42e01f\r\n"; + + auto desc = CreateSessionDescription(SdpType::kOffer, sdp); + ASSERT_NE(desc, nullptr); + RTCError error; + pc->SetRemoteDescription(std::move(desc), &error); + EXPECT_FALSE(error.ok()); + EXPECT_THAT(error.type(), Eq(RTCErrorType::INVALID_PARAMETER)); + EXPECT_METRIC_EQ( + 1, metrics::NumEvents("WebRTC.PeerConnection.ValidBundledPayloadTypes", + false)); +} + TEST_F(SdpOfferAnswerTest, BundleCodecCollisionInDifferentBundlesAllowed) { auto pc = CreatePeerConnection(); std::string sdp =