Make RTX pt/apt reconfigurable by calling WebRtcVideoChannel2::SetRecvParameters.

webrtc/media/engine/webrtcvideoengine2_unittest.cc

 /*
  *  Copyright (c) 2004 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 1210 matching lines @@
   FakeVideoSendStream* send_stream = AddSendStream(
       cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs));
 
   ASSERT_EQ(rtx_ssrcs.size(), send_stream->GetConfig().rtp.rtx.ssrcs.size());
   for (size_t i = 0; i < rtx_ssrcs.size(); ++i)
     EXPECT_EQ(rtx_ssrcs[i], send_stream->GetConfig().rtp.rtx.ssrcs[i]);
 
   // Receiver side.
   FakeVideoReceiveStream* recv_stream = AddRecvStream(
       cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs));
-  EXPECT_FALSE(recv_stream->GetConfig().rtp.rtx.empty());
+  EXPECT_FALSE(recv_stream->GetConfig().rtp.rtx_payload_types.empty());
   EXPECT_EQ(recv_stream->GetConfig().decoders.size(),
-            recv_stream->GetConfig().rtp.rtx.size())
+            recv_stream->GetConfig().rtp.rtx_payload_types.size())
       << "RTX should be mapped for all decoders/payload types.";
-  for (const auto& kv : recv_stream->GetConfig().rtp.rtx) {
-    EXPECT_EQ(rtx_ssrcs[0], kv.second.ssrc);
-  }
+  EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc);
 }
 
 TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) {
   // 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);
-  EXPECT_EQ(kRtxSsrcs1[0],
-            recv_stream->GetConfig().rtp.rtx.begin()->second.ssrc);
+  EXPECT_EQ(kRtxSsrcs1[0], recv_stream->GetConfig().rtp.rtx_ssrc);
 }
 
 TEST_F(WebRtcVideoChannel2Test, RecvStreamNoRtx) {
   // 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());
+  ASSERT_EQ(0U, recv_stream->GetConfig().rtp.rtx_ssrc);
 }
 
 TEST_F(WebRtcVideoChannel2Test, NoHeaderExtesionsByDefault) {
   FakeVideoSendStream* send_stream =
       AddSendStream(cricket::StreamParams::CreateLegacy(kSsrcs1[0]));
   ASSERT_TRUE(send_stream->GetConfig().rtp.extensions.empty());
 
   FakeVideoReceiveStream* recv_stream =
       AddRecvStream(cricket::StreamParams::CreateLegacy(kSsrcs1[0]));
   ASSERT_TRUE(recv_stream->GetConfig().rtp.extensions.empty());
@@ skipping 1188 matching lines @@
     cricket::VideoCodec rtx_codec = cricket::VideoCodec::CreateRtxCodec(
         kUnusedPayloadType1, kUnusedPayloadType2);
     cricket::VideoSendParameters parameters;
     parameters.codecs.push_back(GetEngineCodec("VP8"));
     parameters.codecs.push_back(rtx_codec);
     EXPECT_FALSE(channel_->SetSendParameters(parameters))
         << "RTX without matching video codec should be rejected.";
   }
 }
 
+TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithChangedRtxPayloadType) {
+  const int kUnusedPayloadType1 = 126;

[brandtr, 2017/01/20 14:49:23]

Prior to this CL, this test already passes. I.e., the reconfiguring on the send
side is not a problem.

+  const int kUnusedPayloadType2 = 127;
+  EXPECT_FALSE(FindCodecById(engine_.codecs(), kUnusedPayloadType1));
+  EXPECT_FALSE(FindCodecById(engine_.codecs(), kUnusedPayloadType2));
+
+  // SSRCs for RTX.
+  cricket::StreamParams params =
+      cricket::StreamParams::CreateLegacy(kSsrcs1[0]);
+  params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]);
+  AddSendStream(params);
+
+  // Original payload type for RTX.
+  cricket::VideoSendParameters parameters;
+  parameters.codecs.push_back(GetEngineCodec("VP8"));
+  cricket::VideoCodec rtx_codec(kUnusedPayloadType1, "rtx");
+  rtx_codec.SetParam("apt", GetEngineCodec("VP8").id);
+  parameters.codecs.push_back(rtx_codec);
+  EXPECT_TRUE(channel_->SetSendParameters(parameters));
+  ASSERT_EQ(1U, fake_call_->GetVideoSendStreams().size());
+  const webrtc::VideoSendStream::Config& config_before =
+      fake_call_->GetVideoSendStreams()[0]->GetConfig();
+  EXPECT_EQ(kUnusedPayloadType1, config_before.rtp.rtx.payload_type);
+  ASSERT_EQ(1U, config_before.rtp.rtx.ssrcs.size());
+  EXPECT_EQ(kRtxSsrcs1[0], config_before.rtp.rtx.ssrcs[0]);
+
+  // Change payload type for RTX.
+  parameters.codecs[1].id = kUnusedPayloadType2;
+  EXPECT_TRUE(channel_->SetSendParameters(parameters));
+  ASSERT_EQ(1U, fake_call_->GetVideoSendStreams().size());
+  const webrtc::VideoSendStream::Config& config_after =
+      fake_call_->GetVideoSendStreams()[0]->GetConfig();
+  EXPECT_EQ(kUnusedPayloadType2, config_after.rtp.rtx.payload_type);
+  ASSERT_EQ(1U, config_before.rtp.rtx.ssrcs.size());
+  EXPECT_EQ(kRtxSsrcs1[0], config_before.rtp.rtx.ssrcs[0]);

[stefan-webrtc, 2017/01/23 12:45:11]

Is it possible to break this code out into a helper function that can be used
both here and in SetRecvCodecsWithChangedRtxPayloadType? Looks like it could be
possible, maybe with a template function.

May not be worth it, I'll leave it to you to decide. :)

[brandtr, 2017/01/24 10:04:41]

It would reduce the verbosity a bit, but I think it would reduce clarity for new
readers of the tests.

+}
+
 TEST_F(WebRtcVideoChannel2Test, SetSendCodecsWithoutFecDisablesFec) {
   cricket::VideoSendParameters parameters;
   parameters.codecs.push_back(GetEngineCodec("VP8"));
   parameters.codecs.push_back(GetEngineCodec("ulpfec"));
   ASSERT_TRUE(channel_->SetSendParameters(parameters));
 
   FakeVideoSendStream* stream = AddSendStream();
   webrtc::VideoSendStream::Config config = stream->GetConfig().Copy();
 
   EXPECT_EQ(GetEngineCodec("ulpfec").id, config.rtp.ulpfec.ulpfec_payload_type);
@@ skipping 310 matching lines @@
 
   cricket::VideoCodec rtx_codec2(kUnusedPayloadType2, "rtx");
   rtx_codec2.SetParam("apt", rtx_codec.id);
   parameters.codecs.push_back(rtx_codec2);
 
   EXPECT_FALSE(channel_->SetRecvParameters(parameters)) <<
       "RTX codec with another RTX as associated payload type should be "
       "rejected.";
 }
 
+TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsWithChangedRtxPayloadType) {
+  const int kUnusedPayloadType1 = 126;

[brandtr, 2017/01/20 14:49:23]

Prior to this CL, this test fails.

+  const int kUnusedPayloadType2 = 127;
+  EXPECT_FALSE(FindCodecById(engine_.codecs(), kUnusedPayloadType1));
+  EXPECT_FALSE(FindCodecById(engine_.codecs(), kUnusedPayloadType2));
+
+  // SSRCs for RTX.
+  cricket::StreamParams params =
+      cricket::StreamParams::CreateLegacy(kSsrcs1[0]);
+  params.AddFidSsrc(kSsrcs1[0], kRtxSsrcs1[0]);
+  AddRecvStream(params);
+
+  // Original payload type for RTX.
+  cricket::VideoRecvParameters parameters;
+  parameters.codecs.push_back(GetEngineCodec("VP8"));
+  cricket::VideoCodec rtx_codec(kUnusedPayloadType1, "rtx");
+  rtx_codec.SetParam("apt", GetEngineCodec("VP8").id);
+  parameters.codecs.push_back(rtx_codec);
+  EXPECT_TRUE(channel_->SetRecvParameters(parameters));
+  ASSERT_EQ(1U, fake_call_->GetVideoReceiveStreams().size());
+  const webrtc::VideoReceiveStream::Config& config_before =
+      fake_call_->GetVideoReceiveStreams()[0]->GetConfig();
+  EXPECT_EQ(1U, config_before.rtp.rtx_payload_types.size());
+  auto it_before =
+      config_before.rtp.rtx_payload_types.find(GetEngineCodec("VP8").id);
+  ASSERT_NE(it_before, config_before.rtp.rtx_payload_types.end());
+  EXPECT_EQ(kUnusedPayloadType1, it_before->second);
+  EXPECT_EQ(kRtxSsrcs1[0], config_before.rtp.rtx_ssrc);
+
+  // Change payload type for RTX.
+  parameters.codecs[1].id = kUnusedPayloadType2;
+  EXPECT_TRUE(channel_->SetRecvParameters(parameters));
+  ASSERT_EQ(1U, fake_call_->GetVideoReceiveStreams().size());
+  const webrtc::VideoReceiveStream::Config& config_after =
+      fake_call_->GetVideoReceiveStreams()[0]->GetConfig();
+  EXPECT_EQ(1U, config_after.rtp.rtx_payload_types.size());
+  auto it_after =
+      config_after.rtp.rtx_payload_types.find(GetEngineCodec("VP8").id);
+  ASSERT_NE(it_after, config_after.rtp.rtx_payload_types.end());
+  EXPECT_EQ(kUnusedPayloadType2, it_after->second);
+  EXPECT_EQ(kRtxSsrcs1[0], config_before.rtp.rtx_ssrc);

[brandtr, 2017/01/24 10:04:41]

... Also, the EXPECTs are slightly difference between the receive and send
stream objects, since the receive stream object only has a single RTX SSRC,
compared to the multiple RTX SSRCs of the send stream object.

+}
+
 TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsDifferentPayloadType) {
   cricket::VideoRecvParameters parameters;
   parameters.codecs.push_back(GetEngineCodec("VP8"));
   parameters.codecs[0].id = 99;
   EXPECT_TRUE(channel_->SetRecvParameters(parameters));
 }
 
 TEST_F(WebRtcVideoChannel2Test, SetRecvCodecsAcceptDefaultCodecs) {
   cricket::VideoRecvParameters parameters;
   parameters.codecs = engine_.codecs();
@@ skipping 603 matching lines @@
   uint8_t data[kDataLength];
   memset(data, 0, sizeof(data));
   rtc::SetBE32(&data[8], ssrcs[0]);
   rtc::CopyOnWriteBuffer packet(data, kDataLength);
   rtc::PacketTime packet_time;
   channel_->OnPacketReceived(&packet, packet_time);
 
   ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size())
       << "No default receive stream created.";
   FakeVideoReceiveStream* recv_stream = fake_call_->GetVideoReceiveStreams()[0];
-  EXPECT_TRUE(recv_stream->GetConfig().rtp.rtx.empty())
+  EXPECT_EQ(0u, recv_stream->GetConfig().rtp.rtx_ssrc)
       << "Default receive stream should not have configured RTX";
 
   EXPECT_TRUE(channel_->AddRecvStream(
       cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs)));
   ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size())
-      << "AddRecvStream should've reconfigured, not added a new receiver.";
+      << "AddRecvStream should have reconfigured, not added a new receiver.";
   recv_stream = fake_call_->GetVideoReceiveStreams()[0];
-  EXPECT_FALSE(recv_stream->GetConfig().rtp.rtx.empty());
+  EXPECT_FALSE(recv_stream->GetConfig().rtp.rtx_payload_types.empty());
   EXPECT_EQ(recv_stream->GetConfig().decoders.size(),
-            recv_stream->GetConfig().rtp.rtx.size())
+            recv_stream->GetConfig().rtp.rtx_payload_types.size())
       << "RTX should be mapped for all decoders/payload types.";
-  for (const auto& kv : recv_stream->GetConfig().rtp.rtx) {
-    EXPECT_EQ(rtx_ssrcs[0], kv.second.ssrc);
-  }
+  EXPECT_EQ(rtx_ssrcs[0], recv_stream->GetConfig().rtp.rtx_ssrc);
 }
 
 TEST_F(WebRtcVideoChannel2Test, RejectsAddingStreamsWithMissingSsrcsForRtx) {
   EXPECT_TRUE(channel_->SetSendParameters(send_parameters_));
 
   const std::vector<uint32_t> ssrcs = MAKE_VECTOR(kSsrcs1);
   const std::vector<uint32_t> rtx_ssrcs = MAKE_VECTOR(kRtxSsrcs1);
 
   StreamParams sp =
       cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs);
@@ skipping 623 matching lines @@
 
 TEST_F(WebRtcVideoChannel2SimulcastTest, SetSendCodecsWith3SimulcastStreams) {
   VerifySimulcastSettings(cricket::VideoCodec("VP8"), 1280, 720, 3, 3);
 }
 
 // Test that we normalize send codec format size in simulcast.
 TEST_F(WebRtcVideoChannel2SimulcastTest, SetSendCodecsWithOddSizeInSimulcast) {
   VerifySimulcastSettings(cricket::VideoCodec("VP8"), 541, 271, 2, 2);
 }
 }  // namespace cricket