Fix RTPPayloadRegistry to correctly restore RTX, if a valid mapping exists.

Also updated the RTPPayloadRegistry::RestoreOriginalPacket signature to not take the first arg as a **, since it isn't modified.

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

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

webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h

@@ -83,7 +83,7 @@ class RTPPayloadRegistry {
 
   bool IsRtx(const RTPHeader& header) const;
 
-  bool RestoreOriginalPacket(uint8_t** restored_packet,
+  bool RestoreOriginalPacket(uint8_t* restored_packet,
                              const uint8_t* packet,
                              size_t* packet_length,
                              uint32_t original_ssrc,
@@ -138,6 +138,16 @@ class RTPPayloadRegistry {
     return last_received_media_payload_type_;
   };
 
+  bool use_rtx_payload_mapping_on_restore() const {
+    CriticalSectionScoped cs(crit_sect_.get());
+    return use_rtx_payload_mapping_on_restore_;
+  }
+
+  void set_use_rtx_payload_mapping_on_restore(bool val) {
+    CriticalSectionScoped cs(crit_sect_.get());
+    use_rtx_payload_mapping_on_restore_ = val;
+  }
+
  private:
   // Prunes the payload type map of the specific payload type, if it exists.
   void DeregisterAudioCodecOrRedTypeRegardlessOfPayloadType(
@@ -163,6 +173,9 @@ class RTPPayloadRegistry {
   int rtx_payload_type_;
   // Mapping rtx_payload_type_map_[rtx] = associated.
   std::map<int, int> rtx_payload_type_map_;
+  // When true, use rtx_payload_type_map_ when restoring RTX packets to get the
+  // correct payload type.
+  bool use_rtx_payload_mapping_on_restore_;
   uint32_t ssrc_rtx_;
 };
 

webrtc/modules/rtp_rtcp/source/nack_rtx_unittest.cc

@@ -109,7 +109,6 @@ class RtxLoopBackTransport : public webrtc::Transport {
     // is hiding a bug either in test setup or other code.
     // https://code.google.com/p/webrtc/issues/detail?id=3183
     uint8_t restored_packet[1500] = {0};
-    uint8_t* restored_packet_ptr = restored_packet;
     RTPHeader header;
     rtc::scoped_ptr<RtpHeaderParser> parser(RtpHeaderParser::Create());
     if (!parser->Parse(ptr, len, &header)) {
@@ -133,23 +132,23 @@ class RtxLoopBackTransport : public webrtc::Transport {
     if (rtp_payload_registry_->IsRtx(header)) {
       // Remove the RTX header and parse the original RTP header.
       EXPECT_TRUE(rtp_payload_registry_->RestoreOriginalPacket(
-          &restored_packet_ptr, ptr, &packet_length, rtp_receiver_->SSRC(),
-          header));
-      if (!parser->Parse(restored_packet_ptr, packet_length, &header)) {
+          restored_packet, ptr, &packet_length, rtp_receiver_->SSRC(), header));
+      if (!parser->Parse(restored_packet, packet_length, &header)) {
         return false;
       }
     } else {
       rtp_payload_registry_->SetIncomingPayloadType(header);
     }
 
-    restored_packet_ptr += header.headerLength;
+    const uint8_t* restored_packet_payload =
+        restored_packet + header.headerLength;
     packet_length -= header.headerLength;
     PayloadUnion payload_specific;
     if (!rtp_payload_registry_->GetPayloadSpecifics(header.payloadType,
                                                     &payload_specific)) {
       return false;
     }
-    if (!rtp_receiver_->IncomingRtpPacket(header, restored_packet_ptr,
+    if (!rtp_receiver_->IncomingRtpPacket(header, restored_packet_payload,
                                           packet_length, payload_specific,
                                           true)) {
       return false;

webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc

@@ -25,8 +25,8 @@ RTPPayloadRegistry::RTPPayloadRegistry(RTPPayloadStrategy* rtp_payload_strategy)
       last_received_media_payload_type_(-1),
       rtx_(false),
       rtx_payload_type_(-1),
-      ssrc_rtx_(0) {
-}
+      use_rtx_payload_mapping_on_restore_(false),
+      ssrc_rtx_(0) {}
 
 RTPPayloadRegistry::~RTPPayloadRegistry() {
   while (!payload_type_map_.empty()) {
@@ -232,7 +232,7 @@ bool RTPPayloadRegistry::IsRtxInternal(const RTPHeader& header) const {
   return rtx_ && ssrc_rtx_ == header.ssrc;
 }
 
-bool RTPPayloadRegistry::RestoreOriginalPacket(uint8_t** restored_packet,
+bool RTPPayloadRegistry::RestoreOriginalPacket(uint8_t* restored_packet,
                                                const uint8_t* packet,
                                                size_t* packet_length,
                                                uint32_t original_ssrc,
@@ -245,30 +245,41 @@ bool RTPPayloadRegistry::RestoreOriginalPacket(uint8_t** restored_packet,
   uint16_t original_sequence_number = (rtx_header[0] << 8) + rtx_header[1];
 
   // Copy the packet into the restored packet, except for the RTX header.
-  memcpy(*restored_packet, packet, header.headerLength);
-  memcpy(*restored_packet + header.headerLength,
+  memcpy(restored_packet, packet, header.headerLength);
+  memcpy(restored_packet + header.headerLength,
          packet + header.headerLength + kRtxHeaderSize,
          *packet_length - header.headerLength - kRtxHeaderSize);
   *packet_length -= kRtxHeaderSize;
 
   // Replace the SSRC and the sequence number with the originals.
-  ByteWriter<uint16_t>::WriteBigEndian(*restored_packet + 2,
+  ByteWriter<uint16_t>::WriteBigEndian(restored_packet + 2,
                                        original_sequence_number);
-  ByteWriter<uint32_t>::WriteBigEndian(*restored_packet + 8, original_ssrc);
+  ByteWriter<uint32_t>::WriteBigEndian(restored_packet + 8, original_ssrc);
 
   CriticalSectionScoped cs(crit_sect_.get());
   if (!rtx_)
     return true;
 
-  if (rtx_payload_type_ == -1 || incoming_payload_type_ == -1) {
-    LOG(LS_WARNING) << "Incorrect RTX configuration, dropping packet.";
-    return false;
+  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_;
   }
-  // TODO(changbin): Will use RTX APT map for restoring packets,
-  // thus incoming_payload_type_ should be removed in future.
-  (*restored_packet)[1] = static_cast<uint8_t>(incoming_payload_type_);
+
+  restored_packet[1] = static_cast<uint8_t>(associated_payload_type);
   if (header.markerBit) {
-    (*restored_packet)[1] |= kRtpMarkerBitMask;  // Marker bit is set.
+    restored_packet[1] |= kRtpMarkerBitMask;  // Marker bit is set.
   }
   return true;
 }

webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc

@@ -13,6 +13,8 @@
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "webrtc/base/scoped_ptr.h"
+#include "webrtc/modules/rtp_rtcp/interface/rtp_header_parser.h"
+#include "webrtc/modules/rtp_rtcp/source/byte_io.h"
 #include "webrtc/modules/rtp_rtcp/source/mock/mock_rtp_payload_strategy.h"
 #include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
 
@@ -257,6 +259,142 @@ TEST_P(RtpPayloadRegistryGenericTest, RegisterGenericReceivePayloadType) {
     19, 1, 17, &ignored)); // dummy values, except for payload_type
 }
 
+// Generates an RTX packet for the given length and original sequence number.
+// The RTX sequence number and ssrc will use the default value of 9999. The
+// caller takes ownership of the returned buffer.
+const uint8_t* GenerateRtxPacket(size_t header_length,
+                                 size_t payload_length,
+                                 uint16_t original_sequence_number) {
+  uint8_t* packet =
+      new uint8_t[kRtxHeaderSize + header_length + payload_length]();
+  // Write the RTP version to the first byte, so the resulting header can be
+  // parsed.
+  static const int kRtpExpectedVersion = 2;
+  packet[0] = static_cast<uint8_t>(kRtpExpectedVersion << 6);
+  // Write a junk sequence number. It should be thrown away when the packet is
+  // restored.
+  ByteWriter<uint16_t>::WriteBigEndian(packet + 2, 9999);
+  // Write a junk ssrc. It should also be thrown away when the packet is
+  // restored.
+  ByteWriter<uint32_t>::WriteBigEndian(packet + 8, 9999);
+
+  // Now write the RTX header. It occurs at the start of the payload block, and
+  // contains just the sequence number.
+  ByteWriter<uint16_t>::WriteBigEndian(packet + header_length,
+                                       original_sequence_number);
+  return packet;
+}
+
+void TestRtxPacket(RTPPayloadRegistry* rtp_payload_registry,
+                   int rtx_payload_type,
+                   int expected_payload_type,
+                   bool should_succeed) {
+  size_t header_length = 100;
+  size_t payload_length = 200;
+  size_t original_length = header_length + payload_length + kRtxHeaderSize;
+
+  RTPHeader header;
+  header.ssrc = 1000;
+  header.sequenceNumber = 100;
+  header.payloadType = rtx_payload_type;
+  header.headerLength = header_length;
+
+  uint16_t original_sequence_number = 1234;
+  uint32_t original_ssrc = 500;
+
+  rtc::scoped_ptr<const uint8_t[]> packet(GenerateRtxPacket(
+      header_length, payload_length, original_sequence_number));
+  rtc::scoped_ptr<uint8_t[]> restored_packet(
+      new uint8_t[header_length + payload_length]);
+  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)
+      << "Test success should match should_succeed.";
+  if (!success) {
+    return;
+  }
+
+  EXPECT_EQ(original_length - kRtxHeaderSize, length)
+      << "The restored packet should be exactly kRtxHeaderSize smaller.";
+
+  rtc::scoped_ptr<RtpHeaderParser> header_parser(RtpHeaderParser::Create());
+  RTPHeader restored_header;
+  ASSERT_TRUE(
+      header_parser->Parse(restored_packet.get(), length, &restored_header));
+  EXPECT_EQ(original_sequence_number, restored_header.sequenceNumber)
+      << "The restored packet should have the original sequence number "
+      << "in the correct location in the RTP header.";
+  EXPECT_EQ(expected_payload_type, restored_header.payloadType)
+      << "The restored packet should have the correct payload type.";
+  EXPECT_EQ(original_ssrc, restored_header.ssrc)
+      << "The restored packet should have the correct ssrc.";
+}
+
+TEST_F(RtpPayloadRegistryTest, MultipleRtxPayloadTypes) {
+  // 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);
+  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) {
+  rtp_payload_registry_->SetRtxSsrc(100);
+  // Fails because no mappings exist and the incoming payload type isn't known.
+  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);
+}
+
 INSTANTIATE_TEST_CASE_P(TestDynamicRange, RtpPayloadRegistryGenericTest,
                         testing::Range(96, 127+1));
 

webrtc/video/video_receive_stream.cc

@@ -167,6 +167,10 @@ VideoReceiveStream::VideoReceiveStream(int num_cpu_cores,
     vie_channel_->SetRemoteSSRCType(kViEStreamTypeRtx, it->second.ssrc);
     vie_channel_->SetRtxReceivePayloadType(it->second.payload_type, it->first);
   }
+  // TODO(holmer): When Chrome no longer depends on this being false by default,
+  // always use the mapping and remove this whole codepath.
+  vie_channel_->SetUseRtxPayloadMappingOnRestore(
+      config_.rtp.use_rtx_payload_mapping_on_restore);
 
   // TODO(pbos): Remove channel_group_ usage from VideoReceiveStream. This
   // should be configured in call.cc.

webrtc/video_engine/vie_channel.cc

@@ -750,6 +750,10 @@ void ViEChannel::SetRtxReceivePayloadType(int payload_type,
   vie_receiver_.SetRtxPayloadType(payload_type, associated_payload_type);
 }
 
+void ViEChannel::SetUseRtxPayloadMappingOnRestore(bool val) {
+  vie_receiver_.SetUseRtxPayloadMappingOnRestore(val);
+}
+
 void ViEChannel::SetRtpStateForSsrc(uint32_t ssrc, const RtpState& rtp_state) {
   RTC_DCHECK(!rtp_rtcp_modules_[0]->Sending());
   for (RtpRtcp* rtp_rtcp : rtp_rtcp_modules_) {

webrtc/video_engine/vie_channel.h

@@ -134,6 +134,11 @@ class ViEChannel : public VCMFrameTypeCallback,
 
   int SetRtxSendPayloadType(int payload_type, int associated_payload_type);
   void SetRtxReceivePayloadType(int payload_type, int associated_payload_type);
+  // If set to true, the RTX payload type mapping supplied in
+  // |SetRtxReceivePayloadType| will be used when restoring RTX packets. Without
+  // it, RTX packets will always be restored to the last non-RTX packet payload
+  // type received.
+  void SetUseRtxPayloadMappingOnRestore(bool val);
 
   void SetRtpStateForSsrc(uint32_t ssrc, const RtpState& rtp_state);
   RtpState GetRtpStateForSsrc(uint32_t ssrc);

webrtc/video_engine/vie_receiver.cc

@@ -118,6 +118,10 @@ void ViEReceiver::SetRtxPayloadType(int payload_type,
                                            associated_payload_type);
 }
 
+void ViEReceiver::SetUseRtxPayloadMappingOnRestore(bool val) {
+  rtp_payload_registry_->set_use_rtx_payload_mapping_on_restore(val);
+}
+
 void ViEReceiver::SetRtxSsrc(uint32_t ssrc) {
   rtp_payload_registry_->SetRtxSsrc(ssrc);
 }
@@ -361,15 +365,14 @@ bool ViEReceiver::ParseAndHandleEncapsulatingHeader(const uint8_t* packet,
       LOG(LS_WARNING) << "Multiple RTX headers detected, dropping packet.";
       return false;
     }
-    uint8_t* restored_packet_ptr = restored_packet_;
     if (!rtp_payload_registry_->RestoreOriginalPacket(
-        &restored_packet_ptr, packet, &packet_length, rtp_receiver_->SSRC(),
-        header)) {
+            restored_packet_, packet, &packet_length, rtp_receiver_->SSRC(),
+            header)) {
       LOG(LS_WARNING) << "Incoming RTX packet: Invalid RTP header";
       return false;
     }
     restored_packet_in_use_ = true;
-    bool ret = OnRecoveredPacket(restored_packet_ptr, packet_length);
+    bool ret = OnRecoveredPacket(restored_packet_, packet_length);
     restored_packet_in_use_ = false;
     return ret;
   }

webrtc/video_engine/vie_receiver.h

@@ -46,6 +46,11 @@ class ViEReceiver : public RtpData {
 
   void SetNackStatus(bool enable, int max_nack_reordering_threshold);
   void SetRtxPayloadType(int payload_type, int associated_payload_type);
+  // If set to true, the RTX payload type mapping supplied in
+  // |SetRtxPayloadType| will be used when restoring RTX packets. Without it,
+  // RTX packets will always be restored to the last non-RTX packet payload type
+  // received.
+  void SetUseRtxPayloadMappingOnRestore(bool val);
   void SetRtxSsrc(uint32_t ssrc);
   bool GetRtxSsrc(uint32_t* ssrc) const;
 

webrtc/video_receive_stream.h

@@ -133,6 +133,11 @@ class VideoReceiveStream : public ReceiveStream {
       typedef std::map<int, Rtx> RtxMap;
       RtxMap rtx;
 
+      // If set to true, the RTX payload type mapping supplied in |rtx| will be
+      // used when restoring RTX packets. Without it, RTX packets will always be
+      // restored to the last non-RTX packet payload type received.
+      bool use_rtx_payload_mapping_on_restore = false;
+
       // RTP header extensions used for the received stream.
       std::vector<RtpExtension> extensions;
     } rtp;

webrtc/voice_engine/channel.cc

@@ -1627,16 +1627,15 @@ bool Channel::HandleRtxPacket(const uint8_t* packet,
                  "Multiple RTX headers detected, dropping packet");
     return false;
   }
-  uint8_t* restored_packet_ptr = restored_packet_;
   if (!rtp_payload_registry_->RestoreOriginalPacket(
-      &restored_packet_ptr, packet, &packet_length, rtp_receiver_->SSRC(),
-      header)) {
+          restored_packet_, packet, &packet_length, rtp_receiver_->SSRC(),
+          header)) {
     WEBRTC_TRACE(webrtc::kTraceDebug, webrtc::kTraceVoice, _channelId,
                  "Incoming RTX packet: invalid RTP header");
     return false;
   }
   restored_packet_in_use_ = true;
-  bool ret = OnRecoveredPacket(restored_packet_ptr, packet_length);
+  bool ret = OnRecoveredPacket(restored_packet_, packet_length);
   restored_packet_in_use_ = false;
   return ret;
 }