[Unified Plan] Avoid offering two senders with the same ID
This can happen with the following sequence of API calls:
1) AddTrack(track) + offer/answer
2) RemoveTrack(track's sender) + offer/answer
3) AddTrack(same track)
Since the first transceiver had already been used to send, it will
not get re-used by the second call to AddTrack. Another RtpSender
will be created with its ID = the track ID. But the code hits a
DCHECK when CreateOffer is later called since both m= sections will
offer the same track ID component of the MSID.
The fix implemented here is to randomly generate a sender ID if
there is already an RtpSender with the track's ID.
Bug: webrtc:8734
Change-Id: Ic2dda23d66e364e77ff7505e1c37e53105a17dae
Reviewed-on: https://webrtc-review.googlesource.com/84249
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23748}diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index db18ca3..d953833 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -1254,7 +1254,14 @@
: cricket::MEDIA_TYPE_VIDEO);
RTC_LOG(LS_INFO) << "Adding " << cricket::MediaTypeToString(media_type)
<< " transceiver in response to a call to AddTrack.";
- auto sender = CreateSender(media_type, track->id(), track, stream_ids);
+ std::string sender_id = track->id();
+ // Avoid creating a sender with an existing ID by generating a random ID.
+ // This can happen if this is the second time AddTrack has created a sender
+ // for this track.
+ if (FindSenderById(sender_id)) {
+ sender_id = rtc::CreateRandomUuid();
+ }
+ auto sender = CreateSender(media_type, sender_id, track, stream_ids);
auto receiver = CreateReceiver(media_type, rtc::CreateRandomUuid());
transceiver = CreateAndAddTransceiver(sender, receiver);
transceiver->internal()->set_created_by_addtrack(true);
@@ -1399,9 +1406,12 @@
RTC_LOG(LS_INFO) << "Adding " << cricket::MediaTypeToString(media_type)
<< " transceiver in response to a call to AddTransceiver.";
- auto sender =
- CreateSender(media_type, (track ? track->id() : rtc::CreateRandomUuid()),
- track, init.stream_ids);
+ // Set the sender ID equal to the track ID if the track is specified unless
+ // that sender ID is already in use.
+ std::string sender_id =
+ (track && !FindSenderById(track->id()) ? track->id()
+ : rtc::CreateRandomUuid());
+ auto sender = CreateSender(media_type, sender_id, track, init.stream_ids);
auto receiver = CreateReceiver(media_type, rtc::CreateRandomUuid());
auto transceiver = CreateAndAddTransceiver(sender, receiver);
transceiver->internal()->set_direction(init.direction);
@@ -1466,6 +1476,11 @@
rtc::scoped_refptr<RtpSenderProxyWithInternal<RtpSenderInternal>> sender,
rtc::scoped_refptr<RtpReceiverProxyWithInternal<RtpReceiverInternal>>
receiver) {
+ // Ensure that the new sender does not have an ID that is already in use by
+ // another sender.
+ // Allow receiver IDs to conflict since those come from remote SDP (which
+ // could be invalid, but should not cause a crash).
+ RTC_DCHECK(!FindSenderById(sender->id()));
auto transceiver = RtpTransceiverProxyWithInternal<RtpTransceiver>::Create(
signaling_thread(), new RtpTransceiver(sender, receiver));
transceivers_.push_back(transceiver);
diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc
index ca2d3b1..2bbf77c 100644
--- a/pc/peerconnection_rtp_unittest.cc
+++ b/pc/peerconnection_rtp_unittest.cc
@@ -1253,6 +1253,67 @@
EXPECT_EQ(sender1, sender2);
}
+// Test that CreateOffer succeeds if two tracks with the same label are added.
+TEST_F(PeerConnectionRtpTestUnifiedPlan, CreateOfferSameTrackLabel) {
+ auto caller = CreatePeerConnection();
+
+ auto audio_sender = caller->AddAudioTrack("track", {});
+ auto video_sender = caller->AddVideoTrack("track", {});
+
+ EXPECT_TRUE(caller->CreateOffer());
+
+ EXPECT_EQ(audio_sender->track()->id(), video_sender->track()->id());
+ EXPECT_NE(audio_sender->id(), video_sender->id());
+}
+
+// Test that CreateAnswer succeeds if two tracks with the same label are added.
+TEST_F(PeerConnectionRtpTestUnifiedPlan, CreateAnswerSameTrackLabel) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+
+ RtpTransceiverInit recvonly;
+ recvonly.direction = RtpTransceiverDirection::kRecvOnly;
+ caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO, recvonly);
+ caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, recvonly);
+
+ ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+
+ auto audio_sender = callee->AddAudioTrack("track", {});
+ auto video_sender = callee->AddVideoTrack("track", {});
+
+ EXPECT_TRUE(callee->CreateAnswer());
+
+ EXPECT_EQ(audio_sender->track()->id(), video_sender->track()->id());
+ EXPECT_NE(audio_sender->id(), video_sender->id());
+}
+
+// Test that calling AddTrack, RemoveTrack and AddTrack again creates a second
+// m= section with a random sender id (different from the first, now rejected,
+// m= section).
+TEST_F(PeerConnectionRtpTestUnifiedPlan,
+ AddRemoveAddTrackGeneratesNewSenderId) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+
+ auto track = caller->CreateVideoTrack("video");
+ auto sender1 = caller->AddTrack(track);
+ ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
+
+ caller->pc()->RemoveTrack(sender1);
+ ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
+
+ auto sender2 = caller->AddTrack(track);
+
+ EXPECT_NE(sender1, sender2);
+ EXPECT_NE(sender1->id(), sender2->id());
+ std::string sender2_id = sender2->id();
+
+ ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get()));
+
+ // The sender's ID should not change after negotiation.
+ EXPECT_EQ(sender2_id, sender2->id());
+}
+
// Test that OnRenegotiationNeeded is fired if SetDirection is called on an
// active RtpTransceiver with a new direction.
TEST_F(PeerConnectionRtpTestUnifiedPlan,
diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc
index 26cf6a5..a974316 100644
--- a/pc/peerconnectioninterface_unittest.cc
+++ b/pc/peerconnectioninterface_unittest.cc
@@ -1762,7 +1762,7 @@
// Test that CreateOffer and CreateAnswer will fail if the track labels are
// not unique.
-TEST_P(PeerConnectionInterfaceTest, CreateOfferAnswerWithInvalidStream) {
+TEST_F(PeerConnectionInterfaceTestPlanB, CreateOfferAnswerWithInvalidStream) {
CreatePeerConnectionWithoutDtls();
// Create a regular offer for the CreateAnswer test later.
std::unique_ptr<SessionDescriptionInterface> offer;