diff --git a/pc/peer_connection_bundle_unittest.cc b/pc/peer_connection_bundle_unittest.cc index a71b5138659623fad2d8fe66085b891e3b16a7bb..875a77cb904cb758228a5ba55e9c7943b19c63ea 100644 --- a/pc/peer_connection_bundle_unittest.cc +++ b/pc/peer_connection_bundle_unittest.cc @@ -730,16 +730,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] @@ -751,20 +752,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 0cb10b7d079779edbfe5dfc5c0abf1da50d74cc2..a314778b7f2de70bb05c1eb45616859348bedfcc 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -1738,7 +1738,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) << ")"; @@ -1770,6 +1770,23 @@ RTCError SdpOfferAnswerHandler::ApplyLocalDescription( // SCTP sids. AllocateSctpSids(); + // Validate SSRCs, we do not allow duplicates. + if (ConfiguredForMedia()) { + 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()) { if (ConfiguredForMedia()) { // We must use List and not ListInternal here because diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index 4c4554f12cbc179064a965fa42b0816b7b0eea3a..4f3ce60ceca391853b6d4a03a04d1cd304bd35f9 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -12,6 +12,7 @@ #include #include +#include "absl/strings/str_replace.h" #include "api/audio/audio_mixer.h" #include "api/audio_codecs/builtin_audio_decoder_factory.h" #include "api/audio_codecs/builtin_audio_encoder_factory.h" @@ -547,4 +548,21 @@ TEST_F(SdpOfferAnswerTest, RejectedDataChannelsDoGetReofferedWhenActive) { #endif // WEBRTC_HAVE_SCTP +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