From 0e209b03bf55d6daf209e35b3a8e8b6eab3d4d52 Mon Sep 17 00:00:00 2001 From: Donald Curtis Date: Tue, 24 Mar 2015 09:29:54 -0700 Subject: [PATCH] Update bundle behavior to match BundlePolicy spec in http://rtcweb-wg.github.io/jsep/. BUG=1574 R=juberti@webrtc.org, pthatcher@webrtc.org Review URL: https://webrtc-codereview.appspot.com/36659004 Cr-Commit-Position: refs/heads/master@{#8851} --- talk/app/webrtc/peerconnection.cc | 3 +- talk/app/webrtc/webrtcsession.cc | 22 ++- talk/app/webrtc/webrtcsession_unittest.cc | 178 +++++++++++++++++----- webrtc/p2p/base/session.cc | 73 ++++----- webrtc/p2p/base/session.h | 12 +- 5 files changed, 188 insertions(+), 100 deletions(-) diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index d0c4cc276..46cae4cfc 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -339,8 +339,7 @@ bool PeerConnection::Initialize( // To handle both internal and externally created port allocator, we will // enable BUNDLE here. int portallocator_flags = port_allocator_->flags(); - portallocator_flags |= cricket::PORTALLOCATOR_ENABLE_BUNDLE | - cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | + portallocator_flags |= cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | cricket::PORTALLOCATOR_ENABLE_SHARED_SOCKET | cricket::PORTALLOCATOR_ENABLE_IPV6; bool value; diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 1af0e1956..9d0a8e4c2 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -1571,13 +1571,6 @@ void WebRtcSession::RemoveUnusedChannelsAndTransports( // TODO(mallinath) - Add a correct error code if the channels are not creatued // due to BUNDLE is enabled but rtcp-mux is disabled. bool WebRtcSession::CreateChannels(const SessionDescription* desc) { - // Disabling the BUNDLE flag in PortAllocator if offer disabled it. - bool bundle_enabled = desc->HasGroup(cricket::GROUP_TYPE_BUNDLE); - if (state() == STATE_INIT && !bundle_enabled) { - port_allocator()->set_flags(port_allocator()->flags() & - ~cricket::PORTALLOCATOR_ENABLE_BUNDLE); - } - // Creating the media channels and transport proxies. const cricket::ContentInfo* voice = cricket::GetFirstAudioContent(desc); if (voice && !voice->rejected && !voice_channel_) { @@ -1604,6 +1597,21 @@ bool WebRtcSession::CreateChannels(const SessionDescription* desc) { } } + // Enable bundle before when kMaxBundle policy is in effect. + if (bundle_policy_ == PeerConnectionInterface::kBundlePolicyMaxBundle) { + const cricket::ContentGroup* local_bundle_group = + BaseSession::local_description()->GetGroupByName( + cricket::GROUP_TYPE_BUNDLE); + if (!local_bundle_group) { + LOG(LS_WARNING) << "max-bundle specified without BUNDLE specified"; + return false; + } + if (!BaseSession::BundleContentGroup(local_bundle_group)) { + LOG(LS_WARNING) << "max-bundle failed to enable bundling."; + return false; + } + } + return true; } diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc index 9267c4c6a..4e0066b0f 100644 --- a/talk/app/webrtc/webrtcsession_unittest.cc +++ b/talk/app/webrtc/webrtcsession_unittest.cc @@ -371,7 +371,6 @@ class WebRtcSessionTest : public testing::Test { SocketAddress(), SocketAddress(), SocketAddress())); allocator_->set_flags(cricket::PORTALLOCATOR_DISABLE_TCP | cricket::PORTALLOCATOR_DISABLE_RELAY | - cricket::PORTALLOCATOR_ENABLE_BUNDLE | cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG); EXPECT_TRUE(channel_manager_->Init()); desc_factory_->set_add_legacy_streams(false); @@ -1248,7 +1247,6 @@ class WebRtcSessionTest : public testing::Test { allocator_->AddRelay(relay_server); allocator_->set_step_delay(cricket::kMinimumStepDelay); allocator_->set_flags(cricket::PORTALLOCATOR_DISABLE_TCP | - cricket::PORTALLOCATOR_ENABLE_BUNDLE | cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG); } @@ -2597,39 +2595,97 @@ TEST_F(WebRtcSessionTest, TestSetRemoteDescriptionInvalidIceCredentials) { EXPECT_FALSE(session_->SetRemoteDescription(modified_offer, &error)); } -TEST_F(WebRtcSessionTest, VerifyBundleFlagInPA) { - // This test verifies BUNDLE flag in PortAllocator, if BUNDLE information in - // local description is removed by the application, BUNDLE flag should be - // disabled in PortAllocator. By default BUNDLE is enabled in the WebRtc. - Init(); - EXPECT_TRUE((cricket::PORTALLOCATOR_ENABLE_BUNDLE & - allocator_->flags()) == cricket::PORTALLOCATOR_ENABLE_BUNDLE); - rtc::scoped_ptr offer(CreateOffer()); - - cricket::SessionDescription* offer_copy = - offer->description()->Copy(); - offer_copy->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); - JsepSessionDescription* modified_offer = - new JsepSessionDescription(JsepSessionDescription::kOffer); - modified_offer->Initialize(offer_copy, "1", "1"); - - SetLocalDescriptionWithoutError(modified_offer); - EXPECT_FALSE(allocator_->flags() & cricket::PORTALLOCATOR_ENABLE_BUNDLE); -} - -TEST_F(WebRtcSessionTest, TestDisabledBundleInAnswer) { - Init(); +// kBundlePolicyBalanced bundle policy with and answer contains BUNDLE. +TEST_F(WebRtcSessionTest, TestBalancedBundleInAnswer) { + InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyBalanced); mediastream_signaling_.SendAudioVideoStream1(); - EXPECT_TRUE((cricket::PORTALLOCATOR_ENABLE_BUNDLE & - allocator_->flags()) == cricket::PORTALLOCATOR_ENABLE_BUNDLE); PeerConnectionInterface::RTCOfferAnswerOptions options; options.use_rtp_mux = true; SessionDescriptionInterface* offer = CreateOffer(options); - SetLocalDescriptionWithoutError(offer); + + EXPECT_NE(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); + mediastream_signaling_.SendAudioVideoStream2(); + SessionDescriptionInterface* answer = + CreateRemoteAnswer(session_->local_description()); + SetRemoteDescriptionWithoutError(answer); + + EXPECT_EQ(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); +} + +// kBundlePolicyBalanced bundle policy but no BUNDLE in the answer. +TEST_F(WebRtcSessionTest, TestBalancedNoBundleInAnswer) { + InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyBalanced); + mediastream_signaling_.SendAudioVideoStream1(); + PeerConnectionInterface::RTCOfferAnswerOptions options; + options.use_rtp_mux = true; + + SessionDescriptionInterface* offer = CreateOffer(options); + SetLocalDescriptionWithoutError(offer); + + EXPECT_NE(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); + + mediastream_signaling_.SendAudioVideoStream2(); + + // Remove BUNDLE from the answer. + rtc::scoped_ptr answer( + CreateRemoteAnswer(session_->local_description())); + cricket::SessionDescription* answer_copy = answer->description()->Copy(); + answer_copy->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + JsepSessionDescription* modified_answer = + new JsepSessionDescription(JsepSessionDescription::kAnswer); + modified_answer->Initialize(answer_copy, "1", "1"); + SetRemoteDescriptionWithoutError(modified_answer); // + + EXPECT_NE(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); +} + +// kBundlePolicyMaxBundle policy with BUNDLE in the answer. +TEST_F(WebRtcSessionTest, TestMaxBundleBundleInAnswer) { + InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyMaxBundle); + mediastream_signaling_.SendAudioVideoStream1(); + + PeerConnectionInterface::RTCOfferAnswerOptions options; + options.use_rtp_mux = true; + + SessionDescriptionInterface* offer = CreateOffer(options); + SetLocalDescriptionWithoutError(offer); + + EXPECT_EQ(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); + + mediastream_signaling_.SendAudioVideoStream2(); + SessionDescriptionInterface* answer = + CreateRemoteAnswer(session_->local_description()); + SetRemoteDescriptionWithoutError(answer); + + EXPECT_EQ(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); +} + +// kBundlePolicyMaxBundle policy but no BUNDLE in the answer. +TEST_F(WebRtcSessionTest, TestMaxBundleNoBundleInAnswer) { + InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyMaxBundle); + mediastream_signaling_.SendAudioVideoStream1(); + PeerConnectionInterface::RTCOfferAnswerOptions options; + options.use_rtp_mux = true; + + SessionDescriptionInterface* offer = CreateOffer(options); + SetLocalDescriptionWithoutError(offer); + + EXPECT_EQ(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); + + mediastream_signaling_.SendAudioVideoStream2(); + + // Remove BUNDLE from the answer. rtc::scoped_ptr answer( CreateRemoteAnswer(session_->local_description())); cricket::SessionDescription* answer_copy = answer->description()->Copy(); @@ -2638,22 +2694,63 @@ TEST_F(WebRtcSessionTest, TestDisabledBundleInAnswer) { new JsepSessionDescription(JsepSessionDescription::kAnswer); modified_answer->Initialize(answer_copy, "1", "1"); SetRemoteDescriptionWithoutError(modified_answer); - EXPECT_TRUE((cricket::PORTALLOCATOR_ENABLE_BUNDLE & - allocator_->flags()) == cricket::PORTALLOCATOR_ENABLE_BUNDLE); - video_channel_ = media_engine_->GetVideoChannel(0); - voice_channel_ = media_engine_->GetVoiceChannel(0); + EXPECT_EQ(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); +} - ASSERT_EQ(1u, video_channel_->recv_streams().size()); - EXPECT_TRUE(kVideoTrack2 == video_channel_->recv_streams()[0].id); +// kBundlePolicyMaxCompat bundle policy with and answer contains BUNDLE. +TEST_F(WebRtcSessionTest, TestMaxCompatBundleInAnswer) { + InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyMaxCompat); + mediastream_signaling_.SendAudioVideoStream1(); - ASSERT_EQ(1u, voice_channel_->recv_streams().size()); - EXPECT_TRUE(kAudioTrack2 == voice_channel_->recv_streams()[0].id); + PeerConnectionInterface::RTCOfferAnswerOptions options; + options.use_rtp_mux = true; - ASSERT_EQ(1u, video_channel_->send_streams().size()); - EXPECT_TRUE(kVideoTrack1 == video_channel_->send_streams()[0].id); - ASSERT_EQ(1u, voice_channel_->send_streams().size()); - EXPECT_TRUE(kAudioTrack1 == voice_channel_->send_streams()[0].id); + SessionDescriptionInterface* offer = CreateOffer(options); + SetLocalDescriptionWithoutError(offer); + + EXPECT_NE(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); + + mediastream_signaling_.SendAudioVideoStream2(); + SessionDescriptionInterface* answer = + CreateRemoteAnswer(session_->local_description()); + SetRemoteDescriptionWithoutError(answer); + + // This should lead to an audio-only call but isn't implemented + // correctly yet. + EXPECT_EQ(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); +} + +// kBundlePolicyMaxCompat bundle policy but no BUNDLE in the answer. +TEST_F(WebRtcSessionTest, TestMaxCompatNoBundleInAnswer) { + InitWithBundlePolicy(PeerConnectionInterface::kBundlePolicyMaxCompat); + mediastream_signaling_.SendAudioVideoStream1(); + PeerConnectionInterface::RTCOfferAnswerOptions options; + options.use_rtp_mux = true; + + SessionDescriptionInterface* offer = CreateOffer(options); + SetLocalDescriptionWithoutError(offer); + + EXPECT_NE(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); + + mediastream_signaling_.SendAudioVideoStream2(); + + // Remove BUNDLE from the answer. + rtc::scoped_ptr answer( + CreateRemoteAnswer(session_->local_description())); + cricket::SessionDescription* answer_copy = answer->description()->Copy(); + answer_copy->RemoveGroupByName(cricket::GROUP_TYPE_BUNDLE); + JsepSessionDescription* modified_answer = + new JsepSessionDescription(JsepSessionDescription::kAnswer); + modified_answer->Initialize(answer_copy, "1", "1"); + SetRemoteDescriptionWithoutError(modified_answer); // + + EXPECT_NE(session_->GetTransportProxy("audio")->impl(), + session_->GetTransportProxy("video")->impl()); } // This test verifies that SetLocalDescription and SetRemoteDescription fails @@ -2661,8 +2758,6 @@ TEST_F(WebRtcSessionTest, TestDisabledBundleInAnswer) { TEST_F(WebRtcSessionTest, TestDisabledRtcpMuxWithBundleEnabled) { Init(); mediastream_signaling_.SendAudioVideoStream1(); - EXPECT_TRUE((cricket::PORTALLOCATOR_ENABLE_BUNDLE & - allocator_->flags()) == cricket::PORTALLOCATOR_ENABLE_BUNDLE); PeerConnectionInterface::RTCOfferAnswerOptions options; options.use_rtp_mux = true; @@ -3153,7 +3248,6 @@ TEST_F(WebRtcSessionTest, TestIceStatesBasicIPv6) { // Runs the loopback call test with BUNDLE and STUN enabled. TEST_F(WebRtcSessionTest, TestIceStatesBundle) { allocator_->set_flags(cricket::PORTALLOCATOR_ENABLE_SHARED_UFRAG | - cricket::PORTALLOCATOR_ENABLE_BUNDLE | cricket::PORTALLOCATOR_DISABLE_TCP | cricket::PORTALLOCATOR_DISABLE_RELAY); TestLoopbackCall(); diff --git a/webrtc/p2p/base/session.cc b/webrtc/p2p/base/session.cc index 4da65192e..136f3919a 100644 --- a/webrtc/p2p/base/session.cc +++ b/webrtc/p2p/base/session.cc @@ -537,23 +537,6 @@ TransportProxy* BaseSession::GetTransportProxy( return (iter != transports_.end()) ? iter->second : NULL; } -TransportProxy* BaseSession::GetTransportProxy(const Transport* transport) { - for (TransportMap::iterator iter = transports_.begin(); - iter != transports_.end(); ++iter) { - TransportProxy* transproxy = iter->second; - if (transproxy->impl() == transport) { - return transproxy; - } - } - return NULL; -} - -TransportProxy* BaseSession::GetFirstTransportProxy() { - if (transports_.empty()) - return NULL; - return transports_.begin()->second; -} - void BaseSession::DestroyTransportProxy( const std::string& content_name) { TransportMap::iterator iter = transports_.find(content_name); @@ -643,8 +626,9 @@ bool BaseSession::MaybeEnableMuxingSupport() { for (TransportMap::iterator iter = transports_.begin(); iter != transports_.end(); ++iter) { ASSERT(iter->second->negotiated()); - if (!iter->second->negotiated()) + if (!iter->second->negotiated()) { return false; + } } // If both sides agree to BUNDLE, mux all the specified contents onto the @@ -654,56 +638,63 @@ bool BaseSession::MaybeEnableMuxingSupport() { // BUNDLE the same way? bool candidates_allocated = IsCandidateAllocationDone(); const ContentGroup* local_bundle_group = - local_description()->GetGroupByName(GROUP_TYPE_BUNDLE); + local_description_->GetGroupByName(GROUP_TYPE_BUNDLE); const ContentGroup* remote_bundle_group = - remote_description()->GetGroupByName(GROUP_TYPE_BUNDLE); - if (local_bundle_group && remote_bundle_group && - local_bundle_group->FirstContentName()) { - const std::string* content_name = local_bundle_group->FirstContentName(); - const ContentInfo* content = - local_description_->GetContentByName(*content_name); - if (!content) { - LOG(LS_WARNING) << "Content \"" << *content_name - << "\" referenced in BUNDLE group is not present"; - return false; - } - - if (!SetSelectedProxy(content->name, local_bundle_group)) { + remote_description_->GetGroupByName(GROUP_TYPE_BUNDLE); + if (local_bundle_group && remote_bundle_group) { + if (!BundleContentGroup(local_bundle_group)) { LOG(LS_WARNING) << "Failed to set up BUNDLE"; return false; } // If we weren't done gathering before, we might be done now, as a result // of enabling mux. - LOG(LS_INFO) << "Enabling BUNDLE, bundling onto transport: " - << *content_name; if (!candidates_allocated) { MaybeCandidateAllocationDone(); } } else { - LOG(LS_INFO) << "No BUNDLE information, not bundling."; + LOG(LS_INFO) << "BUNDLE group missing from remote or local description."; } return true; } -bool BaseSession::SetSelectedProxy(const std::string& content_name, - const ContentGroup* muxed_group) { - TransportProxy* selected_proxy = GetTransportProxy(content_name); - if (!selected_proxy) { +bool BaseSession::BundleContentGroup(const ContentGroup* bundle_group) { + const std::string* content_name = bundle_group->FirstContentName(); + if (!content_name) { + LOG(LS_INFO) << "No content names specified in BUNDLE group."; + return true; + } + + const ContentInfo* content = + local_description_->GetContentByName(*content_name); + if (!content) { + LOG(LS_WARNING) << "Content \"" << *content_name + << "\" referenced in BUNDLE group" + << " not present in local description"; + return false; + } + + TransportProxy* selected_proxy = GetTransportProxy(*content_name); + if (!selected_proxy) { + LOG(LS_WARNING) << "No transport found for content \"" + << *content_name << "\"."; return false; } - ASSERT(selected_proxy->negotiated()); for (TransportMap::iterator iter = transports_.begin(); iter != transports_.end(); ++iter) { // If content is part of the mux group, then repoint its proxy at the // transport object that we have chosen to mux onto. If the proxy // is already pointing at the right object, it will be a no-op. - if (muxed_group->HasContentName(iter->first) && + if (bundle_group->HasContentName(iter->first) && !iter->second->SetupMux(selected_proxy)) { + LOG(LS_WARNING) << "Failed to bundle " << iter->first << " to " + << *content_name; return false; } + LOG(LS_INFO) << "Bundling " << iter->first << " to " << *content_name; } + return true; } diff --git a/webrtc/p2p/base/session.h b/webrtc/p2p/base/session.h index cdee8014c..ba4206aee 100644 --- a/webrtc/p2p/base/session.h +++ b/webrtc/p2p/base/session.h @@ -331,8 +331,6 @@ class BaseSession : public sigslot::has_slots<>, const TransportMap& transport_proxies() const { return transports_; } // Get a TransportProxy by content_name or transport. NULL if not found. TransportProxy* GetTransportProxy(const std::string& content_name); - TransportProxy* GetTransportProxy(const Transport* transport); - TransportProxy* GetFirstTransportProxy(); void DestroyTransportProxy(const std::string& content_name); // TransportProxy is owned by session. Return proxy just for convenience. TransportProxy* GetOrCreateTransportProxy(const std::string& content_name); @@ -411,6 +409,10 @@ class BaseSession : public sigslot::has_slots<>, // Fires the new description signal according to the current state. virtual void SignalNewDescription(); + // This method will delete the Transport and TransportChannelImpls + // and replace those with the Transport object of the first + // MediaContent in bundle_group. + bool BundleContentGroup(const ContentGroup* bundle_group); private: // Helper methods to push local and remote transport descriptions. @@ -423,12 +425,6 @@ class BaseSession : public sigslot::has_slots<>, void MaybeCandidateAllocationDone(); - // This method will delete the Transport and TransportChannelImpls and - // replace those with the selected Transport objects. Selection is done - // based on the content_name and in this case first MediaContent information - // is used for mux. - bool SetSelectedProxy(const std::string& content_name, - const ContentGroup* muxed_group); // Log session state. void LogState(State old_state, State new_state);