From 37c0559c1edd108b345abcce1939f9b8d78d02a3 Mon Sep 17 00:00:00 2001 From: "asapersson@webrtc.org" Date: Wed, 28 Jan 2015 13:58:27 +0000 Subject: [PATCH] Notify jitter buffer about received FEC packets (to avoid sending NACK request for these packets). Don't copy codec specific header for empty packets in the jitter buffer. BUG=3135 R=pbos@webrtc.org, stefan@webrtc.org Review URL: https://webrtc-codereview.appspot.com/37659004 Cr-Commit-Position: refs/heads/master@{#8184} git-svn-id: http://webrtc.googlecode.com/svn/trunk@8184 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../rtp_rtcp/source/rtcp_packet_unittest.cc | 2 - .../video_coding/main/source/frame_buffer.cc | 4 +- webrtc/test/rtcp_packet_parser.cc | 3 + webrtc/test/test.gyp | 1 + webrtc/video/end_to_end_tests.cc | 114 ++++++++++++++++++ webrtc/video_engine/vie_receiver.cc | 27 ++++- webrtc/video_engine/vie_receiver.h | 1 + 7 files changed, 148 insertions(+), 4 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc index 095022efc2..c0ba3075d1 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_packet_unittest.cc @@ -572,8 +572,6 @@ TEST(RtcpPacketTest, BuildWithTooSmallBuffer) { size_t len = 0; uint8_t packet[kRrLength + kReportBlockLength - 1]; rr.Build(packet, &len, kRrLength + kReportBlockLength - 1); - RtcpPacketParser parser; - parser.Parse(packet, len); EXPECT_EQ(0U, len); } diff --git a/webrtc/modules/video_coding/main/source/frame_buffer.cc b/webrtc/modules/video_coding/main/source/frame_buffer.cc index fd353009c8..94b06f1ff7 100644 --- a/webrtc/modules/video_coding/main/source/frame_buffer.cc +++ b/webrtc/modules/video_coding/main/source/frame_buffer.cc @@ -127,7 +127,9 @@ VCMFrameBuffer::InsertPacket(const VCMPacket& packet, _encodedHeight = packet.height; } - CopyCodecSpecific(&packet.codecSpecificHeader); + // Don't copy payload specific data for empty packets (e.g padding packets). + if (packet.sizeBytes > 0) + CopyCodecSpecific(&packet.codecSpecificHeader); int retVal = _sessionInfo.InsertPacket(packet, _buffer, decode_error_mode, diff --git a/webrtc/test/rtcp_packet_parser.cc b/webrtc/test/rtcp_packet_parser.cc index 391be85852..b9e430fe9f 100644 --- a/webrtc/test/rtcp_packet_parser.cc +++ b/webrtc/test/rtcp_packet_parser.cc @@ -10,6 +10,8 @@ #include "webrtc/test/rtcp_packet_parser.h" +#include "testing/gtest/include/gtest/gtest.h" + namespace webrtc { namespace test { @@ -20,6 +22,7 @@ RtcpPacketParser::~RtcpPacketParser() {} void RtcpPacketParser::Parse(const void *data, size_t len) { const uint8_t* packet = static_cast(data); RTCPUtility::RTCPParserV2 parser(packet, len, true); + EXPECT_TRUE(parser.IsValid()); for (RTCPUtility::RTCPPacketTypes type = parser.Begin(); type != RTCPUtility::kRtcpNotValidCode; type = parser.Iterate()) { diff --git a/webrtc/test/test.gyp b/webrtc/test/test.gyp index 03e05291a1..83f2316e83 100644 --- a/webrtc/test/test.gyp +++ b/webrtc/test/test.gyp @@ -64,6 +64,7 @@ 'rtp_file_writer.h', ], 'dependencies': [ + '<(DEPTH)/testing/gtest.gyp:gtest', '<(webrtc_root)/modules/modules.gyp:rtp_rtcp', ], }, diff --git a/webrtc/video/end_to_end_tests.cc b/webrtc/video/end_to_end_tests.cc index e888ed80dc..c01035dec1 100644 --- a/webrtc/video/end_to_end_tests.cc +++ b/webrtc/video/end_to_end_tests.cc @@ -35,6 +35,7 @@ #include "webrtc/test/frame_generator.h" #include "webrtc/test/frame_generator_capturer.h" #include "webrtc/test/null_transport.h" +#include "webrtc/test/rtcp_packet_parser.h" #include "webrtc/test/rtp_rtcp_observer.h" #include "webrtc/test/testsupport/fileutils.h" #include "webrtc/test/testsupport/gtest_disable.h" @@ -75,6 +76,7 @@ class EndToEndTest : public test::CallTest { void TestXrReceiverReferenceTimeReport(bool enable_rrtr); void TestSendsSetSsrcs(size_t num_ssrcs, bool send_single_ssrc_first); void TestRtpStatePreservation(bool use_rtx); + void TestReceivedFecPacketsNotNacked(const FakeNetworkPipe::Config& config); }; TEST_F(EndToEndTest, ReceiverCanBeStartedTwice) { @@ -536,6 +538,118 @@ TEST_F(EndToEndTest, CanReceiveFec) { RunBaseTest(&test); } +TEST_F(EndToEndTest, ReceivedFecPacketsNotNacked) { + // At low RTT (< kLowRttNackMs) -> NACK only, no FEC. + // Configure some network delay. + const int kNetworkDelayMs = 50; + FakeNetworkPipe::Config config; + config.queue_delay_ms = kNetworkDelayMs; + TestReceivedFecPacketsNotNacked(config); +} + +void EndToEndTest::TestReceivedFecPacketsNotNacked( + const FakeNetworkPipe::Config& config) { + class FecNackObserver : public test::EndToEndTest { + public: + explicit FecNackObserver(const FakeNetworkPipe::Config& config) + : EndToEndTest(kDefaultTimeoutMs, config), + state_(kDropEveryOtherPacketUntilFec), + fec_sequence_number_(0), + has_last_sequence_number_(false), + last_sequence_number_(0) {} + + private: + virtual Action OnSendRtp(const uint8_t* packet, size_t length) OVERRIDE { + RTPHeader header; + EXPECT_TRUE(parser_->Parse(packet, length, &header)); + EXPECT_EQ(kRedPayloadType, header.payloadType); + + int encapsulated_payload_type = + static_cast(packet[header.headerLength]); + if (encapsulated_payload_type != kFakeSendPayloadType) + EXPECT_EQ(kUlpfecPayloadType, encapsulated_payload_type); + + if (has_last_sequence_number_ && + !IsNewerSequenceNumber(header.sequenceNumber, + last_sequence_number_)) { + // Drop retransmitted packets. + return DROP_PACKET; + } + last_sequence_number_ = header.sequenceNumber; + has_last_sequence_number_ = true; + + bool fec_packet = encapsulated_payload_type == kUlpfecPayloadType; + switch (state_) { + case kDropEveryOtherPacketUntilFec: + if (fec_packet) { + state_ = kDropAllMediaPacketsUntilFec; + } else if (header.sequenceNumber % 2 == 0) { + return DROP_PACKET; + } + break; + case kDropAllMediaPacketsUntilFec: + if (!fec_packet) + return DROP_PACKET; + fec_sequence_number_ = header.sequenceNumber; + state_ = kVerifyFecPacketNotInNackList; + break; + case kVerifyFecPacketNotInNackList: + // Continue to drop packets. Make sure no frame can be decoded. + if (fec_packet || header.sequenceNumber % 2 == 0) + return DROP_PACKET; + break; + } + return SEND_PACKET; + } + + virtual Action OnReceiveRtcp(const uint8_t* packet, size_t length) + OVERRIDE { + if (state_ == kVerifyFecPacketNotInNackList) { + test::RtcpPacketParser rtcp_parser; + rtcp_parser.Parse(packet, length); + std::vector nacks = rtcp_parser.nack_item()->last_nack_list(); + if (!nacks.empty() && + IsNewerSequenceNumber(nacks.back(), fec_sequence_number_)) { + EXPECT_TRUE(std::find( + nacks.begin(), nacks.end(), fec_sequence_number_) == nacks.end()); + observation_complete_->Set(); + } + } + return SEND_PACKET; + } + + virtual void ModifyConfigs( + VideoSendStream::Config* send_config, + std::vector* receive_configs, + VideoEncoderConfig* encoder_config) OVERRIDE { + // Configure hybrid NACK/FEC. + send_config->rtp.nack.rtp_history_ms = kNackRtpHistoryMs; + send_config->rtp.fec.red_payload_type = kRedPayloadType; + send_config->rtp.fec.ulpfec_payload_type = kUlpfecPayloadType; + (*receive_configs)[0].rtp.nack.rtp_history_ms = kNackRtpHistoryMs; + (*receive_configs)[0].rtp.fec.red_payload_type = kRedPayloadType; + (*receive_configs)[0].rtp.fec.ulpfec_payload_type = kUlpfecPayloadType; + } + + virtual void PerformTest() OVERRIDE { + EXPECT_EQ(kEventSignaled, Wait()) + << "Timed out while waiting for FEC packets to be received."; + } + + enum { + kDropEveryOtherPacketUntilFec, + kDropAllMediaPacketsUntilFec, + kVerifyFecPacketNotInNackList, + } state_; + + uint16_t fec_sequence_number_; + bool has_last_sequence_number_; + uint16_t last_sequence_number_; + } test(config); + + RunBaseTest(&test); +} + // This test drops second RTP packet with a marker bit set, makes sure it's // retransmitted and renders. Retransmission SSRCs are also checked. void EndToEndTest::DecodesRetransmittedFrame(bool retransmit_over_rtx) { diff --git a/webrtc/video_engine/vie_receiver.cc b/webrtc/video_engine/vie_receiver.cc index cdff8607c1..1d2485ed4a 100644 --- a/webrtc/video_engine/vie_receiver.cc +++ b/webrtc/video_engine/vie_receiver.cc @@ -322,8 +322,11 @@ bool ViEReceiver::ParseAndHandleEncapsulatingHeader(const uint8_t* packet, const RTPHeader& header) { if (rtp_payload_registry_->IsRed(header)) { int8_t ulpfec_pt = rtp_payload_registry_->ulpfec_payload_type(); - if (packet[header.headerLength] == ulpfec_pt) + if (packet[header.headerLength] == ulpfec_pt) { rtp_receive_statistics_->FecPacketReceived(header, packet_length); + // Notify vcm about received FEC packets to avoid NACKing these packets. + NotifyReceiverOfFecPacket(header); + } if (fec_receiver_->AddReceivedRedPacket( header, packet, packet_length, ulpfec_pt) != 0) { return false; @@ -360,6 +363,28 @@ bool ViEReceiver::ParseAndHandleEncapsulatingHeader(const uint8_t* packet, return false; } +void ViEReceiver::NotifyReceiverOfFecPacket(const RTPHeader& header) { + int8_t last_media_payload_type = + rtp_payload_registry_->last_received_media_payload_type(); + if (last_media_payload_type < 0) { + LOG(LS_WARNING) << "Failed to get last media payload type."; + return; + } + // Fake an empty media packet. + WebRtcRTPHeader rtp_header = {}; + rtp_header.header = header; + rtp_header.header.payloadType = last_media_payload_type; + rtp_header.header.paddingLength = 0; + PayloadUnion payload_specific; + if (!rtp_payload_registry_->GetPayloadSpecifics(last_media_payload_type, + &payload_specific)) { + LOG(LS_WARNING) << "Failed to get payload specifics."; + return; + } + rtp_header.type.Video.codec = payload_specific.Video.videoCodecType; + OnReceivedPayloadData(NULL, 0, &rtp_header); +} + int ViEReceiver::InsertRTCPPacket(const uint8_t* rtcp_packet, size_t rtcp_packet_length) { { diff --git a/webrtc/video_engine/vie_receiver.h b/webrtc/video_engine/vie_receiver.h index b8e6569b4d..2216df12c1 100644 --- a/webrtc/video_engine/vie_receiver.h +++ b/webrtc/video_engine/vie_receiver.h @@ -103,6 +103,7 @@ class ViEReceiver : public RtpData { bool ParseAndHandleEncapsulatingHeader(const uint8_t* packet, size_t packet_length, const RTPHeader& header); + void NotifyReceiverOfFecPacket(const RTPHeader& header); int InsertRTCPPacket(const uint8_t* rtcp_packet, size_t rtcp_packet_length); bool IsPacketInOrder(const RTPHeader& header) const; bool IsPacketRetransmitted(const RTPHeader& header, bool in_order) const;