Implement RTX tests+fixes in WebRtcVideoEngine2.

BUG=1788
R=pthatcher@webrtc.org

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

git-svn-id: http://webrtc.googlecode.com/svn/trunk@6430 4adac7df-926f-26a2-2b94-8c16560cd09d

talk/media/webrtc/webrtcvideoengine2.cc

@@ -329,6 +329,7 @@ WebRtcVideoChannel2* WebRtcVideoEngine2::CreateChannel(
     delete channel;
     return NULL;
   }
+  channel->SetRecvCodecs(video_codecs_);
   return channel;
 }
 
@@ -725,15 +726,6 @@ bool WebRtcVideoChannel2::Init() { return true; }
 
 namespace {
 
-static bool ValidateCodecFormats(const std::vector<VideoCodec>& codecs) {
-  for (size_t i = 0; i < codecs.size(); ++i) {
-    if (!codecs[i].ValidateCodecFormat()) {
-      return false;
-    }
-  }
-  return true;
-}
-
 static std::string CodecVectorToString(const std::vector<VideoCodec>& codecs) {
   std::stringstream out;
   out << '{';
@@ -747,6 +739,24 @@ static std::string CodecVectorToString(const std::vector<VideoCodec>& codecs) {
   return out.str();
 }
 
+static bool ValidateCodecFormats(const std::vector<VideoCodec>& codecs) {
+  bool has_video = false;
+  for (size_t i = 0; i < codecs.size(); ++i) {
+    if (!codecs[i].ValidateCodecFormat()) {
+      return false;
+    }
+    if (codecs[i].GetCodecType() == VideoCodec::CODEC_VIDEO) {
+      has_video = true;
+    }
+  }
+  if (!has_video) {
+    LOG(LS_ERROR) << "Setting codecs without a video codec is invalid: "
+                  << CodecVectorToString(codecs);
+    return false;
+  }
+  return true;
+}
+
 }  // namespace
 
 bool WebRtcVideoChannel2::SetRecvCodecs(const std::vector<VideoCodec>& codecs) {
@@ -852,15 +862,21 @@ static bool ConfigureSendSsrcs(webrtc::VideoSendStream::Config* config,
     return false;
   }
 
+  // Map RTX SSRCs.
+  std::vector<uint32_t> ssrcs;
+  std::vector<uint32_t> rtx_ssrcs;
   const SsrcGroup* sim_group = sp.get_ssrc_group(kSimSsrcGroupSemantics);
   if (sim_group == NULL) {
-    LOG(LS_ERROR) << "Grouped StreamParams without regular SSRC group: "
+    ssrcs.push_back(sp.first_ssrc());
-                  << sp.ToString();
+    uint32_t rtx_ssrc;
+    if (!sp.GetFidSsrc(sp.first_ssrc(), &rtx_ssrc)) {
+      LOG(LS_ERROR) << "Could not find FID ssrc for primary SSRC '"
+                    << sp.first_ssrc() << "':" << sp.ToString();
       return false;
     }
-
+    rtx_ssrcs.push_back(rtx_ssrc);
-  // Map RTX SSRCs.
+  } else {
-  std::vector<uint32_t> rtx_ssrcs;
+    ssrcs = sim_group->ssrcs;
     for (size_t i = 0; i < sim_group->ssrcs.size(); ++i) {
       uint32_t rtx_ssrc;
       if (!sp.GetFidSsrc(sim_group->ssrcs[i], &rtx_ssrc)) {
@@ -868,14 +884,15 @@ static bool ConfigureSendSsrcs(webrtc::VideoSendStream::Config* config,
       }
       rtx_ssrcs.push_back(rtx_ssrc);
     }
-  if (!rtx_ssrcs.empty() && sim_group->ssrcs.size() != rtx_ssrcs.size()) {
+  }
+  if (!rtx_ssrcs.empty() && ssrcs.size() != rtx_ssrcs.size()) {
     LOG(LS_ERROR)
         << "RTX SSRCs exist, but don't cover all SSRCs (unsupported): "
         << sp.ToString();
     return false;
   }
   config->rtp.rtx.ssrcs = rtx_ssrcs;
-  config->rtp.ssrcs = sim_group->ssrcs;
+  config->rtp.ssrcs = ssrcs;
   return true;
 }
 
@@ -1006,13 +1023,6 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp) {
   webrtc::VideoReceiveStream::Config config = call_->GetDefaultReceiveConfig();
   config.rtp.remote_ssrc = ssrc;
   config.rtp.local_ssrc = rtcp_receiver_report_ssrc_;
-  uint32 rtx_ssrc = 0;
-  if (sp.GetFidSsrc(ssrc, &rtx_ssrc)) {
-    // TODO(pbos): Right now, VideoReceiveStream accepts any rtx payload, this
-    //             should use the actual codec payloads that may be received.
-    //             (for each receive payload, set rtx[payload].ssrc = rtx_ssrc.
-    config.rtp.rtx[0].ssrc = rtx_ssrc;
-  }
 
   config.rtp.nack.rtp_history_ms = kNackHistoryMs;
   config.rtp.remb = true;
@@ -1068,7 +1078,9 @@ bool WebRtcVideoChannel2::AddRecvStream(const StreamParams& sp) {
     for (size_t i = 0; i < recv_codecs_.size(); ++i) {
       if (recv_codecs_[i].codec.id == codec.plType) {
         config.rtp.fec = recv_codecs_[i].fec;
-        if (recv_codecs_[i].rtx_payload_type != -1 && rtx_ssrc != 0) {
+        uint32 rtx_ssrc;
+        if (recv_codecs_[i].rtx_payload_type != -1 &&
+            sp.GetFidSsrc(ssrc, &rtx_ssrc)) {
           config.rtp.rtx[codec.plType].ssrc = rtx_ssrc;
           config.rtp.rtx[codec.plType].payload_type =
               recv_codecs_[i].rtx_payload_type;
@@ -1607,6 +1619,7 @@ WebRtcVideoChannel2::MapCodecs(const std::vector<VideoCodec>& codecs) {
 
   std::vector<VideoCodecSettings> video_codecs;
   std::map<int, bool> payload_used;
+  std::map<int, VideoCodec::CodecType> payload_codec_type;
   std::map<int, int> rtx_mapping;  // video payload type -> rtx payload type.
 
   webrtc::FecConfig fec_settings;
@@ -1621,6 +1634,7 @@ WebRtcVideoChannel2::MapCodecs(const std::vector<VideoCodec>& codecs) {
       return std::vector<VideoCodecSettings>();
     }
     payload_used[payload_type] = true;
+    payload_codec_type[payload_type] = in_codec.GetCodecType();
 
     switch (in_codec.GetCodecType()) {
       case VideoCodec::CODEC_RED: {
@@ -1661,6 +1675,19 @@ WebRtcVideoChannel2::MapCodecs(const std::vector<VideoCodec>& codecs) {
   // parameters into this code is a logic error.
   assert(!video_codecs.empty());
 
+  for (std::map<int, int>::const_iterator it = rtx_mapping.begin();
+       it != rtx_mapping.end();
+       ++it) {
+    if (!payload_used[it->first]) {
+      LOG(LS_ERROR) << "RTX mapped to payload not in codec list.";
+      return std::vector<VideoCodecSettings>();
+    }
+    if (payload_codec_type[it->first] != VideoCodec::CODEC_VIDEO) {
+      LOG(LS_ERROR) << "RTX not mapped to regular video codec.";
+      return std::vector<VideoCodecSettings>();
+    }
+  }
+
   // TODO(pbos): Write tests that figure out that I have not verified that RTX
   // codecs aren't mapped to bogus payloads.
   for (size_t i = 0; i < video_codecs.size(); ++i) {

talk/media/webrtc/webrtcvideoengine2_unittest.cc

@@ -328,6 +328,62 @@ TEST_F(WebRtcVideoEngine2Test, CreateChannelWithVoiceEngine) {
       << "Different VoiceChannel set than the provided one.";
 }
 
+TEST_F(WebRtcVideoEngine2Test, FindCodec) {
+  const std::vector<cricket::VideoCodec>& c = engine_.codecs();
+  EXPECT_EQ(4U, c.size());
+
+  cricket::VideoCodec vp8(104, "VP8", 320, 200, 30, 0);
+  EXPECT_TRUE(engine_.FindCodec(vp8));
+
+  cricket::VideoCodec vp8_ci(104, "vp8", 320, 200, 30, 0);
+  EXPECT_TRUE(engine_.FindCodec(vp8));
+
+  cricket::VideoCodec vp8_diff_fr_diff_pref(104, "VP8", 320, 200, 50, 50);
+  EXPECT_TRUE(engine_.FindCodec(vp8_diff_fr_diff_pref));
+
+  cricket::VideoCodec vp8_diff_id(95, "VP8", 320, 200, 30, 0);
+  EXPECT_FALSE(engine_.FindCodec(vp8_diff_id));
+  vp8_diff_id.id = 97;
+  EXPECT_TRUE(engine_.FindCodec(vp8_diff_id));
+
+  cricket::VideoCodec vp8_diff_res(104, "VP8", 320, 111, 30, 0);
+  EXPECT_FALSE(engine_.FindCodec(vp8_diff_res));
+
+  // PeerConnection doesn't negotiate the resolution at this point.
+  // Test that FindCodec can handle the case when width/height is 0.
+  cricket::VideoCodec vp8_zero_res(104, "VP8", 0, 0, 30, 0);
+  EXPECT_TRUE(engine_.FindCodec(vp8_zero_res));
+
+  cricket::VideoCodec red(101, "RED", 0, 0, 30, 0);
+  EXPECT_TRUE(engine_.FindCodec(red));
+
+  cricket::VideoCodec red_ci(101, "red", 0, 0, 30, 0);
+  EXPECT_TRUE(engine_.FindCodec(red));
+
+  cricket::VideoCodec fec(102, "ULPFEC", 0, 0, 30, 0);
+  EXPECT_TRUE(engine_.FindCodec(fec));
+
+  cricket::VideoCodec fec_ci(102, "ulpfec", 0, 0, 30, 0);
+  EXPECT_TRUE(engine_.FindCodec(fec));
+
+  cricket::VideoCodec rtx(96, "rtx", 0, 0, 30, 0);
+  EXPECT_TRUE(engine_.FindCodec(rtx));
+}
+
+TEST_F(WebRtcVideoEngine2Test, DefaultRtxCodecHasAssociatedPayloadTypeSet) {
+  std::vector<VideoCodec> engine_codecs = engine_.codecs();
+  for (size_t i = 0; i < engine_codecs.size(); ++i) {
+    if (engine_codecs[i].name != kRtxCodecName)
+      continue;
+    int associated_payload_type;
+    EXPECT_TRUE(engine_codecs[i].GetParam(kCodecParamAssociatedPayloadType,
+                                           &associated_payload_type));
+    EXPECT_EQ(default_codec_.id, associated_payload_type);
+    return;
+  }
+  FAIL() << "No RTX codec found among default codecs.";
+}
+
 class WebRtcVideoChannel2BaseTest
     : public VideoMediaChannelTest<WebRtcVideoEngine2, WebRtcVideoChannel2> {
  protected:
@@ -607,7 +663,7 @@ TEST_F(WebRtcVideoChannel2Test, DISABLED_RembEnabledOnReceiveChannels) {
   FAIL() << "Not implemented.";  // TODO(pbos): Implement.
 }
 
-TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) {
+TEST_F(WebRtcVideoChannel2Test, RecvStreamWithSimAndRtx) {
   EXPECT_TRUE(channel_->SetSendCodecs(engine_.codecs()));
   EXPECT_TRUE(channel_->SetSend(true));
   cricket::VideoOptions options;
@@ -636,12 +692,23 @@ TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) {
   // TODO(pbos): Make sure we set the RTX for correct payloads etc.
 }
 
-TEST_F(WebRtcVideoChannel2Test, DISABLED_RecvStreamWithRtxOnMultiplePayloads) {
+TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) {
-  FAIL() << "Not implemented.";
+  // Setup one channel with an associated RTX stream.
+  cricket::StreamParams params =
+      cricket::StreamParams::CreateLegacy(kSsrcs1[0]);
+  params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]);
+  FakeVideoReceiveStream* recv_stream = AddRecvStream(params);
+  ASSERT_EQ(1u, recv_stream->GetConfig().rtp.rtx.size());
+  EXPECT_EQ(kRtxSsrcs1[0],
+            recv_stream->GetConfig().rtp.rtx.begin()->second.ssrc);
 }
 
-TEST_F(WebRtcVideoChannel2Test, DISABLED_RecvStreamNoRtx) {
+TEST_F(WebRtcVideoChannel2Test, RecvStreamNoRtx) {
-  FAIL() << "Not implemented.";  // TODO(pbos): Implement.
+  // Setup one channel without an associated RTX stream.
+  cricket::StreamParams params =
+      cricket::StreamParams::CreateLegacy(kSsrcs1[0]);
+  FakeVideoReceiveStream* recv_stream = AddRecvStream(params);
+  ASSERT_TRUE(recv_stream->GetConfig().rtp.rtx.empty());
 }
 
 TEST_F(WebRtcVideoChannel2Test, DISABLED_RtpTimestampOffsetHeaderExtensions) {
@@ -777,61 +844,6 @@ TEST_F(WebRtcVideoChannel2Test, DISABLED_WebRtcShouldNotLog) {
   FAIL() << "Not implemented.";  // TODO(pbos): Implement.
 }
 
-TEST_F(WebRtcVideoEngine2Test, FindCodec) {
-  const std::vector<cricket::VideoCodec>& c = engine_.codecs();
-  EXPECT_EQ(4U, c.size());
-
-  cricket::VideoCodec vp8(104, "VP8", 320, 200, 30, 0);
-  EXPECT_TRUE(engine_.FindCodec(vp8));
-
-  cricket::VideoCodec vp8_ci(104, "vp8", 320, 200, 30, 0);
-  EXPECT_TRUE(engine_.FindCodec(vp8));
-
-  cricket::VideoCodec vp8_diff_fr_diff_pref(104, "VP8", 320, 200, 50, 50);
-  EXPECT_TRUE(engine_.FindCodec(vp8_diff_fr_diff_pref));
-
-  cricket::VideoCodec vp8_diff_id(95, "VP8", 320, 200, 30, 0);
-  EXPECT_FALSE(engine_.FindCodec(vp8_diff_id));
-  vp8_diff_id.id = 97;
-  EXPECT_TRUE(engine_.FindCodec(vp8_diff_id));
-
-  cricket::VideoCodec vp8_diff_res(104, "VP8", 320, 111, 30, 0);
-  EXPECT_FALSE(engine_.FindCodec(vp8_diff_res));
-
-  // PeerConnection doesn't negotiate the resolution at this point.
-  // Test that FindCodec can handle the case when width/height is 0.
-  cricket::VideoCodec vp8_zero_res(104, "VP8", 0, 0, 30, 0);
-  EXPECT_TRUE(engine_.FindCodec(vp8_zero_res));
-
-  cricket::VideoCodec red(101, "RED", 0, 0, 30, 0);
-  EXPECT_TRUE(engine_.FindCodec(red));
-
-  cricket::VideoCodec red_ci(101, "red", 0, 0, 30, 0);
-  EXPECT_TRUE(engine_.FindCodec(red));
-
-  cricket::VideoCodec fec(102, "ULPFEC", 0, 0, 30, 0);
-  EXPECT_TRUE(engine_.FindCodec(fec));
-
-  cricket::VideoCodec fec_ci(102, "ulpfec", 0, 0, 30, 0);
-  EXPECT_TRUE(engine_.FindCodec(fec));
-
-  cricket::VideoCodec rtx(96, "rtx", 0, 0, 30, 0);
-  EXPECT_TRUE(engine_.FindCodec(rtx));
-}
-
-TEST_F(WebRtcVideoEngine2Test, DefaultRtxCodecHasAssociatedPayloadTypeSet) {
-  for (size_t i = 0; i < engine_.codecs().size(); ++i) {
-    if (engine_.codecs()[i].name != kRtxCodecName)
-      continue;
-    int associated_payload_type;
-    EXPECT_TRUE(engine_.codecs()[i].GetParam(kCodecParamAssociatedPayloadType,
-                                             &associated_payload_type));
-    EXPECT_EQ(default_codec_.id, associated_payload_type);
-    return;
-  }
-  FAIL() << "No RTX codec found among default codecs.";
-}
-
 TEST_F(WebRtcVideoChannel2Test, SetDefaultSendCodecs) {
   ASSERT_TRUE(channel_->SetSendCodecs(engine_.codecs()));
 
@@ -975,6 +987,31 @@ TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithOnlyVp8) {
   EXPECT_TRUE(channel_->SetRecvCodecs(codecs));
 }
 
+// Test that we set our inbound RTX codecs properly.
+TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithRtx) {
+  std::vector<cricket::VideoCodec> codecs;
+  codecs.push_back(kVp8Codec);
+  cricket::VideoCodec rtx_codec(96, "rtx", 0, 0, 0, 0);
+  codecs.push_back(rtx_codec);
+  EXPECT_FALSE(channel_->SetRecvCodecs(codecs))
+      << "RTX codec without associated payload should be rejected.";
+
+  codecs[1].SetParam("apt", kVp8Codec.id + 1);
+  EXPECT_FALSE(channel_->SetRecvCodecs(codecs))
+      << "RTX codec with invalid associated payload type should be rejected.";
+
+  codecs[1].SetParam("apt", kVp8Codec.id);
+  EXPECT_TRUE(channel_->SetRecvCodecs(codecs));
+
+  cricket::VideoCodec rtx_codec2(97, "rtx", 0, 0, 0, 0);
+  rtx_codec2.SetParam("apt", rtx_codec.id);
+  codecs.push_back(rtx_codec2);
+
+  EXPECT_FALSE(channel_->SetRecvCodecs(codecs)) << "RTX codec with another RTX "
+                                                   "as associated payload type "
+                                                   "should be rejected.";
+}
+
 TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsDifferentPayloadType) {
   std::vector<cricket::VideoCodec> codecs;
   codecs.push_back(kVp8Codec);