Prepare for ICE-renomination

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 37926c955603a3f27918270b5b43b177cdccddf9..5fa7a4c697451610875c6d4663c11a4f96466221 100644
--- a/webrtc/p2p/base/port.h
+++ b/webrtc/p2p/base/port.h
@@ -412,11 +412,12 @@ class Connection : public CandidatePairInterface,
                    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
@@ -506,8 +507,18 @@ class Connection : public CandidatePairInterface,
   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;
@@ -534,7 +545,7 @@ class Connection : public CandidatePairInterface,
   // 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_;
   }
@@ -626,11 +637,21 @@ class Connection : public CandidatePairInterface,
   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_;
@@ -640,9 +661,19 @@ class Connection : public CandidatePairInterface,
   // 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_;
@@ -655,12 +686,6 @@ class Connection : public CandidatePairInterface,
   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);