Nack count returned on OnReceivedPacket.

webrtc/modules/video_coding/nack_module.cc

Index: webrtc/modules/video_coding/nack_module.cc
diff --git a/webrtc/modules/video_coding/nack_module.cc b/webrtc/modules/video_coding/nack_module.cc
index 1b12afe0f0df2690ed8f0a47f6ff2eb13ac961ba..36353390ecc9d17c786eb4c08a37e772b02f8a4a 100644
--- a/webrtc/modules/video_coding/nack_module.cc
+++ b/webrtc/modules/video_coding/nack_module.cc
@@ -56,10 +56,10 @@ NackModule::NackModule(Clock* clock,
   RTC_DCHECK(keyframe_request_sender_);
 }
 
-void NackModule::OnReceivedPacket(const VCMPacket& packet) {
+int NackModule::OnReceivedPacket(const VCMPacket& packet) {
   rtc::CritScope lock(&crit_);
   if (!running_)
-    return;
+    return -1;
   uint16_t seq_num = packet.seqNum;
   // TODO(philipel): When the packet includes information whether it is
   //                 retransmitted or not, use that value instead. For
@@ -73,36 +73,44 @@ void NackModule::OnReceivedPacket(const VCMPacket& packet) {
     if (is_keyframe)
       keyframe_list_.insert(seq_num);
     initialized_ = true;
-    return;
+    return 0;
   }
 
+  // Since the |last_seq_num_| is a packet we have actually received we know
+  // that packet has never been Nacked.
   if (seq_num == last_seq_num_)
-    return;
+    return 0;
 
   if (AheadOf(last_seq_num_, seq_num)) {

[stefan-webrtc, 2016/05/12 12:16:53]

Should last_seq_num_ be called newest_seq_num_ instead? Clarify that it's not
the most recently received packet, but the "highest" sequence number

[philipel, 2016/05/12 12:55:16]

Done.

     // An out of order packet has been received.
-    nack_list_.erase(seq_num);
+    auto nack_list_it = nack_list_.find(seq_num);
+    if (nack_list_it != nack_list_.end()) {
+      int nacks_sent_for_packet = nack_list_it->second.retries;
+      nack_list_.erase(nack_list_it);
+      return nacks_sent_for_packet;
+    }
     if (!is_retransmitted)
       UpdateReorderingStatistics(seq_num);

[stefan-webrtc, 2016/05/12 12:16:53]

Should we not update reordering statistics if the packet wasn't in the nack
list?

[philipel, 2016/05/12 12:55:16]

If the packet isn't in the nack list then the packet was received in order.

To clarify, the reordering statistics doesn't tell us how often reordering
occurs, it only tells us how severe the reordering is when it occurs. But now
that you mention it, I realize I have to include how often it occurs as well...

-    return;
-  } else {
-    AddPacketsToNack(last_seq_num_ + 1, seq_num);
-    last_seq_num_ = seq_num;
+    return 0;
+  }
+  AddPacketsToNack(last_seq_num_ + 1, seq_num);
+  last_seq_num_ = seq_num;
 
-    // Keep track of new keyframes.
-    if (is_keyframe)
-      keyframe_list_.insert(seq_num);
+  // Keep track of new keyframes.
+  if (is_keyframe)
+    keyframe_list_.insert(seq_num);
 
-    // And remove old ones so we don't accumulate keyframes.
-    auto it = keyframe_list_.lower_bound(seq_num - kMaxPacketAge);
-    if (it != keyframe_list_.begin())
-      keyframe_list_.erase(keyframe_list_.begin(), it);
+  // And remove old ones so we don't accumulate keyframes.
+  auto it = keyframe_list_.lower_bound(seq_num - kMaxPacketAge);
+  if (it != keyframe_list_.begin())
+    keyframe_list_.erase(keyframe_list_.begin(), it);
 
-    // Are there any nacks that are waiting for this seq_num.
-    std::vector<uint16_t> nack_batch = GetNackBatch(kSeqNumOnly);
-    if (!nack_batch.empty())
-      nack_sender_->SendNack(nack_batch);
-  }
+  // Are there any nacks that are waiting for this seq_num.
+  std::vector<uint16_t> nack_batch = GetNackBatch(kSeqNumOnly);
+  if (!nack_batch.empty())
+    nack_sender_->SendNack(nack_batch);
+
+  return 0;
 }
 
 void NackModule::ClearUpTo(uint16_t seq_num) {