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 <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32158}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 303f5f9..22ef75c 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -1988,13 +1988,7 @@
<< "GetTransceivers is only supported with Unified Plan SdpSemantics.";
std::vector<rtc::scoped_refptr<RtpTransceiverInterface>> 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;
}
@@ -2534,6 +2528,47 @@
});
}
+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<SessionDescriptionInterface> desc,
rtc::scoped_refptr<SetLocalDescriptionObserverInterface> observer) {
@@ -2605,34 +2640,7 @@
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...
@@ -3100,6 +3108,7 @@
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<void>(
@@ -3765,23 +3774,17 @@
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 22c0d9a..1385ce1 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -450,6 +450,9 @@
std::unique_ptr<SessionDescriptionInterface> desc,
rtc::scoped_refptr<SetRemoteDescriptionObserverInterface> 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 046d1c9..3f2371c 100644
--- a/pc/peer_connection_interface_unittest.cc
+++ b/pc/peer_connection_interface_unittest.cc
@@ -2668,8 +2668,8 @@
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 4d9884d..ae151f0 100644
--- a/pc/peer_connection_jsep_unittest.cc
+++ b/pc/peer_connection_jsep_unittest.cc
@@ -354,6 +354,10 @@
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());
@@ -558,6 +562,9 @@
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());
}
@@ -593,15 +600,15 @@
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 44f105b..b1670c1 100644
--- a/pc/peer_connection_rtp_unittest.cc
+++ b/pc/peer_connection_rtp_unittest.cc
@@ -1555,6 +1555,24 @@
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,
