Measure packet loss so we can use it to select ICE candidate pairs.

webrtc/p2p/base/packetlossestimator.cc

+/*
+ *  Copyright 2004 The WebRTC Project Authors. All rights reserved.

[Taylor Brandstetter, 2017/03/07 18:16:31]

nit: 2017 

[Zach Stein, 2017/03/08 06:21:14]

Done.

+ *
+ *  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
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/p2p/base/packetlossestimator.h"
+
+namespace cricket {
+
+PacketLossEstimator::PacketLossEstimator(int64_t consider_lost_after,
+                                         int64_t forget_after)
+    : consider_lost_after_(consider_lost_after), forget_after_(forget_after) {}

[Taylor Brandstetter, 2017/03/07 18:16:32]

Could add an "RTC_DCHECK_LT(consider_lost_after, forget_after)" (from
webrtc/base/checks.h)

[Zach Stein, 2017/03/08 06:21:14]

Done.

+
+void PacketLossEstimator::ExpectResponse(std::string id, int64_t sent_time) {
+  tracked_packets_[id] = PacketInfo{sent_time, false};
+
+  UpdateResponseRateIfStale(sent_time);

[Taylor Brandstetter, 2017/03/07 18:16:32]

Is this called just to ensure the the map doesn't grow indefinitely if the
client doesn't call "UpdateResponseRate"? If so I'd suggest making it *not*
actually update the response rate since that may be an unexpected side effect.

There could be a method called "ForgetOldRequests", for example, that only
serves to clear things from the map and is called by both this and
UpdateResponseRate.

[Zach Stein, 2017/03/08 06:21:14]

Having ForgetOldRequests not update the response rate does sound better.

Do you have a preference for how to test this gets called from
ReceivedResponse/UpdateResponseRate if we aren't updating the response rate? I
don't see how to do it without either expanding the public interface or
providing something like a protected getter just for the tests (though maybe
that is more okay in this case, since we are explicitly testing something that
isn't part of the public interface).

[Taylor Brandstetter, 2017/03/08 17:48:20]

How to test that the map doesn't grow indefinitely, you mean? I haven't seen
many examples of that kind of test, but that's a situation where it does seem
appropriate to have a "tracked_request_count_for_testing()" (for example)
method. We add the "for_testing" suffix to make it clear that the method is only
used for testing, and I believe there's a presubmit check that complains if you
call a "for_testing" method outside a test.

[Zach Stein, 2017/03/08 23:00:08]

Done.

+}
+
+void PacketLossEstimator::ReceivedResponse(std::string id,
+                                           int64_t received_time) {
+  auto iter = tracked_packets_.find(id);
+  if (iter != tracked_packets_.end()) {
+    auto& packet_info = iter->second;
+    packet_info.response_received = true;
+  }
+
+  UpdateResponseRateIfStale(received_time);
+}
+
+void PacketLossEstimator::UpdateResponseRate(int64_t now) {
+  int responses_expected = 0;
+  int responses_received = 0;
+
+  for (auto iter = tracked_packets_.begin(); iter != tracked_packets_.end();) {
+    const auto& packet_info = iter->second;
+    if (Forget(packet_info, now)) {
+      iter = tracked_packets_.erase(iter);
+      continue;
+    }
+    if (packet_info.response_received) {
+      responses_expected += 1;
+      responses_received += 1;
+    } else if (ConsiderLost(packet_info, now)) {
+      responses_expected += 1;
+    }
+    ++iter;
+  }
+
+  if (responses_expected > 0) {
+    response_rate_ =
+        static_cast<double>(responses_received) / responses_expected;
+  } else {
+    response_rate_ = 1.0;
+  }
+
+  last_update_time_ = now;
+}
+
+void PacketLossEstimator::UpdateResponseRateIfStale(int64_t now) {
+  if (now - last_update_time_ > forget_after_) {
+    UpdateResponseRate(now);
+  }
+}
+
+bool PacketLossEstimator::ConsiderLost(const PacketInfo& packet_info,
+                                       int64_t now) const {
+  return packet_info.sent_time < now - consider_lost_after_;
+}
+
+bool PacketLossEstimator::Forget(const PacketInfo& packet_info,
+                                 int64_t now) const {
+  return now - packet_info.sent_time > forget_after_;
+}
+
+}  // namespace cricket