From 65220a70a38ffe252b587775c5e9104606ab7c2c Mon Sep 17 00:00:00 2001 From: noahric Date: Wed, 14 Oct 2015 11:29:49 -0700 Subject: [PATCH] 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} --- .../rtp_rtcp/interface/rtp_payload_registry.h | 15 +- .../rtp_rtcp/source/nack_rtx_unittest.cc | 11 +- .../rtp_rtcp/source/rtp_payload_registry.cc | 39 +++-- .../source/rtp_payload_registry_unittest.cc | 138 ++++++++++++++++++ webrtc/video/video_receive_stream.cc | 4 + webrtc/video_engine/vie_channel.cc | 4 + webrtc/video_engine/vie_channel.h | 5 + webrtc/video_engine/vie_receiver.cc | 11 +- webrtc/video_engine/vie_receiver.h | 5 + webrtc/video_receive_stream.h | 5 + webrtc/voice_engine/channel.cc | 7 +- 11 files changed, 215 insertions(+), 29 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h b/webrtc/modules/rtp_rtcp/interface/rtp_payload_registry.h index bcafdfb7f6..a60a6c5e73 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 @@ 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 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 483ee133d5..07a3693507 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 @@ 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 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; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc b/webrtc/modules/rtp_rtcp/source/rtp_payload_registry.cc index 20e650c04b..5958fea230 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 @@ 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::WriteBigEndian(*restored_packet + 2, + ByteWriter::WriteBigEndian(restored_packet + 2, original_sequence_number); - ByteWriter::WriteBigEndian(*restored_packet + 8, original_ssrc); + ByteWriter::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(incoming_payload_type_); + + restored_packet[1] = static_cast(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 5026986858..0b9bf2751e 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 @@ 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(kRtpExpectedVersion << 6); + // Write a junk sequence number. It should be thrown away when the packet is + // restored. + ByteWriter::WriteBigEndian(packet + 2, 9999); + // Write a junk ssrc. It should also be thrown away when the packet is + // restored. + ByteWriter::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::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 packet(GenerateRtxPacket( + header_length, payload_length, original_sequence_number)); + rtc::scoped_ptr 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 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 018da738fa..0d0953ea91 100644 --- a/webrtc/video/video_receive_stream.cc +++ b/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. diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc index 84a86f8735..bd722cfeda 100644 --- a/webrtc/video_engine/vie_channel.cc +++ b/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_) { diff --git a/webrtc/video_engine/vie_channel.h b/webrtc/video_engine/vie_channel.h index ed6521c20d..b36ce2149e 100644 --- a/webrtc/video_engine/vie_channel.h +++ b/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); diff --git a/webrtc/video_engine/vie_receiver.cc b/webrtc/video_engine/vie_receiver.cc index f10f287017..b1197899f4 100644 --- a/webrtc/video_engine/vie_receiver.cc +++ b/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; } diff --git a/webrtc/video_engine/vie_receiver.h b/webrtc/video_engine/vie_receiver.h index c7d4c33bba..cd069eaa5b 100644 --- a/webrtc/video_engine/vie_receiver.h +++ b/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; diff --git a/webrtc/video_receive_stream.h b/webrtc/video_receive_stream.h index 68751ddab8..275162ca1c 100644 --- a/webrtc/video_receive_stream.h +++ b/webrtc/video_receive_stream.h @@ -133,6 +133,11 @@ class VideoReceiveStream : public ReceiveStream { typedef std::map 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 extensions; } rtp; diff --git a/webrtc/voice_engine/channel.cc b/webrtc/voice_engine/channel.cc index e4dc320417..cd35468400 100644 --- a/webrtc/voice_engine/channel.cc +++ b/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; }