Prepare for ICE-renomination

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 394 matching lines @@
   friend class Connection;
 };
 
 // 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)
-        : id(id), sent_time(sent_time) {}
+    SentPing(const std::string id, int64_t sent_time, int nomination_value)
+        : id(id), sent_time(sent_time), nomination_value(nomination_value) {}
 
     std::string id;
     int64_t sent_time;
+    int nomination_value;

[pthatcher1, 2016/07/28 22:57:26]

I'd call this "nomination".

[honghaiz3, 2016/08/03 04:46:56]

Done.

   };
 
   // States are from RFC 5245. http://tools.ietf.org/html/rfc5245#section-5.7.4
   enum State {
     STATE_WAITING = 0,  // Check has not been performed, Waiting pair on CL.
     STATE_INPROGRESS,   // Check has been sent, transaction is in progress.
     STATE_SUCCEEDED,    // Check already done, produced a successful result.
     STATE_FAILED        // Check for this connection failed.
   };
 
@@ skipping 69 matching lines @@
   // Called when a connection is determined to be no longer useful to us.  We
   // still keep it around in case the other side wants to use it.  But we can
   // safely stop pinging on it and we can allow it to time out if the other
   // side stops using it as well.
   bool pruned() const { return pruned_; }
   void Prune();
 
   bool use_candidate_attr() const { return use_candidate_attr_; }
   void set_use_candidate_attr(bool enable);
 
-  bool nominated() const { return nominated_; }
-  void set_nominated(bool nominated) { nominated_ = nominated; }
+  int nominating_value() const { return nominating_value_; }
+  void set_nominating_value(int value) { nominating_value_ = value; }
+
+  int nominated_value() const { return nominated_value_; }
+  // Public for unit tests.
+  void set_nominated_value(int nominated_value) {
+    // We don't un-nominate a connection, so we will keep the maximum
+    // nominated value.
+    if (nominated_value > nominated_value_) {
+      nominated_value_ = nominated_value;
+    }

[pthatcher1, 2016/07/28 22:57:26]

I think we should go from including the attribute to not including the attribute
if it's no longer the selected candidate pair.  In other words, just
"nomination_ = nomination" here. 

Even if we wanted fancier logic, it should be in p2ptransportchannel.cc, not
here.

[honghaiz3, 2016/08/03 04:46:56]

The check protects packet re-ordering. 
If for some reason, a ping with lower-nominated value arrived later, we should
not set the nominated value to a smaller one. nominated is set in Port class.
Not sure why/how to do it in p2ptransportchannel. 

[pthatcher1, 2016/08/03 22:13:25]

Ah, good point about reordering.  Do we have a unit test that covers that?

Also, I still think it shouldn't be in set_remote_nomination.  Putting it in
port.cc would make sense as:

if (remote_nomination > 0 && remote_nomination > remote_nomination()) {
  set_remote_nomination(remote_nomination);
  ...
}

Actually, it's that guaranteed to be the same as:

if (remote_nomination > remote_nomination()) {
  set_remote_nomination(remote_nomination);
  ...
}

?


By the way, we should make sure that in our standardization that we make it
clear that the 0 value must not be used.  Call it reserved or something.

[honghaiz3, 2016/08/03 23:39:51]

Done. Add a test for ignoring smaller nomination. 
Will remember this about the standardization. 

+  }
 
   void set_remote_ice_mode(IceMode mode) {
     remote_ice_mode_ = mode;
   }
 
   void set_receiving_timeout(int64_t receiving_timeout_ms) {
     receiving_timeout_ = receiving_timeout_ms;
   }
 
   // Makes the connection go away.
   void Destroy();
 
   // Makes the connection go away, in a failed state.
   void FailAndDestroy();
 
   // Prunes the connection and sets its state to STATE_FAILED,
   // It will not be used or send pings although it can still receive packets.
   void FailAndPrune();
 
   // Checks that the state of this connection is up-to-date.  The argument is
   // the current time, which is compared against various timeouts.
   void UpdateState(int64_t now);
 
   // Called when this connection should try checking writability again.
   int64_t last_ping_sent() const { return last_ping_sent_; }
   void Ping(int64_t now);
-  void ReceivedPingResponse(int rtt);
+  void ReceivedPingResponse(int rtt, const std::string& request_id);
   int64_t last_ping_response_received() const {
     return last_ping_response_received_;
   }
 
   // Called whenever a valid ping is received on this connection.  This is
   // public because the connection intercepts the first ping for us.
   int64_t last_ping_received() const { return last_ping_received_; }
   void ReceivedPing();
   // Handles the binding request; sends a response if this is a valid request.
   void HandleBindingRequest(IceMessage* msg);
@@ skipping 71 matching lines @@
   // If the response is not received within 2 * RTT, the response is assumed to
   // be missing.
   bool missing_responses(int64_t now) const;
 
   // Changes the state and signals if necessary.
   void set_write_state(WriteState value);
   void UpdateReceiving(int64_t now);
   void set_state(State state);
   void set_connected(bool value);
 
+  int acknowledged_nominating_value() const {
+    return acknowledged_nominating_value_;

[pthatcher1, 2016/07/28 22:57:26]

acked_nomination would be a better name.

[honghaiz3, 2016/08/03 04:46:56]

Done.

+  }
+
   void OnMessage(rtc::Message *pmsg);
 
   Port* port_;
   size_t local_candidate_index_;
   Candidate remote_candidate_;
+
+  ConnectionInfo stats_;
+  rtc::RateTracker recv_rate_tracker_;
+  rtc::RateTracker send_rate_tracker_;
+
+ private:
   WriteState write_state_;
   bool receiving_;
   bool connected_;
   bool pruned_;
   // By default |use_candidate_attr_| flag will be true,
   // as we will be using aggressive nomination.
   // But when peer is ice-lite, this flag "must" be initialized to false and
   // turn on when connection becomes "best connection".
   bool use_candidate_attr_;
-  // Whether this connection has been nominated by the controlling side via
-  // the use_candidate attribute.
-  bool nominated_;
+  // Used by the controlling side to indicate that this connection will be
+  // selected for transmission if the peer supports ICE-renomination when this
+  // value is positive. A larger-value indicates that a connection is nominated
+  // later and should be selected by the controlled side with higher precedence.
+  // A zero-value indicates not nominating this connection.
+  int nominating_value_ = 0;
+  // The last nominating value that has been acknowledged.
+  int acknowledged_nominating_value_ = 0;
+  // Used by the controlled side to remember the nomination value received from
+  // the controlling side. When the peer does not support ICE re-nomination,
+  // its value will be 1 if the connection is nominated.
+  int nominated_value_ = 0;

[pthatcher1, 2016/07/28 22:57:26]

nominating_value/nominated_value is confusing.  Perhaps
local_nomination/remote_nomination or just nomination/remote_nomination would
make more sense.  

Usually only one will be set, but I guess if there's a controlling conflict,
you'd need both.

[honghaiz3, 2016/08/03 04:46:56]

Done. There might be role switch, so better to have both.  

+
   IceMode remote_ice_mode_;
   StunRequestManager requests_;
   int rtt_;
   int rtt_samples_ = 0;
   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_;
 
-  rtc::RateTracker recv_rate_tracker_;
-  rtc::RateTracker send_rate_tracker_;
-
-  ConnectionInfo stats_;
-
- private:
   void MaybeAddPrflxCandidate(ConnectionRequest* request,
                               StunMessage* response);
 
   bool reported_;
   State state_;
   // Time duration to switch from receiving to not receiving.
   int receiving_timeout_;
   int64_t time_created_ms_;
   int num_pings_sent_ = 0;
 
@@ skipping 11 matching lines @@
            const rtc::PacketOptions& options) override;
   int GetError() override { return error_; }
 
  private:
   int error_ = 0;
 };
 
 }  // namespace cricket
 
 #endif  // WEBRTC_P2P_BASE_PORT_H_