[M148] Relax RTP header extension ID reuse checks in RtpTransport Removing the historical_rtp_header_extensions_ list and instead only check header_extensions_by_mid_ for conflicts with active extensions. Also borrowing a test for this provided by philipp.hancke. (cherry picked from commit 8915e3564c0593501ba1a0ffa1ac08b8cb7cd00f) Fixed: chromium:504980502 Bug: webrtc:503013383 Change-Id: I08874edb1d46d638641c74255fe5379622a43c0e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/464501 Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Reviewed-by: Harald Alvestrand <hta@webrtc.org> Cr-Original-Commit-Position: refs/heads/main@{#47472} Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/465760 Cr-Commit-Position: refs/branch-heads/7778@{#4} Cr-Branched-From: ca896b7ffef011bbf6957c99d413c5aac602c99f-refs/heads/main@{#47319}
diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index 8a91c47..62ea4d8 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc
@@ -769,8 +769,10 @@ }); RTCError error = channel1_->SetLocalContent(&local_updated, SdpType::kOffer); - EXPECT_FALSE(error.ok()); - EXPECT_THAT(error.message(), HasSubstr("RTP extension ID reassignment")); + // Expected to succeed because the mapping for ID 1 was cleared by the + // previous SetLocalContent call (which set extensions to empty). + // RtpTransport allows reuse when not in use by any active MID. + EXPECT_TRUE(error.ok()); } // Test that SetLocalContent and SetRemoteContent properly configure
diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index 2cd0df0..e8cd73f 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc
@@ -556,4 +556,59 @@ EXPECT_TRUE(modified_offer); EXPECT_TRUE(local->SetLocalDescription(std::move(modified_offer))); } + +// Reproduces the bug reported by @ibc where RTP extension IDs are reassigned +// to different URIs in subsequent offers, causing SetLocalDescription to fail. +TEST_F(PeerConnectionSimulcastTests, + NoRtpExtensionIdReassignmentWhenAddingTransceiver) { + auto local = CreatePeerConnectionWrapper(); + auto layers = CreateLayers({"f", "h", "q"}, true); + + // Add video transceiver with simulcast. + AddTransceiver(local.get(), layers); + ASSERT_TRUE(local->CreateOfferAndSetAsLocal()); + + // Set remote answer without header extensions. + std::string remote_answer_sdp = + "v=0\r\n" + "o=- 8403615332048243445 2 IN IP4 127.0.0.1\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=group:BUNDLE 0\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 96\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=mid:0\r\n" + "a=ice-ufrag:IZeV\r\n" + "a=ice-pwd:uaZhQD4rYM/Tta2qWBT1Bbt4\r\n" + "a=fingerprint:sha-256 " + "D8:6C:3D:FA:23:E2:2C:63:11:2D:D0:86:BE:C4:D0:65:F9:42:F7:1C:06:04:27:E6:" + "1C:2C:74:01:8D:50:67:23\r\n" + "a=setup:active\r\n" + "a=rtcp-mux\r\n" + "a=extmap:9 urn:ietf:params:rtp-hdrext:sdes:mid\r\n" + "a=extmap:10 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id\r\n" + "a=extmap:11 urn:ietf:params:rtp-hdrext:sdes:repaired-rtp-stream-id\r\n" + "a=extmap:2 " + "http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time\r\n" + "a=extmap:4 " + "http://www.ietf.org/id/" + "draft-holmer-rmcat-transport-wide-cc-extensions-01\r\n" + "a=extmap:3 urn:3gpp:video-orientation\r\n" + "a=extmap:1 urn:ietf:params:rtp-hdrext:toffset\r\n" + "a=extmap:5 " + "http://www.webrtc.org/experiments/rtp-hdrext/playout-delay\r\n" + "a=rtpmap:96 VP8/90000\r\n"; + // Answer recvonly. + ASSERT_TRUE(local->SetRemoteDescription(CreateSessionDescription( + SdpType::kAnswer, remote_answer_sdp + "a=recvonly\r\n"))); + + ASSERT_TRUE(local->CreateOfferAndSetAsLocal()); + // Answer inactive. + ASSERT_TRUE(local->SetRemoteDescription(CreateSessionDescription( + SdpType::kAnswer, remote_answer_sdp + "a=inactive\r\n"))); + + // Add an audio transceiver. + local->AddAudioTrack("audio"); + EXPECT_TRUE(local->CreateOfferAndSetAsLocal()); +} } // namespace webrtc
diff --git a/pc/rtp_transport.cc b/pc/rtp_transport.cc index 05ef651..086ea72 100644 --- a/pc/rtp_transport.cc +++ b/pc/rtp_transport.cc
@@ -242,15 +242,22 @@ if (new_extension.id == 0) { continue; } - auto it = absl::c_find_if( - historical_rtp_header_extensions_, - [&](const RtpExtension& ext) { return ext.id == new_extension.id; }); - if (it != historical_rtp_header_extensions_.end() && - it->uri != new_extension.uri) { - return RTCError::InvalidParameter() - << "RTP extension ID reassignment not supported (id=" - << new_extension.id << ", old_uri=\"" << it->uri - << "\", new_uri=\"" << new_extension.uri << "\")."; + // TODO: bugs.webrtc.org/503013383 - Introduce checking against IDs that are + // currently not present in the SDP, but have been used in previous + // negotiation rounds. Reusing extensions with a different ID is a protocol + // violation, but we cannot check this until we check against the same + // protocol violation on the sender side. + for (const auto& [mid, active_extensions] : header_extensions_by_mid_) { + auto it = absl::c_find_if( + active_extensions, + [&](const RtpExtension& ext) { return ext.id == new_extension.id; }); + if (it != active_extensions.end() && it->uri != new_extension.uri) { + return RTCError::InvalidParameter() + << "RTP extension ID reassignment not supported (collision on " + "active MID " + << mid << ", id=" << new_extension.id << ", old_uri=\"" + << it->uri << "\", new_uri=\"" << new_extension.uri << "\")."; + } } } @@ -275,21 +282,6 @@ return RTCError::OK(); } - for (const RtpExtension& extension : extensions) { - if (extension.id == 0) { - continue; - } - auto it = absl::c_find_if(historical_rtp_header_extensions_, - [&extension](const RtpExtension& ext) { - return ext.id == extension.id; - }); - if (it == historical_rtp_header_extensions_.end()) { - historical_rtp_header_extensions_.push_back(extension); - } else { - RTC_DCHECK_EQ(it->uri, extension.uri); - } - } - RemoveExtensionMapForMid(mid, header_extensions_by_mid_); header_extensions_by_mid_.emplace_back(std::string(mid), extensions);
diff --git a/pc/rtp_transport.h b/pc/rtp_transport.h index e51a124..1852015 100644 --- a/pc/rtp_transport.h +++ b/pc/rtp_transport.h
@@ -181,9 +181,6 @@ std::vector<std::pair<std::string, RtpHeaderExtensions>> header_extensions_by_mid_ RTC_GUARDED_BY(network_thread_checker_); - RtpHeaderExtensions historical_rtp_header_extensions_ - RTC_GUARDED_BY(network_thread_checker_); - // Guard against recursive "ready to send" signals bool processing_ready_to_send_ = false; RTC_NO_UNIQUE_ADDRESS SequenceChecker network_thread_checker_;
diff --git a/pc/rtp_transport_unittest.cc b/pc/rtp_transport_unittest.cc index 65762fa..07430e6 100644 --- a/pc/rtp_transport_unittest.cc +++ b/pc/rtp_transport_unittest.cc
@@ -390,6 +390,28 @@ EXPECT_EQ(error.type(), RTCErrorType::INVALID_PARAMETER); } +TEST(RtpTransportTest, + VerifyRtpHeaderExtensionMapAllowsIdReuseAfterUnregister) { + test::RunLoop loop; + RtpTransport transport(kMuxDisabled, CreateTestFieldTrials()); + RtpHeaderExtensions extensions1 = { + RtpExtension("urn:ietf:params:rtp-hdrext:ssrc-audio-level", 1)}; + RtpHeaderExtensions extensions2 = {RtpExtension( + "http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time", 1)}; + + // Registering the first map should succeed. + EXPECT_TRUE( + transport.RegisterRtpHeaderExtensionMap("audio", extensions1).ok()); + + // Unregister the first map. + transport.UnregisterRtpHeaderExtensionMap("audio"); + + // Registering the second map with same ID but different URI should now + // succeed! + EXPECT_TRUE( + transport.RegisterRtpHeaderExtensionMap("video", extensions2).ok()); +} + // Test that SignalPacketReceived fires with rtcp=true when a RTCP packet is // received. TEST(RtpTransportTest, SignalDemuxedRtcp) {