From d85ea75cbd3a57197075d3676f8ccd34d5546c63 Mon Sep 17 00:00:00 2001 From: Eldar Rello Date: Wed, 19 Feb 2020 20:41:07 +0200 Subject: [PATCH] Rollback transport created by data channel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No-Try: True Bug: chromium:1032987 Change-Id: I2c0dbd6a19e71a391dc2e0d30676d4efa26a9525 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168306 Commit-Queue: Steve Anton Reviewed-by: Harald Alvestrand Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#30561} --- pc/jsep_transport_controller.cc | 20 +++++---- pc/jsep_transport_controller.h | 7 +-- pc/peer_connection.cc | 5 +-- pc/peer_connection_jsep_unittest.cc | 69 +++++++++++++++++++++++++++++ 4 files changed, 86 insertions(+), 15 deletions(-) diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 8d4eee0ff7..0687a067f7 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -443,19 +443,19 @@ void JsepTransportController::SetMediaTransportSettings( use_datagram_transport_for_data_channels_receive_only; } -void JsepTransportController::RollbackTransportForMids( - const std::vector& mids) { +void JsepTransportController::RollbackTransports() { if (!network_thread_->IsCurrent()) { - network_thread_->Invoke(RTC_FROM_HERE, - [=] { RollbackTransportForMids(mids); }); + network_thread_->Invoke(RTC_FROM_HERE, [=] { RollbackTransports(); }); return; } - for (auto&& mid : mids) { + RTC_DCHECK_RUN_ON(network_thread_); + for (auto&& mid : pending_mids_) { RemoveTransportForMid(mid); } - for (auto&& mid : mids) { + for (auto&& mid : pending_mids_) { MaybeDestroyJsepTransport(mid); } + pending_mids_.clear(); } rtc::scoped_refptr @@ -605,7 +605,7 @@ RTCError JsepTransportController::ApplyDescription_n( bool local, SdpType type, const cricket::SessionDescription* description) { - RTC_DCHECK(network_thread_->IsCurrent()); + RTC_DCHECK_RUN_ON(network_thread_); RTC_DCHECK(description); if (local) { @@ -718,6 +718,9 @@ RTCError JsepTransportController::ApplyDescription_n( content_info.name + ": " + error.message()); } } + if (type == SdpType::kAnswer) { + pending_mids_.clear(); + } return RTCError::OK(); } @@ -874,7 +877,8 @@ bool JsepTransportController::SetTransportForMid( if (mid_to_transport_[mid] == jsep_transport) { return true; } - + RTC_DCHECK_RUN_ON(network_thread_); + pending_mids_.push_back(mid); mid_to_transport_[mid] = jsep_transport; return config_.transport_observer->OnTransportChanged( mid, jsep_transport->rtp_transport(), jsep_transport->RtpDtlsTransport(), diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index 9c3f691302..c966e744c6 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -224,9 +224,9 @@ class JsepTransportController : public sigslot::has_slots<> { bool use_datagram_transport_for_data_channels, bool use_datagram_transport_for_data_channels_receive_only); - // TODO(elrello): For now the rollback only removes mid to transport mappings + // For now the rollback only removes mid to transport mappings // and deletes unused transports, but doesn't consider anything more complex. - void RollbackTransportForMids(const std::vector& mids); + void RollbackTransports(); // Gets the transport parameters for the transport identified by |mid|. // If |mid| is bundled, returns the parameters for the bundled transport. @@ -430,7 +430,8 @@ class JsepTransportController : public sigslot::has_slots<> { // This keeps track of the mapping between media section // (BaseChannel/SctpTransport) and the JsepTransport underneath. std::map mid_to_transport_; - + // Keep track of mids that have been mapped to transports. Used for rollback. + std::vector pending_mids_ RTC_GUARDED_BY(network_thread_); // Aggregate states for Transports. // standardized_ice_connection_state_ is intended to replace // ice_connection_state, see bugs.webrtc.org/9308 diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index b9e9d29937..6678552a42 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -7536,7 +7536,6 @@ RTCError PeerConnection::Rollback(SdpType sdp_type) { } RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(IsUnifiedPlan()); - std::vector mids; std::vector> all_added_streams; std::vector> all_removed_streams; std::vector> removed_receivers; @@ -7563,8 +7562,6 @@ RTCError PeerConnection::Rollback(SdpType sdp_type) { } RTC_DCHECK(transceiver->internal()->mid().has_value()); - std::string mid = transceiver->internal()->mid().value(); - mids.push_back(mid); DestroyTransceiverChannel(transceiver); if (signaling_state() == PeerConnectionInterface::kHaveRemoteOffer && @@ -7589,7 +7586,7 @@ RTCError PeerConnection::Rollback(SdpType sdp_type) { transceiver->internal()->set_mid(state.mid()); transceiver->internal()->set_mline_index(state.mline_index()); } - transport_controller_->RollbackTransportForMids(mids); + transport_controller_->RollbackTransports(); transceiver_stable_states_by_transceivers_.clear(); pending_local_description_.reset(); pending_remote_description_.reset(); diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index 3186e8f39b..2a3c4c60cd 100644 --- a/pc/peer_connection_jsep_unittest.cc +++ b/pc/peer_connection_jsep_unittest.cc @@ -2129,4 +2129,73 @@ TEST_F(PeerConnectionJsepTest, RollbackMultipleStreamChanges) { "id_1"); } +TEST_F(PeerConnectionJsepTest, DataChannelImplicitRollback) { + RTCConfiguration config; + config.sdp_semantics = SdpSemantics::kUnifiedPlan; + config.enable_implicit_rollback = true; + auto caller = CreatePeerConnection(config); + caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + auto callee = CreatePeerConnection(config); + callee->CreateDataChannel("dummy"); + EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal()); + EXPECT_TRUE(callee->observer()->negotiation_needed()); + EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); +} + +TEST_F(PeerConnectionJsepTest, RollbackRemoteDataChannelThenAddTransceiver) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + caller->CreateDataChannel("dummy"); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback())); + callee->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); +} + +TEST_F(PeerConnectionJsepTest, + RollbackRemoteDataChannelThenAddTransceiverAndDataChannel) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + caller->CreateDataChannel("dummy"); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback())); + callee->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + callee->CreateDataChannel("dummy"); + EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); +} + +TEST_F(PeerConnectionJsepTest, RollbackRemoteDataChannelThenAddDataChannel) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + caller->CreateDataChannel("dummy"); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback())); + callee->CreateDataChannel("dummy"); + EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); +} + +TEST_F(PeerConnectionJsepTest, RollbackRemoteTransceiverThenAddDataChannel) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback())); + callee->CreateDataChannel("dummy"); + EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); +} + +TEST_F(PeerConnectionJsepTest, + RollbackRemoteTransceiverThenAddDataChannelAndTransceiver) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback())); + callee->CreateDataChannel("dummy"); + callee->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + EXPECT_TRUE(callee->CreateOfferAndSetAsLocal()); +} + } // namespace webrtc