From 2e7a09800595d4d82f67acfd7de04794642cef7d Mon Sep 17 00:00:00 2001
From: Noah Richards <noahric@chromium.org>
Date: Mon, 18 May 2015 14:02:54 -0700
Subject: [PATCH] Ensure mediasession generated offers with RTX contain an RTX
 ssrc for each video ssrc.

BUG=
R=pthatcher@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/49989004

Cr-Commit-Position: refs/heads/master@{#9210}
---
 talk/session/media/mediasession.cc          | 22 ++++++-----
 talk/session/media/mediasession_unittest.cc | 41 +++++++++++++++++++++
 2 files changed, 54 insertions(+), 9 deletions(-)

diff --git a/talk/session/media/mediasession.cc b/talk/session/media/mediasession.cc
index b72851796..b29994451 100644
--- a/talk/session/media/mediasession.cc
+++ b/talk/session/media/mediasession.cc
@@ -436,8 +436,8 @@ static bool AddStreamParams(
     StreamParamsVec* current_streams,
     MediaContentDescriptionImpl<C>* content_description,
     const bool add_legacy_stream) {
-  const bool include_rtx_stream =
-    ContainsRtxCodec(content_description->codecs());
+  const bool include_rtx_streams =
+      ContainsRtxCodec(content_description->codecs());
 
   if (streams.empty() && add_legacy_stream) {
     // TODO(perkj): Remove this legacy stream when all apps use StreamParams.
@@ -445,10 +445,10 @@ static bool AddStreamParams(
     if (IsSctp(content_description)) {
       GenerateSctpSids(*current_streams, &ssrcs);
     } else {
-      int num_ssrcs = include_rtx_stream ? 2 : 1;
+      int num_ssrcs = include_rtx_streams ? 2 : 1;
       GenerateSsrcs(*current_streams, num_ssrcs, &ssrcs);
     }
-    if (include_rtx_stream) {
+    if (include_rtx_streams) {
       content_description->AddLegacyStream(ssrcs[0], ssrcs[1]);
       content_description->set_multistream(true);
     } else {
@@ -492,11 +492,15 @@ static bool AddStreamParams(
         SsrcGroup group(kSimSsrcGroupSemantics, stream_param.ssrcs);
         stream_param.ssrc_groups.push_back(group);
       }
-      // Generate an extra ssrc for include_rtx_stream case.
-      if (include_rtx_stream) {
-        std::vector<uint32> rtx_ssrc;
-        GenerateSsrcs(*current_streams, 1, &rtx_ssrc);
-        stream_param.AddFidSsrc(ssrcs[0], rtx_ssrc[0]);
+      // Generate extra ssrcs for include_rtx_streams case.
+      if (include_rtx_streams) {
+        // Generate an RTX ssrc for every ssrc in the group.
+        std::vector<uint32> rtx_ssrcs;
+        GenerateSsrcs(*current_streams, static_cast<int>(ssrcs.size()),
+                      &rtx_ssrcs);
+        for (size_t i = 0; i < ssrcs.size(); ++i) {
+          stream_param.AddFidSsrc(ssrcs[i], rtx_ssrcs[i]);
+        }
         content_description->set_multistream(true);
       }
       stream_param.cname = cname;
diff --git a/talk/session/media/mediasession_unittest.cc b/talk/session/media/mediasession_unittest.cc
index f487baa9c..7fd1d6382 100644
--- a/talk/session/media/mediasession_unittest.cc
+++ b/talk/session/media/mediasession_unittest.cc
@@ -1767,6 +1767,47 @@ TEST_F(MediaSessionDescriptionFactoryTest,
   EXPECT_EQ(expected_codecs, vcd->codecs());
 }
 
+// Test that when RTX is used in conjunction with simulcast, an RTX ssrc is
+// generated for each simulcast ssrc and correctly grouped.
+TEST_F(MediaSessionDescriptionFactoryTest, SimSsrcsGenerateMultipleRtxSsrcs) {
+  MediaSessionOptions opts;
+  opts.recv_video = true;
+  opts.recv_audio = false;
+
+  // Add simulcast streams.
+  opts.AddSendVideoStream("stream1", "stream1label", 3);
+
+  // Use a single real codec, and then add RTX for it.
+  std::vector<VideoCodec> f1_codecs;
+  f1_codecs.push_back(VideoCodec(97, "H264", 320, 200, 30, 1));
+  AddRtxCodec(VideoCodec::CreateRtxCodec(125, 97), &f1_codecs);
+  f1_.set_video_codecs(f1_codecs);
+
+  // Ensure that the offer has an RTX ssrc for each regular ssrc, and that there
+  // is a FID ssrc + grouping for each.
+  rtc::scoped_ptr<SessionDescription> offer(f1_.CreateOffer(opts, NULL));
+  ASSERT_TRUE(offer.get() != NULL);
+  VideoContentDescription* desc = static_cast<VideoContentDescription*>(
+      offer->GetContentDescriptionByName(cricket::CN_VIDEO));
+  ASSERT_TRUE(desc != NULL);
+  EXPECT_TRUE(desc->multistream());
+  const StreamParamsVec& streams = desc->streams();
+  // Single stream.
+  ASSERT_EQ(1u, streams.size());
+  // Stream should have 6 ssrcs: 3 for video, 3 for RTX.
+  EXPECT_EQ(6u, streams[0].ssrcs.size());
+  // And should have a SIM group for the simulcast.
+  EXPECT_TRUE(streams[0].has_ssrc_group("SIM"));
+  // And a FID group for RTX.
+  EXPECT_TRUE(streams[0].has_ssrc_group("FID"));
+  std::vector<uint32> primary_ssrcs;
+  streams[0].GetPrimarySsrcs(&primary_ssrcs);
+  EXPECT_EQ(3u, primary_ssrcs.size());
+  std::vector<uint32> fid_ssrcs;
+  streams[0].GetFidSsrcs(primary_ssrcs, &fid_ssrcs);
+  EXPECT_EQ(3u, fid_ssrcs.size());
+}
+
 // Create an updated offer after creating an answer to the original offer and
 // verify that the RTP header extensions that were part of the original answer
 // are not changed in the updated offer.