Adds support for signaling a=msid lines without a=ssrc lines.
Currently in the SDP we require an a=ssrc line in the m= section in
order for a StreamParams object to be created with that
MediaContentDescription. This change creates a StreamParams object
without ssrcs in the case that a=msid lines are signaled, but ssrcs
are not. When the remote description is set, this allows us to store
the "unsignaled" StreamParams object in the media channel to later
be used when the first packet is received and we create the
receive stream.
Bug: webrtc:7932, webrtc:7933
Change-Id: Ib6734abeee62b8ed688a8208722c402134c074ef
Reviewed-on: https://webrtc-review.googlesource.com/63141
Commit-Queue: Seth Hampson <shampson@webrtc.org>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#22712}diff --git a/media/base/mediachannel.h b/media/base/mediachannel.h
index d1a2d3a..df20582 100644
--- a/media/base/mediachannel.h
+++ b/media/base/mediachannel.h
@@ -203,11 +203,13 @@
// 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 52a9441..52f1918 100644
--- a/media/base/streamparams.h
+++ b/media/base/streamparams.h
@@ -158,6 +158,8 @@
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<uint32_t> ssrcs; // All SSRCs for this source
std::vector<SsrcGroup> ssrc_groups; // e.g. FID, FEC, SIM
// Examples: "camera", "screencast"
@@ -267,6 +269,11 @@
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 db37180..020953b 100644
--- a/media/base/streamparams_unittest.cc
+++ b/media/base/streamparams_unittest.cc
@@ -96,6 +96,17 @@
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<cricket::StreamParams> 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 66b7771..8de05da 100644
--- a/media/engine/webrtcvideoengine.cc
+++ b/media/engine/webrtcvideoengine.cc
@@ -507,8 +507,9 @@
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 @@
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 @@
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 2a03a53..fc9e307 100644
--- a/media/engine/webrtcvideoengine.h
+++ b/media/engine/webrtcvideoengine.h
@@ -177,6 +177,8 @@
rtc::Optional<uint32_t> 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 @@
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 d0b5707..ae85fc0 100644
--- a/media/engine/webrtcvideoengine_unittest.cc
+++ b/media/engine/webrtcvideoengine_unittest.cc
@@ -4184,6 +4184,44 @@
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 df50642..bb1c9c3 100644
--- a/media/engine/webrtcvoiceengine.cc
+++ b/media/engine/webrtcvoiceengine.cc
@@ -1842,6 +1842,13 @@
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 @@
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 @@
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 c66aa95..56cccbc3 100644
--- a/media/engine/webrtcvoiceengine.h
+++ b/media/engine/webrtcvoiceengine.h
@@ -252,6 +252,12 @@
// Queue of unsignaled SSRCs; oldest at the beginning.
std::vector<uint32_t> 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 c8b0a21..d574a72 100644
--- a/media/engine/webrtcvoiceengine_unittest.cc
+++ b/media/engine/webrtcvoiceengine_unittest.cc
@@ -2538,6 +2538,39 @@
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 2aab718..a1869c4 100644
--- a/pc/channel.cc
+++ b/pc/channel.cc
@@ -555,7 +555,7 @@
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 @@
// 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 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 @@
// 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 72a756a..4f30b7c 100644
--- a/pc/mediasession.h
+++ b/pc/mediasession.h
@@ -35,8 +35,6 @@
// 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<std::string> stream_ids;
int num_sim_layers;
};
diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc
index 74f8872..304ab67 100644
--- a/pc/peerconnection.cc
+++ b/pc/peerconnection.cc
@@ -2386,13 +2386,8 @@
// process the addition of a remote track for the media description.
std::vector<std::string> 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 @@
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 @@
// 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 a249d79..d9cad53 100644
--- a/pc/peerconnection_integrationtest.cc
+++ b/pc/peerconnection_integrationtest.cc
@@ -136,6 +136,21 @@
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<std::string> 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<const webrtc::RTCMediaStreamTrackStats*>&
@@ -2181,6 +2196,27 @@
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 bcfe0d8..5f4f34e 100644
--- a/pc/peerconnection_rtp_unittest.cc
+++ b/pc/peerconnection_rtp_unittest.cc
@@ -111,9 +111,7 @@
auto callee = CreatePeerConnection();
ASSERT_TRUE(caller->AddTrack(caller->CreateAudioTrack("audio_track")));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
ASSERT_TRUE(caller->AddTrack(caller->CreateAudioTrack("audio_track"),
{"audio_stream"}));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
auto callee = CreatePeerConnection();
auto sender = caller->AddTrack(caller->CreateAudioTrack("audio_track"), {});
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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<webrtc::RTCError*>(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 @@
auto sender = caller->AddTrack(caller->CreateAudioTrack("audio_track"),
{"audio_stream"});
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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<webrtc::RTCError*>(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 @@
{kSharedStreamId});
auto sender2 = caller->AddTrack(caller->CreateAudioTrack("audio_track2"),
{kSharedStreamId});
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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<webrtc::RTCError*>(nullptr)));
+ ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
ASSERT_EQ(callee->observer()->add_track_events_.size(), 2u);
EXPECT_EQ(
std::vector<rtc::scoped_refptr<RtpReceiverInterface>>{
@@ -212,9 +196,7 @@
// Remove "audio_track2".
EXPECT_TRUE(caller->pc()->RemoveTrack(sender2));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
const char kStreamId1[] = "stream1";
const char kStreamId2[] = "stream2";
caller->AddTrack(caller->CreateAudioTrack("audio_track1"), {kStreamId1});
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
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<webrtc::RTCError*>(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 @@
auto callee = CreatePeerConnection();
ASSERT_TRUE(caller->AddTrack(caller->CreateAudioTrack("audio_track"), {}));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
ASSERT_TRUE(caller->AddTrack(caller->CreateAudioTrack("audio_track"),
{"audio_stream"}));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
auto sender = caller->AddTrack(caller->CreateAudioTrack("audio_track"), {});
ASSERT_TRUE(sender);
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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<webrtc::RTCError*>(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 @@
auto sender = caller->AddTrack(caller->CreateAudioTrack("audio_track"),
{"audio_stream"});
ASSERT_TRUE(sender);
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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<webrtc::RTCError*>(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 @@
{kSharedStreamId});
auto sender2 = caller->AddTrack(caller->CreateAudioTrack("audio_track2"),
{kSharedStreamId});
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(nullptr)));
+ ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
ASSERT_EQ(callee->pc()->GetReceivers().size(), 2u);
rtc::scoped_refptr<webrtc::RtpReceiverInterface> receiver1;
@@ -480,9 +445,7 @@
// Remove "audio_track1".
EXPECT_TRUE(caller->pc()->RemoveTrack(sender1));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
// Remove "audio_track2".
EXPECT_TRUE(caller->pc()->RemoveTrack(sender2));
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
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<std::string> 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 @@
const char kStreamId1[] = "stream1";
const char kStreamId2[] = "stream2";
caller->AddTrack(caller->CreateAudioTrack("audio_track1"), {kStreamId1});
- ASSERT_TRUE(
- callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal(),
- static_cast<webrtc::RTCError*>(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 @@
// Set the remote description and verify that the stream was updated properly.
ASSERT_TRUE(
- callee->SetRemoteDescription(CloneSessionDescription(offer.get()),
- static_cast<webrtc::RTCError*>(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 c040353..4728819 100644
--- a/pc/peerconnectioninterface_unittest.cc
+++ b/pc/peerconnectioninterface_unittest.cc
@@ -3076,6 +3076,27 @@
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 a0887a6..2c34728 100644
--- a/pc/webrtcsdp.cc
+++ b/pc/webrtcsdp.cc
@@ -580,15 +580,26 @@
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<std::string>& 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<std::string>& msid_stream_ids,
const std::string& msid_track_id,
@@ -2945,8 +2956,17 @@
// 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 9d6cfa3..7a974c4 100644
--- a/pc/webrtcsdp_unittest.cc
+++ b/pc/webrtcsdp_unittest.cc
@@ -1191,6 +1191,28 @@
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 @@
video_desc_->set_cryptos(std::vector<CryptoParams>());
}
+ // 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 @@
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));
