From 88b8dec17bb5e2cf9c5c5009a41593a1eb7ac1a9 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 7 Apr 2022 09:54:51 +0000 Subject: [PATCH] Reland "remove mslabel and mslabel ssrc-specific attributes" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit f0a34f2a3061da6ecb302af122a1126d0a65a1b0. Reason for revert: Fixed downstream projects to not require those fields. Original change's description: > Revert "remove mslabel and mslabel ssrc-specific attributes" > > This reverts commit e3f257c4ee2079dee14ec8425eec691db3a9757c. > > Reason for revert: Breaks downstream projects > > Original change's description: > > remove mslabel and mslabel ssrc-specific attributes > > > > Removes support for parsing and serializing > > a=ssrc:1 mslabel:stream > > a=ssrc:1 label:track > > which have been superceeded by > > a=ssrc:1 msid:stream track > > a long time ago. > > > > Bug: webrtc:7110 > > Change-Id: I3aca47728098b6e7e049b82ed34c59426d411c41 > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168244 > > Commit-Queue: Harald Alvestrand > > Reviewed-by: Harald Alvestrand > > Reviewed-by: Kári Helgason > > Cr-Commit-Position: refs/heads/master@{#30801} > > TBR=kthelgason@webrtc.org,hta@webrtc.org,philipp.hancke@googlemail.com > > Change-Id: Ibd0ad11d2dee9f54bacab3dcca61dedccfc2c120 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: webrtc:7110 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/170620 > Reviewed-by: Artem Titov > Commit-Queue: Artem Titov > Cr-Commit-Position: refs/heads/master@{#30805} Change-Id: Iec4bd460b35ce49cdb03dd607d2c8b1dee6210ab Bug: webrtc:7110 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/170622 Reviewed-by: Philipp Hancke Reviewed-by: Henrik Boström Commit-Queue: Harald Alvestrand Cr-Commit-Position: refs/heads/main@{#36640} --- pc/peer_connection_interface_unittest.cc | 25 ++--- pc/webrtc_sdp.cc | 32 +------ pc/webrtc_sdp_unittest.cc | 92 +++++-------------- .../unittests/RTCSessionDescriptionTest.mm | 8 +- 4 files changed, 36 insertions(+), 121 deletions(-) diff --git a/pc/peer_connection_interface_unittest.cc b/pc/peer_connection_interface_unittest.cc index 3c146f7acc..20badb3d50 100644 --- a/pc/peer_connection_interface_unittest.cc +++ b/pc/peer_connection_interface_unittest.cc @@ -125,8 +125,7 @@ static const char kSdpStringWithStream1PlanB[] = "a=rtcp-mux\r\n" "a=rtpmap:103 ISAC/16000\r\n" "a=ssrc:1 cname:stream1\r\n" - "a=ssrc:1 mslabel:stream1\r\n" - "a=ssrc:1 label:audiotrack0\r\n" + "a=ssrc:1 msid:stream1 audiotrack0\r\n" "m=video 1 RTP/AVPF 120\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" @@ -137,11 +136,10 @@ static const char kSdpStringWithStream1PlanB[] = "a=rtcp-mux\r\n" "a=rtpmap:120 VP8/90000\r\n" "a=ssrc:2 cname:stream1\r\n" - "a=ssrc:2 mslabel:stream1\r\n" - "a=ssrc:2 label:videotrack0\r\n"; -// Same string as above but with the MID changed to the Unified Plan default. -// This is needed so that this SDP can be used as an answer for a Unified Plan -// offer. + "a=ssrc:2 msid:stream1 videotrack0\r\n"; +// Same string as above but with the MID changed to the Unified Plan default and +// a=msid added. This is needed so that this SDP can be used as an answer for a +// Unified Plan offer. static const char kSdpStringWithStream1UnifiedPlan[] = "v=0\r\n" "o=- 0 0 IN IP4 127.0.0.1\r\n" @@ -156,9 +154,8 @@ static const char kSdpStringWithStream1UnifiedPlan[] = "a=sendrecv\r\n" "a=rtcp-mux\r\n" "a=rtpmap:103 ISAC/16000\r\n" + "a=msid:stream1 audiotrack0\r\n" "a=ssrc:1 cname:stream1\r\n" - "a=ssrc:1 mslabel:stream1\r\n" - "a=ssrc:1 label:audiotrack0\r\n" "m=video 1 RTP/AVPF 120\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" @@ -168,9 +165,8 @@ static const char kSdpStringWithStream1UnifiedPlan[] = "a=sendrecv\r\n" "a=rtcp-mux\r\n" "a=rtpmap:120 VP8/90000\r\n" - "a=ssrc:2 cname:stream1\r\n" - "a=ssrc:2 mslabel:stream1\r\n" - "a=ssrc:2 label:videotrack0\r\n"; + "a=msid:stream1 videotrack0\r\n" + "a=ssrc:2 cname:stream1\r\n"; // Reference SDP with a MediaStream with label "stream1" and audio track with // id "audio_1"; @@ -188,8 +184,7 @@ static const char kSdpStringWithStream1AudioTrackOnly[] = "a=sendrecv\r\n" "a=rtpmap:103 ISAC/16000\r\n" "a=ssrc:1 cname:stream1\r\n" - "a=ssrc:1 mslabel:stream1\r\n" - "a=ssrc:1 label:audiotrack0\r\n" + "a=ssrc:1 msid:stream1 audiotrack0\r\n" "a=rtcp-mux\r\n"; // Reference SDP with two MediaStreams with label "stream1" and "stream2. Each @@ -430,8 +425,6 @@ static const char kDtlsSdesFallbackSdp[] = "a=rtcp-mux\r\n" "a=mid:audio\r\n" "a=ssrc:1 cname:stream1\r\n" - "a=ssrc:1 mslabel:stream1\r\n" - "a=ssrc:1 label:audiotrack0\r\n" "a=ice-ufrag:e5785931\r\n" "a=ice-pwd:36fb7878390db89481c1d46daa4278d8\r\n" "a=rtpmap:0 pcmu/8000\r\n" diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index 12e9dd8f68..4372b0b743 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -154,8 +154,6 @@ 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"; static const char kAttributeCrypto[] = "crypto"; static const char kAttributeCandidate[] = "candidate"; @@ -267,11 +265,6 @@ struct SsrcInfo { std::string cname; std::string stream_id; std::string track_id; - - // For backward compatibility. - // TODO(ronghuawu): Remove below 2 fields once all the clients support msid. - std::string label; - std::string mslabel; }; typedef std::vector SsrcInfoVec; typedef std::vector SsrcGroupVec; @@ -724,12 +717,6 @@ void CreateTracksFromSsrcInfos(const SsrcInfoVec& ssrc_infos, // This is the case with Plan B SDP msid signaling. stream_ids.push_back(ssrc_info.stream_id); track_id = ssrc_info.track_id; - } else if (!ssrc_info.mslabel.empty()) { - // Since there's no a=msid or a=ssrc msid signaling, this is a sdp from - // an older version of client that doesn't support msid. - // In that case, we use the mslabel and label to construct the track. - stream_ids.push_back(ssrc_info.mslabel); - track_id = ssrc_info.label; } else { // Since no media streams isn't supported with older SDP signaling, we // use a default a stream id. @@ -1740,15 +1727,6 @@ void BuildRtpContentAttributes(const MediaContentDescription* media_desc, << kSsrcAttributeMsid << kSdpDelimiterColon << stream_id << kSdpDelimiterSpace << track.id; AddLine(os.str(), message); - - // TODO(ronghuawu): Remove below code which is for backward - // compatibility. - // draft-alvestrand-rtcweb-mid-01 - // a=ssrc: mslabel: - // The label isn't yet defined. - // a=ssrc: label: - AddSsrcLine(ssrc, kSsrcAttributeMslabel, stream_id, message); - AddSsrcLine(ssrc, kSSrcAttributeLabel, track.id, message); } } @@ -3469,14 +3447,8 @@ bool ParseSsrcAttribute(const std::string& line, ssrc_info.track_id = fields[1]; } *msid_signaling |= cricket::kMsidSignalingSsrcAttribute; - } else if (attribute == kSsrcAttributeMslabel) { - // draft-alvestrand-rtcweb-mid-01 - // mslabel: - ssrc_info.mslabel = value; - } else if (attribute == kSSrcAttributeLabel) { - // The label isn't defined. - // label: - ssrc_info.label = value; + } else { + RTC_LOG(LS_INFO) << "Ignored unknown ssrc-specific attribute: " << line; } return true; } diff --git a/pc/webrtc_sdp_unittest.cc b/pc/webrtc_sdp_unittest.cc index a081d74aa4..86e137ee1a 100644 --- a/pc/webrtc_sdp_unittest.cc +++ b/pc/webrtc_sdp_unittest.cc @@ -188,8 +188,6 @@ static const char kSdpFullString[] = "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" "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" @@ -214,12 +212,8 @@ static const char kSdpFullString[] = "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:3 msid:local_stream_1 video_track_id_1\r\n"; // SDP reference string without the candidates. static const char kSdpString[] = @@ -245,8 +239,6 @@ static const char kSdpString[] = "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" "m=video 9 RTP/SAVPF 120\r\n" "c=IN IP4 0.0.0.0\r\n" "a=rtcp:9 IN IP4 0.0.0.0\r\n" @@ -259,12 +251,8 @@ static const char kSdpString[] = "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:3 msid:local_stream_1 video_track_id_1\r\n"; // draft-ietf-mmusic-sctp-sdp-03 static const char kSdpSctpDataChannelString[] = @@ -339,9 +327,7 @@ static const char kSdpAudioString[] = "a=sendrecv\r\n" "a=rtpmap:111 opus/48000/2\r\n" "a=ssrc:1 cname:stream_1_cname\r\n" - "a=ssrc:1 msid:local_stream audio_track_id_1\r\n" - "a=ssrc:1 mslabel:local_stream\r\n" - "a=ssrc:1 label:audio_track_id_1\r\n"; + "a=ssrc:1 msid:local_stream audio_track_id_1\r\n"; static const char kSdpVideoString[] = "m=video 9 RTP/SAVPF 120\r\n" @@ -352,9 +338,7 @@ static const char kSdpVideoString[] = "a=sendrecv\r\n" "a=rtpmap:120 VP8/90000\r\n" "a=ssrc:2 cname:stream_1_cname\r\n" - "a=ssrc:2 msid:local_stream video_track_id_1\r\n" - "a=ssrc:2 mslabel:local_stream\r\n" - "a=ssrc:2 label:video_track_id_1\r\n"; + "a=ssrc:2 msid:local_stream video_track_id_1\r\n"; // Reference sdp string using bundle-only. static const char kBundleOnlySdpFullString[] = @@ -395,8 +379,6 @@ static const char kBundleOnlySdpFullString[] = "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" "m=video 0 RTP/SAVPF 120\r\n" "c=IN IP4 0.0.0.0\r\n" "a=rtcp:9 IN IP4 0.0.0.0\r\n" @@ -409,12 +391,8 @@ static const char kBundleOnlySdpFullString[] = "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:3 msid:local_stream_1 video_track_id_1\r\n"; // Plan B SDP reference string, with 2 streams, 2 audio tracks and 3 video // tracks. @@ -455,12 +433,8 @@ static const char kPlanBSdpFullString[] = "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" @@ -485,20 +459,12 @@ static const char kPlanBSdpFullString[] = "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"; + "a=ssrc:6 msid:local_stream_2 video_track_id_3\r\n"; // Unified Plan SDP reference string, with 2 streams, 2 audio tracks and 3 video // tracks. @@ -654,8 +620,6 @@ static const char kUnifiedPlanSdpFullStringWithSpecialMsid[] = "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" @@ -677,8 +641,6 @@ static const char kUnifiedPlanSdpFullStringWithSpecialMsid[] = // 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" @@ -696,9 +658,7 @@ static const char kUnifiedPlanSdpFullStringWithSpecialMsid[] = "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 msid:- audio_track_id_3\r\n" - "a=ssrc:7 mslabel:-\r\n" - "a=ssrc:7 label:audio_track_id_3\r\n"; + "a=ssrc:7 msid:- audio_track_id_3\r\n"; // SDP string for unified plan without SSRCs static const char kUnifiedPlanSdpFullStringNoSsrc[] = @@ -2667,21 +2627,6 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithRejectedAudioVideo) { EXPECT_TRUE(TestDeserializeRejected(true, true)); } -// Tests that we can still handle the sdp uses mslabel and label instead of -// msid for backward compatibility. -TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithoutMsid) { - jdesc_.description()->set_msid_supported(false); - JsepSessionDescription jdesc(kDummyType); - std::string sdp_without_msid = kSdpFullString; - Replace("msid", "xmsid", &sdp_without_msid); - // Deserialize - EXPECT_TRUE(SdpDeserialize(sdp_without_msid, &jdesc)); - // Verify - EXPECT_TRUE(CompareSessionDescription(jdesc_, jdesc)); - EXPECT_FALSE(jdesc.description()->msid_signaling() & - ~cricket::kMsidSignalingSsrcAttribute); -} - TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithExtmapAllowMixed) { jdesc_.description()->set_extmap_allow_mixed(true); std::string sdp_with_extmap_allow_mixed = kSdpFullString; @@ -3118,7 +3063,7 @@ TEST_F(WebRtcSdpTest, DeserializeSessionDescriptionWithoutEndLineBreak) { // Deserialize SdpParseError error; EXPECT_FALSE(webrtc::SdpDeserialize(sdp, &jdesc, &error)); - const std::string lastline = "a=ssrc:3 label:video_track_id_1"; + const std::string lastline = "a=ssrc:3 msid:local_stream_1 video_track_id_1"; EXPECT_EQ(lastline, error.line); EXPECT_EQ("Invalid SDP line.", error.description); } @@ -4283,15 +4228,16 @@ TEST_F(WebRtcSdpTest, DeserializeEmptySessionName) { // Simulcast malformed input test for invalid format. TEST_F(WebRtcSdpTest, DeserializeSimulcastNegative_EmptyAttribute) { - ExpectParseFailureWithNewLines("a=ssrc:3 label:video_track_id_1\r\n", - "a=simulcast:\r\n", "a=simulcast:"); + ExpectParseFailureWithNewLines( + "a=ssrc:3 msid:local_stream_1 video_track_id_1\r\n", "a=simulcast:\r\n", + "a=simulcast:"); } // Tests that duplicate simulcast entries in the SDP triggers a parse failure. TEST_F(WebRtcSdpTest, DeserializeSimulcastNegative_DuplicateAttribute) { - ExpectParseFailureWithNewLines("a=ssrc:3 label:video_track_id_1\r\n", - "a=simulcast:send 1\r\na=simulcast:recv 2\r\n", - "a=simulcast:"); + ExpectParseFailureWithNewLines( + "a=ssrc:3 msid:local_stream_1 video_track_id_1\r\n", + "a=simulcast:send 1\r\na=simulcast:recv 2\r\n", "a=simulcast:"); } // Validates that deserialization uses the a=simulcast: attribute @@ -4711,3 +4657,13 @@ TEST_F(WebRtcSdpTest, MaxChannels) { ExpectParseFailure(sdp, "a=rtpmap:108 ISAC/16000/512"); } + +// This tests parsing of SDP with unknown ssrc-specific attributes. +TEST_F(WebRtcSdpTest, ParseIgnoreUnknownSsrcSpecificAttribute) { + std::string sdp = kSdpString; + sdp += "a=ssrc:1 mslabel:something\r\n"; + + JsepSessionDescription output(kDummyType); + SdpParseError error; + ASSERT_TRUE(webrtc::SdpDeserialize(sdp, &output, &error)); +} diff --git a/sdk/objc/unittests/RTCSessionDescriptionTest.mm b/sdk/objc/unittests/RTCSessionDescriptionTest.mm index 25d7ffe67d..cb4a01c805 100644 --- a/sdk/objc/unittests/RTCSessionDescriptionTest.mm +++ b/sdk/objc/unittests/RTCSessionDescriptionTest.mm @@ -88,8 +88,6 @@ "a=maxptime:60\r\n" "a=ssrc:1504474588 cname:V+FdIC5AJpxLhdYQ\r\n" "a=ssrc:1504474588 msid:ARDAMS ARDAMSa0\r\n" - "a=ssrc:1504474588 mslabel:ARDAMS\r\n" - "a=ssrc:1504474588 label:ARDAMSa0\r\n" "m=video 9 UDP/TLS/RTP/SAVPF 100 116 117 96\r\n" "c=IN IP4 0.0.0.0\r\n" "a=rtcp:9 IN IP4 0.0.0.0\r\n" @@ -118,12 +116,8 @@ "a=ssrc-group:FID 498297514 1644357692\r\n" "a=ssrc:498297514 cname:V+FdIC5AJpxLhdYQ\r\n" "a=ssrc:498297514 msid:ARDAMS ARDAMSv0\r\n" - "a=ssrc:498297514 mslabel:ARDAMS\r\n" - "a=ssrc:498297514 label:ARDAMSv0\r\n" "a=ssrc:1644357692 cname:V+FdIC5AJpxLhdYQ\r\n" - "a=ssrc:1644357692 msid:ARDAMS ARDAMSv0\r\n" - "a=ssrc:1644357692 mslabel:ARDAMS\r\n" - "a=ssrc:1644357692 label:ARDAMSv0\r\n"; + "a=ssrc:1644357692 msid:ARDAMS ARDAMSv0\r\n"; } @end