Expose packet loss counts from QUIC to NetworkQualityEstimator

net/quic/quic_connection_logger.cc

Index: net/quic/quic_connection_logger.cc
diff --git a/net/quic/quic_connection_logger.cc b/net/quic/quic_connection_logger.cc
index 96c6425d1620b7a379cc1f8b69126682ce979da2..f3858e20260aae0d41a33599fe025d6f212cdf74 100644
--- a/net/quic/quic_connection_logger.cc
+++ b/net/quic/quic_connection_logger.cc
@@ -503,6 +503,9 @@ void QuicConnectionLogger::OnProtocolVersionMismatch(
 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_;
@@ -528,6 +531,40 @@ void QuicConnectionLogger::OnPacketHeader(const QuicPacketHeader& header) {
                                                  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);
+  if (!socket_performance_watcher_)
+    return;
+
+  size_t packets_lost = 0;
+  size_t packets_received_in_order = 0;
+  size_t packets_received_out_of_order = 0;
+
+  const size_t diff =
+      largest_received_packet_number_ - previous_largest_received_packet_number;
+
+  if (diff == 0) {
+    // The received packet number has a lower sequence number than the expected
+    // sequence number.
+    packets_received_out_of_order = 1;
+  } else if (diff == 1) {
+    // The received packet number has the same sequence number as the expected
+    // sequence number.
+    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.
+    packets_lost = diff - 1;

[Ryan Hamilton, 2016/03/01 18:20:32]

This is a very aggressive definition of loss. Reordering in the network is
expected, and this definition would end up confusing out of order deliver with
loss. Are you sure this is the right metric?

[tbansal1, 2016/03/01 19:05:36]

This API also reports p_r_out_of_order count. So, packets that were delivered
later (due to reorder) would get reported to the callee (NQE). At the callee,
value of packets_lost - p_r_out_of_order should give an estimate of how many
packets are currently considered as lost.

I did not put this logic here because missing packets may give an early
indication of packet losses.

[Ryan Hamilton, 2016/03/02 04:26:49]

If you have a variable called "packets_lost" but in order to compute the number
of lost packets you need to subtract out_of_order from packets_lost, then
packets_lost is the wrong name for that variable, I think. Perhaps you can come
up with a better name?
 
Please do not do this. Reordering is common. This "early warning" is not
advisable.

Also, we have plans is QUIC for the sender to intentionally send gaps. For
example after sending packets 1-100, the sender might jump to 500. (Long story).
I'm concerned that your calculations here might do the wrong thing.

If you wanted to measure up-stream packet loss, we can do that explicitly, of
course.

[tbansal1, 2016/03/03 02:39:05]

I will change the name, and send out the CL. In the meantime, question for you:
If sender is going to skip sequence numbers, surely there must be some way
control signal for receiver to be notified of that? Otherwise, how would the
receiver report the list of missing_packets in QuicAckFrame to the sender?

On 2016/03/02 04:26:49, Ryan Hamilton wrote:

[Ryan Hamilton, 2016/03/03 04:24:24]

The receiver will see packets 1, 2, ... 100, 500, 501, 502
and so it will report packets 101-499 as missing in the ack frame.

[tbansal1, 2016/03/03 23:34:46]

Done.

+    packets_received_in_order = 1;
+  }
+  socket_performance_watcher_->OnUpdatedPacketCountAvailable(
+      packets_lost, packets_received_in_order, packets_received_out_of_order);
 }
 
 void QuicConnectionLogger::OnStreamFrame(const QuicStreamFrame& frame) {
@@ -775,8 +812,8 @@ void QuicConnectionLogger::AddTo21CumulativeHistogram(
 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_;
+  return num_lost / largest_received_packet_number_;
 }
 
 void QuicConnectionLogger::OnRttChanged(QuicTime::Delta rtt) const {