Generalize ssrc-group check to apply to groups other than SIM

BUG=chromium:1477075

Change-Id: I20f094dee11ea26a180471ce52d78d916f922f29
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/322440
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#40888}

media/engine/webrtc_video_engine.cc

@@ -356,8 +356,11 @@ static bool ValidateStreamParams(const StreamParams& sp) {
     }
   }
   for (const auto& group : sp.ssrc_groups) {
-    if (group.semantics != kSimSsrcGroupSemantics)
+    if (!(group.semantics == kFidSsrcGroupSemantics ||
+          group.semantics == kSimSsrcGroupSemantics ||
+          group.semantics == kFecFrSsrcGroupSemantics)) {
       continue;
+    }
     for (uint32_t group_ssrc : group.ssrcs) {
       auto it = absl::c_find_if(sp.ssrcs, [&group_ssrc](uint32_t ssrc) {
         return ssrc == group_ssrc;
@@ -365,7 +368,7 @@ static bool ValidateStreamParams(const StreamParams& sp) {
       if (it == sp.ssrcs.end()) {
         RTC_LOG(LS_ERROR) << "SSRC '" << group_ssrc
                           << "' missing from StreamParams ssrcs with semantics "
-                          << kSimSsrcGroupSemantics << ": " << sp.ToString();
+                          << group.semantics << ": " << sp.ToString();
         return false;
       }
     }

pc/peer_connection_bundle_unittest.cc

@@ -60,6 +60,7 @@
 #include "pc/rtp_transport_internal.h"
 #include "pc/sdp_utils.h"
 #include "pc/session_description.h"
+#include "pc/test/integration_test_helpers.h"
 #include "pc/test/mock_peer_connection_observers.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/logging.h"
@@ -90,8 +91,6 @@ using ::testing::ElementsAre;
 using ::testing::UnorderedElementsAre;
 using ::testing::Values;
 
-constexpr int kDefaultTimeout = 10000;
-
 // TODO(steveanton): These tests should be rewritten to use the standard
 // RtpSenderInterface/DtlsTransportInterface objects once they're available in
 // the API. The RtpSender can be used to determine which transport a given media
@@ -743,18 +742,18 @@ TEST_P(PeerConnectionBundleTest, ApplyDescriptionWithSameSsrcsBundledFails) {
       caller->SetLocalDescription(CloneSessionDescription(offer.get())));
   // Modify the remote SDP to make two m= sections have the same SSRC.
   ASSERT_GE(offer->description()->contents().size(), 2U);
-  offer->description()
+  ReplaceFirstSsrc(offer->description()
                        ->contents()[0]
                        .media_description()
-      ->mutable_streams()[0]
+                       ->mutable_streams()[0],
-      .ssrcs[0] = 1111222;
+                   1111222);
-  offer->description()
+  ReplaceFirstSsrc(offer->description()
                        ->contents()[1]
                        .media_description()
-      ->mutable_streams()[0]
+                       ->mutable_streams()[0],
-      .ssrcs[0] = 1111222;
+                   1111222);
-  EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer)));
 
+  EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer)));
   // When BUNDLE is enabled, applying the description is expected to fail
   // because the demuxing criteria can not be satisfied.
   auto answer = callee->CreateAnswer(options);
@@ -774,16 +773,16 @@ TEST_P(PeerConnectionBundleTest,
       caller->SetLocalDescription(CloneSessionDescription(offer.get())));
   // Modify the remote SDP to make two m= sections have the same SSRC.
   ASSERT_GE(offer->description()->contents().size(), 2U);
-  offer->description()
+  ReplaceFirstSsrc(offer->description()
                        ->contents()[0]
                        .media_description()
-      ->mutable_streams()[0]
+                       ->mutable_streams()[0],
-      .ssrcs[0] = 1111222;
+                   1111222);
-  offer->description()
+  ReplaceFirstSsrc(offer->description()
                        ->contents()[1]
                        .media_description()
-      ->mutable_streams()[0]
+                       ->mutable_streams()[0],
-      .ssrcs[0] = 1111222;
+                   1111222);
   EXPECT_TRUE(callee->SetRemoteDescription(std::move(offer)));
 
   // Without BUNDLE, demuxing is done per-transport.

pc/peer_connection_interface_unittest.cc

@@ -3004,21 +3004,20 @@ TEST_P(PeerConnectionInterfaceTest,
   EXPECT_TRUE(ContainsSender(senders, kVideoTracks[0]));
 
   // Change the ssrc of the audio and video track.
-  cricket::MediaContentDescription* desc =
+  for (auto content : modified_offer->description()->contents()) {
-      cricket::GetFirstAudioContentDescription(modified_offer->description());
+    cricket::MediaContentDescription* desc = content.media_description();
-  ASSERT_TRUE(desc != nullptr);
+    ASSERT_TRUE(desc);
     for (StreamParams& stream : desc->mutable_streams()) {
       for (unsigned int& ssrc : stream.ssrcs) {
-      ++ssrc;
+        unsigned int old_ssrc = ssrc++;
+        for (auto& group : stream.ssrc_groups) {
+          for (unsigned int& secondary_ssrc : group.ssrcs) {
+            if (secondary_ssrc == old_ssrc) {
+              secondary_ssrc = ssrc;
+            }
+          }
         }
       }
-
-  desc =
-      cricket::GetFirstVideoContentDescription(modified_offer->description());
-  ASSERT_TRUE(desc != nullptr);
-  for (StreamParams& stream : desc->mutable_streams()) {
-    for (unsigned int& ssrc : stream.ssrcs) {
-      ++ssrc;
     }
   }
 

pc/peer_connection_rtp_unittest.cc

@@ -53,6 +53,7 @@
 #include "pc/sdp_utils.h"
 #include "pc/session_description.h"
 #include "pc/test/fake_audio_capture_module.h"
+#include "pc/test/integration_test_helpers.h"
 #include "pc/test/mock_peer_connection_observers.h"
 #include "rtc_base/checks.h"
 #include "rtc_base/gunit.h"
@@ -73,8 +74,6 @@ using ::testing::Pair;
 using ::testing::UnorderedElementsAre;
 using ::testing::Values;
 
-const uint32_t kDefaultTimeout = 10000u;
-
 template <typename MethodFunctor>
 class OnSuccessObserver : public webrtc::SetRemoteDescriptionObserverInterface {
  public:
@@ -826,7 +825,8 @@ TEST_F(PeerConnectionRtpTestUnifiedPlan, TracksDoNotEndWhenSsrcChanges) {
   for (size_t i = 0; i < contents.size(); ++i) {
     auto& mutable_streams = contents[i].media_description()->mutable_streams();
     ASSERT_EQ(mutable_streams.size(), 1u);
-    mutable_streams[0].ssrcs[0] = kFirstMungedSsrc + static_cast<uint32_t>(i);
+    ReplaceFirstSsrc(mutable_streams[0],
+                     kFirstMungedSsrc + static_cast<uint32_t>(i));
   }
   ASSERT_TRUE(
       callee->SetLocalDescription(CloneSessionDescription(answer.get())));

pc/test/integration_test_helpers.cc

@@ -55,6 +55,13 @@ int FindFirstMediaStatsIndexByKind(
   return -1;
 }
 
+void ReplaceFirstSsrc(StreamParams& stream, uint32_t ssrc) {
+  stream.ssrcs[0] = ssrc;
+  for (auto& group : stream.ssrc_groups) {
+    group.ssrcs[0] = ssrc;
+  }
+}
+
 TaskQueueMetronome::TaskQueueMetronome(TimeDelta tick_period)
     : tick_period_(tick_period) {}
 

pc/test/integration_test_helpers.h

@@ -173,6 +173,10 @@ void RemoveSsrcsAndMsids(cricket::SessionDescription* desc);
 // endpoint that only signals a=msid lines to convey stream_ids.
 void RemoveSsrcsAndKeepMsids(cricket::SessionDescription* desc);
 
+// Replaces the stream's primary SSRC and updates the first SSRC of all
+// ssrc-groups.
+void ReplaceFirstSsrc(StreamParams& stream, uint32_t ssrc);
+
 int FindFirstMediaStatsIndexByKind(
     const std::string& kind,
     const std::vector<const webrtc::RTCInboundRtpStreamStats*>& inbound_rtps);