Expose packet loss counts from QUIC to NetworkQualityEstimator

net/base/network_quality_estimator.h

Index: net/base/network_quality_estimator.h
diff --git a/net/base/network_quality_estimator.h b/net/base/network_quality_estimator.h
index 9fa0ad1817604c0472f8e5bafb9f78b619cfe477..c8eef7700e8f44786f86a844ed4ba9d1df80a217 100644
--- a/net/base/network_quality_estimator.h
+++ b/net/base/network_quality_estimator.h
@@ -69,9 +69,9 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // Observes measurements of round trip time.
   class NET_EXPORT_PRIVATE RTTObserver {
    public:
-    // Will be called when a new RTT observation is available. The round trip
-    // time is specified in milliseconds. The time when the observation was
-    // taken and the source of the observation are provided.
+    // OnRTTObservation will be called when a new RTT observation is available.
+    // The round trip time is specified in milliseconds. The time when the
+    // observation was taken and the source of the observation are provided.
     virtual void OnRTTObservation(int32_t rtt_ms,
                                   const base::TimeTicks& timestamp,
                                   ObservationSource source) = 0;
@@ -101,6 +101,35 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
     DISALLOW_COPY_AND_ASSIGN(ThroughputObserver);
   };
 
+  // Observes measurements of packet loss.
+  class NET_EXPORT_PRIVATE PacketLossObserver {
+   public:
+    // OnPacketLossObservation will be called when a new packet loss
+    // observation is available. |num_packets_lost| is the number of packets
+    // lost. |num_packets_received_in_order| is the number of packets received
+    // in  order. |num_packets_received_not_in_order| is the number of packets
+    // received out of order. See
+    // SocketPerformanceWatcherFactory::OnUpdatedPacketCountAvailable for more
+    // details on how these values are populated.
+    //
+    // Providing counts of lost, in-order and out-of-order packets provides
+    // consumers with more information than simply providing the packet loss
+    // rate.

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

... for consumers of raw observations. Packet loss estimates do not provide this
extra information.

[tbansal1, 2016/02/26 01:14:59]

Removed that comment. It was not very useful.

+    virtual void OnPacketLossObservation(
+        uint64_t num_packets_lost,

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

Add concise examples, e.g.:

// Last observation after packet #1. Packets #5 and #6 then received:
//   lost = 2, in_order = 2, not_in_order = 0

// Last observation after packet #6. Packet #3 received:
//   lost = 1 (is this right??), in_order = 0, not_in_order = 1

[tbansal1, 2016/02/26 01:14:59]

Done.

+        uint64_t num_packets_received_in_order,
+        uint64_t num_packets_received_not_in_order,
+        const base::TimeTicks& timestamp,
+        ObservationSource source) = 0;
+
+   protected:
+    PacketLossObserver() {}
+    virtual ~PacketLossObserver() {}
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(PacketLossObserver);
+  };
+
   // Creates a new NetworkQualityEstimator.
   // |variation_params| is the map containing all field trial parameters
   // related to NetworkQualityEstimator field trial.
@@ -139,6 +168,13 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // Virtualized for testing. |kbps| should not be null.
   virtual bool GetDownlinkThroughputKbpsEstimate(int32_t* kbps) const;
 
+  // Returns true if the estimated packet loss rate is available and sets
+  // |packet_loss| to the estimated packet loss rate. Virtualized for testing.
+  // |packet_loss| should not be NULL. |packet_loss| is always between 0.0

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

should -> must

[tbansal1, 2016/02/26 01:14:59]

Done.

+  // and 1.0 (both inclusive) with a higher value indicating higher packet loss
+  // rate.
+  virtual bool GetPacketLossRateEstimate(float* packet_loss) const;
+
   // Notifies NetworkQualityEstimator that the response header of |request| has
   // been received.
   void NotifyHeadersReceived(const URLRequest& request);
@@ -166,6 +202,11 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
       const Protocol protocol) override;
   void OnUpdatedRTTAvailable(const Protocol protocol,
                              const base::TimeDelta& rtt) override;
+  void OnUpdatedPacketCountAvailable(
+      const Protocol protocol,

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

No need for const.

[tbansal1, 2016/02/26 01:14:59]

Done.

+      uint64_t num_packets_lost,
+      uint64_t num_packets_received_in_order,
+      uint64_t num_packets_received_not_in_order) override;

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

I'd prefer smaller types. 8 bytes seems like overkill.

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

Using size_t which is the right type for counts.

 
   // Adds |rtt_observer| to the list of round trip time observers. Must be
   // called on the IO thread.
@@ -183,6 +224,14 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // is on the list of observers. Must be called on the IO thread.
   void RemoveThroughputObserver(ThroughputObserver* throughput_observer);
 
+  // Adds |packet_loss_observer| to the list of packet loss observers. Must be
+  // called on the IO thread.
+  void AddPacketLossObserver(PacketLossObserver* packet_loss_observer);
+
+  // Removes |packet_loss_observer| from the list of packet loss observers if it
+  // is on the list of observers. Must be called on the IO thread.
+  void RemovePacketLossObserver(PacketLossObserver* packet_loss_observer);
+
  protected:
   // NetworkID is used to uniquely identify a network.
   // For the purpose of network quality estimation and caching, a network is
@@ -234,6 +283,8 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
  private:
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, StoreObservations);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, TestKbpsRTTUpdates);
+  FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest,
+                           TestPacketLossRateUpdates);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, TestAddObservation);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, ObtainOperatingParams);
   FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, HalfLifeParam);
@@ -398,6 +449,15 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
                        ValueType* result,
                        int percentile) const;
 
+    // Returns true iff the weighted average value of the observations in this
+    // buffer is available. Sets |result| to the computed average
+    // value among all observations since |begin_timestamp|. If the value is

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

among -> of

Is that inclusive of begin_timestamp?

[tbansal1, 2016/02/26 01:14:59]

Done.
Added comment.

+    // unavailable, false is returned and |result| is not modified. The average
+    // value is unavailable if all of the values in the observation buffer are
+    // older than |begin_timestamp|. |result| must not be NULL.
+    bool GetWeightedAverage(const base::TimeTicks& begin_timestamp,
+                            ValueType* result) const;
+
    private:
     FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, StoreObservations);
     FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest,
@@ -441,6 +501,15 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   typedef net::NetworkQualityEstimator::ObservationBuffer<int32_t>
       ThroughputObservationBuffer;
 
+  // Value of a packet loss observation. A value of 0.0 indicates all packets
+  // during the observation period were received successfully in-order, while a
+  // value of 1.0 indicates all packets during the observation period were
+  // lost. Currently, the observation period is set to 1 packet.

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

1 -> one

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

Done.

+  typedef net::NetworkQualityEstimator::Observation<float>
+      PacketLossRateObservation;
+  typedef net::NetworkQualityEstimator::ObservationBuffer<float>
+      PacketLossRateObservationBuffer;
+
   // This does not use a unordered_map or hash_map for code simplicity (key just
   // implements operator<, rather than hash and equality) and because the map is
   // tiny.
@@ -451,6 +520,12 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // |kInvalidThroughput|.
   static const int32_t kInvalidThroughput;
 
+  // Packet loss rate is set to  a value strictly lower than
+  // |kMinimumValidPacketLossRate| if a valid value is unavailable. Readers
+  // should discard the packet loss rate value if it is set to a value strictly
+  // lower than |kMinimumValidPacketLossRate|.
+  static const float kMinimumValidPacketLossRate;
+
   // Tiny transfer sizes may give inaccurate throughput results.
   // Minimum size of the transfer over which the throughput is computed.
   static const int kMinTransferSizeInBytes = 10000;
@@ -509,6 +584,11 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
       const base::TimeTicks& begin_timestamp,
       int percentile) const;
 
+  // Returns an estimate of the packet loss rate. Only the observations later
+  // than |begin_timestamp| are taken into account.
+  float GetPacketLossRateEstimateInternal(
+      const base::TimeTicks& begin_timestamp) const;
+
   // Returns the current network ID checking by calling the platform APIs.
   // Virtualized for testing.
   virtual NetworkID GetCurrentNetworkID() const;
@@ -520,6 +600,12 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
 
   void NotifyObserversOfThroughput(const ThroughputObservation& observation);
 
+  void NotifyObserversOfPacketLoss(uint64_t num_packets_lost,
+                                   uint64_t num_packets_received_in_order,
+                                   uint64_t num_packets_received_not_in_order,
+                                   const base::TimeTicks& timestamp,
+                                   ObservationSource source);
+
   // Records the UMA related to RTT.
   void RecordRTTUMA(int32_t estimated_value_msec,
                     int32_t actual_value_msec) const;
@@ -545,6 +631,14 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   void RecordExternalEstimateProviderMetrics(
       NQEExternalEstimateProviderStatus status) const;
 
+  // Gets the ObservationSource that corresponds to the given |protocol|,
+  // updates |observation_source| and returns true. If the corresponding
+  // ObservationSource is unknown, false is returned and |observation_source| is
+  // not modified.
+  bool GetObservationSourceForProtocol(
+      const Protocol protocol,

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

No need for const.

[tbansal1, 2016/02/26 01:14:59]

Done.

+      ObservationSource* observation_source) const;
+
   // Determines if the requests to local host can be used in estimating the
   // network quality. Set to true only for tests.
   const bool allow_localhost_requests_;
@@ -578,6 +672,9 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // Buffer that holds RTT observations sorted by timestamp.
   RttObservationBuffer rtt_msec_observations_;
 
+  // Buffer that holds packet loss observations sorted by timestamp.
+  PacketLossRateObservationBuffer packet_loss_rate_observations_;
+
   // Default network quality observations obtained from the network quality
   // estimator field trial parameters. The observations are indexed by
   // ConnectionType.
@@ -591,9 +688,11 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // system APIs. May be NULL.
   const scoped_ptr<ExternalEstimateProvider> external_estimate_provider_;
 
-  // Observer lists for round trip times and throughput measurements.
+  // Observer lists for round trip times, throughput and packet loss
+  // measurements.
   base::ObserverList<RTTObserver> rtt_observer_list_;
   base::ObserverList<ThroughputObserver> throughput_observer_list_;
+  base::ObserverList<PacketLossObserver> packet_loss_observer_list_;
 
   base::ThreadChecker thread_checker_;