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

webrtc/p2p/base/port.h

 /*
  *  Copyright 2004 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
  *  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.
  */
 
@@ skipping 415 matching lines @@
   // A virtual cost perceived by the user, usually based on the network type
   // (WiFi. vs. Cellular). It takes precedence over the priority when
   // comparing two connections.
   uint16_t network_cost_;
   State state_ = State::INIT;
   int64_t last_time_all_connections_removed_ = 0;
 
   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,
                    public rtc::MessageHandler,
                    public sigslot::has_slots<> {
  public:
   struct SentPing {
     SentPing(const std::string id, int64_t sent_time, uint32_t nomination)
         : id(id), sent_time(sent_time), nomination(nomination) {}
 
@@ skipping 266 matching lines @@
   // https://w3c.github.io/webrtc-stats/#dom-rtcicecandidatepairstats-currentroundtriptime
   rtc::Optional<uint32_t> current_round_trip_time_ms_;
   int64_t last_ping_sent_;      // last time we sent a ping to the other side
   int64_t last_ping_received_;  // last time we received a ping from the other
                                 // side
   int64_t last_data_received_;
   int64_t last_ping_response_received_;
   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.
   int receiving_timeout_;
   int64_t time_created_ms_;
   int num_pings_sent_ = 0;
 
   friend class Port;
   friend class ConnectionRequest;
 };
 
 // ProxyConnection defers all the interesting work to the port.
 class ProxyConnection : public Connection {
  public:
   ProxyConnection(Port* port, size_t index, const Candidate& remote_candidate);
 
   int Send(const void* data,
            size_t size,
            const rtc::PacketOptions& options) override;
   int GetError() override { return error_; }
 
  private:
   int error_ = 0;
 };
 
 }  // namespace cricket
 
 #endif  // WEBRTC_P2P_BASE_PORT_H_