From a27107004d8544c6dbf8eaa231e6079b73c90efe Mon Sep 17 00:00:00 2001 From: "stefan@webrtc.org" Date: Tue, 5 Mar 2013 09:02:06 +0000 Subject: [PATCH] Split the NACK list into multiple RTCPs if it's too big. TEST=rtp_rtcp_unittests BUG=1434 Review URL: https://webrtc-codereview.appspot.com/1148006 git-svn-id: http://webrtc.googlecode.com/svn/trunk@3609 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/modules/rtp_rtcp/source/rtcp_sender.cc | 2 +- .../modules/rtp_rtcp/source/rtp_rtcp_config.h | 1 + .../modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 13 +- .../modules/rtp_rtcp/source/rtp_rtcp_impl.h | 2 +- .../rtp_rtcp/test/testAPI/test_api_nack.cc | 142 +++++++++++++----- 5 files changed, 119 insertions(+), 41 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc index d5dd4a113..5d598e03a 100644 --- a/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/webrtc/modules/rtp_rtcp/source/rtcp_sender.cc @@ -1324,7 +1324,7 @@ RTCPSender::BuildNACK(WebRtc_UWord8* rtcpbuffer, // add the list int i = 0; int numOfNackFields = 0; - while(nackSize > i && numOfNackFields < 253) + while (nackSize > i && numOfNackFields < kRtcpMaxNackFields) { WebRtc_UWord16 nack = nackList[i]; // put dow our sequence number diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h index c8886365e..408354056 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_config.h @@ -22,6 +22,7 @@ enum { NACK_BYTECOUNT_SIZE = 60}; // size of our NACK history // A sanity for the NACK list parsing at the send-side. enum { kSendSideNackListSizeSanity = 20000 }; enum { kDefaultMaxReorderingThreshold = 50 }; // In sequence numbers. +enum { kRtcpMaxNackFields = 253 }; enum { RTCP_INTERVAL_VIDEO_MS = 1000 }; enum { RTCP_INTERVAL_AUDIO_MS = 5000 }; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 7eded947c..02d1e14d3 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -118,7 +118,7 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) dead_or_alive_timeout_ms_(0), dead_or_alive_last_timer_(0), nack_method_(kNackOff), - nack_last_time_sent_(0), + nack_last_time_sent_full_(0), nack_last_seq_number_sent_(0), simulcast_(false), key_frame_req_method_(kKeyFrameReqFirRtp), @@ -1528,10 +1528,10 @@ WebRtc_Word32 ModuleRtpRtcpImpl::SendNACK(const WebRtc_UWord16* nack_list, WebRtc_UWord16 nackLength = size; WebRtc_UWord16 start_id = 0; - if (nack_last_time_sent_ < time_limit) { + if (nack_last_time_sent_full_ < time_limit) { // Send list. Set the timer to make sure we only send a full NACK list once // within every time_limit. - nack_last_time_sent_ = now; + nack_last_time_sent_full_ = now; } else { // Only send if extended list. if (nack_last_seq_number_sent_ == nack_list[size - 1]) { @@ -1549,7 +1549,12 @@ WebRtc_Word32 ModuleRtpRtcpImpl::SendNACK(const WebRtc_UWord16* nack_list, nackLength = size - start_id; } } - nack_last_seq_number_sent_ = nack_list[size - 1]; + // Our RTCP NACK implementation is limited to kRtcpMaxNackFields sequence + // numbers per RTCP packet. + if (nackLength > kRtcpMaxNackFields) { + nackLength = kRtcpMaxNackFields; + } + nack_last_seq_number_sent_ = nack_list[start_id + nackLength - 1]; switch (nack_method_) { case kNackRtcp: diff --git a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 49f7a28da..a49ca3a57 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -504,7 +504,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp { WebRtc_Word64 dead_or_alive_last_timer_; // Send side NACKMethod nack_method_; - WebRtc_UWord32 nack_last_time_sent_; + WebRtc_UWord32 nack_last_time_sent_full_; WebRtc_UWord16 nack_last_seq_number_sent_; bool simulcast_; diff --git a/webrtc/modules/rtp_rtcp/test/testAPI/test_api_nack.cc b/webrtc/modules/rtp_rtcp/test/testAPI/test_api_nack.cc index 16e460148..c7770a843 100644 --- a/webrtc/modules/rtp_rtcp/test/testAPI/test_api_nack.cc +++ b/webrtc/modules/rtp_rtcp/test/testAPI/test_api_nack.cc @@ -9,7 +9,9 @@ */ #include -#include +#include +#include +#include #include #include "test_api.h" @@ -45,7 +47,7 @@ class VerifyingNackReceiver : public RtpData sequence_numbers_.push_back(rtp_header->header.sequenceNumber); return 0; } - std::vector sequence_numbers_; + std::list sequence_numbers_; }; class NackLoopBackTransport : public webrtc::Transport { @@ -53,6 +55,8 @@ class NackLoopBackTransport : public webrtc::Transport { NackLoopBackTransport(uint32_t rtx_ssrc) : count_(0), packet_loss_(0), + consecutive_drop_start_(0), + consecutive_drop_end_(0), rtx_ssrc_(rtx_ssrc), count_rtx_ssrc_(0), module_(NULL) { @@ -62,17 +66,30 @@ class NackLoopBackTransport : public webrtc::Transport { } void DropEveryNthPacket(int n) { packet_loss_ = n; + consecutive_drop_start_ = 0; + consecutive_drop_end_ = 0; + } + void DropConsecutivePackets(int start, int total) { + consecutive_drop_start_ = start; + consecutive_drop_end_ = start + total; + packet_loss_ = 0; } virtual int SendPacket(int channel, const void *data, int len) { count_++; const unsigned char* ptr = static_cast(data); uint32_t ssrc = (ptr[8] << 24) + (ptr[9] << 16) + (ptr[10] << 8) + ptr[11]; if (ssrc == rtx_ssrc_) count_rtx_ssrc_++; + uint16_t sequence_number = (ptr[2] << 8) + ptr[3]; + expected_sequence_numbers_.insert(expected_sequence_numbers_.end(), + sequence_number); if (packet_loss_ > 0) { if ((count_ % packet_loss_) == 0) { return len; } + } else if (count_ >= consecutive_drop_start_ && + count_ < consecutive_drop_end_) { + return len; } if (module_->IncomingPacket((const WebRtc_UWord8*)data, len) == 0) { return len; @@ -87,9 +104,12 @@ class NackLoopBackTransport : public webrtc::Transport { } int count_; int packet_loss_; + int consecutive_drop_start_; + int consecutive_drop_end_; uint32_t rtx_ssrc_; int count_rtx_ssrc_; RtpRtcp* module_; + std::set expected_sequence_numbers_; }; class RtpRtcpNackTest : public ::testing::Test { @@ -143,31 +163,11 @@ class RtpRtcpNackTest : public ::testing::Test { delete nack_receiver_; } - RtpRtcp* video_module_; - NackLoopBackTransport* transport_; - VerifyingNackReceiver* nack_receiver_; - WebRtc_UWord8 payload_data[65000]; - int payload_data_length; - SimulatedClock fake_clock; -}; + int BuildNackList(uint16_t* nack_list) const { + nack_receiver_->sequence_numbers_.sort(); -TEST_F(RtpRtcpNackTest, RTCP) { - WebRtc_UWord32 timestamp = 3000; - WebRtc_UWord16 nack_list[kVideoNackListSize]; - transport_->DropEveryNthPacket(10); - - for (int frame = 0; frame < 10; ++frame) { - EXPECT_EQ(0, video_module_->SendOutgoingData(webrtc::kVideoFrameDelta, 123, - timestamp, - timestamp / 90, - payload_data, - payload_data_length)); - - std::sort(nack_receiver_->sequence_numbers_.begin(), - nack_receiver_->sequence_numbers_.end()); - - std::vector missing_sequence_numbers; - std::vector::iterator it = + std::list missing_sequence_numbers; + std::list::iterator it = nack_receiver_->sequence_numbers_.begin(); while (it != nack_receiver_->sequence_numbers_.end()) { @@ -187,15 +187,48 @@ TEST_F(RtpRtcpNackTest, RTCP) { it != missing_sequence_numbers.end(); ++it) { nack_list[n++] = (*it); } - video_module_->SendNACK(nack_list, n); + return n; + } + + bool ExpectedPacketsReceived() { + std::list received_sorted; + std::copy(nack_receiver_->sequence_numbers_.begin(), + nack_receiver_->sequence_numbers_.end(), + std::back_inserter(received_sorted)); + received_sorted.sort(); + return std::equal(received_sorted.begin(), received_sorted.end(), + transport_->expected_sequence_numbers_.begin()); + } + + RtpRtcp* video_module_; + NackLoopBackTransport* transport_; + VerifyingNackReceiver* nack_receiver_; + WebRtc_UWord8 payload_data[65000]; + int payload_data_length; + SimulatedClock fake_clock; +}; + +TEST_F(RtpRtcpNackTest, RTCP) { + WebRtc_UWord32 timestamp = 3000; + uint16_t nack_list[kVideoNackListSize]; + transport_->DropEveryNthPacket(10); + + for (int frame = 0; frame < 10; ++frame) { + EXPECT_EQ(0, video_module_->SendOutgoingData(webrtc::kVideoFrameDelta, 123, + timestamp, + timestamp / 90, + payload_data, + payload_data_length)); + + int length = BuildNackList(nack_list); + video_module_->SendNACK(nack_list, length); fake_clock.AdvanceTimeMilliseconds(33); video_module_->Process(); // Prepare next frame. timestamp += 3000; } - std::sort(nack_receiver_->sequence_numbers_.begin(), - nack_receiver_->sequence_numbers_.end()); + nack_receiver_->sequence_numbers_.sort(); EXPECT_EQ(kTestSequenceNumber, *(nack_receiver_->sequence_numbers_.begin())); EXPECT_EQ(kTestSequenceNumber + kTestNumberOfPackets - 1, *(nack_receiver_->sequence_numbers_.rbegin())); @@ -203,6 +236,47 @@ TEST_F(RtpRtcpNackTest, RTCP) { EXPECT_EQ(0, transport_->count_rtx_ssrc_); } +TEST_F(RtpRtcpNackTest, LongNackList) { + const int kNumPacketsToDrop = 900; + const int kNumFrames = 30; + const int kNumRequiredRtcp = 4; + WebRtc_UWord32 timestamp = 3000; + uint16_t nack_list[kNumPacketsToDrop]; + // Disable StorePackets to be able to set a larger packet history. + EXPECT_EQ(0, video_module_->SetStorePacketsStatus(false, 0)); + // Enable StorePackets with a packet history of 2000 packets. + EXPECT_EQ(0, video_module_->SetStorePacketsStatus(true, 2000)); + // Drop 900 packets from the second one so that we get a NACK list which is + // big enough to require 4 RTCP packets to be fully transmitted to the sender. + transport_->DropConsecutivePackets(2, kNumPacketsToDrop); + // Send 30 frames which at the default size is roughly what we need to get + // enough packets. + for (int frame = 0; frame < kNumFrames; ++frame) { + EXPECT_EQ(0, video_module_->SendOutgoingData(webrtc::kVideoFrameDelta, 123, + timestamp, + timestamp / 90, + payload_data, + payload_data_length)); + // Prepare next frame. + timestamp += 3000; + fake_clock.AdvanceTimeMilliseconds(33); + video_module_->Process(); + } + EXPECT_FALSE(transport_->expected_sequence_numbers_.empty()); + EXPECT_FALSE(nack_receiver_->sequence_numbers_.empty()); + size_t last_receive_count = nack_receiver_->sequence_numbers_.size(); + int length = BuildNackList(nack_list); + for (int i = 0; i < kNumRequiredRtcp - 1; ++i) { + video_module_->SendNACK(nack_list, length); + EXPECT_GT(nack_receiver_->sequence_numbers_.size(), last_receive_count); + last_receive_count = nack_receiver_->sequence_numbers_.size(); + EXPECT_FALSE(ExpectedPacketsReceived()); + } + video_module_->SendNACK(nack_list, length); + EXPECT_GT(nack_receiver_->sequence_numbers_.size(), last_receive_count); + EXPECT_TRUE(ExpectedPacketsReceived()); +} + TEST_F(RtpRtcpNackTest, RTX) { EXPECT_EQ(0, video_module_->SetRTXReceiveStatus(true, kTestSsrc + 1)); EXPECT_EQ(0, video_module_->SetRTXSendStatus(true, true, kTestSsrc + 1)); @@ -220,13 +294,12 @@ TEST_F(RtpRtcpNackTest, RTX) { payload_data, payload_data_length)); - std::sort(nack_receiver_->sequence_numbers_.begin(), - nack_receiver_->sequence_numbers_.end()); + nack_receiver_->sequence_numbers_.sort(); - std::vector missing_sequence_numbers; + std::list missing_sequence_numbers; - std::vector::iterator it = + std::list::iterator it = nack_receiver_->sequence_numbers_.begin(); while (it != nack_receiver_->sequence_numbers_.end()) { int sequence_number_1 = *it; @@ -251,8 +324,7 @@ TEST_F(RtpRtcpNackTest, RTX) { // Prepare next frame. timestamp += 3000; } - std::sort(nack_receiver_->sequence_numbers_.begin(), - nack_receiver_->sequence_numbers_.end()); + nack_receiver_->sequence_numbers_.sort(); EXPECT_EQ(kTestSequenceNumber, *(nack_receiver_->sequence_numbers_.begin())); EXPECT_EQ(kTestSequenceNumber + kTestNumberOfPackets - 1, *(nack_receiver_->sequence_numbers_.rbegin()));