diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index cf9a1f564c..6d60b4c36f 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -1152,13 +1152,6 @@ class PeerConnectionObserver { virtual void OnTrack( rtc::scoped_refptr transceiver) {} - // TODO(hbos,deadbeef): Add |OnAssociatedStreamsUpdated| with |receiver| and - // |streams| as arguments. This should be called when an existing receiver its - // associated streams updated. https://crbug.com/webrtc/8315 - // This may be blocked on supporting multiple streams per sender or else - // this may count as the removal and addition of a track? - // https://crbug.com/webrtc/7932 - // Called when a receiver is completely removed. This is current (Plan B SDP) // behavior that occurs when processing the removal of a remote track, and is // called when the receiver is removed and the track is muted. When Unified diff --git a/api/rtpsenderinterface.h b/api/rtpsenderinterface.h index 2ca2edc830..01279a55a0 100644 --- a/api/rtpsenderinterface.h +++ b/api/rtpsenderinterface.h @@ -48,8 +48,9 @@ class RtpSenderInterface : public rtc::RefCountInterface { // to uniquely identify a receiver until we implement Unified Plan SDP. virtual std::string id() const = 0; - // Returns a list of streams associated with this sender's track. Although we - // only support one track per stream, in theory the API allows for multiple. + // Returns a list of media stream ids associated with this sender's track. + // These are signalled in the SDP so that the remote side can associate + // tracks. virtual std::vector stream_ids() const = 0; virtual RtpParameters GetParameters() const = 0; diff --git a/api/rtptransceiverinterface.h b/api/rtptransceiverinterface.h index 3ea75fd027..c9a86e6c63 100644 --- a/api/rtptransceiverinterface.h +++ b/api/rtptransceiverinterface.h @@ -40,9 +40,6 @@ struct RtpTransceiverInit final { RtpTransceiverDirection direction = RtpTransceiverDirection::kSendRecv; // The added RtpTransceiver will be added to these streams. - // TODO(shampson): Change name to stream_id & update native wrapper's naming - // as well. - // TODO(bugs.webrtc.org/7600): Not implemented. std::vector stream_ids; // TODO(bugs.webrtc.org/7600): Not implemented. diff --git a/media/base/streamparams.cc b/media/base/streamparams.cc index 421111acba..2efa0d0976 100644 --- a/media/base/streamparams.cc +++ b/media/base/streamparams.cc @@ -141,9 +141,15 @@ std::string StreamParams::ToString() const { if (!cname.empty()) { ost << "cname:" << cname << ";"; } - if (!sync_label.empty()) { - ost << "sync_label:" << sync_label; + ost << "stream_ids:"; + for (std::vector::const_iterator it = stream_ids_.begin(); + it != stream_ids_.end(); ++it) { + if (it != stream_ids_.begin()) { + ost << ","; + } + ost << *it; } + ost << ";"; ost << "}"; return ost.str(); } @@ -200,22 +206,15 @@ bool StreamParams::GetSecondarySsrc(const std::string& semantics, } std::vector StreamParams::stream_ids() const { - if (sync_label.empty()) { - return {}; - } - return {sync_label}; + return stream_ids_; } void StreamParams::set_stream_ids(const std::vector& stream_ids) { - // TODO(bugs.webrtc.org/7932): Support an arbitrary number of stream ids. - RTC_DCHECK_LE(stream_ids.size(), 1) << "set_stream_ids currently only " - "supports exactly 0 or 1 stream " - "id."; - sync_label = (stream_ids.empty() ? "" : stream_ids[0]); + stream_ids_ = stream_ids; } std::string StreamParams::first_stream_id() const { - return sync_label; + return stream_ids_.empty() ? "" : stream_ids_[0]; } bool IsOneSsrcStream(const StreamParams& sp) { diff --git a/media/base/streamparams.h b/media/base/streamparams.h index 016c27ad0e..22b98c7427 100644 --- a/media/base/streamparams.h +++ b/media/base/streamparams.h @@ -72,7 +72,7 @@ struct StreamParams { return (groupid == other.groupid && id == other.id && ssrcs == other.ssrcs && ssrc_groups == other.ssrc_groups && type == other.type && display == other.display && - cname == other.cname && sync_label == other.sync_label); + cname == other.cname && stream_ids_ == other.stream_ids_); } bool operator!=(const StreamParams &other) const { return !(*this == other); @@ -165,8 +165,6 @@ struct StreamParams { // Friendly name describing stream std::string display; std::string cname; // RTCP CNAME - // TODO(shampson): Move callers to |stream_ids()| and make private. - std::string sync_label; // Friendly name of cname. private: bool AddSecondarySsrc(const std::string& semantics, @@ -175,6 +173,8 @@ struct StreamParams { bool GetSecondarySsrc(const std::string& semantics, uint32_t primary_ssrc, uint32_t* secondary_ssrc) const; + + std::vector stream_ids_; }; // A Stream can be selected by either groupid+id or ssrc. diff --git a/media/base/streamparams_unittest.cc b/media/base/streamparams_unittest.cc index 6e934ae7d6..db3718075f 100644 --- a/media/base/streamparams_unittest.cc +++ b/media/base/streamparams_unittest.cc @@ -208,8 +208,11 @@ TEST(StreamParams, FecFrFunctions) { TEST(StreamParams, ToString) { cricket::StreamParams sp = CreateStreamParamsWithSsrcGroup("XYZ", kSsrcs2, arraysize(kSsrcs2)); - EXPECT_STREQ("{ssrcs:[1,2];ssrc_groups:{semantics:XYZ;ssrcs:[1,2]};}", - sp.ToString().c_str()); + sp.set_stream_ids({"stream_id"}); + EXPECT_STREQ( + "{ssrcs:[1,2];ssrc_groups:{semantics:XYZ;ssrcs:[1,2]};stream_ids:stream_" + "id;}", + sp.ToString().c_str()); } TEST(StreamParams, TestIsOneSsrcStream_LegacyStream) { diff --git a/pc/mediasession.cc b/pc/mediasession.cc index 8a9c1f802b..474c0b4e45 100644 --- a/pc/mediasession.cc +++ b/pc/mediasession.cc @@ -476,8 +476,6 @@ static bool AddStreamParams( } } stream_param.cname = rtcp_cname; - // TODO(steveanton): Support any number of stream ids. - RTC_CHECK(sender.stream_ids.size() == 1U); stream_param.set_stream_ids(sender.stream_ids); content_description->AddStream(stream_param); @@ -488,8 +486,6 @@ static bool AddStreamParams( // Use existing generated SSRCs/groups, but update the sync_label if // necessary. This may be needed if a MediaStreamTrack was moved from one // MediaStream to another. - // TODO(steveanton): Support any number of stream ids. - RTC_CHECK(sender.stream_ids.size() == 1U); param->set_stream_ids(sender.stream_ids); content_description->AddStream(*param); } diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index eacfb8329b..23ad4234c8 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1143,12 +1143,6 @@ RTCErrorOr> PeerConnection::AddTrack( LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, "Track has invalid kind: " + track->kind()); } - // TODO(bugs.webrtc.org/7932): Support adding a track to multiple streams. - if (stream_ids.size() > 1u) { - LOG_AND_RETURN_ERROR( - RTCErrorType::UNSUPPORTED_OPERATION, - "AddTrack with more than one stream is not currently supported."); - } if (IsClosed()) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_STATE, "PeerConnection is closed."); @@ -1158,18 +1152,9 @@ RTCErrorOr> PeerConnection::AddTrack( RTCErrorType::INVALID_PARAMETER, "Sender already exists for track " + track->id() + "."); } - // TODO(bugs.webrtc.org/7933): MediaSession expects the sender to have exactly - // one stream. AddTrackInternal will return an error if there is more than one - // stream, but if the caller specifies none then we need to generate a random - // stream id. - std::vector adjusted_stream_ids = stream_ids; - if (stream_ids.empty()) { - adjusted_stream_ids.push_back(rtc::CreateRandomUuid()); - } - RTC_DCHECK_EQ(1, adjusted_stream_ids.size()); auto sender_or_error = - (IsUnifiedPlan() ? AddTrackUnifiedPlan(track, adjusted_stream_ids) - : AddTrackPlanB(track, adjusted_stream_ids)); + (IsUnifiedPlan() ? AddTrackUnifiedPlan(track, stream_ids) + : AddTrackPlanB(track, stream_ids)); if (sender_or_error.ok()) { observer_->OnRenegotiationNeeded(); stats_->AddTrack(track); @@ -1181,17 +1166,26 @@ RTCErrorOr> PeerConnection::AddTrackPlanB( rtc::scoped_refptr track, const std::vector& stream_ids) { + if (stream_ids.size() > 1u) { + LOG_AND_RETURN_ERROR(RTCErrorType::UNSUPPORTED_OPERATION, + "AddTrack with more than one stream is not " + "supported with Plan B semantics."); + } + std::vector adjusted_stream_ids = stream_ids; + if (adjusted_stream_ids.empty()) { + adjusted_stream_ids.push_back(rtc::CreateRandomUuid()); + } cricket::MediaType media_type = (track->kind() == MediaStreamTrackInterface::kAudioKind ? cricket::MEDIA_TYPE_AUDIO : cricket::MEDIA_TYPE_VIDEO); - auto new_sender = CreateSender(media_type, track, stream_ids); + auto new_sender = CreateSender(media_type, track, adjusted_stream_ids); if (track->kind() == MediaStreamTrackInterface::kAudioKind) { new_sender->internal()->SetVoiceMediaChannel(voice_media_channel()); GetAudioTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info = FindSenderInfo(local_audio_sender_infos_, - new_sender->internal()->stream_id(), track->id()); + new_sender->internal()->stream_ids()[0], track->id()); if (sender_info) { new_sender->internal()->SetSsrc(sender_info->first_ssrc); } @@ -1201,7 +1195,7 @@ PeerConnection::AddTrackPlanB( GetVideoTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info = FindSenderInfo(local_video_sender_infos_, - new_sender->internal()->stream_id(), track->id()); + new_sender->internal()->stream_ids()[0], track->id()); if (sender_info) { new_sender->internal()->SetSsrc(sender_info->first_ssrc); } @@ -1372,15 +1366,7 @@ PeerConnection::AddTransceiver( // TODO(bugs.webrtc.org/7600): Verify init. - if (init.stream_ids.size() > 1u) { - LOG_AND_RETURN_ERROR(RTCErrorType::UNSUPPORTED_PARAMETER, - "More than one stream is not yet supported."); - } - - std::vector stream_ids = { - !init.stream_ids.empty() ? init.stream_ids[0] : rtc::CreateRandomUuid()}; - - auto sender = CreateSender(media_type, track, stream_ids); + auto sender = CreateSender(media_type, track, init.stream_ids); auto receiver = CreateReceiver(media_type, rtc::CreateRandomUuid()); auto transceiver = CreateAndAddTransceiver(sender, receiver); transceiver->internal()->set_direction(init.direction); @@ -1487,8 +1473,12 @@ rtc::scoped_refptr PeerConnection::CreateSender( return nullptr; } + // Internally we need to have one stream with Plan B semantics, so we + // generate a random stream ID if not specified. std::vector stream_ids; - if (!stream_id.empty()) { + if (stream_id.empty()) { + stream_ids.push_back(rtc::CreateRandomUuid()); + } else { stream_ids.push_back(stream_id); } @@ -2348,28 +2338,33 @@ RTCError PeerConnection::ApplyRemoteDescription( // 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. - // - // Create a sync label in the case of an unsignalled msid. - std::string sync_label = rtc::CreateRandomUuid(); + std::vector stream_ids; if (!media_desc->streams().empty()) { + // TODO(bugs.webrtc.org/7932): Currently stream ids are all populated + // within StreamParams. When they are updated to be stored within the + // MediaContentDescription, change the logic here. const cricket::StreamParams& stream_params = media_desc->streams()[0]; if (!stream_params.stream_ids().empty()) { - sync_label = stream_params.stream_ids()[0]; + stream_ids = stream_params.stream_ids(); } } if (RtpTransceiverDirectionHasRecv(local_direction) && (!transceiver->current_direction() || !RtpTransceiverDirectionHasRecv( *transceiver->current_direction()))) { - rtc::scoped_refptr stream = - remote_streams_->find(sync_label); - if (!stream) { - stream = MediaStreamProxy::Create(rtc::Thread::Current(), - MediaStream::Create(sync_label)); - remote_streams_->AddStream(stream); - added_streams.push_back(stream); + 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); } - transceiver->internal()->receiver_internal()->SetStreams({stream}); + transceiver->internal()->receiver_internal()->SetStreams(media_streams); receiving_transceivers.push_back(transceiver); } // If direction is sendonly or inactive, and transceiver's current @@ -3308,7 +3303,7 @@ void PeerConnection::AddAudioTrack(AudioTrackInterface* track, if (sender) { // We already have a sender for this track, so just change the stream_id // so that it's correct in the next call to CreateOffer. - sender->internal()->set_stream_id(stream->id()); + sender->internal()->set_stream_ids({stream->id()}); return; } @@ -3351,7 +3346,7 @@ void PeerConnection::AddVideoTrack(VideoTrackInterface* track, if (sender) { // We already have a sender for this track, so just change the stream_id // so that it's correct in the next call to CreateOffer. - sender->internal()->set_stream_id(stream->id()); + sender->internal()->set_stream_ids({stream->id()}); return; } @@ -3982,6 +3977,8 @@ void PeerConnection::UpdateRemoteSendersList( bool default_sender_needed, cricket::MediaType media_type, StreamCollection* new_streams) { + RTC_DCHECK(!IsUnifiedPlan()); + std::vector* current_senders = GetRemoteSenderInfos(media_type); @@ -3993,7 +3990,8 @@ void PeerConnection::UpdateRemoteSendersList( const RtpSenderInfo& info = *sender_it; const cricket::StreamParams* params = cricket::GetStreamBySsrc(streams, info.first_ssrc); - bool sender_exists = params && params->id == info.sender_id; + bool sender_exists = params && params->id == info.sender_id && + params->first_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) { @@ -4007,10 +4005,13 @@ void PeerConnection::UpdateRemoteSendersList( // Find new and active senders. for (const cricket::StreamParams& params : streams) { // |params.id| is the sender id and the stream id uses the first of - // |params.stream_ids|. - // TODO(bugs.webrtc.org/7932): Add support for multiple stream ids. + // |params.stream_ids|. The remote description could come from a Unified + // Plan endpoint, with multiple or no stream_ids() signalled. Since this is + // not supported in Plan B, we just take the first here and create an empty + // stream id if none is specified. const std::string& stream_id = - (!params.stream_ids().empty() ? params.stream_ids()[0] : ""); + (!params.stream_ids().empty() ? params.stream_ids()[0] + : kDefaultStreamId); const std::string& sender_id = params.id; uint32_t ssrc = params.first_ssrc(); @@ -4157,6 +4158,7 @@ void PeerConnection::UpdateLocalSenders( void PeerConnection::OnLocalSenderAdded(const RtpSenderInfo& sender_info, cricket::MediaType media_type) { + RTC_DCHECK(!IsUnifiedPlan()); auto sender = FindSenderById(sender_info.sender_id); if (!sender) { RTC_LOG(LS_WARNING) << "An unknown RtpSender with id " @@ -4171,7 +4173,7 @@ void PeerConnection::OnLocalSenderAdded(const RtpSenderInfo& sender_info, return; } - sender->internal()->set_stream_id(sender_info.stream_id); + sender->internal()->set_stream_ids({sender_info.stream_id}); sender->internal()->SetSsrc(sender_info.first_ssrc); } diff --git a/pc/peerconnection_jsep_unittest.cc b/pc/peerconnection_jsep_unittest.cc index 84bd060d6f..caa0ea0587 100644 --- a/pc/peerconnection_jsep_unittest.cc +++ b/pc/peerconnection_jsep_unittest.cc @@ -1162,11 +1162,11 @@ TEST_F(PeerConnectionJsepTest, ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); - ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); - EXPECT_EQ(kTrackLabel, - callee->observer()->add_track_events_[0].receiver->track()->id()); - // TODO(bugs.webrtc.org/7933): Also verify that no stream was added to the - // receiver. + const auto& track_events = callee->observer()->add_track_events_; + ASSERT_EQ(1u, track_events.size()); + const auto& event = track_events[0]; + EXPECT_EQ(kTrackLabel, event.receiver->track()->id()); + EXPECT_EQ(0u, event.streams.size()); } // Test that setting a remote offer with one track that has one stream fires off @@ -1226,7 +1226,27 @@ TEST_F(PeerConnectionJsepTest, UnorderedElementsAre(track1, track2)); } -// TODO(bugs.webrtc.org/7932): Also test multi-stream case. +// Test that setting a remote offer with one track that has two streams fires +// off the correct OnAddTrack event. +TEST_F(PeerConnectionJsepTest, + SetRemoteOfferWithOneTrackTwoStreamFiresOnAddTrack) { + const std::string kTrackLabel = "audio_track"; + const std::string kStreamId1 = "audio_stream1"; + const std::string kStreamId2 = "audio_stream2"; + + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + ASSERT_TRUE(caller->AddAudioTrack(kTrackLabel, {kStreamId1, kStreamId2})); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + + const auto& track_events = callee->observer()->add_track_events_; + ASSERT_EQ(1u, track_events.size()); + const auto& event = track_events[0]; + ASSERT_EQ(2u, event.streams.size()); + EXPECT_EQ(kStreamId1, event.streams[0]->id()); + EXPECT_EQ(kStreamId2, event.streams[1]->id()); +} // Test that if an RtpTransceiver with a current_direction set is stopped, then // current_direction is changed to null. diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index 69f329cc1e..bcfe0d8652 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -17,6 +17,7 @@ #include "api/mediastreaminterface.h" #include "api/peerconnectioninterface.h" #include "api/umametrics.h" +#include "pc/mediasession.h" #include "pc/mediastream.h" #include "pc/mediastreamtrack.h" #include "pc/peerconnectionwrapper.h" @@ -115,8 +116,8 @@ TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithoutStreamFiresOnAddTrack) { static_cast(nullptr))); ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); - // TODO(hbos): When "no stream" is handled correctly we would expect - // |add_track_events_[0].streams| to be empty. https://crbug.com/webrtc/7933 + // Since we are not supporting the no stream case with Plan B, there should be + // a generated stream, even though we didn't set one with AddTrack. auto& add_track_event = callee->observer()->add_track_events_[0]; ASSERT_EQ(add_track_event.streams.size(), 1u); EXPECT_TRUE(add_track_event.streams[0]->FindAudioTrack("audio_track")); @@ -219,15 +220,54 @@ TEST_F(PeerConnectionRtpCallbacksTest, callee->observer()->remove_track_events_); } +// Tests the edge case that if a stream ID changes for a given track that both +// OnRemoveTrack and OnAddTrack is fired. +TEST_F(PeerConnectionRtpCallbacksTest, + RemoteStreamIdChangesFiresOnRemoveAndOnAddTrack) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + const char kStreamId1[] = "stream1"; + const char kStreamId2[] = "stream2"; + caller->AddTrack(caller->CreateAudioTrack("audio_track1"), {kStreamId1}); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + EXPECT_EQ(callee->observer()->add_track_events_.size(), 1u); + + // Change the stream ID of the sender in the session description. + auto offer = caller->CreateOfferAndSetAsLocal(); + auto audio_desc = offer->description()->GetContentDescriptionByName("audio"); + ASSERT_EQ(audio_desc->mutable_streams().size(), 1u); + audio_desc->mutable_streams()[0].set_stream_ids({kStreamId2}); + ASSERT_TRUE( + callee->SetRemoteDescription(CloneSessionDescription(offer.get()), + static_cast(nullptr))); + + ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); + EXPECT_EQ(callee->observer()->add_track_events_[1].streams[0]->id(), + kStreamId2); + ASSERT_EQ(callee->observer()->remove_track_events_.size(), 1u); + EXPECT_EQ(callee->observer()->remove_track_events_[0]->streams()[0]->id(), + kStreamId1); +} + // Tests that setting a remote description with sending transceivers will fire // the OnTrack callback for each transceiver and setting a remote description -// with receive only transceivers will not call OnTrack. +// with receive only transceivers will not call OnTrack. One transceiver is +// created without any stream_ids, while the other is created with multiple +// stream_ids. TEST_F(PeerConnectionRtpCallbacksTest, UnifiedPlanAddTransceiverCallsOnTrack) { + const std::string kStreamId1 = "video_stream1"; + const std::string kStreamId2 = "video_stream2"; auto caller = CreatePeerConnectionWithUnifiedPlan(); auto callee = CreatePeerConnectionWithUnifiedPlan(); auto audio_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_AUDIO); - auto video_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); + RtpTransceiverInit video_transceiver_init; + video_transceiver_init.stream_ids = {kStreamId1, kStreamId2}; + auto video_transceiver = + caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, video_transceiver_init); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -237,6 +277,14 @@ TEST_F(PeerConnectionRtpCallbacksTest, UnifiedPlanAddTransceiverCallsOnTrack) { callee->pc()->GetTransceivers()[0]->mid()); EXPECT_EQ(video_transceiver->mid(), callee->pc()->GetTransceivers()[1]->mid()); + std::vector> audio_streams = + callee->pc()->GetTransceivers()[0]->receiver()->streams(); + std::vector> video_streams = + callee->pc()->GetTransceivers()[1]->receiver()->streams(); + ASSERT_EQ(0u, audio_streams.size()); + ASSERT_EQ(2u, video_streams.size()); + EXPECT_EQ(kStreamId1, video_streams[0]->id()); + EXPECT_EQ(kStreamId2, video_streams[1]->id()); } // Test that doing additional offer/answer exchanges with no changes to tracks @@ -335,8 +383,8 @@ TEST_F(PeerConnectionRtpObserverTest, AddSenderWithoutStreamAddsReceiver) { EXPECT_EQ(callee->pc()->GetReceivers().size(), 1u); auto receiver_added = callee->pc()->GetReceivers()[0]; EXPECT_EQ("audio_track", receiver_added->track()->id()); - // TODO(hbos): When "no stream" is handled correctly we would expect - // |receiver_added->streams()| to be empty. https://crbug.com/webrtc/7933 + // Since we are not supporting the no stream case with Plan B, there should be + // a generated stream, even though we didn't set one with AddTrack. EXPECT_EQ(receiver_added->streams().size(), 1u); EXPECT_TRUE(receiver_added->streams()[0]->FindAudioTrack("audio_track")); } @@ -505,6 +553,74 @@ TEST_F(PeerConnectionRtpObserverTest, EXPECT_TRUE_WAIT(srd2_callback_called, kDefaultTimeout); } +// Tests that with Unified Plan if the the stream id changes for a track when +// when setting a new remote description, that the media stream is updated +// appropriately for the receiver. +TEST_F(PeerConnectionRtpObserverTest, RemoteStreamIdChangesUpdatesReceiver) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + auto callee = CreatePeerConnectionWithUnifiedPlan(); + + const char kStreamId1[] = "stream1"; + const char kStreamId2[] = "stream2"; + caller->AddTrack(caller->CreateAudioTrack("audio_track1"), {kStreamId1}); + ASSERT_TRUE( + callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), + static_cast(nullptr))); + EXPECT_EQ(callee->observer()->add_track_events_.size(), 1u); + + // Change the stream id of the sender in the session description. + auto offer = caller->CreateOfferAndSetAsLocal(); + auto contents = offer->description()->contents(); + ASSERT_EQ(contents.size(), 1u); + ASSERT_EQ(contents[0].media_description()->mutable_streams().size(), 1u); + contents[0].media_description()->mutable_streams()[0].set_stream_ids( + {kStreamId2}); + + // Set the remote description and verify that the stream was updated properly. + ASSERT_TRUE( + callee->SetRemoteDescription(CloneSessionDescription(offer.get()), + static_cast(nullptr))); + auto receivers = callee->pc()->GetReceivers(); + ASSERT_EQ(receivers.size(), 1u); + ASSERT_EQ(receivers[0]->streams().size(), 1u); + EXPECT_EQ(receivers[0]->streams()[0]->id(), kStreamId2); +} + +// This tests a regression caught by a downstream client, that occured when +// applying a remote description with a SessionDescription object that +// contained StreamParams that didn't have ids. Although there were multiple +// remote audio senders, FindSenderInfo didn't find them as unique, because +// it looked up by StreamParam.id, which none had. This meant only one +// AudioRtpReceiver was created, as opposed to one for each remote sender. +TEST_F(PeerConnectionRtpObserverTest, + MultipleRemoteSendersWithoutStreamParamIdAddsMultipleReceivers) { + auto caller = CreatePeerConnection(); + auto callee = CreatePeerConnection(); + + const char kStreamId1[] = "stream1"; + const char kStreamId2[] = "stream2"; + caller->AddAudioTrack("audio_track1", {kStreamId1}); + caller->AddAudioTrack("audio_track2", {kStreamId2}); + + auto offer = caller->CreateOfferAndSetAsLocal(); + auto mutable_streams = + cricket::GetFirstAudioContentDescription(offer->description()) + ->mutable_streams(); + ASSERT_EQ(mutable_streams.size(), 2u); + // Clear the IDs in the StreamParams. + mutable_streams[0].id.clear(); + mutable_streams[1].id.clear(); + ASSERT_TRUE( + callee->SetRemoteDescription(CloneSessionDescription(offer.get()))); + + auto receivers = callee->pc()->GetReceivers(); + ASSERT_EQ(receivers.size(), 2u); + ASSERT_EQ(receivers[0]->streams().size(), 1u); + EXPECT_EQ(kStreamId1, receivers[0]->streams()[0]->id()); + ASSERT_EQ(receivers[1]->streams().size(), 1u); + EXPECT_EQ(kStreamId2, receivers[1]->streams()[0]->id()); +} + // Tests for the legacy SetRemoteDescription() function signature. class PeerConnectionRtpLegacyObserverTest : public PeerConnectionRtpTest {}; @@ -1068,6 +1184,39 @@ TEST_F(PeerConnectionMsidSignalingTest, PlanBOfferToUnifiedPlanAnswer) { answer->description()->msid_signaling()); } +// This tests that a Plan B endpoint appropriately sets the remote description +// from a Unified Plan offer. When the Unified Plan offer contains a=msid lines +// that signal no stream ids or multiple stream ids we expect that the Plan B +// endpoint always has exactly one media stream per track. +TEST_F(PeerConnectionMsidSignalingTest, UnifiedPlanToPlanBAnswer) { + const std::string kStreamId1 = "audio_stream_1"; + const std::string kStreamId2 = "audio_stream_2"; + + auto caller = CreatePeerConnectionWithUnifiedPlan(); + caller->AddAudioTrack("caller_audio", {kStreamId1, kStreamId2}); + caller->AddVideoTrack("caller_video", {}); + auto callee = CreatePeerConnectionWithPlanB(); + callee->AddAudioTrack("callee_audio"); + caller->AddVideoTrack("callee_video"); + + ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); + + // Offer should have had both a=msid and a=ssrc MSID lines. + auto* offer = callee->pc()->remote_description(); + EXPECT_EQ((cricket::kMsidSignalingMediaSection | + cricket::kMsidSignalingSsrcAttribute), + offer->description()->msid_signaling()); + + // Callee should always have 1 stream for all of it's receivers. + const auto& track_events = callee->observer()->add_track_events_; + ASSERT_EQ(2u, track_events.size()); + ASSERT_EQ(1u, track_events[0].streams.size()); + EXPECT_EQ(kStreamId1, track_events[0].streams[0]->id()); + ASSERT_EQ(1u, track_events[1].streams.size()); + // This autogenerated a stream id for the empty one signalled. + EXPECT_FALSE(track_events[1].streams[0]->id().empty()); +} + TEST_F(PeerConnectionMsidSignalingTest, PureUnifiedPlanToUs) { auto caller = CreatePeerConnectionWithUnifiedPlan(); caller->AddAudioTrack("caller_audio"); diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index 62fd3bfbb5..b457150581 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -1628,9 +1628,16 @@ TEST_P(PeerConnectionInterfaceTest, AddTrackWithoutStream) { EXPECT_EQ(audio_track, audio_sender->track()); EXPECT_EQ("video_track", video_sender->id()); EXPECT_EQ(video_track, video_sender->track()); - // If the ID is truly a random GUID, it should be infinitely unlikely they - // will be the same. - EXPECT_NE(video_sender->stream_ids(), audio_sender->stream_ids()); + if (sdp_semantics_ == SdpSemantics::kPlanB) { + // If the ID is truly a random GUID, it should be infinitely unlikely they + // will be the same. + EXPECT_NE(video_sender->stream_ids(), audio_sender->stream_ids()); + } else { + // We allows creating tracks without stream ids under Unified Plan + // semantics. + EXPECT_EQ(0u, video_sender->stream_ids().size()); + EXPECT_EQ(0u, audio_sender->stream_ids().size()); + } } // Test that we can call GetStats() after AddTrack but before connecting @@ -3052,6 +3059,33 @@ TEST_F(PeerConnectionInterfaceTestPlanB, VerifyDefaultStreamIsNotCreated) { EXPECT_EQ(0u, observer_.remote_streams()->count()); } +// This tests that when a Plan B endpoint receives an SDP that signals no media +// stream IDs indicated by the special character "-" in the a=msid line, that +// a default stream ID will be used for the MediaStream ID. This can occur +// when a Unified Plan endpoint signals no media stream IDs, but signals both +// a=ssrc msid and a=msid lines for interop signaling with Plan B. +TEST_F(PeerConnectionInterfaceTestPlanB, + SdpWithEmptyMsidAndSsrcCreatesDefaultStreamId) { + FakeConstraints constraints; + constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, + true); + CreatePeerConnection(&constraints); + // Add a a=msid line to the SDP. This is prioritized when parsing the SDP, so + // the sender's stream ID will be interpreted as no stream IDs. + std::string sdp_string = kSdpStringWithStream1AudioTrackOnly; + sdp_string.append("a=msid:- audiotrack0\n"); + + CreateAndSetRemoteOffer(sdp_string); + + ASSERT_EQ(1u, observer_.remote_streams()->count()); + // Because SSRCs are signaled the track ID will be what was signaled in the + // a=msid line. + EXPECT_EQ("audiotrack0", observer_.last_added_track_label_); + MediaStreamInterface* 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 // after adding a local stream. // TODO(deadbeef): This test and the one below it need to be updated when diff --git a/pc/rtpsender.cc b/pc/rtpsender.cc index 170aa9c507..626c850505 100644 --- a/pc/rtpsender.cc +++ b/pc/rtpsender.cc @@ -79,9 +79,6 @@ AudioRtpSender::AudioRtpSender(rtc::Thread* worker_thread, sink_adapter_(new LocalAudioSinkAdapter()), attachment_id_(track ? GenerateUniqueId() : 0) { RTC_DCHECK(worker_thread); - // TODO(bugs.webrtc.org/7932): Remove once zero or multiple streams are - // supported. - RTC_DCHECK_EQ(stream_ids.size(), 1u); if (track_) { track_->RegisterObserver(this); track_->AddSink(sink_adapter_.get()); @@ -318,9 +315,6 @@ VideoRtpSender::VideoRtpSender(rtc::Thread* worker_thread, : VideoTrackInterface::ContentHint::kNone), attachment_id_(track ? GenerateUniqueId() : 0) { RTC_DCHECK(worker_thread); - // TODO(bugs.webrtc.org/7932): Remove once zero or multiple streams are - // supported. - RTC_DCHECK_EQ(stream_ids.size(), 1u); if (track_) { track_->RegisterObserver(this); } diff --git a/pc/rtpsender.h b/pc/rtpsender.h index 05e11313a5..27b1513379 100644 --- a/pc/rtpsender.h +++ b/pc/rtpsender.h @@ -50,12 +50,6 @@ class RtpSenderInternal : public RtpSenderInterface { // description). virtual void SetSsrc(uint32_t ssrc) = 0; - // TODO(steveanton): With Unified Plan, a track/RTCRTPSender can be part of - // multiple streams (or no stream at all). Replace these singular methods with - // their corresponding plural methods. - // Until these are removed, RtpSenders must have exactly one stream. - virtual void set_stream_id(const std::string& stream_id) = 0; - virtual std::string stream_id() const = 0; virtual void set_stream_ids(const std::vector& stream_ids) = 0; virtual void Stop() = 0; @@ -142,10 +136,6 @@ class AudioRtpSender : public DtmfProviderInterface, // RtpSenderInternal implementation. void SetSsrc(uint32_t ssrc) override; - void set_stream_id(const std::string& stream_id) override { - stream_ids_ = {stream_id}; - } - std::string stream_id() const override { return stream_ids_[0]; } void set_stream_ids(const std::vector& stream_ids) override { stream_ids_ = stream_ids; } @@ -177,8 +167,6 @@ class AudioRtpSender : public DtmfProviderInterface, rtc::Thread* const worker_thread_; const std::string id_; - // TODO(steveanton): Until more Unified Plan work is done, this can only have - // exactly one element. std::vector stream_ids_; cricket::VoiceMediaChannel* media_channel_ = nullptr; StatsCollector* stats_; @@ -236,10 +224,6 @@ class VideoRtpSender : public ObserverInterface, // RtpSenderInternal implementation. void SetSsrc(uint32_t ssrc) override; - void set_stream_id(const std::string& stream_id) override { - stream_ids_ = {stream_id}; - } - std::string stream_id() const override { return stream_ids_[0]; } void set_stream_ids(const std::vector& stream_ids) override { stream_ids_ = stream_ids; } @@ -266,8 +250,6 @@ class VideoRtpSender : public ObserverInterface, rtc::Thread* worker_thread_; const std::string id_; - // TODO(steveanton): Until more Unified Plan work is done, this can only have - // exactly one element. std::vector stream_ids_; cricket::VideoMediaChannel* media_channel_ = nullptr; rtc::scoped_refptr track_; diff --git a/pc/test/mock_rtpsenderinternal.h b/pc/test/mock_rtpsenderinternal.h index b76a9ac0ba..5988c7b3c1 100644 --- a/pc/test/mock_rtpsenderinternal.h +++ b/pc/test/mock_rtpsenderinternal.h @@ -37,8 +37,6 @@ class MockRtpSenderInternal : public RtpSenderInternal { MOCK_METHOD1(SetVoiceMediaChannel, void(cricket::VoiceMediaChannel*)); MOCK_METHOD1(SetVideoMediaChannel, void(cricket::VideoMediaChannel*)); MOCK_METHOD1(SetSsrc, void(uint32_t)); - MOCK_METHOD1(set_stream_id, void(const std::string&)); - MOCK_CONST_METHOD0(stream_id, std::string()); MOCK_METHOD1(set_stream_ids, void(const std::vector&)); MOCK_METHOD0(Stop, void()); MOCK_CONST_METHOD0(AttachmentId, int()); diff --git a/pc/webrtcsdp.cc b/pc/webrtcsdp.cc index 80ac8d58d6..a0887a6f78 100644 --- a/pc/webrtcsdp.cc +++ b/pc/webrtcsdp.cc @@ -120,10 +120,12 @@ static const char kSsrcAttributeCname[] = "cname"; static const char kAttributeExtmap[] = "extmap"; // draft-alvestrand-mmusic-msid-01 // a=msid-semantic: WMS +// This is a legacy field supported only for Plan B semantics. static const char kAttributeMsidSemantics[] = "msid-semantic"; static const char kMediaStreamSemantic[] = "WMS"; static const char kSsrcAttributeMsid[] = "msid"; static const char kDefaultMsid[] = "default"; +static const char kNoStreamMsid[] = "-"; static const char kSsrcAttributeMslabel[] = "mslabel"; static const char kSSrcAttributeLabel[] = "label"; static const char kAttributeSsrcGroup[] = "ssrc-group"; @@ -325,7 +327,7 @@ static bool ParseDtlsSetup(const std::string& line, cricket::ConnectionRole* role, SdpParseError* error); static bool ParseMsidAttribute(const std::string& line, - std::string* stream_id, + std::vector* stream_ids, std::string* track_id, SdpParseError* error); @@ -578,43 +580,50 @@ static bool GetPayloadTypeFromString(const std::string& line, cricket::IsValidRtpPayloadType(*payload_type); } -// |msid_stream_id| and |msid_track_id| represent the stream/track ID from the +// |msid_stream_idss| and |msid_track_id| represent the stream/track ID from the // "a=msid" attribute, if it exists. They are empty if the attribute does not // exist. +// TODO(bugs.webrtc.org/7932): Currently we require that an a=ssrc line is +// signalled in order to add the appropriate stream_ids. Update here to add both +// StreamParams objects for the a=ssrc msid lines, and add the +// stream_id/track_id, to the MediaContentDescription for the a=msid lines. This +// way the logic will get pushed out to peerconnection.cc for interpreting the +// media stream ids. void CreateTracksFromSsrcInfos(const SsrcInfoVec& ssrc_infos, - const std::string& msid_stream_id, + const std::vector& msid_stream_ids, const std::string& msid_track_id, - StreamParamsVec* tracks) { + StreamParamsVec* tracks, + int msid_signaling) { RTC_DCHECK(tracks != NULL); - RTC_DCHECK(msid_stream_id.empty() == msid_track_id.empty()); for (SsrcInfoVec::const_iterator ssrc_info = ssrc_infos.begin(); ssrc_info != ssrc_infos.end(); ++ssrc_info) { if (ssrc_info->cname.empty()) { continue; } - std::string stream_id; + std::vector stream_ids; std::string track_id; - if (ssrc_info->stream_id.empty() && !ssrc_info->mslabel.empty()) { - // If there's no msid and there's mslabel, we consider this is a sdp from - // a older version of client that doesn't support msid. - // In that case, we use the mslabel and label to construct the track. - stream_id = ssrc_info->mslabel; - track_id = ssrc_info->label; - } else if (ssrc_info->stream_id.empty() && !msid_stream_id.empty()) { - // If there's no msid in the SSRC attributes, but there's a global one - // (from a=msid), use that. This is the case with Unified Plan SDP. - stream_id = msid_stream_id; + if (msid_signaling & cricket::kMsidSignalingMediaSection) { + // This is the case with Unified Plan SDP msid signaling. + stream_ids = msid_stream_ids; track_id = msid_track_id; - } else { - stream_id = ssrc_info->stream_id; + } else if (msid_signaling & cricket::kMsidSignalingSsrcAttribute) { + // This is the case with Plan B SDP msid signaling. + stream_ids.push_back(ssrc_info->stream_id); track_id = ssrc_info->track_id; + } else if (!ssrc_info->mslabel.empty()) { + // Since there's no a=msid or a=ssrc msid signaling, this is a sdp from + // an older version of client that doesn't support msid. + // In that case, we use the mslabel and label to construct the track. + stream_ids.push_back(ssrc_info->mslabel); + track_id = ssrc_info->label; + } else { + // Since no media streams isn't supported with older SDP signaling, we + // use a default a stream id. + stream_ids.push_back(kDefaultMsid); } - // If a stream/track ID wasn't populated from the SSRC attributes OR the + // If a track ID wasn't populated from the SSRC attributes OR the // msid attribute, use default/random values. - if (stream_id.empty()) { - stream_id = kDefaultMsid; - } if (track_id.empty()) { // TODO(ronghuawu): What should we do if the track id doesn't appear? // Create random string (which will be used as track label later)? @@ -634,7 +643,7 @@ void CreateTracksFromSsrcInfos(const SsrcInfoVec& ssrc_infos, } track->add_ssrc(ssrc_info->ssrc_id); track->cname = ssrc_info->cname; - track->set_stream_ids({stream_id}); + track->set_stream_ids(stream_ids); track->id = track_id; } } @@ -1472,18 +1481,26 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, } AddLine(os.str(), message); - // Specified in https://tools.ietf.org/html/draft-ietf-mmusic-msid-16 - // a=msid: + // Specified in https://datatracker.ietf.org/doc/draft-ietf-mmusic-msid/16/ + // a=msid: + // The msid-id is a 1*64 token char representing the media stream id, and the + // msid-appdata is a 1*64 token char representing the track id. There is a + // line for every media stream, with a special msid-id value of "-" + // representing no streams. The value of "msid-appdata" MUST be identical for + // all lines. if (msid_signaling & cricket::kMsidSignalingMediaSection) { const StreamParamsVec& streams = media_desc->streams(); if (streams.size() == 1u) { const StreamParams& track = streams[0]; - // TODO(bugs.webrtc.org/7932): Support serializing more than one stream - // label. - const std::string& stream_id = track.first_stream_id(); - InitAttrLine(kAttributeMsid, &os); - os << kSdpDelimiterColon << stream_id << kSdpDelimiterSpace << track.id; - AddLine(os.str(), message); + std::vector stream_ids = track.stream_ids(); + if (stream_ids.empty()) { + stream_ids.push_back(kNoStreamMsid); + } + for (const std::string& stream_id : stream_ids) { + InitAttrLine(kAttributeMsid, &os); + os << kSdpDelimiterColon << stream_id << kSdpDelimiterSpace << track.id; + AddLine(os.str(), message); + } } else if (streams.size() > 1u) { RTC_LOG(LS_WARNING) << "Trying to serialize Unified Plan SDP with more than " @@ -1532,13 +1549,6 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, for (StreamParamsVec::const_iterator track = media_desc->streams().begin(); track != media_desc->streams().end(); ++track) { - // Require that the track belongs to a media stream. This extra check is - // necessary since the MediaContentDescription always contains a - // StreamParams with an ssrc even if no track or media stream have been - // created. - if (track->stream_ids().empty()) - continue; - // Build the ssrc-group lines. for (size_t i = 0; i < track->ssrc_groups.size(); ++i) { // RFC 5576 @@ -1568,8 +1578,9 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, // a=ssrc: msid:identifier [appdata] // The appdata consists of the "id" attribute of a MediaStreamTrack, // which corresponds to the "id" attribute of StreamParams. - // TODO(bugs.webrtc.org/7932): Support serializing more than one stream - // label. + // Since a=ssrc msid signaling is used in Plan B SDP semantics, and + // multiple stream ids are not supported for Plan B, we are only adding + // a line for the first media stream id here. const std::string& stream_id = track->first_stream_id(); InitAttrLine(kAttributeSsrc, &os); os << kSdpDelimiterColon << ssrc << kSdpDelimiterSpace @@ -1583,8 +1594,7 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, // a=ssrc: mslabel: // The label isn't yet defined. // a=ssrc: label: - AddSsrcLine(ssrc, kSsrcAttributeMslabel, track->first_stream_id(), - message); + AddSsrcLine(ssrc, kSsrcAttributeMslabel, stream_id, message); AddSsrcLine(ssrc, kSSrcAttributeLabel, track->id, message); } } @@ -2197,32 +2207,45 @@ static bool ParseDtlsSetup(const std::string& line, } static bool ParseMsidAttribute(const std::string& line, - std::string* stream_id, + std::vector* stream_ids, std::string* track_id, SdpParseError* error) { - // draft-ietf-mmusic-msid-11 + // https://datatracker.ietf.org/doc/draft-ietf-mmusic-msid/16/ // a=msid: // msid-value = msid-id [ SP msid-appdata ] // msid-id = 1*64token-char ; see RFC 4566 // msid-appdata = 1*64token-char ; see RFC 4566 std::string field1; + std::string new_stream_id; + std::string new_track_id; if (!rtc::tokenize_first(line.substr(kLinePrefixLength), kSdpDelimiterSpace, - &field1, track_id)) { + &field1, &new_track_id)) { const size_t expected_fields = 2; return ParseFailedExpectFieldNum(line, expected_fields, error); } - if (track_id->empty()) { + if (new_track_id.empty()) { return ParseFailed(line, "Missing track ID in msid attribute.", error); } + // All track ids should be the same within an m section in a Unified Plan SDP. + if (!track_id->empty() && new_track_id.compare(*track_id) != 0) { + return ParseFailed( + line, "Two different track IDs in msid attribute in one m= section", + error); + } + *track_id = new_track_id; // msid: - if (!GetValue(field1, kAttributeMsid, stream_id, error)) { + if (!GetValue(field1, kAttributeMsid, &new_stream_id, error)) { return false; } - if (stream_id->empty()) { + if (new_stream_id.empty()) { return ParseFailed(line, "Missing stream ID in msid attribute.", error); } + // The special value "-" indicates "no MediaStream". + if (new_stream_id.compare(kNoStreamMsid) != 0) { + stream_ids->push_back(new_stream_id); + } return true; } @@ -2706,7 +2729,7 @@ bool ParseContent(const std::string& message, SsrcGroupVec ssrc_groups; std::string maxptime_as_string; std::string ptime_as_string; - std::string stream_id; + std::vector stream_ids; std::string track_id; // Loop until the next m line @@ -2906,7 +2929,7 @@ bool ParseContent(const std::string& message, if (flag_value.compare(kValueConference) == 0) media_desc->set_conference_mode(true); } else if (HasAttribute(line, kAttributeMsid)) { - if (!ParseMsidAttribute(line, &stream_id, &track_id, error)) { + if (!ParseMsidAttribute(line, &stream_ids, &track_id, error)) { return false; } *msid_signaling |= cricket::kMsidSignalingMediaSection; @@ -2922,7 +2945,8 @@ bool ParseContent(const std::string& message, // If the stream_id/track_id for all SSRCS are identical, one StreamParams // will be created in CreateTracksFromSsrcInfos, containing all the SSRCs from // the m= section. - CreateTracksFromSsrcInfos(ssrc_infos, stream_id, track_id, &tracks); + CreateTracksFromSsrcInfos(ssrc_infos, stream_ids, track_id, &tracks, + *msid_signaling); // Add the ssrc group to the track. for (SsrcGroupVec::iterator ssrc_group = ssrc_groups.begin(); @@ -3038,7 +3062,7 @@ bool ParseSsrcAttribute(const std::string& line, ssrc_info->cname = value; } else if (attribute == kSsrcAttributeMsid) { // draft-alvestrand-mmusic-msid-00 - // "msid:" identifier [ " " appdata ] + // msid:identifier [appdata] std::vector fields; rtc::split(value, kSdpDelimiterSpace, &fields); if (fields.size() < 1 || fields.size() > 2) { diff --git a/pc/webrtcsdp_unittest.cc b/pc/webrtcsdp_unittest.cc index 918dc30023..9d6cfa3005 100644 --- a/pc/webrtcsdp_unittest.cc +++ b/pc/webrtcsdp_unittest.cc @@ -480,91 +480,6 @@ static const char kPlanBSdpFullString[] = "a=ssrc:6 mslabel:local_stream_2\r\n" "a=ssrc:6 label:video_track_id_3\r\n"; -// Plan B SDP reference string, with 2 streams, 2 audio tracks and 3 video -// tracks, but with the unified plan "a=msid" attribute. -static const char kPlanBSdpFullStringWithMsid[] = - "v=0\r\n" - "o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n" - "s=-\r\n" - "t=0 0\r\n" - "a=msid-semantic: WMS local_stream_1 local_stream_2\r\n" - "m=audio 2345 RTP/SAVPF 111 103 104\r\n" - "c=IN IP4 74.125.127.126\r\n" - "a=rtcp:2347 IN IP4 74.125.127.126\r\n" - "a=candidate:a0+B/1 1 udp 2130706432 192.168.1.5 1234 typ host " - "generation 2\r\n" - "a=candidate:a0+B/1 2 udp 2130706432 192.168.1.5 1235 typ host " - "generation 2\r\n" - "a=candidate:a0+B/2 1 udp 2130706432 ::1 1238 typ host " - "generation 2\r\n" - "a=candidate:a0+B/2 2 udp 2130706432 ::1 1239 typ host " - "generation 2\r\n" - "a=candidate:a0+B/3 1 udp 2130706432 74.125.127.126 2345 typ srflx " - "raddr 192.168.1.5 rport 2346 " - "generation 2\r\n" - "a=candidate:a0+B/3 2 udp 2130706432 74.125.127.126 2347 typ srflx " - "raddr 192.168.1.5 rport 2348 " - "generation 2\r\n" - "a=ice-ufrag:ufrag_voice\r\na=ice-pwd:pwd_voice\r\n" - "a=mid:audio_content_name\r\n" - "a=msid:local_stream_1 audio_track_id_1\r\n" - "a=sendrecv\r\n" - "a=rtcp-mux\r\n" - "a=rtcp-rsize\r\n" - "a=crypto:1 AES_CM_128_HMAC_SHA1_32 " - "inline:NzB4d1BINUAvLEw6UzF3WSJ+PSdFcGdUJShpX1Zj|2^20|1:32 " - "dummy_session_params\r\n" - "a=rtpmap:111 opus/48000/2\r\n" - "a=rtpmap:103 ISAC/16000\r\n" - "a=rtpmap:104 ISAC/32000\r\n" - "a=ssrc:1 cname:stream_1_cname\r\n" - "a=ssrc:1 msid:local_stream_1 audio_track_id_1\r\n" - "a=ssrc:1 mslabel:local_stream_1\r\n" - "a=ssrc:1 label:audio_track_id_1\r\n" - "a=ssrc:4 cname:stream_2_cname\r\n" - "a=ssrc:4 msid:local_stream_2 audio_track_id_2\r\n" - "a=ssrc:4 mslabel:local_stream_2\r\n" - "a=ssrc:4 label:audio_track_id_2\r\n" - "m=video 3457 RTP/SAVPF 120\r\n" - "c=IN IP4 74.125.224.39\r\n" - "a=rtcp:3456 IN IP4 74.125.224.39\r\n" - "a=candidate:a0+B/1 2 udp 2130706432 192.168.1.5 1236 typ host " - "generation 2\r\n" - "a=candidate:a0+B/1 1 udp 2130706432 192.168.1.5 1237 typ host " - "generation 2\r\n" - "a=candidate:a0+B/2 2 udp 2130706432 ::1 1240 typ host " - "generation 2\r\n" - "a=candidate:a0+B/2 1 udp 2130706432 ::1 1241 typ host " - "generation 2\r\n" - "a=candidate:a0+B/4 2 udp 2130706432 74.125.224.39 3456 typ relay " - "generation 2\r\n" - "a=candidate:a0+B/4 1 udp 2130706432 74.125.224.39 3457 typ relay " - "generation 2\r\n" - "a=ice-ufrag:ufrag_video\r\na=ice-pwd:pwd_video\r\n" - "a=mid:video_content_name\r\n" - "a=msid:local_stream_1 video_track_id_1\r\n" - "a=sendrecv\r\n" - "a=crypto:1 AES_CM_128_HMAC_SHA1_80 " - "inline:d0RmdmcmVCspeEc3QGZiNWpVLFJhQX1cfHAwJSoj|2^20|1:32\r\n" - "a=rtpmap:120 VP8/90000\r\n" - "a=ssrc-group:FEC 2 3\r\n" - "a=ssrc:2 cname:stream_1_cname\r\n" - "a=ssrc:2 msid:local_stream_1 video_track_id_1\r\n" - "a=ssrc:2 mslabel:local_stream_1\r\n" - "a=ssrc:2 label:video_track_id_1\r\n" - "a=ssrc:3 cname:stream_1_cname\r\n" - "a=ssrc:3 msid:local_stream_1 video_track_id_1\r\n" - "a=ssrc:3 mslabel:local_stream_1\r\n" - "a=ssrc:3 label:video_track_id_1\r\n" - "a=ssrc:5 cname:stream_2_cname\r\n" - "a=ssrc:5 msid:local_stream_2 video_track_id_2\r\n" - "a=ssrc:5 mslabel:local_stream_2\r\n" - "a=ssrc:5 label:video_track_id_2\r\n" - "a=ssrc:6 cname:stream_2_cname\r\n" - "a=ssrc:6 msid:local_stream_2 video_track_id_3\r\n" - "a=ssrc:6 mslabel:local_stream_2\r\n" - "a=ssrc:6 label:video_track_id_3\r\n"; - // Unified Plan SDP reference string, with 2 streams, 2 audio tracks and 3 video // tracks. static const char kUnifiedPlanSdpFullString[] = @@ -672,6 +587,94 @@ static const char kUnifiedPlanSdpFullString[] = "a=rtpmap:120 VP8/90000\r\n" "a=ssrc:6 cname:stream_2_cname\r\n"; +// Unified Plan SDP reference string: +// - audio track 1 has 1 a=msid lines +// - audio track 2 has 2 a=msid lines +// - audio track 3 has 1 a=msid line with the special "-" marker signifying that +// there are 0 media stream ids. +// This Unified Plan SDP represents a SDP that signals the msid using both +// a=msid and a=ssrc msid semantics. +static const char kUnifiedPlanSdpFullStringWithSpecialMsid[] = + "v=0\r\n" + "o=- 18446744069414584320 18446462598732840960 IN IP4 127.0.0.1\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=msid-semantic: WMS local_stream_1\r\n" + // Audio track 1, with 1 stream id. + "m=audio 2345 RTP/SAVPF 111 103 104\r\n" + "c=IN IP4 74.125.127.126\r\n" + "a=rtcp:2347 IN IP4 74.125.127.126\r\n" + "a=candidate:a0+B/1 1 udp 2130706432 192.168.1.5 1234 typ host " + "generation 2\r\n" + "a=candidate:a0+B/1 2 udp 2130706432 192.168.1.5 1235 typ host " + "generation 2\r\n" + "a=candidate:a0+B/2 1 udp 2130706432 ::1 1238 typ host " + "generation 2\r\n" + "a=candidate:a0+B/2 2 udp 2130706432 ::1 1239 typ host " + "generation 2\r\n" + "a=candidate:a0+B/3 1 udp 2130706432 74.125.127.126 2345 typ srflx " + "raddr 192.168.1.5 rport 2346 " + "generation 2\r\n" + "a=candidate:a0+B/3 2 udp 2130706432 74.125.127.126 2347 typ srflx " + "raddr 192.168.1.5 rport 2348 " + "generation 2\r\n" + "a=ice-ufrag:ufrag_voice\r\na=ice-pwd:pwd_voice\r\n" + "a=mid:audio_content_name\r\n" + "a=msid:local_stream_1 audio_track_id_1\r\n" + "a=sendrecv\r\n" + "a=rtcp-mux\r\n" + "a=rtcp-rsize\r\n" + "a=crypto:1 AES_CM_128_HMAC_SHA1_32 " + "inline:NzB4d1BINUAvLEw6UzF3WSJ+PSdFcGdUJShpX1Zj|2^20|1:32 " + "dummy_session_params\r\n" + "a=rtpmap:111 opus/48000/2\r\n" + "a=rtpmap:103 ISAC/16000\r\n" + "a=rtpmap:104 ISAC/32000\r\n" + "a=ssrc:1 cname:stream_1_cname\r\n" + "a=ssrc:1 msid:local_stream_1 audio_track_id_1\r\n" + "a=ssrc:1 mslabel:local_stream_1\r\n" + "a=ssrc:1 label:audio_track_id_1\r\n" + // Audio track 2, with two stream ids. + "m=audio 9 RTP/SAVPF 111 103 104\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp:9 IN IP4 0.0.0.0\r\n" + "a=ice-ufrag:ufrag_voice_2\r\na=ice-pwd:pwd_voice_2\r\n" + "a=mid:audio_content_name_2\r\n" + "a=msid:local_stream_1 audio_track_id_2\r\n" + "a=msid:local_stream_2 audio_track_id_2\r\n" + "a=sendrecv\r\n" + "a=rtcp-mux\r\n" + "a=rtcp-rsize\r\n" + "a=crypto:1 AES_CM_128_HMAC_SHA1_32 " + "inline:NzB4d1BINUAvLEw6UzF3WSJ+PSdFcGdUJShpX1Zj|2^20|1:32 " + "dummy_session_params\r\n" + "a=rtpmap:111 opus/48000/2\r\n" + "a=rtpmap:103 ISAC/16000\r\n" + "a=rtpmap:104 ISAC/32000\r\n" + "a=ssrc:4 cname:stream_1_cname\r\n" + // The support for Plan B msid signaling only includes the + // first media stream id "local_stream_1." + "a=ssrc:4 msid:local_stream_1 audio_track_id_2\r\n" + "a=ssrc:4 mslabel:local_stream_1\r\n" + "a=ssrc:4 label:audio_track_id_2\r\n" + // Audio track 3, with no stream ids. + "m=audio 9 RTP/SAVPF 111 103 104\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp:9 IN IP4 0.0.0.0\r\n" + "a=ice-ufrag:ufrag_voice_3\r\na=ice-pwd:pwd_voice_3\r\n" + "a=mid:audio_content_name_3\r\n" + "a=msid:- audio_track_id_3\r\n" + "a=sendrecv\r\n" + "a=rtcp-mux\r\n" + "a=rtcp-rsize\r\n" + "a=crypto:1 AES_CM_128_HMAC_SHA1_32 " + "inline:NzB4d1BINUAvLEw6UzF3WSJ+PSdFcGdUJShpX1Zj|2^20|1:32 " + "dummy_session_params\r\n" + "a=rtpmap:111 opus/48000/2\r\n" + "a=rtpmap:103 ISAC/16000\r\n" + "a=rtpmap:104 ISAC/32000\r\n" + "a=ssrc:7 cname:stream_2_cname\r\n"; + // One candidate reference string as per W3c spec. // candidate: not a=candidate:CRLF static const char kRawCandidate[] = @@ -724,6 +727,8 @@ static const char kPwdData[] = "pwd_data"; // Extra ufrags/passwords for extra unified plan m= sections. static const char kUfragVoice2[] = "ufrag_voice_2"; static const char kPwdVoice2[] = "pwd_voice_2"; +static const char kUfragVoice3[] = "ufrag_voice_3"; +static const char kPwdVoice3[] = "pwd_voice_3"; static const char kUfragVideo2[] = "ufrag_video_2"; static const char kPwdVideo2[] = "pwd_video_2"; static const char kUfragVideo3[] = "ufrag_video_3"; @@ -736,6 +741,7 @@ static const char kDataContentName[] = "data_content_name"; // Extra content names for extra unified plan m= sections. static const char kAudioContentName2[] = "audio_content_name_2"; +static const char kAudioContentName3[] = "audio_content_name_3"; static const char kVideoContentName2[] = "video_content_name_2"; static const char kVideoContentName3[] = "video_content_name_3"; @@ -757,6 +763,8 @@ static const char kVideoTrackId2[] = "video_track_id_2"; static const uint32_t kVideoTrack2Ssrc = 5; static const char kVideoTrackId3[] = "video_track_id_3"; static const uint32_t kVideoTrack3Ssrc = 6; +static const char kAudioTrackId3[] = "audio_track_id_3"; +static const uint32_t kAudioTrack3Ssrc = 7; // DataChannel static const char kDataChannelLabel[] = "data_channel"; @@ -1010,12 +1018,7 @@ class WebRtcSdpTest : public testing::Test { } } - // Turns the existing reference description into a description using - // a=bundle-only. This means no transport attributes and a 0 port value on - // the m= sections not associated with the BUNDLE-tag. - void MakeBundleOnlyDescription() { - // Remove video candidates. JsepSessionDescription doesn't make it - // simple. + void RemoveVideoCandidates() { const IceCandidateCollection* video_candidates_collection = jdesc_.candidates(1); ASSERT_NE(nullptr, video_candidates_collection); @@ -1026,6 +1029,13 @@ class WebRtcSdpTest : public testing::Test { video_candidates.push_back(c); } jdesc_.RemoveCandidates(video_candidates); + } + + // Turns the existing reference description into a description using + // a=bundle-only. This means no transport attributes and a 0 port value on + // the m= sections not associated with the BUNDLE-tag. + void MakeBundleOnlyDescription() { + RemoveVideoCandidates(); // And the rest of the transport attributes. desc_.transport_infos()[1].description.ice_ufrag.clear(); @@ -1142,6 +1152,45 @@ class WebRtcSdpTest : public testing::Test { return audio; } + // Turns the existing reference description into a unified plan description, + // with 3 audio MediaContentDescriptions with special StreamParams that + // contain 0 or multiple stream ids: - audio track 1 has 1 media stream id - + // audio track 2 has 2 media stream ids - audio track 3 has 0 media stream ids + void MakeUnifiedPlanDescriptionMultipleStreamIds() { + desc_.RemoveContentByName(kVideoContentName); + desc_.RemoveTransportInfoByName(kVideoContentName); + RemoveVideoCandidates(); + + // Audio track 2 has 2 media stream ids. + AudioContentDescription* audio_desc_2 = CreateAudioContentDescription(); + StreamParams audio_track_2; + audio_track_2.id = kAudioTrackId2; + audio_track_2.cname = kStream1Cname; + audio_track_2.set_stream_ids({kStreamId1, kStreamId2}); + audio_track_2.ssrcs.push_back(kAudioTrack2Ssrc); + audio_desc_2->AddStream(audio_track_2); + desc_.AddContent(kAudioContentName2, MediaProtocolType::kRtp, audio_desc_2); + EXPECT_TRUE(desc_.AddTransportInfo(TransportInfo( + kAudioContentName2, TransportDescription(kUfragVoice2, kPwdVoice2)))); + + // Audio track 3 has no stream ids. + AudioContentDescription* audio_desc_3 = CreateAudioContentDescription(); + StreamParams audio_track_3; + audio_track_3.id = kAudioTrackId3; + audio_track_3.cname = kStream2Cname; + audio_track_3.set_stream_ids({}); + audio_track_3.ssrcs.push_back(kAudioTrack3Ssrc); + audio_desc_3->AddStream(audio_track_3); + desc_.AddContent(kAudioContentName3, MediaProtocolType::kRtp, audio_desc_3); + EXPECT_TRUE(desc_.AddTransportInfo(TransportInfo( + kAudioContentName3, TransportDescription(kUfragVoice3, kPwdVoice3)))); + // Make sure to create both a=msid lines. + desc_.set_msid_signaling(cricket::kMsidSignalingMediaSection | + cricket::kMsidSignalingSsrcAttribute); + ASSERT_TRUE(jdesc_.Initialize(desc_.Copy(), jdesc_.session_id(), + jdesc_.session_version())); + } + // Creates a video content description with no streams, and some default // configuration. VideoContentDescription* CreateVideoContentDescription() { @@ -3281,20 +3330,6 @@ TEST_F(WebRtcSdpTest, SerializePlanBSessionDescription) { TestSerialize(jdesc_); } -// Some WebRTC endpoints include the msid in both the Plan B and Unified Plan -// ways, to make SDP that's compatible with both Plan B and Unified Plan (to -// some extent). If we parse this, the Plan B msid attribute (which is more -// specific, since it's at the SSRC level) should take priority. -TEST_F(WebRtcSdpTest, DeserializePlanBSessionDescriptionWithMsid) { - MakePlanBDescription(); - - JsepSessionDescription deserialized_description(kDummyType); - EXPECT_TRUE( - SdpDeserialize(kPlanBSdpFullStringWithMsid, &deserialized_description)); - - EXPECT_TRUE(CompareSessionDescription(jdesc_, deserialized_description)); -} - TEST_F(WebRtcSdpTest, DeserializeUnifiedPlanSessionDescription) { MakeUnifiedPlanDescription(); @@ -3310,6 +3345,26 @@ TEST_F(WebRtcSdpTest, SerializeUnifiedPlanSessionDescription) { TestSerialize(jdesc_); } +// This tests deserializing a Unified Plan SDP that is compatible with both +// Unified Plan and Plan B style SDP. It tests the case for audio/video tracks +// with no stream ids and multiple stream ids. For parsing this, the Unified +// Plan a=msid lines should take priority, because the Plan B style a=ssrc msid +// lines do not support multiple stream ids and no stream ids. +TEST_F(WebRtcSdpTest, DeserializeUnifiedPlanSessionDescriptionSpecialMsid) { + MakeUnifiedPlanDescriptionMultipleStreamIds(); + + JsepSessionDescription deserialized_description(kDummyType); + EXPECT_TRUE(SdpDeserialize(kUnifiedPlanSdpFullStringWithSpecialMsid, + &deserialized_description)); + + EXPECT_TRUE(CompareSessionDescription(jdesc_, deserialized_description)); +} + +TEST_F(WebRtcSdpTest, SerializeUnifiedPlanSessionDescriptionSpecialMsid) { + MakeUnifiedPlanDescriptionMultipleStreamIds(); + TestSerialize(jdesc_); +} + TEST_F(WebRtcSdpTest, EmptyDescriptionHasNoMsidSignaling) { JsepSessionDescription jsep_desc(kDummyType); ASSERT_TRUE(SdpDeserialize(kSdpSessionString, &jsep_desc));