From 353a718dfdc8295fb6175284b5e521d54dd29b82 Mon Sep 17 00:00:00 2001 From: Eldar Rello Date: Mon, 25 Nov 2019 18:49:44 +0200 Subject: [PATCH] Address failing wpt test cases for the rollback feature MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Also fix https://crbug.com/1025542. Bug: chromium:1025557, chromium:1025542 Change-Id: I614ca6282f1f1d4d1e2cd507c0efd6bc6a898408 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/159932 Commit-Queue: Eldar Rello Reviewed-by: Harald Alvestrand Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#29909} --- pc/jsep_transport_controller.cc | 13 +++-- pc/jsep_transport_controller.h | 6 +- pc/peer_connection.cc | 89 +++++++++++++++++++++++------ pc/peer_connection.h | 27 +++++---- pc/peer_connection_jsep_unittest.cc | 66 +++++++++++++++++++++ pc/rtp_transceiver.h | 9 +++ 6 files changed, 174 insertions(+), 36 deletions(-) diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 41907c8f93..590aa6b10c 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -473,14 +473,19 @@ void JsepTransportController::SetMediaTransportSettings( use_datagram_transport_for_data_channels_receive_only; } -void JsepTransportController::RollbackTransportForMid(const std::string& mid) { +void JsepTransportController::RollbackTransportForMids( + const std::vector& mids) { if (!network_thread_->IsCurrent()) { network_thread_->Invoke(RTC_FROM_HERE, - [=] { RollbackTransportForMid(mid); }); + [=] { RollbackTransportForMids(mids); }); return; } - RemoveTransportForMid(mid); - MaybeDestroyJsepTransport(mid); + for (auto&& mid : mids) { + RemoveTransportForMid(mid); + } + for (auto&& mid : mids) { + MaybeDestroyJsepTransport(mid); + } } rtc::scoped_refptr diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index b07783c894..b7121e78dc 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -241,9 +241,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 mapping - // and deletes unused transport, but doesn't consider anything more complex. - void RollbackTransportForMid(const std::string& mid); + // TODO(elrello): 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); // If media transport is present enabled and supported, // when this method is called, it creates a media transport and generates its diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 9fac7485ca..f5f51c43f9 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -985,6 +985,28 @@ bool PeerConnectionInterface::RTCConfiguration::operator!=( return !(*this == o); } +void PeerConnection::TransceiverStableState::set_newly_created() { + RTC_DCHECK(!has_m_section_); + newly_created_ = true; +} + +void PeerConnection::TransceiverStableState::SetMSectionIfUnset( + absl::optional mid, + absl::optional mline_index) { + if (!has_m_section_) { + mid_ = mid; + mline_index_ = mline_index; + has_m_section_ = true; + } +} + +void PeerConnection::TransceiverStableState::SetRemoteStreamIdsIfUnset( + const std::vector& ids) { + if (!remote_stream_ids_.has_value()) { + remote_stream_ids_ = ids; + } +} + // Generate a RTCP CNAME when a PeerConnection is created. std::string GenerateRtcpCname() { std::string cname; @@ -1619,6 +1641,7 @@ PeerConnection::AddTrackUnifiedPlan( } transceiver->sender()->SetTrack(track); transceiver->internal()->sender_internal()->set_stream_ids(stream_ids); + transceiver->internal()->set_reused_for_addtrack(true); } else { cricket::MediaType media_type = (track->kind() == MediaStreamTrackInterface::kAudioKind @@ -3242,6 +3265,9 @@ RTCError PeerConnection::ApplyRemoteDescription( // The remote description has signaled the stream IDs. stream_ids = media_desc->streams()[0].stream_ids(); } + transceiver_stable_states_by_transceivers_[transceiver] + .SetRemoteStreamIdsIfUnset(transceiver->receiver()->stream_ids()); + RTC_LOG(LS_INFO) << "Processing the MSIDs for MID=" << content->name << " (" << GetStreamIdsString(stream_ids) << ")."; SetAssociatedRemoteStreams(transceiver->internal()->receiver_internal(), @@ -3778,9 +3804,8 @@ PeerConnection::AssociateTransceiver(cricket::ContentSource source, transceiver->internal()->set_direction( RtpTransceiverDirection::kRecvOnly); if (type == SdpType::kOffer) { - transceiver_stable_states_by_transceivers_[transceiver] = - TransceiverStableState(RtpTransceiverDirection::kRecvOnly, - absl::nullopt, absl::nullopt, true); + transceiver_stable_states_by_transceivers_[transceiver] + .set_newly_created(); } } // Check if the offer indicated simulcast but the answer rejected it. @@ -3816,19 +3841,14 @@ PeerConnection::AssociateTransceiver(cricket::ContentSource source, } } if (type == SdpType::kOffer) { - // Make sure we don't overwrite existing stable states and that the - // state is really going to change when adding new record to the map. - auto it = transceiver_stable_states_by_transceivers_.find(transceiver); - if (it == transceiver_stable_states_by_transceivers_.end() && - (transceiver->internal()->mid() != content.name || - transceiver->internal()->mline_index() != mline_index)) { - transceiver_stable_states_by_transceivers_[transceiver] = - TransceiverStableState(transceiver->internal()->direction(), - transceiver->internal()->mid(), - transceiver->internal()->mline_index(), false); + bool state_changes = transceiver->internal()->mid() != content.name || + transceiver->internal()->mline_index() != mline_index; + if (state_changes) { + transceiver_stable_states_by_transceivers_[transceiver] + .SetMSectionIfUnset(transceiver->internal()->mid(), + transceiver->internal()->mline_index()); } } - // Associate the found or created RtpTransceiver with the m= section by // setting the value of the RtpTransceiver's mid property to the MID of the m= // section, and establish a mapping between the transceiver and the index of @@ -8017,23 +8037,43 @@ 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; for (auto&& transceivers_stable_state_pair : transceiver_stable_states_by_transceivers_) { auto transceiver = transceivers_stable_state_pair.first; auto state = transceivers_stable_state_pair.second; + + if (state.remote_stream_ids()) { + std::vector> added_streams; + std::vector> removed_streams; + SetAssociatedRemoteStreams(transceiver->internal()->receiver_internal(), + state.remote_stream_ids().value(), + &added_streams, &removed_streams); + all_added_streams.insert(all_added_streams.end(), added_streams.begin(), + added_streams.end()); + all_removed_streams.insert(all_removed_streams.end(), + removed_streams.begin(), + removed_streams.end()); + if (!state.has_m_section() && !state.newly_created()) { + continue; + } + } + RTC_DCHECK(transceiver->internal()->mid().has_value()); std::string mid = transceiver->internal()->mid().value(); - transport_controller_->RollbackTransportForMid(mid); + mids.push_back(mid); DestroyTransceiverChannel(transceiver); if (signaling_state() == PeerConnectionInterface::kHaveRemoteOffer && transceiver->receiver()) { - Observer()->OnRemoveTrack(transceiver->receiver()); + removed_receivers.push_back(transceiver->receiver()); } if (state.newly_created()) { - // Remove added transceivers with no added track. - if (transceiver->internal()->sender()->track()) { + if (transceiver->internal()->reused_for_addtrack()) { transceiver->internal()->set_created_by_addtrack(true); } else { int remaining_transceiver_count = 0; @@ -8047,15 +8087,26 @@ RTCError PeerConnection::Rollback(SdpType sdp_type) { } transceiver->internal()->sender_internal()->set_transport(nullptr); transceiver->internal()->receiver_internal()->set_transport(nullptr); - transceiver->internal()->set_direction(state.direction()); transceiver->internal()->set_mid(state.mid()); transceiver->internal()->set_mline_index(state.mline_index()); } + transport_controller_->RollbackTransportForMids(mids); transceiver_stable_states_by_transceivers_.clear(); pending_local_description_.reset(); pending_remote_description_.reset(); ChangeSignalingState(PeerConnectionInterface::kStable); + // Once all processing has finished, fire off callbacks. + for (const auto& receiver : removed_receivers) { + Observer()->OnRemoveTrack(receiver); + } + for (const auto& stream : all_added_streams) { + Observer()->OnAddStream(stream); + } + for (const auto& stream : all_removed_streams) { + Observer()->OnRemoveStream(stream); + } + // The assumption is that in case of implicit rollback UpdateNegotiationNeeded // gets called in SetRemoteDescription. if (sdp_type == SdpType::kRollback) { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 19af912506..3126348788 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -403,23 +403,26 @@ class PeerConnection : public PeerConnectionInternal, class TransceiverStableState { public: TransceiverStableState() {} - TransceiverStableState(RtpTransceiverDirection direction, - absl::optional mid, - absl::optional mline_index, - bool newly_created) - : direction_(direction), - mid_(mid), - mline_index_(mline_index), - newly_created_(newly_created) {} - RtpTransceiverDirection direction() const { return direction_; } + void set_newly_created(); + void SetMSectionIfUnset(absl::optional mid, + absl::optional mline_index); + void SetRemoteStreamIdsIfUnset(const std::vector& ids); absl::optional mid() const { return mid_; } absl::optional mline_index() const { return mline_index_; } + absl::optional> remote_stream_ids() const { + return remote_stream_ids_; + } + bool has_m_section() const { return has_m_section_; } bool newly_created() const { return newly_created_; } private: - RtpTransceiverDirection direction_ = RtpTransceiverDirection::kRecvOnly; absl::optional mid_; absl::optional mline_index_; + absl::optional> remote_stream_ids_; + // Indicates that mid value from stable state has been captured and + // that rollback has to restore the transceiver. Also protects against + // subsequent overwrites. + bool has_m_section_ = false; // Indicates that the transceiver was created as part of applying a // description to track potential need for removing transceiver during // rollback. @@ -1365,6 +1368,10 @@ class PeerConnection : public PeerConnectionInternal, std::map>, TransceiverStableState> transceiver_stable_states_by_transceivers_; + // Holds remote stream ids for transceivers from stable state. + std::map>, + std::vector> + remote_stream_ids_by_transceivers_; std::vector< rtc::scoped_refptr>> transceivers_; // TODO(bugs.webrtc.org/9987): Accessed on both signaling diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index bb1039ca11..3186e8f39b 100644 --- a/pc/peer_connection_jsep_unittest.cc +++ b/pc/peer_connection_jsep_unittest.cc @@ -1879,6 +1879,27 @@ TEST_F(PeerConnectionJsepTest, RollbackKeepsTransceiverAndClearsMid) { EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u); } +TEST_F(PeerConnectionJsepTest, + RollbackKeepsTransceiverAfterAddTrackEvenWhenTrackIsNulled) { + auto caller = CreatePeerConnection(); + caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + auto callee = CreatePeerConnection(); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + callee->AddAudioTrack("a"); + callee->pc()->GetTransceivers()[0]->sender()->SetTrack(nullptr); + EXPECT_EQ(callee->pc()->GetTransceivers()[0]->sender()->track(), nullptr); + EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateRollback())); + // Transceiver can't be removed as track was added to it. + EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u); + // Mid got cleared to make it reusable. + EXPECT_EQ(callee->pc()->GetTransceivers()[0]->mid(), absl::nullopt); + // Transceiver should be counted as addTrack-created after rollback. + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + EXPECT_EQ(callee->pc()->GetTransceivers().size(), 1u); + EXPECT_EQ(callee->observer()->remove_track_events_.size(), 1u); +} + TEST_F(PeerConnectionJsepTest, RollbackRestoresMid) { auto caller = CreatePeerConnection(); caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); @@ -1986,6 +2007,30 @@ TEST_F(PeerConnectionJsepTest, RollbackToNegotiatedStableState) { audio_transport); // Audio transport is still the same. } +TEST_F(PeerConnectionJsepTest, RollbackHasToDestroyTransport) { + RTCConfiguration config; + config.sdp_semantics = SdpSemantics::kUnifiedPlan; + config.bundle_policy = PeerConnectionInterface::kBundlePolicyMaxBundle; + auto pc = CreatePeerConnection(config); + pc->AddAudioTrack("a"); + pc->AddVideoTrack("b"); + EXPECT_TRUE(pc->CreateOfferAndSetAsLocal()); + auto offer = pc->CreateOffer(); + EXPECT_EQ(pc->pc()->GetTransceivers().size(), 2u); + auto audio_transport = + pc->pc()->GetTransceivers()[0]->sender()->dtls_transport(); + EXPECT_EQ(pc->pc()->GetTransceivers()[0]->sender()->dtls_transport(), + pc->pc()->GetTransceivers()[1]->sender()->dtls_transport()); + EXPECT_NE(pc->pc()->GetTransceivers()[1]->sender()->dtls_transport(), + nullptr); + EXPECT_TRUE(pc->SetRemoteDescription(pc->CreateRollback())); + EXPECT_TRUE(pc->SetLocalDescription(std::move(offer))); + EXPECT_NE(pc->pc()->GetTransceivers()[0]->sender()->dtls_transport(), + nullptr); + EXPECT_NE(pc->pc()->GetTransceivers()[0]->sender()->dtls_transport(), + audio_transport); +} + TEST_F(PeerConnectionJsepTest, RollbackLocalDirectionChange) { auto caller = CreatePeerConnection(); caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); @@ -2063,4 +2108,25 @@ TEST_F(PeerConnectionJsepTest, NoRollbackNeeded) { EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); } +TEST_F(PeerConnectionJsepTest, RollbackMultipleStreamChanges) { + auto callee = CreatePeerConnection(); + auto caller = CreatePeerConnection(); + caller->AddAudioTrack("a_1", {"id_1"}); + caller->AddVideoTrack("v_0", {"id_0"}); // Provide an extra stream id. + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + EXPECT_TRUE( + caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); + caller->pc()->GetTransceivers()[0]->sender()->SetStreams({"id_2"}); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + caller->pc()->GetTransceivers()[0]->sender()->SetStreams({"id_3"}); + EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + EXPECT_EQ(callee->pc()->GetTransceivers()[0]->receiver()->stream_ids()[0], + "id_3"); + EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback())); + EXPECT_EQ(callee->pc()->GetTransceivers()[0]->receiver()->stream_ids().size(), + 1u); + EXPECT_EQ(callee->pc()->GetTransceivers()[0]->receiver()->stream_ids()[0], + "id_1"); +} + } // namespace webrtc diff --git a/pc/rtp_transceiver.h b/pc/rtp_transceiver.h index 990c3cc09a..7ab9e9849a 100644 --- a/pc/rtp_transceiver.h +++ b/pc/rtp_transceiver.h @@ -154,8 +154,16 @@ class RtpTransceiver final void set_created_by_addtrack(bool created_by_addtrack) { created_by_addtrack_ = created_by_addtrack; } + // If AddTrack has been called then transceiver can't be removed during + // rollback. + void set_reused_for_addtrack(bool reused_for_addtrack) { + reused_for_addtrack_ = reused_for_addtrack; + } + bool created_by_addtrack() const { return created_by_addtrack_; } + bool reused_for_addtrack() const { return reused_for_addtrack_; } + // Returns true if this transceiver has ever had the current direction set to // sendonly or sendrecv. bool has_ever_been_used_to_send() const { @@ -201,6 +209,7 @@ class RtpTransceiver final absl::optional mid_; absl::optional mline_index_; bool created_by_addtrack_ = false; + bool reused_for_addtrack_ = false; bool has_ever_been_used_to_send_ = false; cricket::ChannelInterface* channel_ = nullptr;