From 936f1af3bb830262b47a8284db21023ad16a13ad Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Tue, 22 Sep 2020 07:41:50 +0000 Subject: [PATCH] Reland "Remove stopped transceivers at both local and remote SetDescription" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of 6f4de80ddddcc05beaced31146ffb753258bc7be The blocking issue in Chromium is fixed. Original change's description: > Remove stopped transceivers at both local and remote SetDescription > > This should ensure that the correct number of senders and receivers > are shown. > > Bug: webtc:11840 > Change-Id: Id57f8f9b1ceb8900abb3f92bcae79e5f0341de15 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/184606 > Commit-Queue: Harald Alvestrand > Reviewed-by: Henrik Boström > Cr-Commit-Position: refs/heads/master@{#32158} Bug: webtc:11840 Change-Id: Iae8ca01e3f834694dacb36320858096b26f0996b Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/185120 Commit-Queue: Harald Alvestrand Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#32181} --- pc/peer_connection.cc | 93 ++++++++++++------------ pc/peer_connection.h | 3 + pc/peer_connection_interface_unittest.cc | 4 +- pc/peer_connection_jsep_unittest.cc | 15 +++- pc/peer_connection_rtp_unittest.cc | 18 +++++ 5 files changed, 82 insertions(+), 51 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index ecb6601116..3b5c333a98 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -1987,13 +1987,7 @@ PeerConnection::GetTransceivers() const { << "GetTransceivers is only supported with Unified Plan SdpSemantics."; std::vector> all_transceivers; for (const auto& transceiver : transceivers_) { - // Temporary fix: Do not show stopped transceivers. - // The long term fix is to remove them from transceivers_, but this - // turns out to cause issues with audio channel lifetimes. - // TODO(https://crbug.com/webrtc/11840): Fix issue. - if (!transceiver->stopped()) { - all_transceivers.push_back(transceiver); - } + all_transceivers.push_back(transceiver); } return all_transceivers; } @@ -2533,6 +2527,47 @@ void PeerConnection::SetLocalDescription( }); } +void PeerConnection::RemoveStoppedTransceivers() { + // 3.2.10.1: For each transceiver in the connection's set of transceivers + // run the following steps: + if (!IsUnifiedPlan()) + return; + for (auto it = transceivers_.begin(); it != transceivers_.end();) { + const auto& transceiver = *it; + // 3.2.10.1.1: If transceiver is stopped, associated with an m= section + // and the associated m= section is rejected in + // connection.[[CurrentLocalDescription]] or + // connection.[[CurrentRemoteDescription]], remove the + // transceiver from the connection's set of transceivers. + if (!transceiver->stopped()) { + ++it; + continue; + } + const ContentInfo* local_content = + FindMediaSectionForTransceiver(transceiver, local_description()); + const ContentInfo* remote_content = + FindMediaSectionForTransceiver(transceiver, remote_description()); + if ((local_content && local_content->rejected) || + (remote_content && remote_content->rejected)) { + RTC_LOG(LS_INFO) << "Dissociating transceiver" + << " since the media section is being recycled."; + (*it)->internal()->set_mid(absl::nullopt); + (*it)->internal()->set_mline_index(absl::nullopt); + it = transceivers_.erase(it); + continue; + } + if (!local_content && !remote_content) { + // TODO(bugs.webrtc.org/11973): Consider if this should be removed already + // See https://github.com/w3c/webrtc-pc/issues/2576 + RTC_LOG(LS_INFO) + << "Dropping stopped transceiver that was never associated"; + it = transceivers_.erase(it); + continue; + } + ++it; + } +} + void PeerConnection::DoSetLocalDescription( std::unique_ptr desc, rtc::scoped_refptr observer) { @@ -2604,34 +2639,7 @@ void PeerConnection::DoSetLocalDescription( RTC_DCHECK(local_description()); if (local_description()->GetType() == SdpType::kAnswer) { - // 3.2.10.1: For each transceiver in the connection's set of transceivers - // run the following steps: - if (IsUnifiedPlan()) { - for (auto it = transceivers_.begin(); it != transceivers_.end();) { - const auto& transceiver = *it; - // 3.2.10.1.1: If transceiver is stopped, associated with an m= section - // and the associated m= section is rejected in - // connection.[[CurrentLocalDescription]] or - // connection.[[CurrentRemoteDescription]], remove the - // transceiver from the connection's set of transceivers. - if (transceiver->stopped()) { - const ContentInfo* content = - FindMediaSectionForTransceiver(transceiver, local_description()); - - if (content && content->rejected) { - RTC_LOG(LS_INFO) << "Dissociating transceiver" - << " since the media section is being recycled."; - (*it)->internal()->set_mid(absl::nullopt); - (*it)->internal()->set_mline_index(absl::nullopt); - it = transceivers_.erase(it); - } else { - ++it; - } - } else { - ++it; - } - } - } + RemoveStoppedTransceivers(); // TODO(deadbeef): We already had to hop to the network thread for // MaybeStartGathering... @@ -3099,6 +3107,7 @@ void PeerConnection::DoSetRemoteDescription( RTC_DCHECK(remote_description()); if (type == SdpType::kAnswer) { + RemoveStoppedTransceivers(); // TODO(deadbeef): We already had to hop to the network thread for // MaybeStartGathering... network_thread()->Invoke( @@ -3764,23 +3773,17 @@ PeerConnection::AssociateTransceiver(cricket::ContentSource source, RTC_DCHECK(IsUnifiedPlan()); // If this is an offer then the m= section might be recycled. If the m= // section is being recycled (defined as: rejected in the current local or - // remote description and not rejected in new description), dissociate the - // currently associated RtpTransceiver by setting its mid property to null, - // and discard the mapping between the transceiver and its m= section index. + // remote description and not rejected in new description), the transceiver + // should have been removed by RemoveStoppedTransceivers(). if (IsMediaSectionBeingRecycled(type, content, old_local_content, old_remote_content)) { - // We want to dissociate the transceiver that has the rejected mid. const std::string& old_mid = (old_local_content && old_local_content->rejected) ? old_local_content->name : old_remote_content->name; auto old_transceiver = GetAssociatedTransceiver(old_mid); - if (old_transceiver) { - RTC_LOG(LS_INFO) << "Dissociating transceiver for MID=" << old_mid - << " since the media section is being recycled."; - old_transceiver->internal()->set_mid(absl::nullopt); - old_transceiver->internal()->set_mline_index(absl::nullopt); - } + // The transceiver should be disassociated in RemoveStoppedTransceivers() + RTC_DCHECK(!old_transceiver); } const MediaContentDescription* media_desc = content.media_description(); auto transceiver = GetAssociatedTransceiver(content.name); diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 22c0d9a8b9..1385ce13a6 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -450,6 +450,9 @@ class PeerConnection : public PeerConnectionInternal, std::unique_ptr desc, rtc::scoped_refptr observer); + // Helper function to remove stopped transceivers. + void RemoveStoppedTransceivers() RTC_RUN_ON(signaling_thread()); + void CreateAudioReceiver(MediaStreamInterface* stream, const RtpSenderInfo& remote_sender_info) RTC_RUN_ON(signaling_thread()); diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index 10928768e2..a0e3845fd6 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -2671,8 +2671,8 @@ TEST_P(PeerConnectionInterfaceTest, CloseAndTestStreamsAndStates) { EXPECT_EQ(1u, pc_->local_streams()->count()); EXPECT_EQ(1u, pc_->remote_streams()->count()); } else { - // Verify that the RtpTransceivers are no longer returned. - EXPECT_EQ(0u, pc_->GetTransceivers().size()); + // Verify that the RtpTransceivers are still returned. + EXPECT_EQ(2u, pc_->GetTransceivers().size()); } auto audio_receiver = GetFirstReceiverOfType(cricket::MEDIA_TYPE_AUDIO); diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc index ba51214ac3..c3e093617b 100644 --- a/pc/peer_connection_jsep_unittest.cc +++ b/pc/peer_connection_jsep_unittest.cc @@ -357,6 +357,10 @@ TEST_F(PeerConnectionJsepTest, SetRemoteOfferDoesNotReuseStoppedTransceiver) { ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); auto transceivers = callee->pc()->GetTransceivers(); + ASSERT_EQ(2u, transceivers.size()); + // The stopped transceiver is removed in SetLocalDescription(answer) + ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); + transceivers = callee->pc()->GetTransceivers(); ASSERT_EQ(1u, transceivers.size()); EXPECT_EQ(caller->pc()->GetTransceivers()[0]->mid(), transceivers[0]->mid()); EXPECT_FALSE(transceivers[0]->stopped()); @@ -561,6 +565,9 @@ TEST_F(PeerConnectionJsepTest, ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); auto transceivers = callee->pc()->GetTransceivers(); + EXPECT_EQ(1u, transceivers.size()); + ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); + transceivers = callee->pc()->GetTransceivers(); EXPECT_EQ(0u, transceivers.size()); } @@ -596,15 +603,15 @@ TEST_F(PeerConnectionJsepTest, auto callee = CreatePeerConnection(); ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + std::string first_mid = *first_transceiver->mid(); ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); callee->pc()->GetTransceivers()[0]->StopInternal(); - ASSERT_EQ(0u, callee->pc()->GetTransceivers().size()); + ASSERT_EQ(1u, callee->pc()->GetTransceivers().size()); ASSERT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); EXPECT_TRUE(first_transceiver->stopped()); - // First transceivers aren't dissociated yet on caller side. - ASSERT_NE(absl::nullopt, first_transceiver->mid()); - std::string first_mid = *first_transceiver->mid(); + // First transceivers are dissociated on caller side. + ASSERT_EQ(absl::nullopt, first_transceiver->mid()); // They are disassociated on callee side. ASSERT_EQ(0u, callee->pc()->GetTransceivers().size()); diff --git a/pc/peer_connection_rtp_unittest.cc b/pc/peer_connection_rtp_unittest.cc index 44f105b1d9..b1670c1684 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc @@ -1555,6 +1555,24 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); } +TEST_F(PeerConnectionRtpTestUnifiedPlan, + StopAndNegotiateCausesTransceiverToDisappear) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + auto transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + callee->pc()->GetTransceivers()[0]->StopStandard(); + ASSERT_TRUE(callee->ExchangeOfferAnswerWith(caller.get())); + EXPECT_EQ(RtpTransceiverDirection::kStopped, + transceiver->current_direction()); + EXPECT_EQ(0U, caller->pc()->GetTransceivers().size()); + EXPECT_EQ(0U, callee->pc()->GetTransceivers().size()); + EXPECT_EQ(0U, caller->pc()->GetSenders().size()); + EXPECT_EQ(0U, callee->pc()->GetSenders().size()); + EXPECT_EQ(0U, caller->pc()->GetReceivers().size()); + EXPECT_EQ(0U, callee->pc()->GetReceivers().size()); +} + // Test that AddTransceiver fails if trying to use unimplemented RTP encoding // parameters with the send_encodings parameters. TEST_F(PeerConnectionRtpTestUnifiedPlan,