Nack count returned on OnReceivedPacket.

webrtc/modules/video_coding/nack_module.cc

 /*
  *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 38 matching lines @@
       running_(true),
       initialized_(false),
       rtt_ms_(kDefaultRttMs),
       last_seq_num_(0),
       next_process_time_ms_(-1) {
   RTC_DCHECK(clock_);
   RTC_DCHECK(nack_sender_);
   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
   //                 now set it to true, which will cause the reordering
   //                 statistics to never be updated.
   bool is_retransmitted = true;
   bool is_keyframe = packet.isFirstPacket && packet.frameType == kVideoFrameKey;
 
   if (!initialized_) {
     last_seq_num_ = seq_num;
     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) {
   rtc::CritScope lock(&crit_);
   nack_list_.erase(nack_list_.begin(), nack_list_.lower_bound(seq_num));
   keyframe_list_.erase(keyframe_list_.begin(),
                        keyframe_list_.lower_bound(seq_num));
 }
 
 void NackModule::UpdateRtt(int64_t rtt_ms) {
@@ skipping 137 matching lines @@
   reordering_histogram_.Add(diff);
 }
 
 int NackModule::WaitNumberOfPackets(float probability) const {
   if (reordering_histogram_.NumValues() == 0)
     return 0;
   return reordering_histogram_.InverseCdf(probability);
 }
 
 }  // namespace webrtc