diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index 6d60b4c36f..cf9a1f564c 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -1152,6 +1152,13 @@ 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 01279a55a0..2ca2edc830 100644 --- a/api/rtpsenderinterface.h +++ b/api/rtpsenderinterface.h @@ -48,9 +48,8 @@ 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 media stream ids associated with this sender's track. - // These are signalled in the SDP so that the remote side can associate - // tracks. + // 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. virtual std::vector stream_ids() const = 0; virtual RtpParameters GetParameters() const = 0; diff --git a/api/rtptransceiverinterface.h b/api/rtptransceiverinterface.h index c9a86e6c63..3ea75fd027 100644 --- a/api/rtptransceiverinterface.h +++ b/api/rtptransceiverinterface.h @@ -40,6 +40,9 @@ 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 2efa0d0976..421111acba 100644 --- a/media/base/streamparams.cc +++ b/media/base/streamparams.cc @@ -141,15 +141,9 @@ std::string StreamParams::ToString() const { if (!cname.empty()) { ost << "cname:" << cname << ";"; } - 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; + if (!sync_label.empty()) { + ost << "sync_label:" << sync_label; } - ost << ";"; ost << "}"; return ost.str(); } @@ -206,15 +200,22 @@ bool StreamParams::GetSecondarySsrc(const std::string& semantics, } std::vector StreamParams::stream_ids() const { - return stream_ids_; + if (sync_label.empty()) { + return {}; + } + return {sync_label}; } void StreamParams::set_stream_ids(const std::vector& stream_ids) { - stream_ids_ = 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]); } std::string StreamParams::first_stream_id() const { - return stream_ids_.empty() ? "" : stream_ids_[0]; + return sync_label; } bool IsOneSsrcStream(const StreamParams& sp) { diff --git a/media/base/streamparams.h b/media/base/streamparams.h index 22b98c7427..016c27ad0e 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 && stream_ids_ == other.stream_ids_); + cname == other.cname && sync_label == other.sync_label); } bool operator!=(const StreamParams &other) const { return !(*this == other); @@ -165,6 +165,8 @@ 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, @@ -173,8 +175,6 @@ 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 db3718075f..6e934ae7d6 100644 --- a/media/base/streamparams_unittest.cc +++ b/media/base/streamparams_unittest.cc @@ -208,11 +208,8 @@ TEST(StreamParams, FecFrFunctions) { TEST(StreamParams, ToString) { cricket::StreamParams sp = CreateStreamParamsWithSsrcGroup("XYZ", kSsrcs2, arraysize(kSsrcs2)); - 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()); + EXPECT_STREQ("{ssrcs:[1,2];ssrc_groups:{semantics:XYZ;ssrcs:[1,2]};}", + sp.ToString().c_str()); } TEST(StreamParams, TestIsOneSsrcStream_LegacyStream) { diff --git a/pc/mediasession.cc b/pc/mediasession.cc index 474c0b4e45..8a9c1f802b 100644 --- a/pc/mediasession.cc +++ b/pc/mediasession.cc @@ -476,6 +476,8 @@ 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); @@ -486,6 +488,8 @@ 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 23ad4234c8..eacfb8329b 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -1143,6 +1143,12 @@ 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."); @@ -1152,9 +1158,18 @@ 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, stream_ids) - : AddTrackPlanB(track, stream_ids)); + (IsUnifiedPlan() ? AddTrackUnifiedPlan(track, adjusted_stream_ids) + : AddTrackPlanB(track, adjusted_stream_ids)); if (sender_or_error.ok()) { observer_->OnRenegotiationNeeded(); stats_->AddTrack(track); @@ -1166,26 +1181,17 @@ 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, adjusted_stream_ids); + auto new_sender = CreateSender(media_type, track, 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_ids()[0], track->id()); + new_sender->internal()->stream_id(), track->id()); if (sender_info) { new_sender->internal()->SetSsrc(sender_info->first_ssrc); } @@ -1195,7 +1201,7 @@ PeerConnection::AddTrackPlanB( GetVideoTransceiver()->internal()->AddSender(new_sender); const RtpSenderInfo* sender_info = FindSenderInfo(local_video_sender_infos_, - new_sender->internal()->stream_ids()[0], track->id()); + new_sender->internal()->stream_id(), track->id()); if (sender_info) { new_sender->internal()->SetSsrc(sender_info->first_ssrc); } @@ -1366,7 +1372,15 @@ PeerConnection::AddTransceiver( // TODO(bugs.webrtc.org/7600): Verify init. - auto sender = CreateSender(media_type, track, init.stream_ids); + 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 receiver = CreateReceiver(media_type, rtc::CreateRandomUuid()); auto transceiver = CreateAndAddTransceiver(sender, receiver); transceiver->internal()->set_direction(init.direction); @@ -1473,12 +1487,8 @@ 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()) { - stream_ids.push_back(rtc::CreateRandomUuid()); - } else { + if (!stream_id.empty()) { stream_ids.push_back(stream_id); } @@ -2338,33 +2348,28 @@ 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. - std::vector stream_ids; + // + // Create a sync label in the case of an unsignalled msid. + std::string sync_label = rtc::CreateRandomUuid(); 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()) { - stream_ids = stream_params.stream_ids(); + sync_label = stream_params.stream_ids()[0]; } } if (RtpTransceiverDirectionHasRecv(local_direction) && (!transceiver->current_direction() || !RtpTransceiverDirectionHasRecv( *transceiver->current_direction()))) { - 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); + 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); } - transceiver->internal()->receiver_internal()->SetStreams(media_streams); + transceiver->internal()->receiver_internal()->SetStreams({stream}); receiving_transceivers.push_back(transceiver); } // If direction is sendonly or inactive, and transceiver's current @@ -3303,7 +3308,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_ids({stream->id()}); + sender->internal()->set_stream_id(stream->id()); return; } @@ -3346,7 +3351,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_ids({stream->id()}); + sender->internal()->set_stream_id(stream->id()); return; } @@ -3977,8 +3982,6 @@ void PeerConnection::UpdateRemoteSendersList( bool default_sender_needed, cricket::MediaType media_type, StreamCollection* new_streams) { - RTC_DCHECK(!IsUnifiedPlan()); - std::vector* current_senders = GetRemoteSenderInfos(media_type); @@ -3990,8 +3993,7 @@ 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 && - params->first_stream_id() == info.stream_id; + bool sender_exists = params && params->id == info.sender_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) { @@ -4005,13 +4007,10 @@ 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|. 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. + // |params.stream_ids|. + // TODO(bugs.webrtc.org/7932): Add support for multiple stream ids. const std::string& stream_id = - (!params.stream_ids().empty() ? params.stream_ids()[0] - : kDefaultStreamId); + (!params.stream_ids().empty() ? params.stream_ids()[0] : ""); const std::string& sender_id = params.id; uint32_t ssrc = params.first_ssrc(); @@ -4158,7 +4157,6 @@ 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 " @@ -4173,7 +4171,7 @@ void PeerConnection::OnLocalSenderAdded(const RtpSenderInfo& sender_info, return; } - sender->internal()->set_stream_ids({sender_info.stream_id}); + sender->internal()->set_stream_id(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 caa0ea0587..84bd060d6f 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())); - 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()); + 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. } // Test that setting a remote offer with one track that has one stream fires off @@ -1226,27 +1226,7 @@ TEST_F(PeerConnectionJsepTest, UnorderedElementsAre(track1, track2)); } -// 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()); -} +// TODO(bugs.webrtc.org/7932): Also test multi-stream case. // 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 bcfe0d8652..69f329cc1e 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -17,7 +17,6 @@ #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" @@ -116,8 +115,8 @@ TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithoutStreamFiresOnAddTrack) { static_cast(nullptr))); ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); - // 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. + // TODO(hbos): When "no stream" is handled correctly we would expect + // |add_track_events_[0].streams| to be empty. https://crbug.com/webrtc/7933 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")); @@ -220,54 +219,15 @@ 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. One transceiver is -// created without any stream_ids, while the other is created with multiple -// stream_ids. +// with receive only transceivers will not call OnTrack. 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); - RtpTransceiverInit video_transceiver_init; - video_transceiver_init.stream_ids = {kStreamId1, kStreamId2}; - auto video_transceiver = - caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO, video_transceiver_init); + auto video_transceiver = caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO); ASSERT_TRUE(caller->ExchangeOfferAnswerWith(callee.get())); @@ -277,14 +237,6 @@ 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 @@ -383,8 +335,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()); - // 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. + // TODO(hbos): When "no stream" is handled correctly we would expect + // |receiver_added->streams()| to be empty. https://crbug.com/webrtc/7933 EXPECT_EQ(receiver_added->streams().size(), 1u); EXPECT_TRUE(receiver_added->streams()[0]->FindAudioTrack("audio_track")); } @@ -553,74 +505,6 @@ 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 {}; @@ -1184,39 +1068,6 @@ 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 b457150581..62fd3bfbb5 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -1628,16 +1628,9 @@ 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 (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()); - } + // 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()); } // Test that we can call GetStats() after AddTrack but before connecting @@ -3059,33 +3052,6 @@ 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 626c850505..170aa9c507 100644 --- a/pc/rtpsender.cc +++ b/pc/rtpsender.cc @@ -79,6 +79,9 @@ 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()); @@ -315,6 +318,9 @@ 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 27b1513379..05e11313a5 100644 --- a/pc/rtpsender.h +++ b/pc/rtpsender.h @@ -50,6 +50,12 @@ 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; @@ -136,6 +142,10 @@ 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; } @@ -167,6 +177,8 @@ 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_; @@ -224,6 +236,10 @@ 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; } @@ -250,6 +266,8 @@ 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 5988c7b3c1..b76a9ac0ba 100644 --- a/pc/test/mock_rtpsenderinternal.h +++ b/pc/test/mock_rtpsenderinternal.h @@ -37,6 +37,8 @@ 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 a0887a6f78..80ac8d58d6 100644 --- a/pc/webrtcsdp.cc +++ b/pc/webrtcsdp.cc @@ -120,12 +120,10 @@ 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"; @@ -327,7 +325,7 @@ static bool ParseDtlsSetup(const std::string& line, cricket::ConnectionRole* role, SdpParseError* error); static bool ParseMsidAttribute(const std::string& line, - std::vector* stream_ids, + std::string* stream_id, std::string* track_id, SdpParseError* error); @@ -580,50 +578,43 @@ static bool GetPayloadTypeFromString(const std::string& line, cricket::IsValidRtpPayloadType(*payload_type); } -// |msid_stream_idss| and |msid_track_id| represent the stream/track ID from the +// |msid_stream_id| 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::vector& msid_stream_ids, + const std::string& msid_stream_id, const std::string& msid_track_id, - StreamParamsVec* tracks, - int msid_signaling) { + StreamParamsVec* tracks) { 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::vector stream_ids; + std::string stream_id; std::string track_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 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. + 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_ids.push_back(ssrc_info->mslabel); + 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; + track_id = msid_track_id; } else { - // Since no media streams isn't supported with older SDP signaling, we - // use a default a stream id. - stream_ids.push_back(kDefaultMsid); + stream_id = ssrc_info->stream_id; + track_id = ssrc_info->track_id; } - // If a track ID wasn't populated from the SSRC attributes OR the + // If a stream/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)? @@ -643,7 +634,7 @@ void CreateTracksFromSsrcInfos(const SsrcInfoVec& ssrc_infos, } track->add_ssrc(ssrc_info->ssrc_id); track->cname = ssrc_info->cname; - track->set_stream_ids(stream_ids); + track->set_stream_ids({stream_id}); track->id = track_id; } } @@ -1481,26 +1472,18 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, } AddLine(os.str(), message); - // 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. + // Specified in https://tools.ietf.org/html/draft-ietf-mmusic-msid-16 + // a=msid: if (msid_signaling & cricket::kMsidSignalingMediaSection) { const StreamParamsVec& streams = media_desc->streams(); if (streams.size() == 1u) { const StreamParams& track = streams[0]; - 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); - } + // 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); } else if (streams.size() > 1u) { RTC_LOG(LS_WARNING) << "Trying to serialize Unified Plan SDP with more than " @@ -1549,6 +1532,13 @@ 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 @@ -1578,9 +1568,8 @@ 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. - // 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. + // TODO(bugs.webrtc.org/7932): Support serializing more than one stream + // label. const std::string& stream_id = track->first_stream_id(); InitAttrLine(kAttributeSsrc, &os); os << kSdpDelimiterColon << ssrc << kSdpDelimiterSpace @@ -1594,7 +1583,8 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, // a=ssrc: mslabel: // The label isn't yet defined. // a=ssrc: label: - AddSsrcLine(ssrc, kSsrcAttributeMslabel, stream_id, message); + AddSsrcLine(ssrc, kSsrcAttributeMslabel, track->first_stream_id(), + message); AddSsrcLine(ssrc, kSSrcAttributeLabel, track->id, message); } } @@ -2207,45 +2197,32 @@ static bool ParseDtlsSetup(const std::string& line, } static bool ParseMsidAttribute(const std::string& line, - std::vector* stream_ids, + std::string* stream_id, std::string* track_id, SdpParseError* error) { - // https://datatracker.ietf.org/doc/draft-ietf-mmusic-msid/16/ + // draft-ietf-mmusic-msid-11 // 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, &new_track_id)) { + &field1, track_id)) { const size_t expected_fields = 2; return ParseFailedExpectFieldNum(line, expected_fields, error); } - if (new_track_id.empty()) { + if (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, &new_stream_id, error)) { + if (!GetValue(field1, kAttributeMsid, stream_id, error)) { return false; } - if (new_stream_id.empty()) { + if (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; } @@ -2729,7 +2706,7 @@ bool ParseContent(const std::string& message, SsrcGroupVec ssrc_groups; std::string maxptime_as_string; std::string ptime_as_string; - std::vector stream_ids; + std::string stream_id; std::string track_id; // Loop until the next m line @@ -2929,7 +2906,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_ids, &track_id, error)) { + if (!ParseMsidAttribute(line, &stream_id, &track_id, error)) { return false; } *msid_signaling |= cricket::kMsidSignalingMediaSection; @@ -2945,8 +2922,7 @@ 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_ids, track_id, &tracks, - *msid_signaling); + CreateTracksFromSsrcInfos(ssrc_infos, stream_id, track_id, &tracks); // Add the ssrc group to the track. for (SsrcGroupVec::iterator ssrc_group = ssrc_groups.begin(); @@ -3062,7 +3038,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 9d6cfa3005..918dc30023 100644 --- a/pc/webrtcsdp_unittest.cc +++ b/pc/webrtcsdp_unittest.cc @@ -480,6 +480,91 @@ 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[] = @@ -587,94 +672,6 @@ 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[] = @@ -727,8 +724,6 @@ 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"; @@ -741,7 +736,6 @@ 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"; @@ -763,8 +757,6 @@ 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"; @@ -1018,7 +1010,12 @@ class WebRtcSdpTest : public testing::Test { } } - void RemoveVideoCandidates() { + // 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. const IceCandidateCollection* video_candidates_collection = jdesc_.candidates(1); ASSERT_NE(nullptr, video_candidates_collection); @@ -1029,13 +1026,6 @@ 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(); @@ -1152,45 +1142,6 @@ 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() { @@ -3330,6 +3281,20 @@ 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(); @@ -3345,26 +3310,6 @@ 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));