Use |probe_cluster_id| to cluster packets.

webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc

Index: webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc
diff --git a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc b/webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc
similarity index 74%
copy from webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc
copy to webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc
index 8ad60aea65aff66f3a0856b94bad65ed280285e5..f0cc27f0a1ad11eb9434a091114f3b66a4d87220 100644
--- a/webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.cc
+++ b/webrtc/modules/congestion_controller/delay_based_bitrate_probing.cc
@@ -1,5 +1,5 @@
 /*
- *  Copyright (c) 2013 The WebRTC project authors. All Rights Reserved.
+ *  Copyright (c) 2016 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
@@ -8,7 +8,7 @@
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
-#include "webrtc/modules/remote_bitrate_estimator/remote_bitrate_estimator_abs_send_time.h"
+#include "webrtc/modules/congestion_controller/delay_based_bitrate_probing.h"
 
 #include <math.h>
 
@@ -23,8 +23,7 @@
 #include "webrtc/system_wrappers/include/critical_section_wrapper.h"
 #include "webrtc/typedefs.h"
 
-namespace webrtc {
-
+namespace {
 enum {
   kTimestampGroupLengthMs = 5,
   kAbsSendTimeFraction = 18,
@@ -36,20 +35,6 @@ enum {
   kExpectedNumberOfProbes = 3
 };
 
-static const double kTimestampToMs = 1000.0 /
-    static_cast<double>(1 << kInterArrivalShift);
-
-template<typename K, typename V>
-std::vector<K> Keys(const std::map<K, V>& map) {
-  std::vector<K> keys;
-  keys.reserve(map.size());
-  for (typename std::map<K, V>::const_iterator it = map.begin();
-      it != map.end(); ++it) {
-    keys.push_back(it->first);
-  }
-  return keys;
-}
-
 uint32_t ConvertMsTo24Bits(int64_t time_ms) {
   uint32_t time_24_bits =
       static_cast<uint32_t>(
@@ -58,57 +43,66 @@ uint32_t ConvertMsTo24Bits(int64_t time_ms) {
       0x00FFFFFF;
   return time_24_bits;
 }
+}  // namespace
 
-bool RemoteBitrateEstimatorAbsSendTime::IsWithinClusterBounds(
-    int send_delta_ms,
-    const Cluster& cluster_aggregate) {
-    if (cluster_aggregate.count == 0)
-      return true;
-    float cluster_mean = cluster_aggregate.send_mean_ms /
-                         static_cast<float>(cluster_aggregate.count);
-    return fabs(static_cast<float>(send_delta_ms) - cluster_mean) < 2.5f;
-  }
+namespace webrtc {

[danilchap, 2016/06/03 14:11:45]

Why keep this constant and function in webrtc instead of unnamed namespace?
They would likely collide with same-named from
RemoteBitrateEstimatorAbsSendTime.cc

[philipel, 2016/06/03 14:54:55]

Wanted to do as few changes as possible, but I guess it makes sense to move them
to the anonymous namespace as well.

+
+static const double kTimestampToMs =
+    1000.0 / static_cast<double>(1 << kInterArrivalShift);
 
-  void RemoteBitrateEstimatorAbsSendTime::AddCluster(
-      std::list<Cluster>* clusters,
-      Cluster* cluster) {
-    cluster->send_mean_ms /= static_cast<float>(cluster->count);
-    cluster->recv_mean_ms /= static_cast<float>(cluster->count);
-    cluster->mean_size /= cluster->count;
-    clusters->push_back(*cluster);
+template <typename K, typename V>
+std::vector<K> Keys(const std::map<K, V>& map) {
+  std::vector<K> keys;
+  keys.reserve(map.size());
+  for (typename std::map<K, V>::const_iterator it = map.begin();
+       it != map.end(); ++it) {
+    keys.push_back(it->first);
   }
+  return keys;
+}
 
-  RemoteBitrateEstimatorAbsSendTime::RemoteBitrateEstimatorAbsSendTime(
-      RemoteBitrateObserver* observer)
-      : observer_(observer),
-        inter_arrival_(),
-        estimator_(),
-        detector_(OverUseDetectorOptions()),
-        incoming_bitrate_(kBitrateWindowMs, 8000),
-        total_probes_received_(0),
-        first_packet_time_ms_(-1),
-        last_update_ms_(-1),
-        ssrcs_() {
-    RTC_DCHECK(observer_);
-    LOG(LS_INFO) << "RemoteBitrateEstimatorAbsSendTime: Instantiating.";
-    network_thread_.DetachFromThread();
+void DelayBasedProbingEstimator::AddCluster(std::list<Cluster>* clusters,
+                                            Cluster* cluster) {
+  cluster->send_mean_ms /= static_cast<float>(cluster->count);
+  cluster->recv_mean_ms /= static_cast<float>(cluster->count);
+  cluster->mean_size /= cluster->count;
+  clusters->push_back(*cluster);
 }
 
-void RemoteBitrateEstimatorAbsSendTime::ComputeClusters(
+DelayBasedProbingEstimator::DelayBasedProbingEstimator(
+    RemoteBitrateObserver* observer)
+    : observer_(observer),
+      inter_arrival_(),
+      estimator_(),
+      detector_(OverUseDetectorOptions()),
+      incoming_bitrate_(kBitrateWindowMs, 8000),
+      total_probes_received_(0),
+      first_packet_time_ms_(-1),
+      last_update_ms_(-1),
+      ssrcs_() {
+  RTC_DCHECK(observer_);
+  // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line.

[danilchap, 2016/06/03 14:11:45]

XXX=TODO? ( fix the test)

[stefan-webrtc, 2016/06/03 14:30:40]

The test is only useful for the receive-side BWE, so we should be able to remove
this on the send-side.

[philipel, 2016/06/03 14:54:55]

XXX = dirty code. I think it is best to not change the existing unit tests for
the sake of this class, but to refactor and write new tests for this class. Send
side and receive side BWE is starting to diverge. Stefan WDYT?

[stefan-webrtc, 2016/06/08 08:57:09]

I think that the particular test that relies on this log line provides no value
on the send-side, so we shouldn't need to run it there. Can we adapt it so that
it only runs with receive-side BWE?

+  LOG(LS_INFO) << "RemoteBitrateEstimatorAbsSendTime: Instantiating.";
+  network_thread_.DetachFromThread();
+}
+
+void DelayBasedProbingEstimator::ComputeClusters(
     std::list<Cluster>* clusters) const {
   Cluster current;
   int64_t prev_send_time = -1;
   int64_t prev_recv_time = -1;
+  int last_probe_cluster_id = -1;
   for (std::list<Probe>::const_iterator it = probes_.begin();
-       it != probes_.end();
-       ++it) {
+       it != probes_.end(); ++it) {
+    if (last_probe_cluster_id == -1)
+      last_probe_cluster_id = it->probe_cluster_id;
     if (prev_send_time >= 0) {
       int send_delta_ms = it->send_time_ms - prev_send_time;
       int recv_delta_ms = it->recv_time_ms - prev_recv_time;
       if (send_delta_ms >= 1 && recv_delta_ms >= 1) {
         ++current.num_above_min_delta;
       }
-      if (!IsWithinClusterBounds(send_delta_ms, current)) {
+      if (it->probe_cluster_id == last_probe_cluster_id) {

[stefan-webrtc, 2016/06/03 13:46:23]

Could you perhaps introduce these changes in a separate CL, so that the
refactoring is easier to review?

[philipel, 2016/06/03 14:54:55]

Not 100% sure what you mean. This logic must be changed in order to actually use
cluster ids instead of guessing.

[stefan-webrtc, 2016/06/08 08:57:09]

What I meant was that you could make a separate CL for simply copying the old
class / renaming, but without any functionality changes. That would be easier to
review.

         if (current.count >= kMinClusterSize)
           AddCluster(clusters, &current);
         current = Cluster();
@@ -125,14 +119,12 @@ void RemoteBitrateEstimatorAbsSendTime::ComputeClusters(
     AddCluster(clusters, &current);
 }
 
-std::list<Cluster>::const_iterator
-RemoteBitrateEstimatorAbsSendTime::FindBestProbe(
+std::list<Cluster>::const_iterator DelayBasedProbingEstimator::FindBestProbe(
     const std::list<Cluster>& clusters) const {
   int highest_probe_bitrate_bps = 0;
   std::list<Cluster>::const_iterator best_it = clusters.end();
   for (std::list<Cluster>::const_iterator it = clusters.begin();
-       it != clusters.end();
-       ++it) {
+       it != clusters.end(); ++it) {
     if (it->send_mean_ms == 0 || it->recv_mean_ms == 0)
       continue;
     int send_bitrate_bps = it->mean_size * 8 * 1000 / it->send_mean_ms;
@@ -158,8 +150,8 @@ RemoteBitrateEstimatorAbsSendTime::FindBestProbe(
   return best_it;
 }
 
-RemoteBitrateEstimatorAbsSendTime::ProbeResult
-RemoteBitrateEstimatorAbsSendTime::ProcessClusters(int64_t now_ms) {
+DelayBasedProbingEstimator::ProbeResult
+DelayBasedProbingEstimator::ProcessClusters(int64_t now_ms) {
   std::list<Cluster> clusters;
   ComputeClusters(&clusters);
   if (clusters.empty()) {
@@ -195,8 +187,7 @@ RemoteBitrateEstimatorAbsSendTime::ProcessClusters(int64_t now_ms) {
   return ProbeResult::kNoUpdate;
 }
 
-bool RemoteBitrateEstimatorAbsSendTime::IsBitrateImproving(
-    int new_bitrate_bps) const {
+bool DelayBasedProbingEstimator::IsBitrateImproving(int new_bitrate_bps) const {
   bool initial_probe = !remote_rate_.ValidEstimate() && new_bitrate_bps > 0;
   bool bitrate_above_estimate =
       remote_rate_.ValidEstimate() &&
@@ -204,36 +195,39 @@ bool RemoteBitrateEstimatorAbsSendTime::IsBitrateImproving(
   return initial_probe || bitrate_above_estimate;
 }
 
-void RemoteBitrateEstimatorAbsSendTime::IncomingPacketFeedbackVector(
+void DelayBasedProbingEstimator::IncomingPacketFeedbackVector(
     const std::vector<PacketInfo>& packet_feedback_vector) {
   RTC_DCHECK(network_thread_.CalledOnValidThread());
   for (const auto& packet_info : packet_feedback_vector) {
     IncomingPacketInfo(packet_info.arrival_time_ms,
                        ConvertMsTo24Bits(packet_info.send_time_ms),
-                       packet_info.payload_size, 0, packet_info.was_paced);
+                       packet_info.payload_size, 0, packet_info.was_paced,
+                       packet_info.probe_cluster_id);
   }
 }
 
-void RemoteBitrateEstimatorAbsSendTime::IncomingPacket(int64_t arrival_time_ms,
-                                                       size_t payload_size,
-                                                       const RTPHeader& header,
-                                                       bool was_paced) {
+void DelayBasedProbingEstimator::IncomingPacket(int64_t arrival_time_ms,
+                                                size_t payload_size,
+                                                const RTPHeader& header,
+                                                bool was_paced) {
   RTC_DCHECK(network_thread_.CalledOnValidThread());
   if (!header.extension.hasAbsoluteSendTime) {
-    LOG(LS_WARNING) << "RemoteBitrateEstimatorAbsSendTimeImpl: Incoming packet "
+    // XXX(philipel): The BitrateEstimatorTest relies on this EXACT log line.

[danilchap, 2016/06/03 14:11:45]

ditto

[philipel, 2016/06/03 14:54:55]

Acknowledged.

+    LOG(LS_WARNING) << "RemoteBitrateEstimatorAbsSendTime: Incoming packet "
                        "is missing absolute send time extension!";
     return;
   }
   IncomingPacketInfo(arrival_time_ms, header.extension.absoluteSendTime,
-                     payload_size, header.ssrc, was_paced);
+                     payload_size, header.ssrc, was_paced,
+                     PacketInfo::kNotAProbe);
 }
 
-void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
-    int64_t arrival_time_ms,
-    uint32_t send_time_24bits,
-    size_t payload_size,
-    uint32_t ssrc,
-    bool was_paced) {
+void DelayBasedProbingEstimator::IncomingPacketInfo(int64_t arrival_time_ms,
+                                                    uint32_t send_time_24bits,
+                                                    size_t payload_size,
+                                                    uint32_t ssrc,
+                                                    bool was_paced,
+                                                    int probe_cluster_id) {
   assert(send_time_24bits < (1ul << 24));
   // Shift up send time to use the full 32 bits that inter_arrival works with,
   // so wrapping works properly.
@@ -251,10 +245,7 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
   uint32_t ts_delta = 0;
   int64_t t_delta = 0;
   int size_delta = 0;
-  // For now only try to detect probes while we don't have a valid estimate, and
-  // make sure the packet was paced. We currently assume that only packets
-  // larger than 200 bytes are paced by the sender.
-  was_paced = was_paced && payload_size > PacedSender::kMinProbePacketSize;
+
   bool update_estimate = false;
   uint32_t target_bitrate_bps = 0;
   std::vector<uint32_t> ssrcs;
@@ -266,7 +257,8 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
     RTC_DCHECK(estimator_.get());
     ssrcs_[ssrc] = now_ms;
 
-    if (was_paced &&
+    if (probe_cluster_id != PacketInfo::kNotAProbe &&
+        payload_size > PacedSender::kMinProbePacketSize &&
         (!remote_rate_.ValidEstimate() ||
          now_ms - first_packet_time_ms_ < kInitialProbingIntervalMs)) {
       // TODO(holmer): Use a map instead to get correct order?
@@ -282,7 +274,8 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
                      << " ms, send delta=" << send_delta_ms
                      << " ms, recv delta=" << recv_delta_ms << " ms.";
       }
-      probes_.push_back(Probe(send_time_ms, arrival_time_ms, payload_size));
+      probes_.push_back(
+          Probe(send_time_ms, arrival_time_ms, payload_size, probe_cluster_id));
       ++total_probes_received_;
       // Make sure that a probe which updated the bitrate immediately has an
       // effect by calling the OnReceiveBitrateChanged callback.
@@ -329,14 +322,14 @@ void RemoteBitrateEstimatorAbsSendTime::IncomingPacketInfo(
   }
 }
 
-void RemoteBitrateEstimatorAbsSendTime::Process() {}
+void DelayBasedProbingEstimator::Process() {}
 
-int64_t RemoteBitrateEstimatorAbsSendTime::TimeUntilNextProcess() {
+int64_t DelayBasedProbingEstimator::TimeUntilNextProcess() {
   const int64_t kDisabledModuleTime = 1000;
   return kDisabledModuleTime;
 }
 
-void RemoteBitrateEstimatorAbsSendTime::TimeoutStreams(int64_t now_ms) {
+void DelayBasedProbingEstimator::TimeoutStreams(int64_t now_ms) {
   for (Ssrcs::iterator it = ssrcs_.begin(); it != ssrcs_.end();) {
     if ((now_ms - it->second) > kStreamTimeOutMs) {
       ssrcs_.erase(it++);
@@ -355,20 +348,19 @@ void RemoteBitrateEstimatorAbsSendTime::TimeoutStreams(int64_t now_ms) {
   }
 }
 
-void RemoteBitrateEstimatorAbsSendTime::OnRttUpdate(int64_t avg_rtt_ms,
-                                                    int64_t max_rtt_ms) {
+void DelayBasedProbingEstimator::OnRttUpdate(int64_t avg_rtt_ms,
+                                             int64_t max_rtt_ms) {
   rtc::CritScope lock(&crit_);
   remote_rate_.SetRtt(avg_rtt_ms);
 }
 
-void RemoteBitrateEstimatorAbsSendTime::RemoveStream(uint32_t ssrc) {
+void DelayBasedProbingEstimator::RemoveStream(uint32_t ssrc) {
   rtc::CritScope lock(&crit_);
   ssrcs_.erase(ssrc);
 }
 
-bool RemoteBitrateEstimatorAbsSendTime::LatestEstimate(
-    std::vector<uint32_t>* ssrcs,
-    uint32_t* bitrate_bps) const {
+bool DelayBasedProbingEstimator::LatestEstimate(std::vector<uint32_t>* ssrcs,
+                                                uint32_t* bitrate_bps) const {
   // Currently accessed from both the process thread (see
   // ModuleRtpRtcpImpl::Process()) and the configuration thread (see
   // Call::GetStats()). Should in the future only be accessed from a single
@@ -388,7 +380,7 @@ bool RemoteBitrateEstimatorAbsSendTime::LatestEstimate(
   return true;
 }
 
-void RemoteBitrateEstimatorAbsSendTime::SetMinBitrate(int min_bitrate_bps) {
+void DelayBasedProbingEstimator::SetMinBitrate(int min_bitrate_bps) {
   // Called from both the configuration thread and the network thread. Shouldn't
   // be called from the network thread in the future.
   rtc::CritScope lock(&crit_);