Expose packet loss counts from QUIC to NetworkQualityEstimator

net/quic/quic_connection_logger.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/quic/quic_connection_logger.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 485 matching lines @@
 }
 
 void QuicConnectionLogger::OnProtocolVersionMismatch(
     QuicVersion received_version) {
   // TODO(rtenneti): Add logging.
 }
 
 void QuicConnectionLogger::OnPacketHeader(const QuicPacketHeader& header) {
   net_log_.AddEvent(NetLog::TYPE_QUIC_SESSION_PACKET_AUTHENTICATED);
   ++num_packets_received_;
+
+  const QuicPacketCount previous_largest_received_packet_number =
+      largest_received_packet_number_;
   if (largest_received_packet_number_ < header.packet_number) {
     QuicPacketNumber delta =
         header.packet_number - largest_received_packet_number_;
     if (delta > 1) {
       // There is a gap between the largest packet previously received and
       // the current packet.  This indicates either loss, or out-of-order
       // delivery.
       UMA_HISTOGRAM_COUNTS("Net.QuicSession.PacketGapReceived",
                            static_cast<base::HistogramBase::Sample>(delta - 1));
     }
     largest_received_packet_number_ = header.packet_number;
   }
   if (header.packet_number < received_packets_.size()) {
     received_packets_[static_cast<size_t>(header.packet_number)] = true;
   }
   if (header.packet_number < last_received_packet_number_) {
     ++num_out_of_order_received_packets_;
     if (previous_received_packet_size_ < last_received_packet_size_)
       ++num_out_of_order_large_received_packets_;
     UMA_HISTOGRAM_COUNTS(
         "Net.QuicSession.OutOfOrderGapReceived",
         static_cast<base::HistogramBase::Sample>(last_received_packet_number_ -
                                                  header.packet_number));
   }
   last_received_packet_number_ = header.packet_number;
+  ReportPacketCounts(previous_largest_received_packet_number);
+}
+
+void QuicConnectionLogger::ReportPacketCounts(
+    QuicPacketCount previous_largest_received_packet_number) const {
+  DCHECK_GE(largest_received_packet_number_,
+            previous_largest_received_packet_number);
+  const uint64_t diff =

[bengr, 2016/02/19 00:45:08]

Move this to after 548

[tbansal1, 2016/02/26 01:15:00]

Done.

+      largest_received_packet_number_ - previous_largest_received_packet_number;
+
+  uint64_t num_packets_lost = 0;
+  uint64_t num_packets_received_in_order = 0;
+  uint64_t num_packets_received_not_in_order = 0;
+
+  if (socket_performance_watcher_) {

[bengr, 2016/02/19 00:45:08]

if (!socket_performance_watcher)
  return;
...

[tbansal1, 2016/02/26 01:15:00]

Done.

+    if (diff == 0) {
+      // The received packet number has alower sequence number than the expected

[bengr, 2016/02/19 00:45:08]

alower -> a lower

[tbansal1, 2016/02/26 01:15:00]

Done.

+      // sequence number.
+      num_packets_received_not_in_order = 1;
+    } else if (diff == 1) {
+      // The received packet number has the same sequence number as the expected
+      // sequence number.
+      num_packets_received_in_order = 1;
+    } else {
+      // The received packet number has higher sequence number as the expected
+      // sequence number. Report all packets with intermediate sequence numbers
+      // as lost.
+      num_packets_lost = diff - 1;
+      num_packets_received_in_order = 1;
+    }
+    socket_performance_watcher_->OnUpdatedPacketCountAvailable(
+        num_packets_lost, num_packets_received_in_order,
+        num_packets_received_not_in_order);

[bengr, 2016/02/19 00:45:08]

I don't think you need "num_" but could be convinced you could replace it with
the suffix "_count"

I'd change "not_in" to "out_of"

[tbansal1, 2016/02/26 01:15:00]

Done. This file uses num_ at multiple places. I was just going with the
convention in this file.

+  }
 }
 
 void QuicConnectionLogger::OnStreamFrame(const QuicStreamFrame& frame) {
   net_log_.AddEvent(NetLog::TYPE_QUIC_SESSION_STREAM_FRAME_RECEIVED,
                     base::Bind(&NetLogQuicStreamFrameCallback, &frame));
 }
 
 void QuicConnectionLogger::OnAckFrame(const QuicAckFrame& frame) {
   net_log_.AddEvent(NetLog::TYPE_QUIC_SESSION_ACK_FRAME_RECEIVED,
                     base::Bind(&NetLogQuicAckFrameCallback, &frame));
@@ skipping 227 matching lines @@
     bit_mask_of_packets >>= 1;
     DCHECK_LT(range_start + bits_so_far, kBoundingSampleInCumulativeHistogram);
     histogram->Add(range_start + bits_so_far);
     range_start += i + 1;
   }
 }
 
 float QuicConnectionLogger::ReceivedPacketLossRate() const {
   if (largest_received_packet_number_ <= num_packets_received_)
     return 0.0f;
-  float num_received = largest_received_packet_number_ - num_packets_received_;
-  return num_received / largest_received_packet_number_;
+  float num_lost = largest_received_packet_number_ - num_packets_received_;

[bengr, 2016/02/19 00:45:08]

I don't like "num." Maybe lost_count? Please remove "num_" everywhere.

[tbansal1, 2016/02/26 01:15:00]

num is already there in this code (also, this is not my code). I changed this
line in this CL because I found the variable name to be wrong and confusing
(num_received has almost opposite meaning of num_lost).

Not done.

+  return num_lost / largest_received_packet_number_;
 }
 
 void QuicConnectionLogger::OnRttChanged(QuicTime::Delta rtt) const {
   // Notify socket performance watcher of the updated RTT value.
   if (!socket_performance_watcher_)
     return;
 
   int64_t microseconds = rtt.ToMicroseconds();
   if (microseconds != 0) {
     socket_performance_watcher_->OnUpdatedRTTAvailable(
@@ skipping 84 matching lines @@
       continue;
     }
     // Record some overlapping patterns, to get a better picture, since this is
     // not very expensive.
     if (i % 3 == 0)
       six_packet_histogram->Add(recent_6_mask);
   }
 }
 
 }  // namespace net