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

webrtc/p2p/base/port.h

Index: webrtc/p2p/base/port.h
diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h
index 4d156567766746405c54cc94d4911effbc4dd2a6..46d18c514d5bb7258093b2e225f181ce610e5435 100644
--- a/webrtc/p2p/base/port.h
+++ b/webrtc/p2p/base/port.h
@@ -433,6 +433,47 @@ class Port : public PortInterface, public rtc::MessageHandler,
   friend class Connection;
 };
 
+class PacketLossEstimator {

[Taylor Brandstetter, 2017/03/01 02:26:12]

Some comments around this class may be helpful. Such as: "Tracks the percentage
of STUN pings that received a response. A response is counted as lost only after
|expire_after| milliseconds."

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

Done.

+ public:
+  struct PacketInfo {

[Taylor Brandstetter, 2017/03/01 02:26:12]

It looks like this struct can be private.

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

Don't need this now that outstanding_packets_ is an unordered_map (see below).

+    PacketInfo(const std::string id, int64_t sent_time)
+        : id(id), sent_time(sent_time) {}
+
+    std::string id;
+    int64_t sent_time;
+  };
+
+  PacketLossEstimator(int64_t expire_after);

[Taylor Brandstetter, 2017/03/01 02:26:12]

Doesn't have to be in this CL, but it may be useful to be able to set "expire
after" after creation. For example, to set it to "RTT * 1.5", or "RTT + some
factor of RTT variance" after RTT converges.

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

I like this idea but will leave it for another CL. I renamed expire_after to
consider_lost_after. Maybe we can differentiate between messages that are
considered expired (we will still listen for the responses) and lost (we will
stop listening for the responses) later.

+
+  void ExpectResponse(std::string id, int64_t sent_time);
+
+  void ReceivedResponse(std::string id, int64_t received_time);
+
+  void RemoveExpiredPackets(int64_t now);
+
+  // This is updated during ReceivedResponse and RemoveExpiredPackets.

[Taylor Brandstetter, 2017/03/01 02:26:12]

The code may be more readable if only "RemoveExpiredPackets" updates the rate.
And actually, it could just be called "UpdateResponseRate" instead. 

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

Good suggestion, I think this will be cleaner.

+  // You may want to call RemoveExpiredPackets to ensure the data is up to date.
+  double get_response_rate() const { return response_rate_; }
+
+ protected:
+  // for unittests
+  int64_t get_responses_expected() const { return responses_expected_; }
+  // for unittests
+  int64_t get_responses_received() const { return responses_received_; }

[Taylor Brandstetter, 2017/03/01 02:26:13]

I'd prefer not having "for test" methods unless absolutely necessary; in this
case I think you should be able to test everything just using
get_response_rate().

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

I think this gives better coverage (ResponseLost/ResponseLate will fail if
responses_expected_ is not incremented, but we wouldn't notice if we were only
checking response_rate_), but I will delete them if you feel strongly.

[Taylor Brandstetter, 2017/03/02 03:09:57]

ResponseLate and ResponseLost could be updated to send one request (or multiple)
that succeeds, then one whose response is lost, and check that the percentage is
what's expected. Maybe I'm a little opinionated about this, but my experience so
far with "for_testing" stuff is usually negative; it's hard to maintain and
often results in less overall test coverage. And almost always, you can get the
same test coverage with a black box test.

Maybe this CL isn't the best example, but we have lots of tests in this area of
code that call internal event handler methods (like "OnReadPacket"). We'd like
to change the implementation of these objects but it's made more difficult by
all the tests depending on implementation details. Plus, in some cases they're
only testing a small part of the normal code path.

+
+ private:
+  void UpdateResponseRate();
+
+  bool IsExpired(const PacketInfo& packet, int64_t received_time) const;
+
+  int64_t expire_after_;  // in milliseconds
+
+  std::vector<PacketInfo> outstanding_packets_;

[Taylor Brandstetter, 2017/03/01 02:26:12]

nit: Probably doesn't matter, but would an unordered_map of IDs to times be more
efficient?

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

Maybe. Thinking about this did make me realize that I was assuming the vector
would be sorted by sent_time in RemoveExpiredPackets (which isn't documented or
necessarily true), so I will switch to an unordered_map. Hashing strings might
be slower, but I agree it probably doesn't matter (though I have never seen
profiles of any of this code).

+  int64_t responses_expected_ = 0;
+  int64_t responses_received_ = 0;
+  double response_rate_ = 1.0;
+};

[Taylor Brandstetter, 2017/03/01 02:26:12]

I think we can move this to its own file; port.h already has a lot of stuff, and
more files doesn't really hurt.

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

Acknowledged. I will do this in a separate upload so the diffs are easier to
read.

[Taylor Brandstetter, 2017/03/02 03:09:57]

Thanks; that does help a lot.

+
 // Represents a communication link between a port on the local client and a
 // port on the remote client.
 class Connection : public CandidatePairInterface,
@@ -719,6 +760,8 @@ class Connection : public CandidatePairInterface,
   int64_t receiving_unchanged_since_ = 0;
   std::vector<SentPing> pings_since_last_response_;
 
+  PacketLossEstimator packet_loss_estimator_;
+
   bool reported_;
   IceCandidatePairState state_;
   // Time duration to switch from receiving to not receiving.