From 2bed397a1c0aac34112a1fce3e251ba6e749cb97 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Fri, 4 Jan 2019 17:04:30 -0800 Subject: [PATCH] Support changing the tagged BUNDLE media section section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The behavior implemented in this CL matches Firefox: 1. If there are no common media sections from the previous BUNDLE group, then the previous transport is stopped and a new transport created. 2. If there is at least one common media section from the previous BUNDLE group, then the existing transport is reused. This will only happen if the tagged media section is rejected. Bug: webrtc:9954 Change-Id: If0f0733c0ab91858594304828d126640e2ab9520 Reviewed-on: https://webrtc-review.googlesource.com/c/114920 Reviewed-by: Henrik Boström Reviewed-by: Harald Alvestrand Commit-Queue: Steve Anton Cr-Commit-Position: refs/heads/master@{#26150} --- pc/jseptransportcontroller.cc | 7 -- pc/jseptransportcontroller_unittest.cc | 48 ++++++++ pc/mediasession.cc | 26 +++-- pc/mediasession_unittest.cc | 153 +++++++++++++++++++++++++ pc/peerconnection_integrationtest.cc | 36 ++++++ 5 files changed, 253 insertions(+), 17 deletions(-) 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