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 <orphis@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#40347}

media/base/stream_params.cc

@@ -183,16 +183,23 @@ void StreamParams::GetPrimarySsrcs(std::vector<uint32_t>* ssrcs) const {
   }
 }
 
-void StreamParams::GetFidSsrcs(const std::vector<uint32_t>& primary_ssrcs,
+void StreamParams::GetSecondarySsrcs(
-                               std::vector<uint32_t>* fid_ssrcs) const {
+    const std::string& semantics,
+    const std::vector<uint32_t>& primary_ssrcs,
+    std::vector<uint32_t>* secondary_ssrcs) const {
   for (uint32_t primary_ssrc : primary_ssrcs) {
-    uint32_t fid_ssrc;
+    uint32_t secondary_ssrc;
-    if (GetFidSsrc(primary_ssrc, &fid_ssrc)) {
+    if (GetSecondarySsrc(semantics, primary_ssrc, &secondary_ssrc)) {
-      fid_ssrcs->push_back(fid_ssrc);
+      secondary_ssrcs->push_back(secondary_ssrc);
     }
   }
 }
 
+void StreamParams::GetFidSsrcs(const std::vector<uint32_t>& primary_ssrcs,
+                               std::vector<uint32_t>* 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) {

media/base/stream_params.h

@@ -166,6 +166,14 @@ struct StreamParams {
   // the first SSRC otherwise.
   void GetPrimarySsrcs(std::vector<uint32_t>* 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<uint32_t>& primary_ssrcs,
+                         std::vector<uint32_t>* 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.

media/base/test_utils.cc

@@ -35,26 +35,20 @@ cricket::StreamParams CreateSimWithRtxStreamParams(
     const std::vector<uint32_t>& rtx_ssrcs) {
   cricket::StreamParams sp = CreateSimStreamParams(cname, ssrcs);
   for (size_t i = 0; i < ssrcs.size(); ++i) {
-    sp.ssrcs.push_back(rtx_ssrcs[i]);
+    sp.AddFidSsrc(ssrcs[i], rtx_ssrcs[i]);
-    std::vector<uint32_t> 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);
   }
   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;
 }
 

media/engine/webrtc_video_engine.cc

@@ -311,30 +311,37 @@ static bool ValidateStreamParams(const StreamParams& sp) {
 
   std::vector<uint32_t> primary_ssrcs;
   sp.GetPrimarySsrcs(&primary_ssrcs);
-  std::vector<uint32_t> rtx_ssrcs;
+  for (const auto& semantic :
-  sp.GetFidSsrcs(primary_ssrcs, &rtx_ssrcs);
+       {kFidSsrcGroupSemantics, kFecFrSsrcGroupSemantics}) {
-  for (uint32_t rtx_ssrc : rtx_ssrcs) {
+    if (!sp.has_ssrc_group(semantic)) {
-    bool rtx_ssrc_present = false;
+      continue;
-    for (uint32_t sp_ssrc : sp.ssrcs) {
+    }
-      if (sp_ssrc == rtx_ssrc) {
+    std::vector<uint32_t> secondary_ssrcs;
-        rtx_ssrc_present = true;
+    sp.GetSecondarySsrcs(semantic, primary_ssrcs, &secondary_ssrcs);
-        break;
+    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) {
+    if (!secondary_ssrcs.empty() &&
-      RTC_LOG(LS_ERROR) << "RTX SSRC '" << rtx_ssrc
+        primary_ssrcs.size() != secondary_ssrcs.size()) {
-                        << "' missing from StreamParams ssrcs: "
+      RTC_LOG(LS_ERROR)
-                        << sp.ToString();
+          << 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;
 }
 

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 <ssrc-id> 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<int, RtpExtension>& 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.
 

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