From 07563732f66cddc77213e84c70a1a38f3e8961a5 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Tue, 26 Jun 2018 11:13:50 -0700 Subject: [PATCH] [Unified Plan] Avoid offering two senders with the same ID MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Taylor Brandstetter Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/master@{#23748} --- pc/peerconnection.cc | 23 ++++++++-- pc/peerconnection_rtp_unittest.cc | 61 ++++++++++++++++++++++++++ pc/peerconnectioninterface_unittest.cc | 2 +- 3 files changed, 81 insertions(+), 5 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index db18ca3aa9..d9538335a2 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1254,7 +1254,14 @@ PeerConnection::AddTrackUnifiedPlan( : 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 @@ PeerConnection::AddTransceiver( 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 @@ PeerConnection::CreateAndAddTransceiver( rtc::scoped_refptr> sender, rtc::scoped_refptr> 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::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 ca2d3b1fd9..2bbf77c426 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -1253,6 +1253,67 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, AddRemoveAddTrackOffersWorksVideo) { 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 26cf6a5303..a974316cfa 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -1762,7 +1762,7 @@ TEST_P(PeerConnectionInterfaceTest, IceCandidates) { // 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 offer;