diff --git a/media/base/mediachannel.h b/media/base/mediachannel.h index d1a2d3af1d..df20582eea 100644 --- a/media/base/mediachannel.h +++ b/media/base/mediachannel.h @@ -203,11 +203,13 @@ class MediaChannel : public sigslot::has_slots<> { // by sp. virtual bool AddSendStream(const StreamParams& sp) = 0; // Removes an outgoing media stream. - // ssrc must be the first SSRC of the media stream if the stream uses - // multiple SSRCs. + // SSRC must be the first SSRC of the media stream if the stream uses + // multiple SSRCs. In the case of an ssrc of 0, the possibly cached + // StreamParams is removed. virtual bool RemoveSendStream(uint32_t ssrc) = 0; - // Creates a new incoming media stream with SSRCs and CNAME as described - // by sp. + // Creates a new incoming media stream with SSRCs, CNAME as described + // by sp. In the case of a sp without SSRCs, the unsignaled sp is cached + // to be used later for unsignaled streams received. virtual bool AddRecvStream(const StreamParams& sp) = 0; // Removes an incoming media stream. // ssrc must be the first SSRC of the media stream if the stream uses diff --git a/media/base/streamparams.h b/media/base/streamparams.h index 52a94415b7..52f1918268 100644 --- a/media/base/streamparams.h +++ b/media/base/streamparams.h @@ -158,6 +158,8 @@ struct StreamParams { std::string groupid; // Unique per-groupid, not across all groupids std::string id; + // There may be no SSRCs stored in unsignaled case when stream_ids are + // signaled with a=msid lines. std::vector ssrcs; // All SSRCs for this source std::vector ssrc_groups; // e.g. FID, FEC, SIM // Examples: "camera", "screencast" @@ -267,6 +269,11 @@ StreamParams* GetStream(StreamParamsVec& streams, Condition condition) { return found == streams.end() ? nullptr : &(*found); } +inline bool HasStreamWithNoSsrcs(const StreamParamsVec& streams) { + return GetStream(streams, + [](const StreamParams& sp) { return !sp.has_ssrcs(); }); +} + inline const StreamParams* GetStreamBySsrc(const StreamParamsVec& streams, uint32_t ssrc) { return GetStream(streams, diff --git a/media/base/streamparams_unittest.cc b/media/base/streamparams_unittest.cc index db3718075f..020953b4cf 100644 --- a/media/base/streamparams_unittest.cc +++ b/media/base/streamparams_unittest.cc @@ -96,6 +96,17 @@ TEST(StreamParams, GetSsrcGroup) { EXPECT_EQ(&sp.ssrc_groups[0], sp.get_ssrc_group("XYZ")); } +TEST(StreamParams, HasStreamWithNoSsrcs) { + cricket::StreamParams sp_1 = cricket::StreamParams::CreateLegacy(kSsrcs1[0]); + cricket::StreamParams sp_2 = cricket::StreamParams::CreateLegacy(kSsrcs2[0]); + std::vector streams({sp_1, sp_2}); + EXPECT_FALSE(HasStreamWithNoSsrcs(streams)); + + cricket::StreamParams unsignaled_stream; + streams.push_back(unsignaled_stream); + EXPECT_TRUE(HasStreamWithNoSsrcs(streams)); +} + TEST(StreamParams, EqualNotEqual) { cricket::StreamParams l1 = cricket::StreamParams::CreateLegacy(1); cricket::StreamParams l2 = cricket::StreamParams::CreateLegacy(2); diff --git a/media/engine/webrtcvideoengine.cc b/media/engine/webrtcvideoengine.cc index 66b77718b0..8de05dafc8 100644 --- a/media/engine/webrtcvideoengine.cc +++ b/media/engine/webrtcvideoengine.cc @@ -507,8 +507,9 @@ UnsignalledSsrcHandler::Action DefaultUnsignalledSsrcHandler::OnUnsignalledSsrc( channel->RemoveRecvStream(*default_recv_ssrc); } - StreamParams sp; + StreamParams sp = channel->unsignaled_stream_params(); sp.ssrcs.push_back(ssrc); + RTC_LOG(LS_INFO) << "Creating default receive stream for SSRC=" << ssrc << "."; if (!channel->AddRecvStream(sp, true)) { @@ -1215,6 +1216,13 @@ bool WebRtcVideoChannel::AddRecvStream(const StreamParams& sp, RTC_LOG(LS_INFO) << "AddRecvStream" << (default_stream ? " (default stream)" : "") << ": " << sp.ToString(); + if (!sp.has_ssrcs()) { + // This is a StreamParam with unsignaled SSRCs. Store it, so it can be used + // later when we know the SSRC on the first packet arrival. + unsignaled_stream_params_ = sp; + return true; + } + if (!ValidateStreamParams(sp)) return false; @@ -1313,8 +1321,10 @@ void WebRtcVideoChannel::ConfigureReceiverRtp( bool WebRtcVideoChannel::RemoveRecvStream(uint32_t ssrc) { RTC_LOG(LS_INFO) << "RemoveRecvStream: " << ssrc; if (ssrc == 0) { - RTC_LOG(LS_ERROR) << "RemoveRecvStream with 0 ssrc is not supported."; - return false; + // This indicates that we need to remove the unsignaled stream parameters + // that are cached. + unsignaled_stream_params_ = StreamParams(); + return true; } rtc::CritScope stream_lock(&stream_crit_); diff --git a/media/engine/webrtcvideoengine.h b/media/engine/webrtcvideoengine.h index 2a03a530ea..fc9e307a08 100644 --- a/media/engine/webrtcvideoengine.h +++ b/media/engine/webrtcvideoengine.h @@ -177,6 +177,8 @@ class WebRtcVideoChannel : public VideoMediaChannel, public webrtc::Transport { rtc::Optional GetDefaultReceiveStreamSsrc(); + StreamParams unsignaled_stream_params() { return unsignaled_stream_params_; } + // AdaptReason is used for expressing why a WebRtcVideoSendStream request // a lower input frame size than the currently configured camera input frame // size. There can be more than one reason OR:ed together. @@ -504,6 +506,11 @@ class WebRtcVideoChannel : public VideoMediaChannel, public webrtc::Transport { VideoOptions default_send_options_; VideoRecvParameters recv_params_; int64_t last_stats_log_ms_; + // This is a stream param that comes from the remote description, but wasn't + // signaled with any a=ssrc lines. It holds information that was signaled + // before the unsignaled receive stream is created when the first packet is + // received. + StreamParams unsignaled_stream_params_; }; class EncoderStreamFactory diff --git a/media/engine/webrtcvideoengine_unittest.cc b/media/engine/webrtcvideoengine_unittest.cc index d0b570725a..ae85fc0c73 100644 --- a/media/engine/webrtcvideoengine_unittest.cc +++ b/media/engine/webrtcvideoengine_unittest.cc @@ -4184,6 +4184,44 @@ TEST_F(WebRtcVideoChannelTest, MapsReceivedPayloadTypeToCodecName) { EXPECT_STREQ("", info.receivers[0].codec_name.c_str()); } +// Tests that when we add a stream without SSRCs, but contains a stream_id +// that it is stored and its stream id is later used when the first packet +// arrives to properly create a receive stream with a sync label. +TEST_F(WebRtcVideoChannelTest, RecvUnsignaledSsrcWithSignaledStreamId) { + const char kSyncLabel[] = "sync_label"; + cricket::StreamParams unsignaled_stream; + unsignaled_stream.set_stream_ids({kSyncLabel}); + ASSERT_TRUE(channel_->AddRecvStream(unsignaled_stream)); + // The stream shouldn't have been created at this point because it doesn't + // have any SSRCs. + EXPECT_EQ(0, fake_call_->GetVideoReceiveStreams().size()); + + // Create and deliver packet. + const size_t kDataLength = 12; + uint8_t data[kDataLength]; + memset(data, 0, sizeof(data)); + rtc::SetBE32(&data[8], kIncomingUnsignalledSsrc); + rtc::CopyOnWriteBuffer packet(data, kDataLength); + rtc::PacketTime packet_time; + channel_->OnPacketReceived(&packet, packet_time); + + // The stream should now be created with the appropriate sync label. + EXPECT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()); + EXPECT_EQ(kSyncLabel, + fake_call_->GetVideoReceiveStreams()[0]->GetConfig().sync_group); + + // Removing the unsignaled stream should clear the cache. This time when + // a default video receive stream is created it won't have a sync_group. + ASSERT_TRUE(channel_->RemoveRecvStream(0)); + ASSERT_TRUE(channel_->RemoveRecvStream(kIncomingUnsignalledSsrc)); + EXPECT_EQ(0u, fake_call_->GetVideoReceiveStreams().size()); + + channel_->OnPacketReceived(&packet, packet_time); + EXPECT_EQ(1u, fake_call_->GetVideoReceiveStreams().size()); + EXPECT_TRUE( + fake_call_->GetVideoReceiveStreams()[0]->GetConfig().sync_group.empty()); +} + void WebRtcVideoChannelTest::TestReceiveUnsignaledSsrcPacket( uint8_t payload_type, bool expect_created_receive_stream) { diff --git a/media/engine/webrtcvoiceengine.cc b/media/engine/webrtcvoiceengine.cc index df50642ea6..bb1c9c3c83 100644 --- a/media/engine/webrtcvoiceengine.cc +++ b/media/engine/webrtcvoiceengine.cc @@ -1842,6 +1842,13 @@ bool WebRtcVoiceMediaChannel::AddRecvStream(const StreamParams& sp) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); RTC_LOG(LS_INFO) << "AddRecvStream: " << sp.ToString(); + if (!sp.has_ssrcs()) { + // This is a StreamParam with unsignaled SSRCs. Store it, so it can be used + // later when we know the SSRCs on the first packet arrival. + unsignaled_stream_params_ = sp; + return true; + } + if (!ValidateStreamParams(sp)) { return false; } @@ -1882,6 +1889,13 @@ bool WebRtcVoiceMediaChannel::RemoveRecvStream(uint32_t ssrc) { RTC_DCHECK(worker_thread_checker_.CalledOnValidThread()); RTC_LOG(LS_INFO) << "RemoveRecvStream: " << ssrc; + if (ssrc == 0) { + // This indicates that we need to remove the unsignaled stream parameters + // that are cached. + unsignaled_stream_params_ = StreamParams(); + return true; + } + const auto it = recv_streams_.find(ssrc); if (it == recv_streams_.end()) { RTC_LOG(LS_WARNING) << "Try to remove stream with ssrc " << ssrc @@ -1993,7 +2007,7 @@ void WebRtcVoiceMediaChannel::OnPacketReceived( unsignaled_recv_ssrcs_.end(), ssrc) == unsignaled_recv_ssrcs_.end()); // Add new stream. - StreamParams sp; + StreamParams sp = unsignaled_stream_params_; sp.ssrcs.push_back(ssrc); RTC_LOG(LS_INFO) << "Creating unsignaled receive stream for SSRC=" << ssrc; if (!AddRecvStream(sp)) { diff --git a/media/engine/webrtcvoiceengine.h b/media/engine/webrtcvoiceengine.h index c66aa95a38..56cccbc3fd 100644 --- a/media/engine/webrtcvoiceengine.h +++ b/media/engine/webrtcvoiceengine.h @@ -252,6 +252,12 @@ class WebRtcVoiceMediaChannel final : public VoiceMediaChannel, // Queue of unsignaled SSRCs; oldest at the beginning. std::vector unsignaled_recv_ssrcs_; + // This is a stream param that comes from the remote description, but wasn't + // signaled with any a=ssrc lines. It holds the information that was signaled + // before the unsignaled receive stream is created when the first packet is + // received. + StreamParams unsignaled_stream_params_; + // Volume for unsignaled streams, which may be set before the stream exists. double default_recv_volume_ = 1.0; // Sink for latest unsignaled stream - may be set before the stream exists. diff --git a/media/engine/webrtcvoiceengine_unittest.cc b/media/engine/webrtcvoiceengine_unittest.cc index c8b0a21725..d574a72777 100644 --- a/media/engine/webrtcvoiceengine_unittest.cc +++ b/media/engine/webrtcvoiceengine_unittest.cc @@ -2538,6 +2538,39 @@ TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignaled) { sizeof(kPcmuFrame))); } +// Tests that when we add a stream without SSRCs, but contains a stream_id +// that it is stored and its stream id is later used when the first packet +// arrives to properly create a receive stream with a sync label. +TEST_F(WebRtcVoiceEngineTestFake, RecvUnsignaledSsrcWithSignaledStreamId) { + const char kSyncLabel[] = "sync_label"; + EXPECT_TRUE(SetupChannel()); + cricket::StreamParams unsignaled_stream; + unsignaled_stream.set_stream_ids({kSyncLabel}); + ASSERT_TRUE(channel_->AddRecvStream(unsignaled_stream)); + // The stream shouldn't have been created at this point because it doesn't + // have any SSRCs. + EXPECT_EQ(0, call_.GetAudioReceiveStreams().size()); + + DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame)); + + EXPECT_EQ(1, call_.GetAudioReceiveStreams().size()); + EXPECT_TRUE( + GetRecvStream(kSsrc1).VerifyLastPacket(kPcmuFrame, sizeof(kPcmuFrame))); + EXPECT_EQ(kSyncLabel, GetRecvStream(kSsrc1).GetConfig().sync_group); + + // Removing the unsignaled stream clears the cached parameters. If a new + // default unsignaled receive stream is created it will not have a sync group. + channel_->RemoveRecvStream(0); + channel_->RemoveRecvStream(kSsrc1); + + DeliverPacket(kPcmuFrame, sizeof(kPcmuFrame)); + + EXPECT_EQ(1, call_.GetAudioReceiveStreams().size()); + EXPECT_TRUE( + GetRecvStream(kSsrc1).VerifyLastPacket(kPcmuFrame, sizeof(kPcmuFrame))); + EXPECT_TRUE(GetRecvStream(kSsrc1).GetConfig().sync_group.empty()); +} + // Test that receiving N unsignaled stream works (streams will be created), and // that packets are forwarded to them all. TEST_F(WebRtcVoiceEngineTestFake, RecvMultipleUnsignaled) { diff --git a/pc/channel.cc b/pc/channel.cc index 2aab718ec8..a1869c43ca 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -555,7 +555,7 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, bool ret = true; for (StreamParamsVec::const_iterator it = local_streams_.begin(); it != local_streams_.end(); ++it) { - if (!GetStreamBySsrc(streams, it->first_ssrc())) { + if (it->has_ssrcs() && !GetStreamBySsrc(streams, it->first_ssrc())) { if (!media_channel()->RemoveSendStream(it->first_ssrc())) { std::ostringstream desc; desc << "Failed to remove send stream with ssrc " @@ -568,7 +568,7 @@ bool BaseChannel::UpdateLocalStreams_w(const std::vector& streams, // Check for new streams. for (StreamParamsVec::const_iterator it = streams.begin(); it != streams.end(); ++it) { - if (!GetStreamBySsrc(local_streams_, it->first_ssrc())) { + if (it->has_ssrcs() && !GetStreamBySsrc(local_streams_, it->first_ssrc())) { if (media_channel()->AddSendStream(*it)) { RTC_LOG(LS_INFO) << "Add send stream ssrc: " << it->ssrcs[0]; } else { @@ -591,7 +591,10 @@ bool BaseChannel::UpdateRemoteStreams_w( bool ret = true; for (StreamParamsVec::const_iterator it = remote_streams_.begin(); it != remote_streams_.end(); ++it) { - if (!GetStreamBySsrc(streams, it->first_ssrc())) { + // If we no longer have an unsignaled stream, we would like to remove + // the unsignaled stream params that are cached. + if ((!it->has_ssrcs() && !HasStreamWithNoSsrcs(streams)) || + !GetStreamBySsrc(streams, it->first_ssrc())) { if (!RemoveRecvStream_w(it->first_ssrc())) { std::ostringstream desc; desc << "Failed to remove remote stream with ssrc " @@ -604,9 +607,13 @@ bool BaseChannel::UpdateRemoteStreams_w( // Check for new streams. for (StreamParamsVec::const_iterator it = streams.begin(); it != streams.end(); ++it) { - if (!GetStreamBySsrc(remote_streams_, it->first_ssrc())) { + // We allow a StreamParams with an empty list of SSRCs, in which case the + // MediaChannel will cache the parameters and use them for any unsignaled + // stream received later. + if ((!it->has_ssrcs() && !HasStreamWithNoSsrcs(remote_streams_)) || + !GetStreamBySsrc(remote_streams_, it->first_ssrc())) { if (AddRecvStream_w(*it)) { - RTC_LOG(LS_INFO) << "Add remote ssrc: " << it->ssrcs[0]; + RTC_LOG(LS_INFO) << "Add remote ssrc: " << it->first_ssrc(); } else { std::ostringstream desc; desc << "Failed to add remote stream ssrc: " << it->first_ssrc(); diff --git a/pc/mediasession.h b/pc/mediasession.h index 72a756aa09..4f30b7c84a 100644 --- a/pc/mediasession.h +++ b/pc/mediasession.h @@ -35,8 +35,6 @@ const char kDefaultRtcpCname[] = "DefaultRtcpCname"; // Options for an RtpSender contained with an media description/"m=" section. struct SenderOptions { std::string track_id; - // TODO(steveanton): As part of work towards Unified Plan, this has been - // changed to be a vector. But for now this can only have exactly one. std::vector stream_ids; int num_sim_layers; }; diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 74f8872de6..304ab67f53 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -2386,13 +2386,8 @@ RTCError PeerConnection::ApplyRemoteDescription( // process the addition of a remote track for the media description. 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()) { - stream_ids = stream_params.stream_ids(); - } + // The remote description has signaled the stream IDs. + stream_ids = media_desc->streams()[0].stream_ids(); } if (RtpTransceiverDirectionHasRecv(local_direction) && (!transceiver->current_direction() || @@ -2439,7 +2434,8 @@ RTCError PeerConnection::ApplyRemoteDescription( RtpTransceiverDirectionHasRecv(local_direction)) { // Set ssrc to 0 in the case of an unsignalled ssrc. uint32_t ssrc = 0; - if (!media_desc->streams().empty()) { + if (!media_desc->streams().empty() && + media_desc->streams()[0].has_ssrcs()) { ssrc = media_desc->streams()[0].first_ssrc(); } transceiver->internal()->receiver_internal()->SetupMediaChannel(ssrc); @@ -4062,11 +4058,19 @@ void PeerConnection::UpdateRemoteSendersList( // Find new and active senders. for (const cricket::StreamParams& params : streams) { + if (!params.has_ssrcs()) { + // The remote endpoint has streams, but didn't signal ssrcs. For an active + // sender, this means it is coming from a Unified Plan endpoint,so we just + // create a default. + default_sender_needed = true; + break; + } + // |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. + // Plan endpoint, with multiple or no stream_ids() signaled. Since this is + // not supported in Plan B, we just take the first here and create the + // default stream ID if none is specified. const std::string& stream_id = (!params.stream_ids().empty() ? params.stream_ids()[0] : kDefaultStreamId); diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc index a249d79dd8..d9cad53dd2 100644 --- a/pc/peerconnection_integrationtest.cc +++ b/pc/peerconnection_integrationtest.cc @@ -136,6 +136,21 @@ void RemoveSsrcsAndMsids(cricket::SessionDescription* desc) { desc->set_msid_supported(false); } +// Removes all stream information besides the stream ids, simulating an +// endpoint that only signals a=msid lines to convey stream_ids. +void RemoveSsrcsAndKeepMsids(cricket::SessionDescription* desc) { + for (ContentInfo& content : desc->contents()) { + std::vector stream_ids; + if (!content.media_description()->streams().empty()) { + stream_ids = content.media_description()->streams()[0].stream_ids(); + } + content.media_description()->mutable_streams().clear(); + cricket::StreamParams new_stream; + new_stream.set_stream_ids(stream_ids); + content.media_description()->AddStream(new_stream); + } +} + int FindFirstMediaStatsIndexByKind( const std::string& kind, const std::vector& @@ -2181,6 +2196,27 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithoutSsrcOrMsidSignaling) { ASSERT_TRUE(ExpectNewFrames(media_expectations)); } +// Basic end-to-end test, without SSRC signaling. This means that the track +// was created properly and frames are delivered when the MSIDs are communicated +// with a=msid lines and no a=ssrc lines. +TEST_F(PeerConnectionIntegrationTestUnifiedPlan, + EndToEndCallWithoutSsrcSignaling) { + const char kStreamId[] = "streamId"; + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + // Add just audio tracks. + caller()->AddTrack(caller()->CreateLocalAudioTrack(), {kStreamId}); + callee()->AddAudioTrack(); + + // Remove SSRCs from the received offer SDP. + callee()->SetReceivedSdpMunger(RemoveSsrcsAndKeepMsids); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + MediaExpectations media_expectations; + media_expectations.ExpectBidirectionalAudio(); + ASSERT_TRUE(ExpectNewFrames(media_expectations)); +} + // Test that if two video tracks are sent (from caller to callee, in this test), // they're transmitted correctly end-to-end. TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithTwoVideoTracks) { diff --git a/pc/peerconnection_rtp_unittest.cc b/pc/peerconnection_rtp_unittest.cc index bcfe0d8652..5f4f34e634 100644 --- a/pc/peerconnection_rtp_unittest.cc +++ b/pc/peerconnection_rtp_unittest.cc @@ -111,9 +111,7 @@ TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithoutStreamFiresOnAddTrack) { auto callee = CreatePeerConnection(); ASSERT_TRUE(caller->AddTrack(caller->CreateAudioTrack("audio_track"))); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); // Since we are not supporting the no stream case with Plan B, there should be @@ -130,9 +128,7 @@ TEST_F(PeerConnectionRtpCallbacksTest, AddTrackWithStreamFiresOnAddTrack) { ASSERT_TRUE(caller->AddTrack(caller->CreateAudioTrack("audio_track"), {"audio_stream"})); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); auto& add_track_event = callee->observer()->add_track_events_[0]; @@ -148,14 +144,10 @@ TEST_F(PeerConnectionRtpCallbacksTest, auto callee = CreatePeerConnection(); auto sender = caller->AddTrack(caller->CreateAudioTrack("audio_track"), {}); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), @@ -169,14 +161,10 @@ TEST_F(PeerConnectionRtpCallbacksTest, auto sender = caller->AddTrack(caller->CreateAudioTrack("audio_track"), {"audio_stream"}); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); EXPECT_TRUE(caller->pc()->RemoveTrack(sender)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->observer()->add_track_events_.size(), 1u); EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), @@ -193,17 +181,13 @@ TEST_F(PeerConnectionRtpCallbacksTest, {kSharedStreamId}); auto sender2 = caller->AddTrack(caller->CreateAudioTrack("audio_track2"), {kSharedStreamId}); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); // Remove "audio_track1". EXPECT_TRUE(caller->pc()->RemoveTrack(sender1)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); EXPECT_EQ( std::vector>{ @@ -212,9 +196,7 @@ TEST_F(PeerConnectionRtpCallbacksTest, // Remove "audio_track2". EXPECT_TRUE(caller->pc()->RemoveTrack(sender2)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); EXPECT_EQ(callee->observer()->GetAddTrackReceivers(), callee->observer()->remove_track_events_); @@ -230,9 +212,7 @@ TEST_F(PeerConnectionRtpCallbacksTest, const char kStreamId1[] = "stream1"; const char kStreamId2[] = "stream2"; caller->AddTrack(caller->CreateAudioTrack("audio_track1"), {kStreamId1}); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); EXPECT_EQ(callee->observer()->add_track_events_.size(), 1u); // Change the stream ID of the sender in the session description. @@ -241,8 +221,7 @@ TEST_F(PeerConnectionRtpCallbacksTest, 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))); + callee->SetRemoteDescription(CloneSessionDescription(offer.get()))); ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u); EXPECT_EQ(callee->observer()->add_track_events_[1].streams[0]->id(), @@ -376,9 +355,7 @@ TEST_F(PeerConnectionRtpObserverTest, AddSenderWithoutStreamAddsReceiver) { auto callee = CreatePeerConnection(); ASSERT_TRUE(caller->AddTrack(caller->CreateAudioTrack("audio_track"), {})); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); EXPECT_EQ(callee->pc()->GetReceivers().size(), 1u); auto receiver_added = callee->pc()->GetReceivers()[0]; @@ -395,9 +372,7 @@ TEST_F(PeerConnectionRtpObserverTest, AddSenderWithStreamAddsReceiver) { ASSERT_TRUE(caller->AddTrack(caller->CreateAudioTrack("audio_track"), {"audio_stream"})); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); EXPECT_EQ(callee->pc()->GetReceivers().size(), 1u); auto receiver_added = callee->pc()->GetReceivers()[0]; @@ -414,15 +389,11 @@ TEST_F(PeerConnectionRtpObserverTest, auto sender = caller->AddTrack(caller->CreateAudioTrack("audio_track"), {}); ASSERT_TRUE(sender); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->pc()->GetReceivers().size(), 1u); auto receiver = callee->pc()->GetReceivers()[0]; ASSERT_TRUE(caller->pc()->RemoveTrack(sender)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); // TODO(hbos): When we implement Unified Plan, receivers will not be removed. // Instead, the transceiver owning the receiver will become inactive. @@ -436,15 +407,11 @@ TEST_F(PeerConnectionRtpObserverTest, RemoveSenderWithStreamRemovesReceiver) { auto sender = caller->AddTrack(caller->CreateAudioTrack("audio_track"), {"audio_stream"}); ASSERT_TRUE(sender); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->pc()->GetReceivers().size(), 1u); auto receiver = callee->pc()->GetReceivers()[0]; ASSERT_TRUE(caller->pc()->RemoveTrack(sender)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); // TODO(hbos): When we implement Unified Plan, receivers will not be removed. // Instead, the transceiver owning the receiver will become inactive. @@ -461,9 +428,7 @@ TEST_F(PeerConnectionRtpObserverTest, {kSharedStreamId}); auto sender2 = caller->AddTrack(caller->CreateAudioTrack("audio_track2"), {kSharedStreamId}); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); ASSERT_EQ(callee->pc()->GetReceivers().size(), 2u); rtc::scoped_refptr receiver1; @@ -480,9 +445,7 @@ TEST_F(PeerConnectionRtpObserverTest, // Remove "audio_track1". EXPECT_TRUE(caller->pc()->RemoveTrack(sender1)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); // Only |receiver2| should remain. // TODO(hbos): When we implement Unified Plan, receivers will not be removed. // Instead, the transceiver owning the receiver will become inactive. @@ -492,9 +455,7 @@ TEST_F(PeerConnectionRtpObserverTest, // Remove "audio_track2". EXPECT_TRUE(caller->pc()->RemoveTrack(sender2)); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); // TODO(hbos): When we implement Unified Plan, receivers will not be removed. // Instead, the transceiver owning the receiver will become inactive. EXPECT_EQ(callee->pc()->GetReceivers().size(), 0u); @@ -553,6 +514,40 @@ TEST_F(PeerConnectionRtpObserverTest, EXPECT_TRUE_WAIT(srd2_callback_called, kDefaultTimeout); } +// Tests that a remote track is created with the signaled MSIDs when they are +// communicated with a=msid and no SSRCs are signaled at all (i.e., no a=ssrc +// lines). +TEST_F(PeerConnectionRtpObserverTest, UnsignaledSsrcCreatesReceiverStreams) { + auto caller = CreatePeerConnectionWithUnifiedPlan(); + auto callee = CreatePeerConnectionWithUnifiedPlan(); + const char kStreamId1[] = "stream1"; + const char kStreamId2[] = "stream2"; + caller->AddTrack(caller->CreateAudioTrack("audio_track1"), + {kStreamId1, kStreamId2}); + + auto offer = caller->CreateOfferAndSetAsLocal(); + // Munge the offer to take out everything but the stream_ids. + auto contents = offer->description()->contents(); + ASSERT_TRUE(!contents.empty()); + ASSERT_TRUE(!contents[0].media_description()->streams().empty()); + std::vector stream_ids = + contents[0].media_description()->streams()[0].stream_ids(); + contents[0].media_description()->mutable_streams().clear(); + cricket::StreamParams new_stream; + new_stream.set_stream_ids(stream_ids); + contents[0].media_description()->AddStream(new_stream); + + // Set the remote description and verify that the streams were added to the + // receiver correctly. + ASSERT_TRUE( + callee->SetRemoteDescription(CloneSessionDescription(offer.get()))); + auto receivers = callee->pc()->GetReceivers(); + ASSERT_EQ(receivers.size(), 1u); + ASSERT_EQ(receivers[0]->streams().size(), 2u); + EXPECT_EQ(receivers[0]->streams()[0]->id(), kStreamId1); + EXPECT_EQ(receivers[0]->streams()[1]->id(), kStreamId2); +} + // 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. @@ -563,9 +558,7 @@ TEST_F(PeerConnectionRtpObserverTest, RemoteStreamIdChangesUpdatesReceiver) { const char kStreamId1[] = "stream1"; const char kStreamId2[] = "stream2"; caller->AddTrack(caller->CreateAudioTrack("audio_track1"), {kStreamId1}); - ASSERT_TRUE( - callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(), - static_cast(nullptr))); + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); EXPECT_EQ(callee->observer()->add_track_events_.size(), 1u); // Change the stream id of the sender in the session description. @@ -578,8 +571,7 @@ TEST_F(PeerConnectionRtpObserverTest, RemoteStreamIdChangesUpdatesReceiver) { // Set the remote description and verify that the stream was updated properly. ASSERT_TRUE( - callee->SetRemoteDescription(CloneSessionDescription(offer.get()), - static_cast(nullptr))); + callee->SetRemoteDescription(CloneSessionDescription(offer.get()))); auto receivers = callee->pc()->GetReceivers(); ASSERT_EQ(receivers.size(), 1u); ASSERT_EQ(receivers[0]->streams().size(), 1u); diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index c040353383..4728819037 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -3076,6 +3076,27 @@ TEST_F(PeerConnectionInterfaceTestPlanB, VerifyDefaultStreamIsNotCreated) { EXPECT_EQ(0u, observer_.remote_streams()->count()); } +// This tests that a default MediaStream is created if a remote SDP comes from +// an endpoint that doesn't signal SSRCs, but signals media stream IDs. +TEST_F(PeerConnectionInterfaceTestPlanB, + SdpWithMsidWithoutSsrcCreatesDefaultStream) { + FakeConstraints constraints; + constraints.AddMandatory(webrtc::MediaConstraintsInterface::kEnableDtlsSrtp, + true); + CreatePeerConnection(&constraints); + std::string sdp_string = kSdpStringWithoutStreamsAudioOnly; + // Add a=msid lines to simulate a Unified Plan endpoint that only + // signals stream IDs with a=msid lines. + sdp_string.append("a=msid:audio_stream_id audio_track_id\n"); + + CreateAndSetRemoteOffer(sdp_string); + + ASSERT_EQ(1u, observer_.remote_streams()->count()); + MediaStreamInterface* remote_stream = observer_.remote_streams()->at(0); + EXPECT_EQ("default", remote_stream->id()); + ASSERT_EQ(1u, remote_stream->GetAudioTracks().size()); +} + // 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 diff --git a/pc/webrtcsdp.cc b/pc/webrtcsdp.cc index a0887a6f78..2c34728917 100644 --- a/pc/webrtcsdp.cc +++ b/pc/webrtcsdp.cc @@ -580,15 +580,26 @@ 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 +// Creates a StreamParams track in the case when no SSRC lines are signaled. +// This is a track that does not contain SSRCs and only contains +// stream_ids/track_id if it's signaled with a=msid lines. +void CreateTrackWithNoSsrcs(const std::vector& msid_stream_ids, + const std::string& msid_track_id, + StreamParamsVec* tracks) { + StreamParams track; + if (msid_track_id.empty() || msid_stream_ids.empty()) { + // We only create an unsignaled track if a=msid lines were signaled. + return; + } + track.set_stream_ids(msid_stream_ids); + track.id = msid_track_id; + tracks->push_back(track); +} + +// Creates the StreamParams tracks, for the case when SSRC lines are signaled. +// |msid_stream_ids| 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. +// exist. We prioritize getting stream_ids/track_ids signaled in a=msid lines. void CreateTracksFromSsrcInfos(const SsrcInfoVec& ssrc_infos, const std::vector& msid_stream_ids, const std::string& msid_track_id, @@ -2945,8 +2956,17 @@ 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); + if (!ssrc_infos.empty()) { + CreateTracksFromSsrcInfos(ssrc_infos, stream_ids, track_id, &tracks, + *msid_signaling); + } else if (media_type != cricket::MEDIA_TYPE_DATA && + (*msid_signaling & cricket::kMsidSignalingMediaSection)) { + // If the stream_ids/track_id was signaled but SSRCs were unsignaled we + // still create a track. This isn't done for data media types because + // StreamParams aren't used for SCTP streams, and RTP data channels don't + // support unsignaled SSRCs. + CreateTrackWithNoSsrcs(stream_ids, track_id, &tracks); + } // Add the ssrc group to the track. for (SsrcGroupVec::iterator ssrc_group = ssrc_groups.begin(); diff --git a/pc/webrtcsdp_unittest.cc b/pc/webrtcsdp_unittest.cc index 9d6cfa3005..7a974c49f3 100644 --- a/pc/webrtcsdp_unittest.cc +++ b/pc/webrtcsdp_unittest.cc @@ -1191,6 +1191,28 @@ class WebRtcSdpTest : public testing::Test { jdesc_.session_version())); } + // Turns the existing reference description into a unified plan description + // with one audio MediaContentDescription that contains one StreamParams with + // 0 ssrcs. + void MakeUnifiedPlanDescriptionNoSsrcSignaling() { + desc_.RemoveContentByName(kVideoContentName); + desc_.RemoveContentByName(kAudioContentName); + desc_.RemoveTransportInfoByName(kVideoContentName); + RemoveVideoCandidates(); + + AudioContentDescription* audio_desc = CreateAudioContentDescription(); + StreamParams audio_track; + audio_track.id = kAudioTrackId1; + audio_track.set_stream_ids({kStreamId1}); + audio_desc->AddStream(audio_track); + desc_.AddContent(kAudioContentName, MediaProtocolType::kRtp, audio_desc); + + // Enable signaling a=msid lines. + desc_.set_msid_signaling(cricket::kMsidSignalingMediaSection); + 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() { @@ -1503,6 +1525,30 @@ class WebRtcSdpTest : public testing::Test { video_desc_->set_cryptos(std::vector()); } + // Removes everything in StreamParams from the session description that is + // used for a=ssrc lines. + void RemoveSsrcSignalingFromStreamParams() { + for (cricket::ContentInfo content_info : jdesc_.description()->contents()) { + // With Unified Plan there should be one StreamParams per m= section. + StreamParams& stream = + content_info.media_description()->mutable_streams()[0]; + stream.ssrcs.clear(); + stream.ssrc_groups.clear(); + stream.cname.clear(); + } + } + + // Removes all a=ssrc lines from the SDP string. + void RemoveSsrcLinesFromSdpString(std::string* sdp_string) { + const char kAttributeSsrc[] = "a=ssrc"; + while (sdp_string->find(kAttributeSsrc) != std::string::npos) { + size_t pos_ssrc_attribute = sdp_string->find(kAttributeSsrc); + size_t beg_line_pos = sdp_string->rfind('\n', pos_ssrc_attribute); + size_t end_line_pos = sdp_string->find('\n', pos_ssrc_attribute); + sdp_string->erase(beg_line_pos, end_line_pos - beg_line_pos); + } + } + bool TestSerializeDirection(RtpTransceiverDirection direction) { audio_desc_->set_direction(direction); video_desc_->set_direction(direction); @@ -3365,6 +3411,30 @@ TEST_F(WebRtcSdpTest, SerializeUnifiedPlanSessionDescriptionSpecialMsid) { TestSerialize(jdesc_); } +// This tests that a Unified Plan SDP with no a=ssrc lines is +// serialized/deserialized appropriately. In this case the +// MediaContentDescription will contain a StreamParams object that doesn't have +// any SSRCs. Vice versa, this will be created upon deserializing an SDP with no +// SSRC lines. +TEST_F(WebRtcSdpTest, DeserializeUnifiedPlanSessionDescriptionNoSsrcSignaling) { + MakeUnifiedPlanDescription(); + RemoveSsrcSignalingFromStreamParams(); + std::string unified_plan_sdp_string = kUnifiedPlanSdpFullString; + RemoveSsrcLinesFromSdpString(&unified_plan_sdp_string); + + JsepSessionDescription deserialized_description(kDummyType); + EXPECT_TRUE( + SdpDeserialize(unified_plan_sdp_string, &deserialized_description)); + EXPECT_TRUE(CompareSessionDescription(jdesc_, deserialized_description)); +} + +TEST_F(WebRtcSdpTest, SerializeUnifiedPlanSessionDescriptionNoSsrcSignaling) { + MakeUnifiedPlanDescription(); + RemoveSsrcSignalingFromStreamParams(); + + TestSerialize(jdesc_); +} + TEST_F(WebRtcSdpTest, EmptyDescriptionHasNoMsidSignaling) { JsepSessionDescription jsep_desc(kDummyType); ASSERT_TRUE(SdpDeserialize(kSdpSessionString, &jsep_desc));