Change type of nack_last_time_sent_full_ from uint32_t to int64_t.

Could cause nack requests to be sent too frequently.

R=stefan@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/27339004

git-svn-id: http://webrtc.googlecode.com/svn/trunk@7825 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.cc

@@ -87,6 +87,7 @@ ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration)
       padding_index_(static_cast<size_t>(-1)),  // Start padding at first child.
       nack_method_(kNackOff),
       nack_last_time_sent_full_(0),
+      nack_last_time_sent_full_prev_(0),
       nack_last_seq_number_sent_(0),
       simulcast_(false),
       key_frame_req_method_(kKeyFrameReqFirRtp),
@@ -911,51 +912,57 @@ int ModuleRtpRtcpImpl::SetSelectiveRetransmissions(uint8_t settings) {
 // Send a Negative acknowledgment packet.
 int32_t ModuleRtpRtcpImpl::SendNACK(const uint16_t* nack_list,
                                     const uint16_t size) {
-  // Use RTT from RtcpRttStats class if provided.
+  uint16_t nack_length = size;
-  uint16_t rtt = rtt_ms();
-  if (rtt == 0) {
-    rtcp_receiver_.RTT(rtcp_receiver_.RemoteSSRC(), NULL, &rtt, NULL, NULL);
-  }
-
-  int64_t wait_time = 5 + ((rtt * 3) >> 1);  // 5 + RTT * 1.5.
-  if (wait_time == 5) {
-    wait_time = 100;  // During startup we don't have an RTT.
-  }
-  const int64_t now = clock_->TimeInMilliseconds();
-  const int64_t time_limit = now - wait_time;
-  uint16_t nackLength = size;
   uint16_t start_id = 0;
-
+  int64_t now = clock_->TimeInMilliseconds();
-  if (nack_last_time_sent_full_ < time_limit) {
+  if (TimeToSendFullNackList(now)) {
-    // Send list. Set the timer to make sure we only send a full NACK list once
-    // within every time_limit.
     nack_last_time_sent_full_ = now;
+    nack_last_time_sent_full_prev_ = now;
   } else {
-    // Only send if extended list.
+    // Only send extended list.
     if (nack_last_seq_number_sent_ == nack_list[size - 1]) {
-      // Last seq num is the same don't send list.
+      // Last sequence number is the same, do not send list.
       return 0;
-    } else {
+    }
-      // Send NACKs only for new sequence numbers to avoid re-sending
+    // Send new sequence numbers.
-      // NACKs for sequences we have already sent.
     for (int i = 0; i < size; ++i) {
       if (nack_last_seq_number_sent_ == nack_list[i]) {
         start_id = i + 1;
         break;
       }
     }
-      nackLength = size - start_id;
+    nack_length = size - start_id;
-    }
   }
+
   // Our RTCP NACK implementation is limited to kRtcpMaxNackFields sequence
   // numbers per RTCP packet.
-  if (nackLength > kRtcpMaxNackFields) {
+  if (nack_length > kRtcpMaxNackFields) {
-    nackLength = kRtcpMaxNackFields;
+    nack_length = kRtcpMaxNackFields;
   }
-  nack_last_seq_number_sent_ = nack_list[start_id + nackLength - 1];
+  nack_last_seq_number_sent_ = nack_list[start_id + nack_length - 1];
 
   return rtcp_sender_.SendRTCP(
-      GetFeedbackState(), kRtcpNack, nackLength, &nack_list[start_id]);
+      GetFeedbackState(), kRtcpNack, nack_length, &nack_list[start_id]);
+}
+
+bool ModuleRtpRtcpImpl::TimeToSendFullNackList(int64_t now) const {
+  // Use RTT from RtcpRttStats class if provided.
+  uint16_t rtt = rtt_ms();
+  if (rtt == 0) {
+    rtcp_receiver_.RTT(rtcp_receiver_.RemoteSSRC(), NULL, &rtt, NULL, NULL);
+  }
+
+  const int64_t kStartUpRttMs = 100;
+  int64_t wait_time = 5 + ((rtt * 3) >> 1);  // 5 + RTT * 1.5.
+  if (rtt == 0) {
+    wait_time = kStartUpRttMs;
+  }
+
+  // Send a full NACK list once within every |wait_time|.
+  if (rtt_stats_) {
+    return now - nack_last_time_sent_full_ > wait_time;
+  }
+  return now - nack_last_time_sent_full_prev_ > wait_time;
 }
 
 // Store the sent packets, needed to answer to a Negative acknowledgment

webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl.h

@@ -391,6 +391,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp {
   void set_rtt_ms(uint32_t rtt_ms);
   uint32_t rtt_ms() const;
 
+  bool TimeToSendFullNackList(int64_t now) const;
+
   bool IsDefaultModule() const;
 
   int32_t             id_;
@@ -409,7 +411,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp {
 
   // Send side
   NACKMethod            nack_method_;
-  uint32_t        nack_last_time_sent_full_;
+  int64_t nack_last_time_sent_full_;
+  uint32_t nack_last_time_sent_full_prev_;
   uint16_t nack_last_seq_number_sent_;
 
   bool                  simulcast_;

webrtc/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc

@@ -145,7 +145,7 @@ class RtpRtcpModule {
 class RtpRtcpImplTest : public ::testing::Test {
  protected:
   RtpRtcpImplTest()
-      : clock_(1335900000),
+      : clock_(133590000000000),
         sender_(&clock_),
         receiver_(&clock_) {
     // Send module.
@@ -195,12 +195,13 @@ class RtpRtcpImplTest : public ::testing::Test {
   }
 
   void IncomingRtcpNack(const RtpRtcpModule* module, uint16_t sequence_number) {
+    bool sender = module->impl_->SSRC() == kSenderSsrc;
     rtcp::Nack nack;
     uint16_t list[1];
     list[0] = sequence_number;
     const uint16_t kListLength = sizeof(list) / sizeof(list[0]);
-    nack.From(kReceiverSsrc);
+    nack.From(sender ? kReceiverSsrc : kSenderSsrc);
-    nack.To(kSenderSsrc);
+    nack.To(sender ? kSenderSsrc : kReceiverSsrc);
     nack.WithList(list, kListLength);
     rtcp::RawPacket packet = nack.Build();
     EXPECT_EQ(0, module->impl_->IncomingRtcpPacket(packet.buffer(),
@@ -354,6 +355,61 @@ TEST_F(RtpRtcpImplTest, RtcpPacketTypeCounter_FirAndPli) {
   EXPECT_EQ(1U, sender_.RtcpReceived().pli_packets);
 }
 
+TEST_F(RtpRtcpImplTest, SendsInitialNackList) {
+  // Send module sends a NACK.
+  const uint16_t kNackLength = 1;
+  uint16_t nack_list[kNackLength] = {123};
+  EXPECT_EQ(0U, sender_.RtcpSent().nack_packets);
+  EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+  EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+  EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123));
+}
+
+TEST_F(RtpRtcpImplTest, SendsExtendedNackList) {
+  // Send module sends a NACK.
+  const uint16_t kNackLength = 1;
+  uint16_t nack_list[kNackLength] = {123};
+  EXPECT_EQ(0U, sender_.RtcpSent().nack_packets);
+  EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+  EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+  EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123));
+
+  // Same list not re-send.
+  EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+  EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+  EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123));
+
+  // Only extended list sent.
+  const uint16_t kNackExtLength = 2;
+  uint16_t nack_list_ext[kNackExtLength] = {123, 124};
+  EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list_ext, kNackExtLength));
+  EXPECT_EQ(2U, sender_.RtcpSent().nack_packets);
+  EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(124));
+}
+
+TEST_F(RtpRtcpImplTest, ReSendsNackListAfterRttMs) {
+  sender_.transport_.SimulateNetworkDelay(0, &clock_);
+  // Send module sends a NACK.
+  const uint16_t kNackLength = 2;
+  uint16_t nack_list[kNackLength] = {123, 125};
+  EXPECT_EQ(0U, sender_.RtcpSent().nack_packets);
+  EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+  EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+  EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123, 125));
+
+  // Same list not re-send, rtt interval has not passed.
+  const int kStartupRttMs = 100;
+  clock_.AdvanceTimeMilliseconds(kStartupRttMs);
+  EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+  EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+
+  // Rtt interval passed, full list sent.
+  clock_.AdvanceTimeMilliseconds(1);
+  EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+  EXPECT_EQ(2U, sender_.RtcpSent().nack_packets);
+  EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123, 125));
+}
+
 TEST_F(RtpRtcpImplTest, UniqueNackRequests) {
   receiver_.transport_.SimulateNetworkDelay(0, &clock_);
   EXPECT_EQ(0U, receiver_.RtcpSent().nack_packets);