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}
diff --git a/webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h b/webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h
index bcafdfb..a60a6c5 100644
--- a/webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h
+++ b/webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h
@@ -83,7 +83,7 @@
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 @@
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 @@
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_;
};
diff --git a/webrtc/modules/rtp_rtcp/source/nack_rtx_unittest.cc b/webrtc/modules/rtp_rtcp/source/nack_rtx_unittest.cc
index 483ee13..07a3693 100644
--- a/webrtc/modules/rtp_rtcp/source/nack_rtx_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/nack_rtx_unittest.cc
@@ -109,7 +109,6 @@
// 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 @@
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;
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc
index 20e650c..5958fea 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc
@@ -25,8 +25,8 @@
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 @@
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 @@
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;
}
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc
index 5026986..0b9bf27 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry_unittest.cc
+++ b/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 @@
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));
diff --git a/webrtc/video/video_receive_stream.cc b/webrtc/video/video_receive_stream.cc
index 018da73..0d0953e 100644
--- a/webrtc/video/video_receive_stream.cc
+++ b/webrtc/video/video_receive_stream.cc
@@ -167,6 +167,10 @@
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.
diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc
index 84a86f8..bd722cf 100644
--- a/webrtc/video_engine/vie_channel.cc
+++ b/webrtc/video_engine/vie_channel.cc
@@ -750,6 +750,10 @@
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_) {
diff --git a/webrtc/video_engine/vie_channel.h b/webrtc/video_engine/vie_channel.h
index ed6521c..b36ce21 100644
--- a/webrtc/video_engine/vie_channel.h
+++ b/webrtc/video_engine/vie_channel.h
@@ -134,6 +134,11 @@
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);
diff --git a/webrtc/video_engine/vie_receiver.cc b/webrtc/video_engine/vie_receiver.cc
index f10f287..b119789 100644
--- a/webrtc/video_engine/vie_receiver.cc
+++ b/webrtc/video_engine/vie_receiver.cc
@@ -118,6 +118,10 @@
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 @@
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;
}
diff --git a/webrtc/video_engine/vie_receiver.h b/webrtc/video_engine/vie_receiver.h
index c7d4c33..cd069ea 100644
--- a/webrtc/video_engine/vie_receiver.h
+++ b/webrtc/video_engine/vie_receiver.h
@@ -46,6 +46,11 @@
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;
diff --git a/webrtc/video_receive_stream.h b/webrtc/video_receive_stream.h
index 68751dd..275162c 100644
--- a/webrtc/video_receive_stream.h
+++ b/webrtc/video_receive_stream.h
@@ -133,6 +133,11 @@
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;
diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc
index e4dc320..cd35468 100644
--- a/webrtc/voice_engine/channel.cc
+++ b/webrtc/voice_engine/channel.cc
@@ -1627,16 +1627,15 @@
"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;
}
