Support multiple rtx codecs.

Adds negotiation of rtx codecs for red and vp9. To keep backwards
compatibility with older Chrome versions, this change includes two
hacks:
1. Red packets will be retransmitted over the rtx codec associated with
   vp8 if no rtx codec is associated with red. This is how Chrome does
   it today and ensures that we still can send red over rtx to older
   versions.

2. If rtx packets associated with the media codec (vp8/vp9 etc) are
   received and red has been negotiated, we will assume that the sender
   incorrectly has packetized red inside the rtx header associated with
   media. We will therefore restore it with the red payload type
   instead, which ensures that we can still receive rtx associated with
   red from old versions.

Offering multiple rtx codecs to older versions should not be a problem
since old versions themselves only try to negotiate rtx for vp8.

R=pbos@webrtc.org
TBR=mflodman@webrtc.org
BUG=webrtc:4024
TEST=Verified by running apprtc and emulating packet loss between Chrome with and without the patch.

Review URL: https://codereview.webrtc.org/1649493004 .

Cr-Commit-Position: refs/heads/master@{#11472}

talk/media/base/constants.cc

@@ -139,6 +139,8 @@ const int kDefaultH264PlType = 107;
 const int kDefaultRedPlType = 116;
 const int kDefaultUlpfecType = 117;
 const int kDefaultRtxVp8PlType = 96;
+const int kDefaultRtxVp9PlType = 97;
+const int kDefaultRtxRedPlType = 98;
 
 const int kDefaultVideoMaxWidth = 640;
 const int kDefaultVideoMaxHeight = 400;

talk/media/base/constants.h

@@ -172,6 +172,8 @@ extern const int kDefaultH264PlType;
 extern const int kDefaultRedPlType;
 extern const int kDefaultUlpfecType;
 extern const int kDefaultRtxVp8PlType;
+extern const int kDefaultRtxVp9PlType;
+extern const int kDefaultRtxRedPlType;
 
 extern const int kDefaultVideoMaxWidth;
 extern const int kDefaultVideoMaxHeight;

talk/media/webrtc/webrtcvideoengine2.cc

@@ -357,7 +357,6 @@ std::vector<VideoCodec> DefaultVideoCodecList() {
   if (CodecIsInternallySupported(kVp9CodecName)) {
     codecs.push_back(MakeVideoCodecWithDefaultFeedbackParams(kDefaultVp9PlType,
                                                              kVp9CodecName));
-    // TODO(andresp): Add rtx codec for vp9 and verify it works.
   }
   if (CodecIsInternallySupported(kH264CodecName)) {
     codecs.push_back(MakeVideoCodecWithDefaultFeedbackParams(kDefaultH264PlType,
@@ -365,7 +364,13 @@ std::vector<VideoCodec> DefaultVideoCodecList() {
   }
   codecs.push_back(
       VideoCodec::CreateRtxCodec(kDefaultRtxVp8PlType, kDefaultVp8PlType));
+  if (CodecIsInternallySupported(kVp9CodecName)) {
+    codecs.push_back(
+        VideoCodec::CreateRtxCodec(kDefaultRtxVp9PlType, kDefaultVp9PlType));
+  }
   codecs.push_back(VideoCodec(kDefaultRedPlType, kRedCodecName));
+  codecs.push_back(
+      VideoCodec::CreateRtxCodec(kDefaultRtxRedPlType, kDefaultRedPlType));
   codecs.push_back(VideoCodec(kDefaultUlpfecType, kUlpfecCodecName));
   return codecs;
 }

talk/media/webrtc/webrtcvideoengine2_unittest.cc

@@ -1109,8 +1109,6 @@ TEST_F(WebRtcVideoChannel2Test, RecvStreamWithSimAndRtx) {
       cricket::CreateSimWithRtxStreamParams("cname", ssrcs, rtx_ssrcs));
   ASSERT_GT(recv_stream->GetConfig().rtp.rtx.size(), 0u)
       << "No SSRCs for RTX configured by AddRecvStream.";
-  ASSERT_EQ(1u, recv_stream->GetConfig().rtp.rtx.size())
-      << "This test only works with one receive codec. Please update the test.";
   EXPECT_EQ(rtx_ssrcs[0],
             recv_stream->GetConfig().rtp.rtx.begin()->second.ssrc);
   // TODO(pbos): Make sure we set the RTX for correct payloads etc.
@@ -1122,7 +1120,6 @@ TEST_F(WebRtcVideoChannel2Test, RecvStreamWithRtx) {
       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);
 }
@@ -2736,7 +2733,7 @@ TEST_F(WebRtcVideoChannel2Test, DefaultReceiveStreamReconfiguresToUseRtx) {
   ASSERT_EQ(1u, fake_call_->GetVideoReceiveStreams().size())
       << "AddRecvStream should've reconfigured, not added a new receiver.";
   recv_stream = fake_call_->GetVideoReceiveStreams()[0];
-  ASSERT_EQ(1u, recv_stream->GetConfig().rtp.rtx.size());
+  ASSERT_GE(2u, recv_stream->GetConfig().rtp.rtx.size());
   EXPECT_EQ(rtx_ssrcs[0],
             recv_stream->GetConfig().rtp.rtx.begin()->second.ssrc);
 }

webrtc/config.cc

@@ -17,6 +17,7 @@ std::string FecConfig::ToString() const {
   std::stringstream ss;
   ss << "{ulpfec_payload_type: " << ulpfec_payload_type;
   ss << ", red_payload_type: " << red_payload_type;
+  ss << ", red_rtx_payload_type: " << red_rtx_payload_type;
   ss << '}';
   return ss.str();
 }

webrtc/modules/rtp_rtcp/include/rtp_rtcp.h

@@ -249,10 +249,6 @@ class RtpRtcp : public Module {
     virtual void SetRtxSendPayloadType(int payload_type,
                                        int associated_payload_type) = 0;
 
-    // Gets the payload type pair of (RTX, associated) to use when sending RTX
-    // packets.
-    virtual std::pair<int, int> RtxSendPayloadType() const = 0;
-
     /*
     *   sends kRtcpByeCode when going from true to false
     *

webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc

@@ -25,7 +25,6 @@ RTPPayloadRegistry::RTPPayloadRegistry(RTPPayloadStrategy* rtp_payload_strategy)
       last_received_media_payload_type_(-1),
       rtx_(false),
       rtx_payload_type_(-1),
-      use_rtx_payload_mapping_on_restore_(false),
       ssrc_rtx_(0) {}
 
 RTPPayloadRegistry::~RTPPayloadRegistry() {
@@ -271,21 +270,18 @@ bool RTPPayloadRegistry::RestoreOriginalPacket(uint8_t* restored_packet,
 
   int associated_payload_type;
   auto apt_mapping = rtx_payload_type_map_.find(header.payloadType);
-  if (use_rtx_payload_mapping_on_restore_ &&
-      apt_mapping != rtx_payload_type_map_.end()) {
-    associated_payload_type = apt_mapping->second;
-  } else {
-    // In the future, this will be a bug. For now, just assume this RTX packet
-    // matches the last non-RTX payload type we received. There are cases where
-    // this could break, especially where RTX is sent outside of NACKing (e.g.
-    // padding with redundant payloads).
-    if (rtx_payload_type_ == -1 || incoming_payload_type_ == -1) {
-      LOG(LS_WARNING) << "Incorrect RTX configuration, dropping packet.";
-      return false;
-    }
-    associated_payload_type = incoming_payload_type_;
+  if (apt_mapping == rtx_payload_type_map_.end())
+    return false;
+  associated_payload_type = apt_mapping->second;
+  if (red_payload_type_ != -1) {
+    // Assume red will be used if it's configured.
+    // This is a workaround for a Chrome sdp bug where rtx is associated
+    // with the media codec even though media is sent over red.
+    // TODO(holmer): Remove once the Chrome versions exposing this bug are
+    // old enough, which should be once Chrome Stable reaches M53 as this
+    // work-around went into M50.
+    associated_payload_type = red_payload_type_;
   }
-
   restored_packet[1] = static_cast<uint8_t>(associated_payload_type);
   if (header.markerBit) {
     restored_packet[1] |= kRtpMarkerBitMask;  // Marker bit is set.

webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc

@@ -303,7 +303,7 @@ void TestRtxPacket(RTPPayloadRegistry* rtp_payload_registry,
   size_t length = original_length;
   bool success = rtp_payload_registry->RestoreOriginalPacket(
       restored_packet.get(), packet.get(), &length, original_ssrc, header);
-  ASSERT_EQ(should_succeed, success)
+  EXPECT_EQ(should_succeed, success)
       << "Test success should match should_succeed.";
   if (!success) {
     return;
@@ -335,47 +335,9 @@ TEST_F(RtpPayloadRegistryTest, MultipleRtxPayloadTypes) {
   // Map two RTX payload types.
   rtp_payload_registry_->SetRtxPayloadType(105, 95);
   rtp_payload_registry_->SetRtxPayloadType(106, 96);
-  rtp_payload_registry_->set_use_rtx_payload_mapping_on_restore(true);
 
   TestRtxPacket(rtp_payload_registry_.get(), 105, 95, true);
   TestRtxPacket(rtp_payload_registry_.get(), 106, 96, true);
-
-  // If the option is off, the map will be ignored.
-  rtp_payload_registry_->set_use_rtx_payload_mapping_on_restore(false);
-  TestRtxPacket(rtp_payload_registry_.get(), 105, 90, true);
-  TestRtxPacket(rtp_payload_registry_.get(), 106, 90, true);
-}
-
-// TODO(holmer): Ignored by default for compatibility with misconfigured RTX
-// streams in Chrome. When that is fixed, remove this.
-TEST_F(RtpPayloadRegistryTest, IgnoresRtxPayloadTypeMappingByDefault) {
-  // Set the incoming payload type to 90.
-  RTPHeader header;
-  header.payloadType = 90;
-  header.ssrc = 1;
-  rtp_payload_registry_->SetIncomingPayloadType(header);
-  rtp_payload_registry_->SetRtxSsrc(100);
-  // Map two RTX payload types.
-  rtp_payload_registry_->SetRtxPayloadType(105, 95);
-  rtp_payload_registry_->SetRtxPayloadType(106, 96);
-
-  TestRtxPacket(rtp_payload_registry_.get(), 105, 90, true);
-  TestRtxPacket(rtp_payload_registry_.get(), 106, 90, true);
-}
-
-TEST_F(RtpPayloadRegistryTest, InferLastReceivedPacketIfPayloadTypeUnknown) {
-  rtp_payload_registry_->SetRtxSsrc(100);
-  // Set the incoming payload type to 90.
-  RTPHeader header;
-  header.payloadType = 90;
-  header.ssrc = 1;
-  rtp_payload_registry_->SetIncomingPayloadType(header);
-  rtp_payload_registry_->SetRtxPayloadType(105, 95);
-  rtp_payload_registry_->set_use_rtx_payload_mapping_on_restore(true);
-  // Mapping respected for known type.
-  TestRtxPacket(rtp_payload_registry_.get(), 105, 95, true);
-  // Mapping ignored for unknown type, even though the option is on.
-  TestRtxPacket(rtp_payload_registry_.get(), 106, 90, true);
 }
 
 TEST_F(RtpPayloadRegistryTest, InvalidRtxConfiguration) {
@@ -384,11 +346,30 @@ TEST_F(RtpPayloadRegistryTest, InvalidRtxConfiguration) {
   TestRtxPacket(rtp_payload_registry_.get(), 105, 0, false);
   // Succeeds when the mapping is used, but fails for the implicit fallback.
   rtp_payload_registry_->SetRtxPayloadType(105, 95);
-  rtp_payload_registry_->set_use_rtx_payload_mapping_on_restore(true);
   TestRtxPacket(rtp_payload_registry_.get(), 105, 95, true);
   TestRtxPacket(rtp_payload_registry_.get(), 106, 0, false);
 }
 
+TEST_F(RtpPayloadRegistryTest, AssumeRtxWrappingRed) {
+  rtp_payload_registry_->SetRtxSsrc(100);
+  // Succeeds when the mapping is used, but fails for the implicit fallback.
+  rtp_payload_registry_->SetRtxPayloadType(105, 95);
+  // Set the incoming payload type to 96, which we assume is red.
+  RTPHeader header;
+  header.payloadType = 96;
+  header.ssrc = 1;
+  rtp_payload_registry_->SetIncomingPayloadType(header);
+  // Recovers with correct, but unexpected, payload type since we haven't
+  // configured red.
+  TestRtxPacket(rtp_payload_registry_.get(), 105, 95, true);
+  bool created_new_payload;
+  rtp_payload_registry_->RegisterReceivePayload(
+      "RED", header.payloadType, 90000, 1, 0, &created_new_payload);
+  // Now that red is configured we expect to get the red payload type back on
+  // recovery because of the workaround to always recover red when configured.
+  TestRtxPacket(rtp_payload_registry_.get(), 105, header.payloadType, true);
+}
+
 INSTANTIATE_TEST_CASE_P(TestDynamicRange,
                         RtpPayloadRegistryGenericTest,
                         testing::Range(96, 127 + 1));

webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -218,10 +218,6 @@ void ModuleRtpRtcpImpl::SetRtxSendPayloadType(int payload_type,
   rtp_sender_.SetRtxPayloadType(payload_type, associated_payload_type);
 }
 
-std::pair<int, int> ModuleRtpRtcpImpl::RtxSendPayloadType() const {
-  return rtp_sender_.RtxPayloadType();
-}
-
 int32_t ModuleRtpRtcpImpl::IncomingRtcpPacket(
     const uint8_t* rtcp_packet,
     const size_t length) {

webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h

@@ -93,7 +93,6 @@ class ModuleRtpRtcpImpl : public RtpRtcp {
 
   void SetRtxSendPayloadType(int payload_type,
                              int associated_payload_type) override;
-  std::pair<int, int> RtxSendPayloadType() const override;
 
   // Sends kRtcpByeCode when going from true to false.
   int32_t SetSendingStatus(bool sending) override;

webrtc/modules/rtp_rtcp/source/rtp_sender.cc

@@ -168,7 +168,6 @@ RTPSender::RTPSender(
       last_packet_marker_bit_(false),
       csrcs_(),
       rtx_(kRtxOff),
-      rtx_payload_type_(-1),
       target_bitrate_critsect_(CriticalSectionWrapper::CreateCriticalSection()),
       target_bitrate_(0) {
   memset(nack_byte_count_times_, 0, sizeof(nack_byte_count_times_));
@@ -440,17 +439,6 @@ void RTPSender::SetRtxPayloadType(int payload_type,
   }
 
   rtx_payload_type_map_[associated_payload_type] = payload_type;
-  rtx_payload_type_ = payload_type;
-}
-
-std::pair<int, int> RTPSender::RtxPayloadType() const {
-  rtc::CritScope lock(&send_critsect_);
-  for (const auto& kv : rtx_payload_type_map_) {
-    if (kv.second == rtx_payload_type_) {
-      return std::make_pair(rtx_payload_type_, kv.first);
-    }
-  }
-  return std::make_pair(-1, -1);
 }
 
 int32_t RTPSender::CheckPayloadType(int8_t payload_type,
@@ -666,7 +654,7 @@ size_t RTPSender::SendPadData(size_t bytes,
         ssrc = ssrc_rtx_;
         sequence_number = sequence_number_rtx_;
         ++sequence_number_rtx_;
-        payload_type = rtx_payload_type_;
+        payload_type = rtx_payload_type_map_.begin()->second;
         over_rtx = true;
       }
     }
@@ -1854,11 +1842,16 @@ void RTPSender::BuildRtxPacket(uint8_t* buffer, size_t* length,
   memcpy(data_buffer_rtx, buffer, rtp_header.headerLength);
 
   // Replace payload type, if a specific type is set for RTX.
-  if (rtx_payload_type_ != -1) {
-    data_buffer_rtx[1] = static_cast<uint8_t>(rtx_payload_type_);
-    if (rtp_header.markerBit)
-      data_buffer_rtx[1] |= kRtpMarkerBitMask;
-  }
+  auto kv = rtx_payload_type_map_.find(rtp_header.payloadType);
+  // Use rtx mapping associated with media codec if we can't find one, assuming
+  // it's red.
+  // TODO(holmer): Remove once old Chrome versions don't rely on this.
+  if (kv == rtx_payload_type_map_.end())
+    kv = rtx_payload_type_map_.find(payload_type_);
+  if (kv != rtx_payload_type_map_.end())
+    data_buffer_rtx[1] = kv->second;
+  if (rtp_header.markerBit)
+    data_buffer_rtx[1] |= kRtpMarkerBitMask;
 
   // Replace sequence number.
   uint8_t* ptr = data_buffer_rtx + 2;

webrtc/modules/rtp_rtcp/source/rtp_sender.h

@@ -234,7 +234,6 @@ class RTPSender : public RTPSenderInterface {
   void SetRtxSsrc(uint32_t ssrc);
 
   void SetRtxPayloadType(int payload_type, int associated_payload_type);
-  std::pair<int, int> RtxPayloadType() const;
 
   // Functions wrapping RTPSenderInterface.
   int32_t BuildRTPheader(uint8_t* data_buffer,
@@ -486,9 +485,6 @@ class RTPSender : public RTPSenderInterface {
   std::vector<uint32_t> csrcs_ GUARDED_BY(send_critsect_);
   int rtx_ GUARDED_BY(send_critsect_);
   uint32_t ssrc_rtx_ GUARDED_BY(send_critsect_);
-  // TODO(changbin): Remove rtx_payload_type_ once interop with old clients that
-  // only understand one RTX PT is no longer needed.
-  int rtx_payload_type_ GUARDED_BY(send_critsect_);
   // Mapping rtx_payload_type_map_[associated] = rtx.
   std::map<int8_t, int8_t> rtx_payload_type_map_ GUARDED_BY(send_critsect_);
 

webrtc/video/end_to_end_tests.cc

@@ -736,7 +736,9 @@ void EndToEndTest::DecodesRetransmittedFrame(bool use_rtx, bool use_red) {
         return SEND_PACKET;
       }
 
-      EXPECT_EQ(kVideoSendSsrcs[0], header.ssrc);
+      EXPECT_EQ(kVideoSendSsrcs[0], header.ssrc)
+          << "Payload type " << static_cast<int>(header.payloadType)
+          << " not expected.";
       EXPECT_EQ(payload_type_, header.payloadType);
 
       // Found the final packet of the frame to inflict loss to, drop this and
@@ -768,16 +770,21 @@ void EndToEndTest::DecodesRetransmittedFrame(bool use_rtx, bool use_red) {
       if (payload_type_ == kRedPayloadType) {
         send_config->rtp.fec.ulpfec_payload_type = kUlpfecPayloadType;
         send_config->rtp.fec.red_payload_type = kRedPayloadType;
-        (*receive_configs)[0].rtp.fec.red_payload_type = kRedPayloadType;
-        (*receive_configs)[0].rtp.fec.ulpfec_payload_type = kUlpfecPayloadType;
+        if (retransmission_ssrc_ == kSendRtxSsrcs[0])
+          send_config->rtp.fec.red_rtx_payload_type = kRtxRedPayloadType;
+        (*receive_configs)[0].rtp.fec.ulpfec_payload_type =
+            send_config->rtp.fec.ulpfec_payload_type;
+        (*receive_configs)[0].rtp.fec.red_payload_type =
+            send_config->rtp.fec.red_payload_type;
+        (*receive_configs)[0].rtp.fec.red_rtx_payload_type =
+            send_config->rtp.fec.red_rtx_payload_type;
       }
 
       if (retransmission_ssrc_ == kSendRtxSsrcs[0]) {
         send_config->rtp.rtx.ssrcs.push_back(kSendRtxSsrcs[0]);
         send_config->rtp.rtx.payload_type = kSendRtxPayloadType;
-        (*receive_configs)[0].rtp.rtx[kFakeVideoSendPayloadType].ssrc =
-            kSendRtxSsrcs[0];
-        (*receive_configs)[0].rtp.rtx[kFakeVideoSendPayloadType].payload_type =
+        (*receive_configs)[0].rtp.rtx[payload_type_].ssrc = kSendRtxSsrcs[0];
+        (*receive_configs)[0].rtp.rtx[payload_type_].payload_type =
             kSendRtxPayloadType;
       }
     }
@@ -788,8 +795,14 @@ void EndToEndTest::DecodesRetransmittedFrame(bool use_rtx, bool use_red) {
     }
 
     int GetPayloadType(bool use_rtx, bool use_red) {
-      return use_rtx ? kSendRtxPayloadType
-                     : (use_red ? kRedPayloadType : kFakeVideoSendPayloadType);
+      if (use_red) {
+        if (use_rtx)
+          return kRtxRedPayloadType;
+        return kRedPayloadType;
+      }
+      if (use_rtx)
+        return kSendRtxPayloadType;
+      return kFakeVideoSendPayloadType;
     }
 
     rtc::CriticalSection crit_;

webrtc/video/video_quality_test.cc

@@ -827,9 +827,8 @@ void VideoQualityTest::SetupCommon(Transport* send_transport,
 
   for (size_t i = 0; i < num_streams; ++i) {
     video_receive_configs_[i].rtp.nack.rtp_history_ms = kNackRtpHistoryMs;
-    video_receive_configs_[i].rtp.rtx[kSendRtxPayloadType].ssrc =
-        kSendRtxSsrcs[i];
-    video_receive_configs_[i].rtp.rtx[kSendRtxPayloadType].payload_type =
+    video_receive_configs_[i].rtp.rtx[payload_type].ssrc = kSendRtxSsrcs[i];
+    video_receive_configs_[i].rtp.rtx[payload_type].payload_type =
         kSendRtxPayloadType;
     video_receive_configs_[i].rtp.transport_cc = params_.common.send_side_bwe;
   }

webrtc/video/video_send_stream.cc

@@ -494,6 +494,11 @@ void VideoSendStream::ConfigureSsrcs() {
   RTC_DCHECK_GE(config_.rtp.rtx.payload_type, 0);
   vie_channel_->SetRtxSendPayloadType(config_.rtp.rtx.payload_type,
                                       config_.encoder_settings.payload_type);
+  if (config_.rtp.fec.red_payload_type != -1 &&
+      config_.rtp.fec.red_rtx_payload_type != -1) {
+    vie_channel_->SetRtxSendPayloadType(config_.rtp.fec.red_rtx_payload_type,
+                                        config_.rtp.fec.red_payload_type);
+  }
 }
 
 std::map<uint32_t, RtpState> VideoSendStream::GetRtpStates() const {

webrtc/video/vie_receiver.cc

@@ -366,7 +366,9 @@ bool ViEReceiver::ParseAndHandleEncapsulatingHeader(const uint8_t* packet,
     if (!rtp_payload_registry_->RestoreOriginalPacket(
             restored_packet_, packet, &packet_length, rtp_receiver_->SSRC(),
             header)) {
-      LOG(LS_WARNING) << "Incoming RTX packet: Invalid RTP header";
+      LOG(LS_WARNING) << "Incoming RTX packet: Invalid RTP header ssrc: "
+                      << header.ssrc << " payload type: "
+                      << static_cast<int>(header.payloadType);
       return false;
     }
     restored_packet_in_use_ = true;