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..ee6ddf100f1b0e1863b3c18baaa343e3b1f6fde8 100644
--- a/net/base/network_quality_estimator.h
+++ b/net/base/network_quality_estimator.h
@@ -63,7 +63,9 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
     // for a given type of network connection.
     DEFAULT_FROM_PLATFORM,
     // The observation came from a Chromium-external source.
-    EXTERNAL_ESTIMATE
+    EXTERNAL_ESTIMATE,
+    // Unknown source.

[bengr, 2016/02/09 15:41:03]

Why would the source ever be unknown?

[tbansal1, 2016/02/09 17:54:12]

Fixed the function definition, and eliminated UNKNOWN.

+    UNKNOWN
   };
 
   // Observes measurements of round trip time.
@@ -101,6 +103,30 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
     DISALLOW_COPY_AND_ASSIGN(ThroughputObserver);
   };
 
+  // Observes measurements of packet loss.
+  class NET_EXPORT_PRIVATE PacketLossObserver {
+   public:
+    // 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. All the counts are for the duration since the last
+    // call to the OnPacketLossObservation to |timestamp|.
+    virtual void OnPacketLossObservation(

[bengr, 2016/02/09 15:41:03]

Is num_packets_received_in_order the number of consecutive packets that are
received without a loss? What would the value be if we receive packets
1,2,3,5,6,7,8? Is it 3 or 4? I have no idea what
num_packets_received_no_in_order means, so please explain.

As for the duration, I find it a little strange that an observation is not self
contained. I.e., it doesn't know when it began.

I also find it strange that the observer interface does not expose observations
as you define them below.

[tbansal1, 2016/02/09 17:54:11]

Added an example in socket_performance_watcher_factory.h to make it clearer.
Removed that comment, it was unnecessary and not clear.
Agreed, but I am not sure what's the best way out. It would be ideal for raw
observations and processed observations to have the same format, but it is not
clear how we can do it (or even if it is necessary for them to have the same
format).

+        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) = 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 +165,12 @@ 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 nullptr. |packet_loss| is always between 0.0

[bengr, 2016/02/09 15:41:02]

In a comment, I would say "should not be null."

[tbansal1, 2016/02/09 17:54:11]

Done.

+  // and 1.0 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 +198,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,
+      uint64_t num_packets_lost,
+      uint64_t num_packets_received_in_order,
+      uint64_t num_packets_received_not_in_order) override;
 
   // Adds |rtt_observer| to the list of round trip time observers. Must be
   // called on the IO thread.
@@ -183,6 +220,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 +279,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 +445,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
+    // unavailable, false is returned and |result| is not modified. Average
+    // value is unavailable if all the values in observation buffer are older
+    // than |begin_timestamp|. |result| must not be nullptr.
+    bool GetWeightedAverage(const base::TimeTicks& begin_timestamp,

[bengr, 2016/02/09 15:41:03]

If this is computed over observations then it is computed over uneven intervals.
Is that representative? E.g., one interval could lose 1 out of 2 packets, and
another could lose 1 out of 1000, but the reported loss rate would be ~1/2.

[tbansal1, 2016/02/09 17:54:11]

I do not understand this comment. Two observations taken at the same time have
same weight. Two observations may differ in weight only if they were taken at
different times. Each observation corresponds to 1 packet received or 1 packet
lost.

+                            ValueType* result) const;
+
    private:
     FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest, StoreObservations);
     FRIEND_TEST_ALL_PREFIXES(NetworkQualityEstimatorTest,
@@ -441,6 +497,12 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   typedef net::NetworkQualityEstimator::ObservationBuffer<int32_t>
       ThroughputObservationBuffer;
 
+  // Value of a packet loss observation. A value of 0.0 indicates one packet
+  // was received successfully in-order, while a value of 1.0 indicates one
+  // packet was lost.
+  typedef net::NetworkQualityEstimator::Observation<float>
+      PacketLossObservation;
+
   // 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 +513,11 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   // |kInvalidThroughput|.
   static const int32_t kInvalidThroughput;
 
+  // Packet loss rate is set to |kInvalidPacketLossRate| if a valid value is
+  // unavailable. Readers should discard the packet loss rate value if it is
+  // set to |kInvalidPacketLossRate|.
+  static const float kInvalidPacketLossRate;
+
   // 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 +576,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 +592,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,

[bengr, 2016/02/09 15:41:02]

Why not a const base::TimeTicks& ?

[tbansal1, 2016/02/09 17:54:11]

Done.

+                                   ObservationSource source);
+
   // Records the UMA related to RTT.
   void RecordRTTUMA(int32_t estimated_value_msec,
                     int32_t actual_value_msec) const;
@@ -545,6 +623,9 @@ class NET_EXPORT_PRIVATE NetworkQualityEstimator
   void RecordExternalEstimateProviderMetrics(
       NQEExternalEstimateProviderStatus status) const;
 
+  ObservationSource GetObservationSourceForProtocol(
+      const Protocol protocol) 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 +659,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.
+  ObservationBuffer<float> 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 +675,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_;