From 7fa6ee6250c2616b829d2dd83f0620e862ac78af Mon Sep 17 00:00:00 2001 From: Seth Hampson Date: Wed, 17 Oct 2018 10:25:28 -0700 Subject: [PATCH] Adds support for "-" to a=ssrc msid lines. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently with in Unified Plan an initial offer will include both "a=ssrc:... msid:..." lines and "a=msid:... ..." lines. The a=ssrc line is added in order to support signaling to a Plan B endpoint. Although if no stream is associated to a given track it will only be signaled in the "a=msid" line with "-". The "a=ssrc msid" line will simply put an empty string for the msid, which does not interoperate with FF. This change adds support so that both lines will signal a "-". Bug: webrtc:9880 Change-Id: I73655ce3c11a924b508616d820555bf24aae1bd3 Reviewed-on: https://webrtc-review.googlesource.com/c/106605 Commit-Queue: Seth Hampson Reviewed-by: Henrik Boström Reviewed-by: Seth Hampson Cr-Commit-Position: refs/heads/master@{#25241} --- pc/webrtcsdp.cc | 7 ++- pc/webrtcsdp_unittest.cc | 94 ++++++++++++++++++++++++++++++++++------ 2 files changed, 87 insertions(+), 14 deletions(-) diff --git a/pc/webrtcsdp.cc b/pc/webrtcsdp.cc index cf6b10dcd7..2ea24c00c5 100644 --- a/pc/webrtcsdp.cc +++ b/pc/webrtcsdp.cc @@ -1628,7 +1628,12 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, // Since a=ssrc msid signaling is used in Plan B SDP semantics, and // multiple stream ids are not supported for Plan B, we are only adding // a line for the first media stream id here. - const std::string& stream_id = track.first_stream_id(); + const std::string& track_stream_id = track.first_stream_id(); + // We use a special msid-id value of "-" to represent no streams, + // for Unified Plan compatibility. Plan B will always have a + // track_stream_id. + const std::string& stream_id = + track_stream_id.empty() ? kNoStreamMsid : track_stream_id; InitAttrLine(kAttributeSsrc, &os); os << kSdpDelimiterColon << ssrc << kSdpDelimiterSpace << kSsrcAttributeMsid << kSdpDelimiterColon << stream_id diff --git a/pc/webrtcsdp_unittest.cc b/pc/webrtcsdp_unittest.cc index e3ccd06b89..bba10f2054 100644 --- a/pc/webrtcsdp_unittest.cc +++ b/pc/webrtcsdp_unittest.cc @@ -623,8 +623,8 @@ static const char kUnifiedPlanSdpFullStringWithSpecialMsid[] = "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=msid:local_stream_1 audio_track_id_1\r\n" "a=rtcp-mux\r\n" "a=rtcp-rsize\r\n" "a=crypto:1 AES_CM_128_HMAC_SHA1_32 " @@ -643,9 +643,9 @@ static const char kUnifiedPlanSdpFullStringWithSpecialMsid[] = "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=sendrecv\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 " @@ -666,8 +666,8 @@ static const char kUnifiedPlanSdpFullStringWithSpecialMsid[] = "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=msid:- audio_track_id_3\r\n" "a=rtcp-mux\r\n" "a=rtcp-rsize\r\n" "a=crypto:1 AES_CM_128_HMAC_SHA1_32 " @@ -676,7 +676,10 @@ static const char kUnifiedPlanSdpFullStringWithSpecialMsid[] = "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"; + "a=ssrc:7 cname:stream_2_cname\r\n" + "a=ssrc:7 msid:- audio_track_id_3\r\n" + "a=ssrc:7 mslabel:-\r\n" + "a=ssrc:7 label:audio_track_id_3\r\n"; // One candidate reference string as per W3c spec. // candidate: not a=candidate:CRLF @@ -1162,7 +1165,7 @@ class WebRtcSdpTest : public testing::Test { // 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() { + void MakeUnifiedPlanDescriptionMultipleStreamIds(const int msid_signaling) { desc_.RemoveContentByName(kVideoContentName); desc_.RemoveTransportInfoByName(kVideoContentName); RemoveVideoCandidates(); @@ -1190,9 +1193,7 @@ class WebRtcSdpTest : public testing::Test { 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); + desc_.set_msid_signaling(msid_signaling); ASSERT_TRUE(jdesc_.Initialize(desc_.Copy(), jdesc_.session_id(), jdesc_.session_version())); } @@ -1547,6 +1548,27 @@ class WebRtcSdpTest : public testing::Test { } } + // Removes all a=ssrc lines from the SDP string, except for the + // "a=ssrc:... cname:..." lines. + void RemoveSsrcMsidLinesFromSdpString(std::string* sdp_string) { + const char kAttributeSsrc[] = "a=ssrc"; + const char kAttributeCname[] = "cname"; + size_t ssrc_line_pos = sdp_string->find(kAttributeSsrc); + while (ssrc_line_pos != std::string::npos) { + size_t beg_line_pos = sdp_string->rfind('\n', ssrc_line_pos); + size_t end_line_pos = sdp_string->find('\n', ssrc_line_pos); + size_t cname_pos = sdp_string->find(kAttributeCname, ssrc_line_pos); + if (cname_pos == std::string::npos || cname_pos > end_line_pos) { + // Only erase a=ssrc lines that don't contain "cname". + sdp_string->erase(beg_line_pos, end_line_pos - beg_line_pos); + ssrc_line_pos = sdp_string->find(kAttributeSsrc, beg_line_pos); + } else { + // Skip the "a=ssrc:... cname" line and find the next "a=ssrc" line. + ssrc_line_pos = sdp_string->find(kAttributeSsrc, end_line_pos); + } + } + } + // Removes all a=ssrc lines from the SDP string. void RemoveSsrcLinesFromSdpString(std::string* sdp_string) { const char kAttributeSsrc[] = "a=ssrc"; @@ -3434,12 +3456,16 @@ TEST_F(WebRtcSdpTest, SerializeUnifiedPlanSessionDescription) { } // 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 +// Unified Plan and Plan B style SDP, meaning that it contains both "a=ssrc +// msid" lines and "a=msid " lines. 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(); +TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionSpecialMsid) { + // Create both msid lines for Plan B and Unified Plan support. + MakeUnifiedPlanDescriptionMultipleStreamIds( + cricket::kMsidSignalingMediaSection | + cricket::kMsidSignalingSsrcAttribute); JsepSessionDescription deserialized_description(kDummyType); EXPECT_TRUE(SdpDeserialize(kUnifiedPlanSdpFullStringWithSpecialMsid, @@ -3448,8 +3474,50 @@ TEST_F(WebRtcSdpTest, DeserializeUnifiedPlanSessionDescriptionSpecialMsid) { EXPECT_TRUE(CompareSessionDescription(jdesc_, deserialized_description)); } -TEST_F(WebRtcSdpTest, SerializeUnifiedPlanSessionDescriptionSpecialMsid) { - MakeUnifiedPlanDescriptionMultipleStreamIds(); +// Tests the serialization of a Unified Plan SDP that is compatible for both +// Unified Plan and Plan B style SDPs, meaning that it contains both "a=ssrc +// msid" lines and "a=msid " lines. It tests the case for no stream ids and +// multiple stream ids. +TEST_F(WebRtcSdpTest, SerializeSessionDescriptionSpecialMsid) { + // Create both msid lines for Plan B and Unified Plan support. + MakeUnifiedPlanDescriptionMultipleStreamIds( + cricket::kMsidSignalingMediaSection | + cricket::kMsidSignalingSsrcAttribute); + std::string serialized_sdp = webrtc::SdpSerialize(jdesc_); + // We explicitly test that the serialized SDP string is equal to the hard + // coded SDP string. This is necessary, because in the parser "a=msid" lines + // take priority over "a=ssrc msid" lines. This means if we just used + // TestSerialize(), it could serialize an SDP that omits "a=ssrc msid" lines, + // and still pass, because the deserialized version would be the same. + EXPECT_EQ(kUnifiedPlanSdpFullStringWithSpecialMsid, serialized_sdp); +} + +// Tests that a Unified Plan style SDP (does not contain "a=ssrc msid" lines +// that signal stream IDs) is deserialized appropriately. It tests the case for +// no stream ids and multiple stream ids. +TEST_F(WebRtcSdpTest, UnifiedPlanDeserializeSessionDescriptionSpecialMsid) { + // Only create a=msid lines for strictly Unified Plan stream ID support. + MakeUnifiedPlanDescriptionMultipleStreamIds( + cricket::kMsidSignalingMediaSection); + + JsepSessionDescription deserialized_description(kDummyType); + std::string unified_plan_sdp_string = + kUnifiedPlanSdpFullStringWithSpecialMsid; + RemoveSsrcMsidLinesFromSdpString(&unified_plan_sdp_string); + EXPECT_TRUE( + SdpDeserialize(unified_plan_sdp_string, &deserialized_description)); + + EXPECT_TRUE(CompareSessionDescription(jdesc_, deserialized_description)); +} + +// Tests that a Unified Plan style SDP (does not contain "a=ssrc msid" lines +// that signal stream IDs) is serialized appropriately. It tests the case for no +// stream ids and multiple stream ids. +TEST_F(WebRtcSdpTest, UnifiedPlanSerializeSessionDescriptionSpecialMsid) { + // Only create a=msid lines for strictly Unified Plan stream ID support. + MakeUnifiedPlanDescriptionMultipleStreamIds( + cricket::kMsidSignalingMediaSection); + TestSerialize(jdesc_); }