From afa07dda42c9253a37d1c220e71e2c79df90d8d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Thu, 20 Dec 2018 11:06:02 +0100 Subject: [PATCH] [Unified Plan] SRD: Always set associated remote streams. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This fixes a bug where the streams are not updated if the "msid" changes without triggering "ontrack", such as if the streams associated with a receiver changes while the receiver is active. Bug: webrtc:10083, chromium:916934 Change-Id: Ic7b19ad5ef648ed6880cae4157bf49f8435467ae Reviewed-on: https://webrtc-review.googlesource.com/c/114161 Commit-Queue: Henrik Boström Reviewed-by: Steve Anton Reviewed-by: Seth Hampson Cr-Commit-Position: refs/heads/master@{#26069} --- pc/peerconnection.cc | 131 ++++++++++++++++++------------ pc/peerconnection.h | 13 +++ pc/peerconnection_rtp_unittest.cc | 27 ++++++ 3 files changed, 119 insertions(+), 52 deletions(-) diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 642ff49bb9..a70ab2e95a 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -2532,51 +2532,32 @@ RTCError PeerConnection::ApplyRemoteDescription( const MediaContentDescription* media_desc = content->media_description(); RtpTransceiverDirection local_direction = RtpTransceiverDirectionReversed(media_desc->direction()); - // From the WebRTC specification, steps 2.2.8.5/6 of section 4.4.1.6 "Set - // the RTCSessionDescription: If direction is sendrecv or recvonly, and - // transceiver's current direction is neither sendrecv nor recvonly, - // process the addition of a remote track for the media description. - std::vector stream_ids; - if (!media_desc->streams().empty()) { - // The remote description has signaled the stream IDs. - stream_ids = media_desc->streams()[0].stream_ids(); - } - if (RtpTransceiverDirectionHasRecv(local_direction) && - (!transceiver->fired_direction() || - !RtpTransceiverDirectionHasRecv(*transceiver->fired_direction()))) { - RTC_LOG(LS_INFO) << "Processing the addition of a new track for MID=" - << content->name << " (added to " - << GetStreamIdsString(stream_ids) << ")."; - - std::vector> media_streams; - for (const std::string& stream_id : stream_ids) { - rtc::scoped_refptr stream = - remote_streams_->find(stream_id); - if (!stream) { - stream = MediaStreamProxy::Create(rtc::Thread::Current(), - MediaStream::Create(stream_id)); - remote_streams_->AddStream(stream); - added_streams.push_back(stream); - } - media_streams.push_back(stream); + // Roughly the same as steps 2.2.8.6 of section 4.4.1.6 "Set the + // RTCSessionDescription: Set the associated remote streams given + // transceiver.[[Receiver]], msids, addList, and removeList". + // https://w3c.github.io/webrtc-pc/#set-the-rtcsessiondescription + if (RtpTransceiverDirectionHasRecv(local_direction)) { + std::vector stream_ids; + if (!media_desc->streams().empty()) { + // The remote description has signaled the stream IDs. + stream_ids = media_desc->streams()[0].stream_ids(); } - // Special case: "a=msid" missing, use random stream ID. - if (media_streams.empty() && - !(remote_description()->description()->msid_signaling() & - cricket::kMsidSignalingMediaSection)) { - if (!missing_msid_default_stream_) { - missing_msid_default_stream_ = MediaStreamProxy::Create( - rtc::Thread::Current(), - MediaStream::Create(rtc::CreateRandomUuid())); - added_streams.push_back(missing_msid_default_stream_); - } - media_streams.push_back(missing_msid_default_stream_); + RTC_LOG(LS_INFO) << "Processing the MSIDs for MID=" << content->name + << " (" << GetStreamIdsString(stream_ids) << ")."; + SetAssociatedRemoteStreams(transceiver->internal()->receiver_internal(), + stream_ids, &added_streams, + &removed_streams); + // From the WebRTC specification, steps 2.2.8.5/6 of section 4.4.1.6 + // "Set the RTCSessionDescription: If direction is sendrecv or recvonly, + // and transceiver's current direction is neither sendrecv nor recvonly, + // process the addition of a remote track for the media description. + if (!transceiver->fired_direction() || + !RtpTransceiverDirectionHasRecv(*transceiver->fired_direction())) { + RTC_LOG(LS_INFO) + << "Processing the addition of a remote track for MID=" + << content->name << "."; + now_receiving_transceivers.push_back(transceiver); } - // This will add the remote track to the streams. - // TODO(hbos): When we remove remote_streams(), use set_stream_ids() - // instead. https://crbug.com/webrtc/9480 - transceiver->internal()->receiver_internal()->SetStreams(media_streams); - now_receiving_transceivers.push_back(transceiver); } // 2.2.8.1.7: If direction is "sendonly" or "inactive", and transceiver's // [[FiredDirection]] slot is either "sendrecv" or "recvonly", process the @@ -2719,6 +2700,46 @@ RTCError PeerConnection::ApplyRemoteDescription( return RTCError::OK(); } +void PeerConnection::SetAssociatedRemoteStreams( + rtc::scoped_refptr receiver, + const std::vector& stream_ids, + std::vector>* added_streams, + std::vector>* removed_streams) { + std::vector> media_streams; + for (const std::string& stream_id : stream_ids) { + rtc::scoped_refptr stream = + remote_streams_->find(stream_id); + if (!stream) { + stream = MediaStreamProxy::Create(rtc::Thread::Current(), + MediaStream::Create(stream_id)); + remote_streams_->AddStream(stream); + added_streams->push_back(stream); + } + media_streams.push_back(stream); + } + // Special case: "a=msid" missing, use random stream ID. + if (media_streams.empty() && + !(remote_description()->description()->msid_signaling() & + cricket::kMsidSignalingMediaSection)) { + if (!missing_msid_default_stream_) { + missing_msid_default_stream_ = MediaStreamProxy::Create( + rtc::Thread::Current(), MediaStream::Create(rtc::CreateRandomUuid())); + added_streams->push_back(missing_msid_default_stream_); + } + media_streams.push_back(missing_msid_default_stream_); + } + std::vector> previous_streams = + receiver->streams(); + // SetStreams() will add/remove the receiver's track to/from the streams. This + // differs from the spec - the spec uses an "addList" and "removeList" to + // update the stream-track relationships in a later step. We do this earlier, + // changing the order of things, but the end-result is the same. + // TODO(hbos): When we remove remote_streams(), use set_stream_ids() + // instead. https://crbug.com/webrtc/9480 + receiver->SetStreams(media_streams); + RemoveRemoteStreamsIfEmpty(previous_streams, removed_streams); +} + void PeerConnection::ProcessRemovalOfRemoteTrack( rtc::scoped_refptr> transceiver, @@ -2727,19 +2748,25 @@ void PeerConnection::ProcessRemovalOfRemoteTrack( RTC_DCHECK(transceiver->mid()); RTC_LOG(LS_INFO) << "Processing the removal of a track for MID=" << *transceiver->mid(); - std::vector> media_streams = + std::vector> previous_streams = transceiver->internal()->receiver_internal()->streams(); // This will remove the remote track from the streams. transceiver->internal()->receiver_internal()->set_stream_ids({}); remove_list->push_back(transceiver); - // Remove any streams that no longer have tracks. - // TODO(https://crbug.com/webrtc/9480): When we use stream IDs instead - // of streams, see if the stream was removed by checking if this was the - // last receiver with that stream ID. - for (auto stream : media_streams) { - if (stream->GetAudioTracks().empty() && stream->GetVideoTracks().empty()) { - remote_streams_->RemoveStream(stream); - removed_streams->push_back(stream); + RemoveRemoteStreamsIfEmpty(previous_streams, removed_streams); +} + +void PeerConnection::RemoveRemoteStreamsIfEmpty( + const std::vector>& remote_streams, + std::vector>* removed_streams) { + // TODO(https://crbug.com/webrtc/9480): When we use stream IDs instead of + // streams, see if the stream was removed by checking if this was the last + // receiver with that stream ID. + for (auto remote_stream : remote_streams) { + if (remote_stream->GetAudioTracks().empty() && + remote_stream->GetVideoTracks().empty()) { + remote_streams_->RemoveStream(remote_stream); + removed_streams->push_back(remote_stream); } } } diff --git a/pc/peerconnection.h b/pc/peerconnection.h index 75295437c3..e596f001ae 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -475,6 +475,14 @@ class PeerConnection : public PeerConnectionInternal, transceiver, const SessionDescriptionInterface* sdesc) const; + // Runs the algorithm **set the associated remote streams** specified in + // https://w3c.github.io/webrtc-pc/#set-associated-remote-streams. + void SetAssociatedRemoteStreams( + rtc::scoped_refptr receiver, + const std::vector& stream_ids, + std::vector>* added_streams, + std::vector>* removed_streams); + // Runs the algorithm **process the removal of a remote track** specified in // the WebRTC specification. // This method will update the following lists: @@ -489,6 +497,11 @@ class PeerConnection : public PeerConnectionInternal, std::vector>* remove_list, std::vector>* removed_streams); + void RemoveRemoteStreamsIfEmpty( + const std::vector>& + remote_streams, + std::vector>* removed_streams); + void OnNegotiationNeeded(); bool IsClosed() const { diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index c1f7656178..1b78ec296e 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -476,6 +476,33 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, EXPECT_EQ(1u, callee->observer()->remove_track_events_.size()); } +TEST_F(PeerConnectionRtpTestUnifiedPlan, ChangeMsidWhileReceiving) { + auto caller = CreatePeerConnection(); + caller->AddAudioTrack("audio_track", {"stream1"}); + auto callee = CreatePeerConnection(); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOffer())); + + ASSERT_EQ(1u, callee->observer()->on_track_transceivers_.size()); + auto transceiver = callee->observer()->on_track_transceivers_[0]; + ASSERT_EQ(1u, transceiver->receiver()->streams().size()); + EXPECT_EQ("stream1", transceiver->receiver()->streams()[0]->id()); + + ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); + + // Change the stream ID in the offer. + // TODO(https://crbug.com/webrtc/10129): When RtpSenderInterface::SetStreams + // is supported, this can use that instead of munging the SDP. + auto offer = caller->CreateOffer(); + auto contents = offer->description()->contents(); + ASSERT_EQ(1u, contents.size()); + auto& stream_params = contents[0].media_description()->mutable_streams(); + ASSERT_EQ(1u, stream_params.size()); + stream_params[0].set_stream_ids({"stream2"}); + ASSERT_TRUE(callee->SetRemoteDescription(std::move(offer))); + ASSERT_EQ(1u, transceiver->receiver()->streams().size()); + EXPECT_EQ("stream2", transceiver->receiver()->streams()[0]->id()); +} + // These tests examine the state of the peer connection as a result of // performing SetRemoteDescription().