From 0776415a412c82bb698ef60f7a434be9f1360d50 Mon Sep 17 00:00:00 2001 From: Philipp Hancke Date: Mon, 26 Jun 2023 12:27:31 +0200 Subject: [PATCH] Generalize stream parameter primary/secondary ssrc checks to ensure consistency for both FID and FEC-FR ssrc-groups. BUG=chromium:1454860 Change-Id: I61277e73e0a28f5773260ec62c268bdc8c2cd738 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/309760 Reviewed-by: Florent Castelli Reviewed-by: Harald Alvestrand Commit-Queue: Philipp Hancke Cr-Commit-Position: refs/heads/main@{#40347} --- media/base/stream_params.cc | 17 +++++--- media/base/stream_params.h | 8 ++++ media/base/test_utils.cc | 12 ++---- media/engine/webrtc_video_engine.cc | 45 +++++++++++++--------- pc/sdp_offer_answer.cc | 24 ++++++++++++ pc/sdp_offer_answer_unittest.cc | 60 +++++++++++++++++++++++++++++ 6 files changed, 133 insertions(+), 33 deletions(-) diff --git a/media/base/stream_params.cc b/media/base/stream_params.cc index 0fe1be6ac7..ac9daee200 100644 --- a/media/base/stream_params.cc +++ b/media/base/stream_params.cc @@ -183,16 +183,23 @@ void StreamParams::GetPrimarySsrcs(std::vector* ssrcs) const { } } -void StreamParams::GetFidSsrcs(const std::vector& primary_ssrcs, - std::vector* fid_ssrcs) const { +void StreamParams::GetSecondarySsrcs( + const std::string& semantics, + const std::vector& primary_ssrcs, + std::vector* secondary_ssrcs) const { for (uint32_t primary_ssrc : primary_ssrcs) { - uint32_t fid_ssrc; - if (GetFidSsrc(primary_ssrc, &fid_ssrc)) { - fid_ssrcs->push_back(fid_ssrc); + uint32_t secondary_ssrc; + if (GetSecondarySsrc(semantics, primary_ssrc, &secondary_ssrc)) { + secondary_ssrcs->push_back(secondary_ssrc); } } } +void StreamParams::GetFidSsrcs(const std::vector& primary_ssrcs, + std::vector* fid_ssrcs) const { + return GetSecondarySsrcs(kFidSsrcGroupSemantics, primary_ssrcs, fid_ssrcs); +} + bool StreamParams::AddSecondarySsrc(const std::string& semantics, uint32_t primary_ssrc, uint32_t secondary_ssrc) { diff --git a/media/base/stream_params.h b/media/base/stream_params.h index 60c67a1a1c..89fc1554cc 100644 --- a/media/base/stream_params.h +++ b/media/base/stream_params.h @@ -166,6 +166,14 @@ struct StreamParams { // the first SSRC otherwise. void GetPrimarySsrcs(std::vector* ssrcs) const; + // Convenience to get all the secondary SSRCs for the given primary ssrcs + // of a particular semantic. + // If a given primary SSRC does not have a secondary SSRC, the list of + // secondary SSRCS will be smaller than the list of primary SSRCs. + void GetSecondarySsrcs(const std::string& semantic, + const std::vector& primary_ssrcs, + std::vector* fid_ssrcs) const; + // Convenience to get all the FID SSRCs for the given primary ssrcs. // If a given primary SSRC does not have a FID SSRC, the list of FID // SSRCS will be smaller than the list of primary SSRCs. diff --git a/media/base/test_utils.cc b/media/base/test_utils.cc index a6d5f61c17..1b288735be 100644 --- a/media/base/test_utils.cc +++ b/media/base/test_utils.cc @@ -35,26 +35,20 @@ cricket::StreamParams CreateSimWithRtxStreamParams( const std::vector& rtx_ssrcs) { cricket::StreamParams sp = CreateSimStreamParams(cname, ssrcs); for (size_t i = 0; i < ssrcs.size(); ++i) { - sp.ssrcs.push_back(rtx_ssrcs[i]); - std::vector fid_ssrcs; - fid_ssrcs.push_back(ssrcs[i]); - fid_ssrcs.push_back(rtx_ssrcs[i]); - cricket::SsrcGroup fid_group(cricket::kFidSsrcGroupSemantics, fid_ssrcs); - sp.ssrc_groups.push_back(fid_group); + sp.AddFidSsrc(ssrcs[i], rtx_ssrcs[i]); } return sp; } +// There should be one fec ssrc per ssrc. cricket::StreamParams CreatePrimaryWithFecFrStreamParams( const std::string& cname, uint32_t primary_ssrc, uint32_t flexfec_ssrc) { cricket::StreamParams sp; - cricket::SsrcGroup sg(cricket::kFecFrSsrcGroupSemantics, - {primary_ssrc, flexfec_ssrc}); sp.ssrcs = {primary_ssrc}; - sp.ssrc_groups.push_back(sg); sp.cname = cname; + sp.AddFecFrSsrc(primary_ssrc, flexfec_ssrc); return sp; } diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index a8309f541f..83bb39d446 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -311,30 +311,37 @@ static bool ValidateStreamParams(const StreamParams& sp) { std::vector primary_ssrcs; sp.GetPrimarySsrcs(&primary_ssrcs); - std::vector rtx_ssrcs; - sp.GetFidSsrcs(primary_ssrcs, &rtx_ssrcs); - for (uint32_t rtx_ssrc : rtx_ssrcs) { - bool rtx_ssrc_present = false; - for (uint32_t sp_ssrc : sp.ssrcs) { - if (sp_ssrc == rtx_ssrc) { - rtx_ssrc_present = true; - break; + for (const auto& semantic : + {kFidSsrcGroupSemantics, kFecFrSsrcGroupSemantics}) { + if (!sp.has_ssrc_group(semantic)) { + continue; + } + std::vector secondary_ssrcs; + sp.GetSecondarySsrcs(semantic, primary_ssrcs, &secondary_ssrcs); + for (uint32_t secondary_ssrc : secondary_ssrcs) { + bool secondary_ssrc_present = false; + for (uint32_t sp_ssrc : sp.ssrcs) { + if (sp_ssrc == secondary_ssrc) { + secondary_ssrc_present = true; + break; + } + } + if (!secondary_ssrc_present) { + RTC_LOG(LS_ERROR) << "SSRC '" << secondary_ssrc + << "' missing from StreamParams ssrcs with semantics " + << semantic << ": " << sp.ToString(); + return false; } } - if (!rtx_ssrc_present) { - RTC_LOG(LS_ERROR) << "RTX SSRC '" << rtx_ssrc - << "' missing from StreamParams ssrcs: " - << sp.ToString(); + if (!secondary_ssrcs.empty() && + primary_ssrcs.size() != secondary_ssrcs.size()) { + RTC_LOG(LS_ERROR) + << semantic + << " secondary SSRCs exist, but don't cover all SSRCs (unsupported): " + << sp.ToString(); return false; } } - if (!rtx_ssrcs.empty() && primary_ssrcs.size() != rtx_ssrcs.size()) { - RTC_LOG(LS_ERROR) - << "RTX SSRCs exist, but don't cover all SSRCs (unsupported): " - << sp.ToString(); - return false; - } - return true; } diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 6596d82167..fa16ec6282 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -491,6 +491,25 @@ RTCError ValidateBundledPayloadTypes( return RTCError::OK(); } +RTCError ValidateSsrcGroups(const cricket::SessionDescription& description) { + // https://www.rfc-editor.org/rfc/rfc5576#section-4.2 + // Every listed in an "ssrc-group" attribute MUST be + // defined by a corresponding "ssrc:" line in the same media + // description. + for (const ContentInfo& content : description.contents()) { + if (!content.media_description()) { + continue; + } + for (const StreamParams& sp : content.media_description()->streams()) { + for (const cricket::SsrcGroup& group : sp.ssrc_groups) { + RTC_LOG(LS_ERROR) << "YO " << group.semantics << " #" + << group.ssrcs.size(); + } + } + } + return RTCError::OK(); +} + RTCError FindDuplicateHeaderExtensionIds( const RtpExtension extension, std::map& id_to_extension) { @@ -3567,6 +3586,11 @@ RTCError SdpOfferAnswerHandler::ValidateSessionDescription( return RTCError(RTCErrorType::INVALID_PARAMETER, kBundleWithoutRtcpMux); } + error = ValidateSsrcGroups(*sdesc->description()); + if (!error.ok()) { + return error; + } + // TODO(skvlad): When the local rtcp-mux policy is Require, reject any // m-lines that do not rtcp-mux enabled. diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc index e9ec17bf08..604ad270b4 100644 --- a/pc/sdp_offer_answer_unittest.cc +++ b/pc/sdp_offer_answer_unittest.cc @@ -599,4 +599,64 @@ TEST_F(SdpOfferAnswerTest, SimulcastAnswerWithNoRidsIsRejected) { EXPECT_TRUE(pc->SetRemoteDescription(std::move(answer_with_extensions))); } +TEST_F(SdpOfferAnswerTest, ExpectAllSsrcsSpecifiedInSsrcGroupFid) { + 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=rtpmap:96 H264/90000\r\n" + "a=fmtp:96 " + "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=" + "42e01f\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"; + auto offer = CreateSessionDescription(SdpType::kOffer, sdp); + EXPECT_FALSE(pc->SetRemoteDescription(std::move(offer))); +} + +TEST_F(SdpOfferAnswerTest, ExpectAllSsrcsSpecifiedInSsrcGroupFecFr) { + 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 98\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=rtpmap:96 H264/90000\r\n" + "a=fmtp:96 " + "level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=" + "42e01f\r\n" + "a=rtpmap:98 flexfec-03/90000\r\n" + "a=fmtp:98 repair-window=10000000\r\n" + "a=ssrc-group:FEC-FR 1 2\r\n" + "a=ssrc:1 cname:test\r\n"; + auto offer = CreateSessionDescription(SdpType::kOffer, sdp); + EXPECT_FALSE(pc->SetRemoteDescription(std::move(offer))); +} + } // namespace webrtc