From 2c07d373b9ae9ec93472c1aa144979616f8ac557 Mon Sep 17 00:00:00 2001 From: slimshady999z Date: Wed, 6 Sep 2023 11:13:49 +0800 Subject: [PATCH] add patch to 09041120 Signed-off-by: slimshady999z Change-Id: I308bb54c1f24e7d731f108b2ccd2742f1090c95a --- modules/pacing/packet_router.cc | 13 ++++-- modules/pacing/packet_router_unittest.cc | 18 ++++++--- pc/peer_connection_bundle_unittest.cc | 51 +++++++++++++++++------- pc/sdp_offer_answer.cc | 17 +++++++- pc/sdp_offer_answer_unittest.cc | 19 ++++++++- 5 files changed, 92 insertions(+), 26 deletions(-) diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index fcc7ee34..0d04497a 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -85,10 +85,15 @@ void PacketRouter::AddSendRtpModuleToMap(RtpRtcpInterface* rtp_module, } void PacketRouter::RemoveSendRtpModuleFromMap(uint32_t ssrc) { - auto kv = send_modules_map_.find(ssrc); - RTC_DCHECK(kv != send_modules_map_.end()); - send_modules_list_.remove(kv->second); - send_modules_map_.erase(kv); + + auto it = send_modules_map_.find(ssrc); + if (it == send_modules_map_.end()) { + RTC_LOG(LS_ERROR) << "No send module found for ssrc " << ssrc; + return; + } + send_modules_list_.remove(it->second); + + send_modules_map_.erase(it); } void PacketRouter::RemoveSendRtpModule(RtpRtcpInterface* rtp_module) { diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index 77fe5f9f..3bed444f 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -423,18 +423,24 @@ TEST_F(PacketRouterDeathTest, DoubleRegistrationOfReceiveModuleDisallowed) { packet_router_.RemoveReceiveRtpModule(&module); } -TEST_F(PacketRouterDeathTest, RemovalOfNeverAddedSendModuleDisallowed) { +TEST_F(PacketRouterDeathTest, RemovalOfNeverAddedReceiveModuleDisallowed) { NiceMock module; - EXPECT_DEATH(packet_router_.RemoveSendRtpModule(&module), ""); + EXPECT_DEATH(packet_router_.RemoveReceiveRtpModule(&module), ""); } +#endif // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -TEST_F(PacketRouterDeathTest, RemovalOfNeverAddedReceiveModuleDisallowed) { +TEST_F(PacketRouterTest, RemovalOfNeverAddedSendModuleIgnored) { NiceMock module; - - EXPECT_DEATH(packet_router_.RemoveReceiveRtpModule(&module), ""); + packet_router_.RemoveSendRtpModule(&module); +} + +TEST_F(PacketRouterTest, DuplicateRemovalOfSendModuleIgnored) { + NiceMock module; + packet_router_.AddSendRtpModule(&module, false); + packet_router_.RemoveSendRtpModule(&module); + packet_router_.RemoveSendRtpModule(&module); } -#endif // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) TEST(PacketRouterRembTest, ChangeSendRtpModuleChangeRembSender) { rtc::ScopedFakeClock clock; diff --git a/pc/peer_connection_bundle_unittest.cc b/pc/peer_connection_bundle_unittest.cc index e5eb6c4e..a59983c8 100644 --- a/pc/peer_connection_bundle_unittest.cc +++ b/pc/peer_connection_bundle_unittest.cc @@ -675,16 +675,17 @@ TEST_P(PeerConnectionBundleTest, BundleOnFirstMidInAnswer) { } // This tests that applying description with conflicted RTP demuxing criteria -// will fail. -TEST_P(PeerConnectionBundleTest, - ApplyDescriptionWithConflictedDemuxCriteriaFail) { +// will fail when using BUNDLE. +TEST_P(PeerConnectionBundleTest, ApplyDescriptionWithSameSsrcsBundledFails) { auto caller = CreatePeerConnectionWithAudioVideo(); auto callee = CreatePeerConnectionWithAudioVideo(); RTCOfferAnswerOptions options; - options.use_rtp_mux = false; + options.use_rtp_mux = true; auto offer = caller->CreateOffer(options); - // Modified the SDP to make two m= sections have the same SSRC. + EXPECT_TRUE( + caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + // Modify the remote SDP to make two m= sections have the same SSRC. ASSERT_GE(offer->description()->contents().size(), 2U); offer->description() ->contents()[0] @@ -696,20 +697,42 @@ TEST_P(PeerConnectionBundleTest, .media_description() ->mutable_streams()[0] .ssrcs[0] = 1111222; + EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer))); + + // When BUNDLE is enabled, applying the description is expected to fail + // because the demuxing criteria can not be satisfied. + auto answer = callee->CreateAnswer(options); + EXPECT_FALSE(callee->SetLocalDescription(std::move(answer))); +} + +// A variant of the above, without BUNDLE duplicate SSRCs are allowed. +TEST_P(PeerConnectionBundleTest, + ApplyDescriptionWithSameSsrcsUnbundledSucceeds) { + auto caller = CreatePeerConnectionWithAudioVideo(); + auto callee = CreatePeerConnectionWithAudioVideo(); + + RTCOfferAnswerOptions options; + options.use_rtp_mux = false; + auto offer = caller->CreateOffer(options); EXPECT_TRUE( caller->SetLocalDescription(CloneSessionDescription(offer.get()))); + // Modify the remote SDP to make two m= sections have the same SSRC. + ASSERT_GE(offer->description()->contents().size(), 2U); + offer->description() + ->contents()[0] + .media_description() + ->mutable_streams()[0] + .ssrcs[0] = 1111222; + offer->description() + ->contents()[1] + .media_description() + ->mutable_streams()[0] + .ssrcs[0] = 1111222; EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer))); - EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal(options)); - // Enable BUNDLE in subsequent offer/answer exchange and two m= sections are - // expectd to use one RtpTransport underneath. - options.use_rtp_mux = true; - EXPECT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(options))); + // Without BUNDLE, demuxing is done per-transport. auto answer = callee->CreateAnswer(options); - // When BUNDLE is enabled, applying the description is expected to fail - // because the demuxing criteria is conflicted. - EXPECT_FALSE(callee->SetLocalDescription(std::move(answer))); + EXPECT_TRUE(callee->SetLocalDescription(std::move(answer))); } // This tests that changing the pre-negotiated BUNDLE tag is not supported. diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index ea1c4c4d..c3a56c63 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -1587,7 +1587,7 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( if (type == SdpType::kOffer) { // TODO(bugs.webrtc.org/4676) - Handle CreateChannel failure, as new local // description is applied. Restore back to old description. - RTCError error = CreateChannels(*local_description()->description()); + error = CreateChannels(*local_description()->description()); if (!error.ok()) { RTC_LOG(LS_ERROR) << error.message() << " (" << SdpTypeToString(type) << ")"; @@ -1622,6 +1622,21 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( data_channel_controller()->AllocateSctpSids(role); } + // Validate SSRCs, we do not allow duplicates. + std::set used_ssrcs; + for (const auto& content : local_description()->description()->contents()) { + for (const auto& stream : content.media_description()->streams()) { + for (uint32_t ssrc : stream.ssrcs) { + auto result = used_ssrcs.insert(ssrc); + if (!result.second) { + LOG_AND_RETURN_ERROR( + RTCErrorType::INVALID_PARAMETER, + "Duplicate ssrc " + rtc::ToString(ssrc) + " is not allowed"); + } + } + } + } + if (IsUnifiedPlan()) { // We must use List and not ListInternal here because // transceivers()->StableState() is indexed by the non-internal refptr. diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index df343cce..d3469393 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -14,7 +14,7 @@ #include #include #include - +#include "absl/strings/str_replace.h" #include "absl/types/optional.h" #include "api/audio/audio_mixer.h" #include "api/audio_codecs/audio_decoder_factory.h" @@ -137,4 +137,21 @@ TEST_F(SdpOfferAnswerTest, OnTrackReturnsProxiedObject) { transceiver->stopped(); } +TEST_F(SdpOfferAnswerTest, DuplicateSsrcsDisallowedInLocalDescription) { + auto pc = CreatePeerConnection(); + pc->AddAudioTrack("audio_track", {}); + pc->AddVideoTrack("video_track", {}); + auto offer = pc->CreateOffer(); + auto& offer_contents = offer->description()->contents(); + ASSERT_EQ(offer_contents.size(), 2u); + uint32_t second_ssrc = offer_contents[1].media_description()->first_ssrc(); + + offer->description() + ->contents()[0] + .media_description() + ->mutable_streams()[0] + .ssrcs[0] = second_ssrc; + EXPECT_FALSE(pc->SetLocalDescription(std::move(offer))); +} + } // namespace webrtc -- Gitee