diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index fcc7ee3449a8b40fa277cb170243db55045e338e..0d04497a3ac6a319fe2d5654b20d8fe0119afe4e 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 77fe5f9f8dc04043467c0016e21a1025339da402..3bed444febfb0ec219129135064264394741da77 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 e5eb6c4e18b7dc1c3369883ec92d345b3e51c66f..a59983c825357b6d2054ac640426458bf03adbaf 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 ea1c4c4d23c700c91dbf61d471afad67ffcb7df3..c3a56c638ad3127925bde8e93b2650dfe3d07b9c 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 df343cce3a4f672af1cea4253847758e323867e8..d3469393570367247e47e8293e6e5739959fda14 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