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

@@ -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

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 };

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:

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_;

webrtc/modules/rtp_rtcp/test/testAPI/test_api_nack.cc

@@ -9,7 +9,9 @@
  */
 
 #include <algorithm>
-#include <vector>
+#include <iterator>
+#include <list>
+#include <set>
 #include <gtest/gtest.h>
 
 #include "test_api.h"
@@ -45,7 +47,7 @@ class VerifyingNackReceiver : public RtpData
     sequence_numbers_.push_back(rtp_header->header.sequenceNumber);
     return 0;
   }
-  std::vector<WebRtc_UWord16 > sequence_numbers_;
+  std::list<uint16_t> 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<const unsigned  char*>(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<uint16_t> expected_sequence_numbers_;
 };
 
 class RtpRtcpNackTest : public ::testing::Test {
@@ -143,31 +163,11 @@ class RtpRtcpNackTest : public ::testing::Test {
     delete nack_receiver_;
   }
 
-  RtpRtcp* video_module_;
+  int BuildNackList(uint16_t* nack_list) const {
-  NackLoopBackTransport* transport_;
+    nack_receiver_->sequence_numbers_.sort();
-  VerifyingNackReceiver* nack_receiver_;
-  WebRtc_UWord8  payload_data[65000];
-  int payload_data_length;
-  SimulatedClock fake_clock;
-};
 
-TEST_F(RtpRtcpNackTest, RTCP) {
+    std::list<uint16_t> missing_sequence_numbers;
-  WebRtc_UWord32 timestamp = 3000;
+    std::list<uint16_t>::iterator it =
-  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<WebRtc_UWord16> missing_sequence_numbers;
-    std::vector<WebRtc_UWord16>::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<uint16_t> 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_.sort();
-            nack_receiver_->sequence_numbers_.end());
   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_.sort();
-              nack_receiver_->sequence_numbers_.end());
 
-    std::vector<WebRtc_UWord16> missing_sequence_numbers;
+    std::list<WebRtc_UWord16> missing_sequence_numbers;
 
 
-    std::vector<WebRtc_UWord16>::iterator it =
+    std::list<WebRtc_UWord16>::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_.sort();
-            nack_receiver_->sequence_numbers_.end());
   EXPECT_EQ(kTestSequenceNumber, *(nack_receiver_->sequence_numbers_.begin()));
   EXPECT_EQ(kTestSequenceNumber + kTestNumberOfPackets - 1,
             *(nack_receiver_->sequence_numbers_.rbegin()));