diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 3b5c333a98..ecb6601116 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -1987,7 +1987,13 @@ PeerConnection::GetTransceivers() const { << "GetTransceivers is only supported with Unified Plan SdpSemantics."; std::vector> all_transceivers; for (const auto& transceiver : transceivers_) { - all_transceivers.push_back(transceiver); + // 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); + } } return all_transceivers; } @@ -2527,47 +2533,6 @@ 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) { @@ -2639,7 +2604,34 @@ void PeerConnection::DoSetLocalDescription( RTC_DCHECK(local_description()); if (local_description()->GetType() == SdpType::kAnswer) { - RemoveStoppedTransceivers(); + // 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; + } + } + } // TODO(deadbeef): We already had to hop to the network thread for // MaybeStartGathering... @@ -3107,7 +3099,6 @@ 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( @@ -3773,17 +3764,23 @@ 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), the transceiver - // should have been removed by RemoveStoppedTransceivers(). + // 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. 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); - // The transceiver should be disassociated in RemoveStoppedTransceivers() - RTC_DCHECK(!old_transceiver); + 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); + } } 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 1385ce13a6..22c0d9a8b9 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -450,9 +450,6 @@ 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 a0e3845fd6..10928768e2 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 still returned. - EXPECT_EQ(2u, pc_->GetTransceivers().size()); + // Verify that the RtpTransceivers are no longer returned. + EXPECT_EQ(0u, 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 c3e093617b..ba51214ac3 100644 --- a/pc/peer_connection_jsep_unittest.cc +++ b/pc/peer_connection_jsep_unittest.cc @@ -357,10 +357,6 @@ 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()); @@ -565,9 +561,6 @@ 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()); } @@ -603,15 +596,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(1u, callee->pc()->GetTransceivers().size()); + ASSERT_EQ(0u, callee->pc()->GetTransceivers().size()); ASSERT_TRUE( caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal())); EXPECT_TRUE(first_transceiver->stopped()); - // First transceivers are dissociated on caller side. - ASSERT_EQ(absl::nullopt, first_transceiver->mid()); + // First transceivers aren't dissociated yet on caller side. + ASSERT_NE(absl::nullopt, first_transceiver->mid()); + std::string first_mid = *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 b1670c1684..44f105b1d9 100644 --- a/pc/peer_connection_rtp_unittest.cc +++ b/pc/peer_connection_rtp_unittest.cc @@ -1555,24 +1555,6 @@ 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,