From 83d676bd1505265fca13470288963b0d6ef5f149 Mon Sep 17 00:00:00 2001 From: Seth Hampson Date: Thu, 5 Apr 2018 18:12:09 -0700 Subject: [PATCH] Bug fix for applying a remote description twice without stream IDs. A downstream bug ocurred because of a lack of symmetry when adding and removing a remote sender in Plan B that specifies SSRCs, but doesn't specify stream IDs. The issue when the first remote description is applied "default" for the stream ID on the remote sender, but the second time it's applied the current remote sender's "default" stream ID does not match the new remote description's empty stream ID. This was incorrectly interpreted as a new remote sender (which removed/added the sender). Bug: webrtc:7933 Change-Id: I87191b9e887b3450ef15111b5e867023c723a86e Reviewed-on: https://webrtc-review.googlesource.com/67191 Reviewed-by: Steve Anton Reviewed-by: Noah Richards Commit-Queue: Seth Hampson Cr-Commit-Position: refs/heads/master@{#22760} --- pc/peerconnection.cc | 16 +++++++++++++--- pc/peerconnectioninterface_unittest.cc | 13 +++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 304ab67f53..1f63874e3b 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -4044,8 +4044,14 @@ void PeerConnection::UpdateRemoteSendersList( const RtpSenderInfo& info = *sender_it; const cricket::StreamParams* params = cricket::GetStreamBySsrc(streams, info.first_ssrc); + std::string params_stream_id; + if (params) { + params_stream_id = + (!params->first_stream_id().empty() ? params->first_stream_id() + : kDefaultStreamId); + } bool sender_exists = params && params->id == info.sender_id && - params->first_stream_id() == info.stream_id; + params_stream_id == info.stream_id; // If this is a default track, and we still need it, don't remove it. if ((info.stream_id == kDefaultStreamId && default_sender_needed) || sender_exists) { @@ -4072,8 +4078,8 @@ void PeerConnection::UpdateRemoteSendersList( // not supported in Plan B, we just take the first here and create the // default stream ID if none is specified. const std::string& stream_id = - (!params.stream_ids().empty() ? params.stream_ids()[0] - : kDefaultStreamId); + (!params.first_stream_id().empty() ? params.first_stream_id() + : kDefaultStreamId); const std::string& sender_id = params.id; uint32_t ssrc = params.first_ssrc(); @@ -4137,6 +4143,10 @@ void PeerConnection::OnRemoteSenderAdded(const RtpSenderInfo& sender_info, void PeerConnection::OnRemoteSenderRemoved(const RtpSenderInfo& sender_info, cricket::MediaType media_type) { + RTC_LOG(LS_INFO) << "Removing " << cricket::MediaTypeToString(media_type) + << " receiver for track_id=" << sender_info.sender_id + << " and stream_id=" << sender_info.stream_id; + MediaStreamInterface* stream = remote_streams_->find(sender_info.stream_id); rtc::scoped_refptr receiver; diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index f702112ca2..8ee39c945b 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -3122,6 +3122,19 @@ TEST_F(PeerConnectionInterfaceTestPlanB, MediaStreamInterface* remote_stream = observer_.remote_streams()->at(0); EXPECT_EQ("default", remote_stream->id()); ASSERT_EQ(1u, remote_stream->GetAudioTracks().size()); + + // Previously a bug ocurred when setting the remote description a second time. + // This is because we checked equality of the remote StreamParams stream ID + // (empty), and the previously set stream ID for the remote sender + // ("default"). This cause a track to be removed, then added, when really + // nothing should occur because it is the same track. + CreateAndSetRemoteOffer(sdp_string); + EXPECT_EQ(0u, observer_.remove_track_events_.size()); + EXPECT_EQ(1u, observer_.add_track_events_.size()); + EXPECT_EQ("audiotrack0", observer_.last_added_track_label_); + remote_stream = observer_.remote_streams()->at(0); + EXPECT_EQ("default", remote_stream->id()); + ASSERT_EQ(1u, remote_stream->GetAudioTracks().size()); } // This tests that an RtpSender is created when the local description is set