Prepare for ICE-renomination

webrtc/p2p/base/port.cc

Index: webrtc/p2p/base/port.cc
diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc
index b6173ebf45768be331c589631f81f9aee8ccbc80..f8aaee0e524570d39140560e09f6d8f93f738404 100644
--- a/webrtc/p2p/base/port.cc
+++ b/webrtc/p2p/base/port.cc
@@ -754,15 +754,20 @@ class ConnectionRequest : public StunRequest {
     if (connection_->port()->GetIceRole() == ICEROLE_CONTROLLING) {
       request->AddAttribute(new StunUInt64Attribute(
           STUN_ATTR_ICE_CONTROLLING, connection_->port()->IceTiebreaker()));
-      // Since we are trying aggressive nomination, sending USE-CANDIDATE
-      // attribute in every ping.
-      // If we are dealing with a ice-lite end point, nomination flag
-      // in Connection will be set to false by default. Once the connection
-      // becomes "best connection", nomination flag will be turned on.
+      // We should really have either USE_CANDIDATE attribute or
+      // ICE_NOMINATION_VALUE attribute but not both. That was enforced in
+      // p2ptransportchannel.
+      // Note: This will keep on sending USE_CANDIDATE_ATTR on the selected
+      // connection if peer does not support re-nomination.
       if (connection_->use_candidate_attr()) {
         request->AddAttribute(new StunByteStringAttribute(
             STUN_ATTR_USE_CANDIDATE));
       }
+      if (connection_->nominating_value() > 0 &&
+          !connection_->nomination_acknowledged()) {
+        request->AddAttribute(new StunUInt32Attribute(
+            STUN_ATTR_NOMINATION_VALUE, connection_->nominating_value()));
+      }
     } else if (connection_->port()->GetIceRole() == ICEROLE_CONTROLLED) {
       request->AddAttribute(new StunUInt64Attribute(
           STUN_ATTR_ICE_CONTROLLED, connection_->port()->IceTiebreaker()));
@@ -830,7 +835,6 @@ Connection::Connection(Port* port,
       connected_(true),
       pruned_(false),
       use_candidate_attr_(false),
-      nominated_(false),
       remote_ice_mode_(ICEMODE_FULL),
       requests_(port->thread()),
       rtt_(DEFAULT_RTT),
@@ -1041,10 +1045,23 @@ void Connection::HandleBindingRequest(IceMessage* msg) {
   }
 
   if (port_->GetIceRole() == ICEROLE_CONTROLLED) {
-    const StunByteStringAttribute* use_candidate_attr =
-        msg->GetByteString(STUN_ATTR_USE_CANDIDATE);
-    if (use_candidate_attr) {
-      set_nominated(true);
+    const StunUInt32Attribute* nomination_value_attr =
+        msg->GetUInt32(STUN_ATTR_NOMINATION_VALUE);
+    int nomination_value = 0;
+    if (nomination_value_attr) {
+      nomination_value = static_cast<int>(nomination_value_attr->value());
+      if (nomination_value < 0) {
+        LOG(LS_ERROR) << "Negative nomination value: " << nomination_value;
+      }
+    } else {
+      const StunByteStringAttribute* use_candidate_attr =
+          msg->GetByteString(STUN_ATTR_USE_CANDIDATE);
+      if (use_candidate_attr) {
+        nomination_value = 1;
+      }
+    }
+    if (nomination_value > 0) {
+      set_nominated_value(nomination_value);
       SignalNominated(this);
     }
   }
@@ -1178,9 +1195,11 @@ void Connection::UpdateState(int64_t now) {
 void Connection::Ping(int64_t now) {
   last_ping_sent_ = now;
   ConnectionRequest *req = new ConnectionRequest(this);
-  pings_since_last_response_.push_back(SentPing(req->id(), now));
+  pings_since_last_response_.push_back(
+      SentPing(req->id(), now, nominating_value_));
   LOG_J(LS_VERBOSE, this) << "Sending STUN ping "
-                          << ", id=" << rtc::hex_encode(req->id());
+                          << ", id=" << rtc::hex_encode(req->id())
+                          << ", nominating_value=" << nominating_value_;
   requests_.Send(req);
   state_ = STATE_INPROGRESS;
   num_pings_sent_++;
@@ -1191,17 +1210,26 @@ void Connection::ReceivedPing() {
   UpdateReceiving(last_ping_received_);
 }
 
-void Connection::ReceivedPingResponse(int rtt) {
+void Connection::ReceivedPingResponse(int rtt, const std::string& request_id) {
   // We've already validated that this is a STUN binding response with
   // the correct local and remote username for this connection.
   // So if we're not already, become writable. We may be bringing a pruned
   // connection back to life, but if we don't really want it, we can always
   // prune it again.
+  if (!nomination_acknowledged_) {
+    auto iter = std::find_if(
+        pings_since_last_response_.begin(), pings_since_last_response_.end(),
+        [request_id](const SentPing& ping) { return ping.id == request_id; });
+    if (iter != pings_since_last_response_.end() &&
+        iter->nomination_value == nominating_value_) {
+      nomination_acknowledged_ = true;
+    }
+  }
+  pings_since_last_response_.clear();
   last_ping_response_received_ = rtc::TimeMillis();
   UpdateReceiving(last_ping_response_received_);
   set_write_state(STATE_WRITABLE);
   set_state(STATE_SUCCEEDED);
-  pings_since_last_response_.clear();
   rtt_samples_++;
   rtt_ = (RTT_RATIO * rtt_ + rtt) / (RTT_RATIO + 1);
 }
@@ -1274,21 +1302,16 @@ std::string Connection::ToString() const {
   const Candidate& local = local_candidate();
   const Candidate& remote = remote_candidate();
   std::stringstream ss;
-  ss << "Conn[" << ToDebugId()
-     << ":" << port_->content_name()
-     << ":" << local.id() << ":" << local.component()
-     << ":" << local.generation()
-     << ":" << local.type() << ":" << local.protocol()
-     << ":" << local.address().ToSensitiveString()
-     << "->" << remote.id() << ":" << remote.component()
-     << ":" << remote.priority()
-     << ":" << remote.type() << ":"
-     << remote.protocol() << ":" << remote.address().ToSensitiveString() << "|"
-     << CONNECT_STATE_ABBREV[connected()]
-     << RECEIVE_STATE_ABBREV[receiving()]
-     << WRITE_STATE_ABBREV[write_state()]
-     << ICESTATE[state()] << "|"
-     << priority() << "|";
+  ss << "Conn[" << ToDebugId() << ":" << port_->content_name() << ":"
+     << local.id() << ":" << local.component() << ":" << local.generation()
+     << ":" << local.type() << ":" << local.protocol() << ":"
+     << local.address().ToSensitiveString() << "->" << remote.id() << ":"
+     << remote.component() << ":" << remote.priority() << ":" << remote.type()
+     << ":" << remote.protocol() << ":" << remote.address().ToSensitiveString()
+     << "|" << CONNECT_STATE_ABBREV[connected()]
+     << RECEIVE_STATE_ABBREV[receiving()] << WRITE_STATE_ABBREV[write_state()]
+     << ICESTATE[state()] << "|" << nominated_value() << "|" << priority()

[Taylor Brandstetter, 2016/07/22 17:36:01]

It seems like nominating_value would also be useful to know here.

[honghaiz3, 2016/07/22 22:50:11]

nominating_value_ could change dynamically. 
Basically it is value put in the Ping message. 
If the controlling side nominates a connection and it is acknowledged, it
doesn't need to nominate that connection any more. 
In that case, the nominating value will become 0. 

[Taylor Brandstetter, 2016/07/22 23:48:28]

I don't see where nominating_value_ is set to 0. And it seems equally dynamic as
nominated_value_.

[honghaiz3, 2016/07/25 22:24:40]

Hope this is clearer now with the updated CL. 

[Taylor Brandstetter, 2016/07/27 22:20:55]

I see that in the updated CL, the nominating value can go to 0. But I still
think it would still be valuable to log it in some form. If we log "highest
received nomination value" I'd also like to see "highest sent nomination value"
when debugging an ICE issue.

+     << "|";
   if (rtt_ < DEFAULT_RTT) {
     ss << rtt_ << "]";
   } else {
@@ -1309,20 +1332,16 @@ void Connection::OnConnectionRequestResponse(ConnectionRequest* request,
 
   int rtt = request->Elapsed();
 
-  ReceivedPingResponse(rtt);
-
   if (LOG_CHECK_LEVEL_V(sev)) {
-    bool use_candidate = (
-        response->GetByteString(STUN_ATTR_USE_CANDIDATE) != nullptr);
     std::string pings;
     PrintPingsSinceLastResponse(&pings, 5);
     LOG_JV(sev, this) << "Received STUN ping response"
                       << ", id=" << rtc::hex_encode(request->id())
                       << ", code=0"  // Makes logging easier to parse.
                       << ", rtt=" << rtt
-                      << ", use_candidate=" << use_candidate

[Taylor Brandstetter, 2016/07/22 17:36:01]

Can we log both use_candidate and nominating_value here? Or are you removing it
because you can just look for the log for "Sent STUN ping"? That seems fine.

[honghaiz3, 2016/07/22 22:50:11]

This is in the PingResponse. 
We don't send use_candidate back in the PingResponse. 
So it will always be false. 

[Taylor Brandstetter, 2016/07/22 23:48:28]

Acknowledged.

[honghaiz3, 2016/07/25 22:24:40]

Acknowledged.

                       << ", pings_since_last_response=" << pings;
   }
+  ReceivedPingResponse(rtt, request->id());
 
   stats_.recv_ping_responses++;
 
@@ -1369,10 +1388,10 @@ void Connection::OnConnectionRequestTimeout(ConnectionRequest* request) {
 void Connection::OnConnectionRequestSent(ConnectionRequest* request) {
   // Log at LS_INFO if we send a ping on an unwritable connection.
   rtc::LoggingSeverity sev = !writable() ? rtc::LS_INFO : rtc::LS_VERBOSE;
-  bool use_candidate = use_candidate_attr();
   LOG_JV(sev, this) << "Sent STUN ping"
                     << ", id=" << rtc::hex_encode(request->id())
-                    << ", use_candidate=" << use_candidate;
+                    << ", use_candidate=" << use_candidate_attr()
+                    << ", nominating value=" << nominating_value();
   stats_.sent_ping_requests_total++;
   if (stats_.recv_ping_responses == 0) {
     stats_.sent_ping_requests_before_first_response++;