diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc index fec681cc6e..c1f18b90bb 100644 --- a/pc/jseptransportcontroller.cc +++ b/pc/jseptransportcontroller.cc @@ -691,13 +691,6 @@ RTCError JsepTransportController::ValidateAndMaybeUpdateBundleGroup( } if (ShouldUpdateBundleGroup(type, description)) { - const std::string* new_bundled_mid = new_bundle_group->FirstContentName(); - if (bundled_mid() && new_bundled_mid && - *bundled_mid() != *new_bundled_mid) { - return RTCError(RTCErrorType::UNSUPPORTED_OPERATION, - "Changing the negotiated BUNDLE-tag is not supported."); - } - bundle_group_ = *new_bundle_group; } diff --git a/pc/jseptransportcontroller_unittest.cc b/pc/jseptransportcontroller_unittest.cc index 3031b8df7a..c019013cf9 100644 --- a/pc/jseptransportcontroller_unittest.cc +++ b/pc/jseptransportcontroller_unittest.cc @@ -11,6 +11,7 @@ #include #include +#include "absl/memory/memory.h" #include "api/media_transport_interface.h" #include "api/test/fake_media_transport.h" #include "p2p/base/fakedtlstransport.h" @@ -1730,4 +1731,51 @@ TEST_F(JsepTransportControllerTest, RemoveContentFromBundleGroup) { .ok()); } +// Test that the JsepTransportController can process a new local and remote +// description that changes the tagged BUNDLE group with the max-bundle policy +// specified. +// This is a regression test for bugs.webrtc.org/9954 +TEST_F(JsepTransportControllerTest, ChangeTaggedMediaSectionMaxBundle) { + CreateJsepTransportController(JsepTransportController::Config()); + + auto local_offer = absl::make_unique(); + AddAudioSection(local_offer.get(), kAudioMid1, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + cricket::ContentGroup bundle_group(cricket::GROUP_TYPE_BUNDLE); + bundle_group.AddContentName(kAudioMid1); + local_offer->AddGroup(bundle_group); + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_offer.get()) + .ok()); + + std::unique_ptr remote_answer( + local_offer->Copy()); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_answer.get()) + .ok()); + + std::unique_ptr local_reoffer( + local_offer->Copy()); + local_reoffer->contents()[0].rejected = true; + AddVideoSection(local_reoffer.get(), kVideoMid1, kIceUfrag1, kIcePwd1, + cricket::ICEMODE_FULL, cricket::CONNECTIONROLE_ACTPASS, + nullptr); + local_reoffer->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + cricket::ContentGroup new_bundle_group(cricket::GROUP_TYPE_BUNDLE); + new_bundle_group.AddContentName(kVideoMid1); + local_reoffer->AddGroup(new_bundle_group); + + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, local_reoffer.get()) + .ok()); + + std::unique_ptr remote_reanswer( + local_reoffer->Copy()); + EXPECT_TRUE( + transport_controller_ + ->SetRemoteDescription(SdpType::kAnswer, remote_reanswer.get()) + .ok()); +} + } // namespace webrtc diff --git a/pc/mediasession.cc b/pc/mediasession.cc index 4f7fae1077..e650dd2c32 100644 --- a/pc/mediasession.cc +++ b/pc/mediasession.cc @@ -1500,9 +1500,12 @@ std::unique_ptr MediaSessionDescriptionFactory::CreateOffer( // Bundle the contents together, if we've been asked to do so, and update any // parameters that need to be tweaked for BUNDLE. - if (session_options.bundle_enabled && offer->contents().size() > 0u) { + if (session_options.bundle_enabled) { ContentGroup offer_bundle(GROUP_TYPE_BUNDLE); for (const ContentInfo& content : offer->contents()) { + if (content.rejected) { + continue; + } // TODO(deadbeef): There are conditions that make bundling two media // descriptions together illegal. For example, they use the same payload // type to represent different codecs, or same IDs for different header @@ -1510,15 +1513,18 @@ std::unique_ptr MediaSessionDescriptionFactory::CreateOffer( // descriptions together. offer_bundle.AddContentName(content.name); } - offer->AddGroup(offer_bundle); - if (!UpdateTransportInfoForBundle(offer_bundle, offer.get())) { - RTC_LOG(LS_ERROR) - << "CreateOffer failed to UpdateTransportInfoForBundle."; - return nullptr; - } - if (!UpdateCryptoParamsForBundle(offer_bundle, offer.get())) { - RTC_LOG(LS_ERROR) << "CreateOffer failed to UpdateCryptoParamsForBundle."; - return nullptr; + if (!offer_bundle.content_names().empty()) { + offer->AddGroup(offer_bundle); + if (!UpdateTransportInfoForBundle(offer_bundle, offer.get())) { + RTC_LOG(LS_ERROR) + << "CreateOffer failed to UpdateTransportInfoForBundle."; + return nullptr; + } + if (!UpdateCryptoParamsForBundle(offer_bundle, offer.get())) { + RTC_LOG(LS_ERROR) + << "CreateOffer failed to UpdateCryptoParamsForBundle."; + return nullptr; + } } } diff --git a/pc/mediasession_unittest.cc b/pc/mediasession_unittest.cc index 16be74f664..c73c19ec19 100644 --- a/pc/mediasession_unittest.cc +++ b/pc/mediasession_unittest.cc @@ -890,6 +890,159 @@ TEST_F(MediaSessionDescriptionFactoryTest, TestCreateImplicitSctpDataOffer) { EXPECT_EQ(cricket::kMediaProtocolSctp, data->media_description()->protocol()); } +// Test that if BUNDLE is enabled and all media sections are rejected then the +// BUNDLE group is not present in the re-offer. +TEST_F(MediaSessionDescriptionFactoryTest, ReOfferNoBundleGroupIfAllRejected) { + MediaSessionOptions opts; + opts.bundle_enabled = true; + AddMediaDescriptionOptions(MEDIA_TYPE_AUDIO, "audio", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + std::unique_ptr offer = f1_.CreateOffer(opts, nullptr); + + opts.media_description_options[0].stopped = true; + std::unique_ptr reoffer = + f1_.CreateOffer(opts, offer.get()); + + EXPECT_FALSE(reoffer->GetGroupByName(cricket::GROUP_TYPE_BUNDLE)); +} + +// Test that if BUNDLE is enabled and the remote re-offer does not include a +// BUNDLE group since all media sections are rejected, then the re-answer also +// does not include a BUNDLE group. +TEST_F(MediaSessionDescriptionFactoryTest, ReAnswerNoBundleGroupIfAllRejected) { + MediaSessionOptions opts; + opts.bundle_enabled = true; + AddMediaDescriptionOptions(MEDIA_TYPE_AUDIO, "audio", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + std::unique_ptr offer = f1_.CreateOffer(opts, nullptr); + std::unique_ptr answer = + f2_.CreateAnswer(offer.get(), opts, nullptr); + + opts.media_description_options[0].stopped = true; + std::unique_ptr reoffer = + f1_.CreateOffer(opts, offer.get()); + std::unique_ptr reanswer = + f2_.CreateAnswer(reoffer.get(), opts, answer.get()); + + EXPECT_FALSE(reanswer->GetGroupByName(cricket::GROUP_TYPE_BUNDLE)); +} + +// Test that if BUNDLE is enabled and the previous offerer-tagged media section +// was rejected then the new offerer-tagged media section is the non-rejected +// media section. +TEST_F(MediaSessionDescriptionFactoryTest, ReOfferChangeBundleOffererTagged) { + MediaSessionOptions opts; + opts.bundle_enabled = true; + AddMediaDescriptionOptions(MEDIA_TYPE_AUDIO, "audio", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + std::unique_ptr offer = f1_.CreateOffer(opts, nullptr); + + // Reject the audio m= section and add a video m= section. + opts.media_description_options[0].stopped = true; + AddMediaDescriptionOptions(MEDIA_TYPE_VIDEO, "video", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + std::unique_ptr reoffer = + f1_.CreateOffer(opts, offer.get()); + + const cricket::ContentGroup* bundle_group = + reoffer->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + ASSERT_TRUE(bundle_group); + EXPECT_FALSE(bundle_group->HasContentName("audio")); + EXPECT_TRUE(bundle_group->HasContentName("video")); +} + +// Test that if BUNDLE is enabled and the previous offerer-tagged media section +// was rejected and a new media section is added, then the re-answer BUNDLE +// group will contain only the non-rejected media section. +TEST_F(MediaSessionDescriptionFactoryTest, ReAnswerChangedBundleOffererTagged) { + MediaSessionOptions opts; + opts.bundle_enabled = true; + AddMediaDescriptionOptions(MEDIA_TYPE_AUDIO, "audio", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + std::unique_ptr offer = f1_.CreateOffer(opts, nullptr); + std::unique_ptr answer = + f2_.CreateAnswer(offer.get(), opts, nullptr); + + // Reject the audio m= section and add a video m= section. + opts.media_description_options[0].stopped = true; + AddMediaDescriptionOptions(MEDIA_TYPE_VIDEO, "video", + RtpTransceiverDirection::kSendRecv, kActive, + &opts); + std::unique_ptr reoffer = + f1_.CreateOffer(opts, offer.get()); + std::unique_ptr reanswer = + f2_.CreateAnswer(reoffer.get(), opts, answer.get()); + + const cricket::ContentGroup* bundle_group = + reanswer->GetGroupByName(cricket::GROUP_TYPE_BUNDLE); + ASSERT_TRUE(bundle_group); + EXPECT_FALSE(bundle_group->HasContentName("audio")); + EXPECT_TRUE(bundle_group->HasContentName("video")); +} + +// Test that if the BUNDLE offerer-tagged media section is changed in a reoffer +// and there is still a non-rejected media section that was in the initial +// offer, then the ICE credentials do not change in the reoffer offerer-tagged +// media section. +TEST_F(MediaSessionDescriptionFactoryTest, + ReOfferChangeBundleOffererTaggedKeepsIceCredentials) { + MediaSessionOptions opts; + opts.bundle_enabled = true; + AddAudioVideoSections(RtpTransceiverDirection::kSendRecv, &opts); + std::unique_ptr offer = f1_.CreateOffer(opts, nullptr); + std::unique_ptr answer = + f2_.CreateAnswer(offer.get(), opts, nullptr); + + // Reject the audio m= section. + opts.media_description_options[0].stopped = true; + std::unique_ptr reoffer = + f1_.CreateOffer(opts, offer.get()); + + const TransportDescription* offer_tagged = + offer->GetTransportDescriptionByName("audio"); + ASSERT_TRUE(offer_tagged); + const TransportDescription* reoffer_tagged = + reoffer->GetTransportDescriptionByName("video"); + ASSERT_TRUE(reoffer_tagged); + EXPECT_EQ(offer_tagged->ice_ufrag, reoffer_tagged->ice_ufrag); + EXPECT_EQ(offer_tagged->ice_pwd, reoffer_tagged->ice_pwd); +} + +// Test that if the BUNDLE offerer-tagged media section is changed in a reoffer +// and there is still a non-rejected media section that was in the initial +// offer, then the ICE credentials do not change in the reanswer answerer-tagged +// media section. +TEST_F(MediaSessionDescriptionFactoryTest, + ReAnswerChangeBundleOffererTaggedKeepsIceCredentials) { + MediaSessionOptions opts; + opts.bundle_enabled = true; + AddAudioVideoSections(RtpTransceiverDirection::kSendRecv, &opts); + std::unique_ptr offer = f1_.CreateOffer(opts, nullptr); + std::unique_ptr answer = + f2_.CreateAnswer(offer.get(), opts, nullptr); + + // Reject the audio m= section. + opts.media_description_options[0].stopped = true; + std::unique_ptr reoffer = + f1_.CreateOffer(opts, offer.get()); + std::unique_ptr reanswer = + f2_.CreateAnswer(reoffer.get(), opts, answer.get()); + + const TransportDescription* answer_tagged = + answer->GetTransportDescriptionByName("audio"); + ASSERT_TRUE(answer_tagged); + const TransportDescription* reanswer_tagged = + reanswer->GetTransportDescriptionByName("video"); + ASSERT_TRUE(reanswer_tagged); + EXPECT_EQ(answer_tagged->ice_ufrag, reanswer_tagged->ice_ufrag); + EXPECT_EQ(answer_tagged->ice_pwd, reanswer_tagged->ice_pwd); +} + // Create an audio, video offer without legacy StreamParams. TEST_F(MediaSessionDescriptionFactoryTest, TestCreateOfferWithoutLegacyStreams) { diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index c296e4c7d7..26acf826a4 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -5004,6 +5004,42 @@ TEST_F(PeerConnectionIntegrationTestPlanB, TwoVideoUnifiedPlanToNoMediaPlanB) { ASSERT_TRUE(ExpectNewFrames(media_expectations)); } +// Test that if the initial offer tagged BUNDLE section is rejected due to its +// associated RtpTransceiver being stopped and another transceiver is added, +// then renegotiation causes the callee to receive the new video track without +// error. +// This is a regression test for bugs.webrtc.org/9954 +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, + ReOfferWithStoppedBundleTaggedTransceiver) { + RTCConfiguration config; + config.bundle_policy = PeerConnectionInterface::kBundlePolicyMaxBundle; + ASSERT_TRUE(CreatePeerConnectionWrappersWithConfig(config, config)); + ConnectFakeSignaling(); + auto audio_transceiver_or_error = + caller()->pc()->AddTransceiver(caller()->CreateLocalAudioTrack()); + ASSERT_TRUE(audio_transceiver_or_error.ok()); + auto audio_transceiver = audio_transceiver_or_error.MoveValue(); + + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + { + MediaExpectations media_expectations; + media_expectations.CalleeExpectsSomeAudio(); + ASSERT_TRUE(ExpectNewFrames(media_expectations)); + } + + audio_transceiver->Stop(); + caller()->pc()->AddTransceiver(caller()->CreateLocalVideoTrack()); + + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + { + MediaExpectations media_expectations; + media_expectations.CalleeExpectsSomeVideo(); + ASSERT_TRUE(ExpectNewFrames(media_expectations)); + } +} + } // namespace } // namespace webrtc