Expose packet loss counts from QUIC to NetworkQualityEstimator

net/base/network_quality_estimator.cc

Index: net/base/network_quality_estimator.cc
diff --git a/net/base/network_quality_estimator.cc b/net/base/network_quality_estimator.cc
index 37cbcc0efe08f40589c770b2a89a0cdd1507ef7e..805b41105eec7a505cfe4d882bcaef94694b4466 100644
--- a/net/base/network_quality_estimator.cc
+++ b/net/base/network_quality_estimator.cc
@@ -119,6 +119,8 @@ namespace net {
 
 const int32_t NetworkQualityEstimator::kInvalidThroughput = 0;
 
+const float NetworkQualityEstimator::kMinimumValidPacketLossRate = 0.0f;

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

I think you can get rid of this constant.

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

Done.

+
 NetworkQualityEstimator::NetworkQualityEstimator(
     scoped_ptr<ExternalEstimateProvider> external_estimates_provider,
     const std::map<std::string, std::string>& variation_params)
@@ -141,6 +143,8 @@ NetworkQualityEstimator::NetworkQualityEstimator(
       downstream_throughput_kbps_observations_(
           GetWeightMultiplierPerSecond(variation_params)),
       rtt_msec_observations_(GetWeightMultiplierPerSecond(variation_params)),
+      packet_loss_rate_observations_(
+          GetWeightMultiplierPerSecond(variation_params)),
       external_estimate_provider_(std::move(external_estimates_provider)) {
   static_assert(kMinRequestDurationMicroseconds > 0,
                 "Minimum request duration must be > 0");
@@ -376,6 +380,18 @@ void NetworkQualityEstimator::RemoveThroughputObserver(
   throughput_observer_list_.RemoveObserver(throughput_observer);
 }
 
+void NetworkQualityEstimator::AddPacketLossObserver(
+    PacketLossObserver* packet_loss_observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  packet_loss_observer_list_.AddObserver(packet_loss_observer);
+}
+
+void NetworkQualityEstimator::RemovePacketLossObserver(
+    PacketLossObserver* packet_loss_observer) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  packet_loss_observer_list_.RemoveObserver(packet_loss_observer);
+}
+
 void NetworkQualityEstimator::RecordRTTUMA(int32_t estimated_value_msec,
                                            int32_t actual_value_msec) const {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -429,93 +445,51 @@ void NetworkQualityEstimator::RecordExternalEstimateProviderMetrics(
                             EXTERNAL_ESTIMATE_PROVIDER_STATUS_BOUNDARY);
 }
 
+bool NetworkQualityEstimator::GetObservationSourceForProtocol(
+    const Protocol protocol,
+    NetworkQualityEstimator::ObservationSource* observation_source) const {
+  switch (protocol) {
+    case PROTOCOL_TCP:
+      *observation_source = TCP;
+      return true;
+    case PROTOCOL_QUIC:
+      *observation_source = QUIC;
+      return true;
+    default:
+      NOTREACHED();
+      return false;
+  }
+}
+
 void NetworkQualityEstimator::OnConnectionTypeChanged(
     NetworkChangeNotifier::ConnectionType type) {
   DCHECK(thread_checker_.CalledOnValidThread());
+
   if (peak_network_quality_.rtt() != InvalidRTT()) {
-    switch (current_network_id_.type) {
-      case NetworkChangeNotifier::CONNECTION_UNKNOWN:
-        UMA_HISTOGRAM_TIMES("NQE.FastestRTT.Unknown",
-                            peak_network_quality_.rtt());
-        break;
-      case NetworkChangeNotifier::CONNECTION_ETHERNET:
-        UMA_HISTOGRAM_TIMES("NQE.FastestRTT.Ethernet",
-                            peak_network_quality_.rtt());
-        break;
-      case NetworkChangeNotifier::CONNECTION_WIFI:
-        UMA_HISTOGRAM_TIMES("NQE.FastestRTT.Wifi", peak_network_quality_.rtt());
-        break;
-      case NetworkChangeNotifier::CONNECTION_2G:
-        UMA_HISTOGRAM_TIMES("NQE.FastestRTT.2G", peak_network_quality_.rtt());
-        break;
-      case NetworkChangeNotifier::CONNECTION_3G:
-        UMA_HISTOGRAM_TIMES("NQE.FastestRTT.3G", peak_network_quality_.rtt());
-        break;
-      case NetworkChangeNotifier::CONNECTION_4G:
-        UMA_HISTOGRAM_TIMES("NQE.FastestRTT.4G", peak_network_quality_.rtt());
-        break;
-      case NetworkChangeNotifier::CONNECTION_NONE:
-        UMA_HISTOGRAM_TIMES("NQE.FastestRTT.None", peak_network_quality_.rtt());
-        break;
-      case NetworkChangeNotifier::CONNECTION_BLUETOOTH:
-        UMA_HISTOGRAM_TIMES("NQE.FastestRTT.Bluetooth",
-                            peak_network_quality_.rtt());
-        break;
-      default:
-        NOTREACHED() << "Unexpected connection type = "
-                     << current_network_id_.type;
-        break;
-    }
+    // Largest bucket is 10 seconds.
+    base::HistogramBase* fastest_rtt_histogram =

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

I'd save this for a different CL.

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

Done.

+        GetHistogram("FastestRTT.", current_network_id_.type, 10 * 1000);
+    fastest_rtt_histogram->Add(peak_network_quality_.rtt().InMilliseconds());
   }
 
   if (peak_network_quality_.downstream_throughput_kbps() !=
       kInvalidThroughput) {
-    switch (current_network_id_.type) {
-      case NetworkChangeNotifier::CONNECTION_UNKNOWN:
-        UMA_HISTOGRAM_COUNTS(
-            "NQE.PeakKbps.Unknown",
-            peak_network_quality_.downstream_throughput_kbps());
-        break;
-      case NetworkChangeNotifier::CONNECTION_ETHERNET:
-        UMA_HISTOGRAM_COUNTS(
-            "NQE.PeakKbps.Ethernet",
-            peak_network_quality_.downstream_throughput_kbps());
-        break;
-      case NetworkChangeNotifier::CONNECTION_WIFI:
-        UMA_HISTOGRAM_COUNTS(
-            "NQE.PeakKbps.Wifi",
-            peak_network_quality_.downstream_throughput_kbps());
-        break;
-      case NetworkChangeNotifier::CONNECTION_2G:
-        UMA_HISTOGRAM_COUNTS(
-            "NQE.PeakKbps.2G",
-            peak_network_quality_.downstream_throughput_kbps());
-        break;
-      case NetworkChangeNotifier::CONNECTION_3G:
-        UMA_HISTOGRAM_COUNTS(
-            "NQE.PeakKbps.3G",
-            peak_network_quality_.downstream_throughput_kbps());
-        break;
-      case NetworkChangeNotifier::CONNECTION_4G:
-        UMA_HISTOGRAM_COUNTS(
-            "NQE.PeakKbps.4G",
-            peak_network_quality_.downstream_throughput_kbps());
-        break;
-      case NetworkChangeNotifier::CONNECTION_NONE:
-        UMA_HISTOGRAM_COUNTS(
-            "NQE.PeakKbps.None",
-            peak_network_quality_.downstream_throughput_kbps());
-        break;
-      case NetworkChangeNotifier::CONNECTION_BLUETOOTH:
-        UMA_HISTOGRAM_COUNTS(
-            "NQE.PeakKbps.Bluetooth",
-            peak_network_quality_.downstream_throughput_kbps());
-        break;
-      default:
-        NOTREACHED() << "Unexpected connection type = "
-                     << current_network_id_.type;
-        break;
-    }
+    // Largest bucket is 1000000 Kbps (~1 Gbps).
+    base::HistogramBase* peak_kbps_histogram =

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

Likewise, this has nothing to do with packet loss.

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

Done.

+        GetHistogram("PeakKbps.", current_network_id_.type,
+                     1000000);  // UMA_HISTOGRAM_COUNTS

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

To what does this comment refer?

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

Obsolete.

+    peak_kbps_histogram->Add(
+        peak_network_quality_.downstream_throughput_kbps());
+  }
+
+  float packet_loss_rate = kMinimumValidPacketLossRate - 1;
+  if (GetPacketLossRateEstimate(&packet_loss_rate)) {
+    DCHECK_LE(kMinimumValidPacketLossRate, packet_loss_rate);

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

I would just do:

float packet_loss_rate;
if (GetPacketLossRateEstimate(&packet_loss_rate)) {
  DCHECK_LE(0.0, packet_loss_rate);
  DCHECK_GE(1.0, packet_loss_rate);
  ...

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

Done.

+
+    // Largest bucket is 101.
+    base::HistogramBase* packet_loss_histogram =
+        GetHistogram("PacketLossRate.", current_network_id_.type, 101);
+    packet_loss_histogram->Add(packet_loss_rate * 100);

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

Add(packet_loss_rate * 100.0f)

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

Done.

   }
 
   base::TimeDelta rtt = GetRTTEstimateInternal(base::TimeTicks(), 50);
@@ -546,6 +520,7 @@ void NetworkQualityEstimator::OnConnectionTypeChanged(
   peak_network_quality_ = NetworkQuality();
   downstream_throughput_kbps_observations_.Clear();
   rtt_msec_observations_.Clear();
+  packet_loss_rate_observations_.Clear();
   current_network_id_ = GetCurrentNetworkID();
 
   QueryExternalEstimateProvider();
@@ -580,6 +555,14 @@ bool NetworkQualityEstimator::GetDownlinkThroughputKbpsEstimate(
   return (*kbps != kInvalidThroughput);
 }
 
+bool NetworkQualityEstimator::GetPacketLossRateEstimate(
+    float* packet_loss) const {

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

packet_loss_rate_estimate

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

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  *packet_loss = GetPacketLossRateEstimateInternal(base::TimeTicks());

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

It might be cleaner to not set packet_loss_rate_estimate if invalid. I.e.:

float estimate = GetPacketLossRateEstimateInternal(base::TimeTicks());
if (estimate < 0.0 || estimate > 1.0)
  return false;
*packet_loss_rate_estimate = estimate;

Also, you might want to introduce a helper IsValidPacketLossRate(), but that
might be overkill.

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

Done.
Right, I do not think this is needed.

+  return (*packet_loss >= kMinimumValidPacketLossRate);
+}
+
 bool NetworkQualityEstimator::GetRecentMedianRTT(
     const base::TimeTicks& begin_timestamp,
     base::TimeDelta* rtt) const {
@@ -644,6 +627,19 @@ int32_t NetworkQualityEstimator::GetDownlinkThroughputKbpsEstimateInternal(
   return kbps;
 }
 
+float NetworkQualityEstimator::GetPacketLossRateEstimateInternal(
+    const base::TimeTicks& begin_timestamp) const {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (packet_loss_rate_observations_.Size() == 0)
+    return kMinimumValidPacketLossRate - 1;
+
+  float packet_loss_rate = kMinimumValidPacketLossRate - 1;
+  packet_loss_rate_observations_.GetWeightedAverage(begin_timestamp,

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

I would try to rewrite GetPacketLossRateEstimate,
GetPacketLossRateEstimateInternal and GetWeightedAverage to eliminate this
method. Ideally it would look like:

bool NQE::GetPacketLossRateEstiamte(...) {
  return packet_loss_rate_observations_.GetWeightedAverage);
}

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

This is probably not possible. NQE exposes two methods:
GetRTTEstimate() and GetRecentMedianRTT(
    const base::TimeTicks& begin_timestamp)
Both of them use the same function internally: 
GetRTTEstimateInternal(const base::TimeTicks& begin_timestamp, int percentile).

Same argument applies for bandwidth and packet loss.

+                                                    &packet_loss_rate);
+  return packet_loss_rate;
+}
+
 template <typename ValueType>
 void NetworkQualityEstimator::ObservationBuffer<ValueType>::
     ComputeWeightedObservations(
@@ -717,6 +713,39 @@ bool NetworkQualityEstimator::ObservationBuffer<ValueType>::GetPercentile(
   return true;
 }
 
+template <typename ValueType>
+bool NetworkQualityEstimator::ObservationBuffer<ValueType>::GetWeightedAverage(
+    const base::TimeTicks& begin_timestamp,
+    ValueType* result) const {
+  DCHECK(result);
+
+  // Stores WeightedObservation in increasing order of value, although for the

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

Is the front or the back the highest value?

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

WeightedObservation -> weighted observations

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

Done.

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

Expanded the comment.

+  // purpose of computing weighted average, the order does not matter.
+  std::vector<WeightedObservation<ValueType>> weighted_observations;
+
+  // Total weight of all observations in |weighted_observations|.

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

The sum of the weights of all observations in |weighted_observations|.

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

Done.

+  double total_weight = 0.0;
+
+  ComputeWeightedObservations(begin_timestamp, weighted_observations,
+                              &total_weight);
+  if (weighted_observations.empty())
+    return false;
+
+  DCHECK_GT(total_weight, 0.0);
+
+  // |weighted_observations| may have a smaller size than |observations_| since
+  // the former only contains the observations later than |begin_timestamp|.
+  DCHECK_GE(observations_.size(), weighted_observations.size());
+
+  double total_weight_times_value = 0.0;
+  for (const auto& weighted_observation : weighted_observations) {
+    total_weight_times_value +=
+        (weighted_observation.weight * weighted_observation.value);
+  }
+  *result = total_weight_times_value / total_weight;
+  return true;
+}
+
 NetworkQualityEstimator::NetworkID
 NetworkQualityEstimator::GetCurrentNetworkID() const {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -900,20 +929,47 @@ void NetworkQualityEstimator::OnUpdatedRTTAvailable(
     const base::TimeDelta& rtt) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  switch (protocol) {
-    case PROTOCOL_TCP:
-      NotifyObserversOfRTT(RttObservation(rtt, base::TimeTicks::Now(), TCP));
-      return;
-    case PROTOCOL_QUIC:
-      NotifyObserversOfRTT(RttObservation(rtt, base::TimeTicks::Now(), QUIC));
-      return;
-    default:
-      NOTREACHED();
+  ObservationSource observation_source;
+  if (!GetObservationSourceForProtocol(protocol, &observation_source))
+    return;
+
+  NotifyObserversOfRTT(
+      RttObservation(rtt, base::TimeTicks::Now(), observation_source));
+}
+
+void NetworkQualityEstimator::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) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  const base::TimeTicks now = base::TimeTicks::Now();
+
+  ObservationSource observation_source;
+  if (!GetObservationSourceForProtocol(protocol, &observation_source))
+    return;
+
+  for (size_t i = 0; i < num_packets_lost; ++i) {
+    packet_loss_rate_observations_.AddObservation(
+        PacketLossRateObservation(1.0, now, observation_source));
   }
+  for (size_t i = 0; i < num_packets_received_in_order; ++i) {
+    packet_loss_rate_observations_.AddObservation(
+        PacketLossRateObservation(0.0, now, observation_source));
+  }
+  // For packet loss estimation, we neglect the packets that were previously
+  // marked as lost but are later received out-of-order.
+  NotifyObserversOfPacketLoss(num_packets_lost, num_packets_received_in_order,
+                              num_packets_received_not_in_order, now,
+                              observation_source);
 }
 
 void NetworkQualityEstimator::NotifyObserversOfRTT(
     const RttObservation& observation) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_NE(EXTERNAL_ESTIMATE, observation.source);
+
   FOR_EACH_OBSERVER(
       RTTObserver, rtt_observer_list_,
       OnRTTObservation(observation.value.InMilliseconds(),
@@ -922,12 +978,30 @@ void NetworkQualityEstimator::NotifyObserversOfRTT(
 
 void NetworkQualityEstimator::NotifyObserversOfThroughput(
     const ThroughputObservation& observation) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_NE(EXTERNAL_ESTIMATE, observation.source);
+
   FOR_EACH_OBSERVER(
       ThroughputObserver, throughput_observer_list_,
       OnThroughputObservation(observation.value, observation.timestamp,
                               observation.source));
 }
 
+void NetworkQualityEstimator::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) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK_NE(EXTERNAL_ESTIMATE, source);
+
+  FOR_EACH_OBSERVER(PacketLossObserver, packet_loss_observer_list_,
+                    OnPacketLossObservation(
+                        num_packets_lost, num_packets_received_in_order,
+                        num_packets_received_not_in_order, timestamp, source));
+}
+
 NetworkQualityEstimator::CachedNetworkQuality::CachedNetworkQuality(
     const NetworkQuality& network_quality)
     : last_update_time_(base::TimeTicks::Now()),