From 7b6faa12435d5f3e9143158c520450ee579f87c3 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Tue, 5 Sep 2023 12:04:15 +0200 Subject: [PATCH] Move assignment of a streams random-msid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit move this a bit later in the process since the current handling will consider two ssrc-lines with a cname in the same RTX FID ssrc-group to be part of separate streams due to the different randomly assigned msids. This leads to a misdetection as plan-b SDP. BUG=None Change-Id: Ie8acce9c2c7fb9eabda479b90e8cc7406dcb1696 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/318820 Reviewed-by: Harald Alvestrand Reviewed-by: Henrik Boström Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#40701} --- pc/sdp_offer_answer_unittest.cc | 29 +++++++++++++++++++++++++++++ pc/webrtc_sdp.cc | 19 ++++++++++--------- 2 files changed, 39 insertions(+), 9 deletions(-) diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index 72ea4f9f22..03bea2a23e 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -930,4 +930,33 @@ TEST_F(SdpOfferAnswerTest, AllowOnlyOneSsrcGroupPerSemanticAndPrimarySsrc) { EXPECT_FALSE(pc->SetLocalDescription(std::move(modified_offer))); } +TEST_F(SdpOfferAnswerTest, OfferWithRtxAndNoMsidIsNotRejected) { + auto pc = CreatePeerConnection(); + std::string sdp = + "v=0\r\n" + "o=- 0 3 IN IP4 127.0.0.1\r\n" + "s=-\r\n" + "t=0 0\r\n" + "a=group:BUNDLE 0\r\n" + "a=fingerprint:sha-1 " + "4A:AD:B9:B1:3F:82:18:3B:54:02:12:DF:3E:5D:49:6B:19:E5:7C:AB\r\n" + "a=setup:actpass\r\n" + "a=ice-ufrag:ETEn\r\n" + "a=ice-pwd:OtSK0WpNtpUjkY4+86js7Z/l\r\n" + "m=video 9 UDP/TLS/RTP/SAVPF 96 97\r\n" + "c=IN IP4 0.0.0.0\r\n" + "a=rtcp-mux\r\n" + "a=sendonly\r\n" + "a=mid:0\r\n" + // "a=msid:stream obsoletetrack\r\n" + "a=rtpmap:96 VP8/90000\r\n" + "a=rtpmap:97 rtx/90000\r\n" + "a=fmtp:97 apt=96\r\n" + "a=ssrc-group:FID 1 2\r\n" + "a=ssrc:1 cname:test\r\n" + "a=ssrc:2 cname:test\r\n"; + auto offer = CreateSessionDescription(SdpType::kOffer, sdp); + EXPECT_TRUE(pc->SetRemoteDescription(std::move(offer))); +} + } // namespace webrtc diff --git a/pc/webrtc_sdp.cc b/pc/webrtc_sdp.cc index ecba22a85a..71cd18cfb6 100644 --- a/pc/webrtc_sdp.cc +++ b/pc/webrtc_sdp.cc @@ -706,7 +706,7 @@ void CreateTracksFromSsrcInfos(const SsrcInfoVec& ssrc_infos, absl::string_view msid_track_id, StreamParamsVec* tracks, int msid_signaling) { - RTC_DCHECK(tracks != NULL); + RTC_DCHECK(tracks); for (const SsrcInfo& ssrc_info : ssrc_infos) { // According to https://tools.ietf.org/html/rfc5576#section-6.1, the CNAME // attribute is mandatory, but we relax that restriction. @@ -726,16 +726,9 @@ void CreateTracksFromSsrcInfos(const SsrcInfoVec& ssrc_infos, track_id = ssrc_info.track_id; } else { // Since no media streams isn't supported with older SDP signaling, we - // use a default a stream id. + // use a default stream id. stream_ids.push_back(kDefaultMsid); } - // If a track ID wasn't populated from the SSRC attributes OR the - // msid attribute, use default/random values. - if (track_id.empty()) { - // TODO(ronghuawu): What should we do if the track id doesn't appear? - // Create random string (which will be used as track label later)? - track_id = rtc::CreateRandomString(8); - } auto track_it = absl::c_find_if( *tracks, @@ -751,6 +744,14 @@ void CreateTracksFromSsrcInfos(const SsrcInfoVec& ssrc_infos, track.set_stream_ids(stream_ids); track.id = track_id; } + for (StreamParams& stream : *tracks) { + // If a track ID wasn't populated from the SSRC attributes OR the + // msid attribute, use default/random values. This happens after + // deduplication. + if (stream.id.empty()) { + stream.id = rtc::CreateRandomString(8); + } + } } void GetMediaStreamIds(const ContentInfo* content,