A few updates on connection states

webrtc/p2p/base/p2ptransportchannel.cc

Index: webrtc/p2p/base/p2ptransportchannel.cc
diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc
index 094a8dcc8f1ffc937a436dc85890633c62d60760..c53bd8839e8d0b74f16ccd5f87bd7f9662811953 100644
--- a/webrtc/p2p/base/p2ptransportchannel.cc
+++ b/webrtc/p2p/base/p2ptransportchannel.cc
@@ -22,11 +22,7 @@
 namespace {
 
 // messages for queuing up work for ourselves
-enum {
-  MSG_SORT = 1,
-  MSG_PING,
-  MSG_CHECK_RECEIVING
-};
+enum { MSG_SORT = 1, MSG_CHECK_AND_PING };

[pthatcher1, 2015/09/17 05:58:42]

Why did you combine these?  I think it made sense to be separate.

[honghaiz3, 2015/09/17 19:47:56]

There are two reasons. 
1. When you do ping, you also check the connections states, which makes it sort
of redundant for checking (except for the frequency).
2. When we check and fing the connection not receiving, we want to switch to a
short ping period without waiting another ping coming. 

 
 // When the socket is unwritable, we will use 10 Kbps (ignoring IP+UDP headers)
 // for pinging.  When the socket is writable, we will use only 1 Kbps because
@@ -34,12 +30,13 @@ enum {
 // well on a 28.8K modem, which is the slowest connection on which the voice
 // quality is reasonable at all.
 static const uint32 PING_PACKET_SIZE = 60 * 8;
-static const uint32 WRITABLE_DELAY = 1000 * PING_PACKET_SIZE / 1000;  // 480ms
-static const uint32 UNWRITABLE_DELAY = 1000 * PING_PACKET_SIZE / 10000;  // 50ms
+static const uint32 LONG_PING_DELAY = 1000 * PING_PACKET_SIZE / 1000;  // 480ms
+static const uint32 SHORT_PING_DELAY = 1000 * PING_PACKET_SIZE / 10000;  // 50ms

[pthatcher1, 2015/09/17 05:58:41]

I think a good name for these would be WEAK_CONNECTION_PING_DELAY and
STRONG_CONNECTION_PING_DELAY,  and then comment that STRONG == writable &&
receiving, and WEAK == !writable || !receiving.

[honghaiz3, 2015/09/17 19:47:55]

Done.

 
 // If there is a current writable connection, then we will also try hard to
 // make sure it is pinged at this rate.
-static const uint32 MAX_CURRENT_WRITABLE_DELAY = 900;  // 2*WRITABLE_DELAY - bit
+// 2 * LONG_PING_DELAY - bit
+static const uint32 MAX_CURRENT_WRITABLE_DELAY = 900;
 
 static const int MIN_CHECK_RECEIVING_DELAY = 50;  // ms
 
@@ -72,7 +69,15 @@ int CompareConnectionCandidates(cricket::Connection* a,
 
 // Compare two connections based on their connected state, writability and
 // static preferences.
-int CompareConnections(cricket::Connection *a, cricket::Connection *b) {
+int CompareConnectionStates(cricket::Connection* a, cricket::Connection* b) {
+  // When the current best connection is not receiving but a backup connection
+  // is receiving, the backup connection has a higher priority regardless
+  // its writable state.

[pthatcher1, 2015/09/17 05:58:42]

It doesn't need to just be "backup".  We could simply say that a receiving
connection is better than a non-receiving one.

[honghaiz3, 2015/09/17 19:47:55]

Done.

+  if (a->receiving() && !b->receiving())
+    return 1;
+  if (!a->receiving() && b->receiving())
+    return -1;
+
   // Sort based on write-state.  Better states have lower values.
   if (a->write_state() < b->write_state())
     return 1;
@@ -110,7 +115,14 @@ int CompareConnections(cricket::Connection *a, cricket::Connection *b) {
       return -1;
     }
   }
+  return 0;
+}
 
+int CompareConnections(cricket::Connection* a, cricket::Connection* b) {
+  int state_cmp = CompareConnectionStates(a, b);
+  if (state_cmp != 0) {
+    return state_cmp;
+  }
   // Compare the candidate information.
   return CompareConnectionCandidates(a, b);
 }
@@ -149,18 +161,31 @@ class ConnectionCompare {
 
 // Determines whether we should switch between two connections, based first on
 // static preferences and then (if those are equal) on latency estimates.
-bool ShouldSwitch(cricket::Connection* a_conn, cricket::Connection* b_conn) {
+bool ShouldSwitch(cricket::Connection* a_conn,
+                  cricket::Connection* b_conn,
+                  cricket::IceRole ice_role) {
   if (a_conn == b_conn)
     return false;
 
   if (!a_conn || !b_conn)  // don't think the latter should happen
     return true;
 
-  int prefs_cmp = CompareConnections(a_conn, b_conn);
-  if (prefs_cmp < 0)
-    return true;
-  if (prefs_cmp > 0)
+  // If the RECEIVING/WRITE/CONNECT states are different, we should switch
+  // regardless of the nominate state. Otherwise, do not switch if |a_conn| is

[pthatcher1, 2015/09/17 05:58:42]

nominate => nominated

[honghaiz3, 2015/09/17 19:47:56]

Done.

+  // nominated but |b_conn| is not.
+  int state_cmp = CompareConnectionStates(a_conn, b_conn);
+  if (state_cmp != 0) {
+    return state_cmp < 0;
+  }
+  if (ice_role == cricket::ICEROLE_CONTROLLED &&
+      a_conn->nominated() > b_conn->nominated()) {
+    LOG(LS_VERBOSE) << "Controlled side did not switch due to nominated status";
     return false;
+  }
+
+  int prefs_cmp = CompareConnectionCandidates(a_conn, b_conn);
+  if (prefs_cmp != 0)
+    return prefs_cmp < 0;
 
   return b_conn->rtt() <= a_conn->rtt() + kMinImprovement;
 }
@@ -172,25 +197,27 @@ namespace cricket {
 P2PTransportChannel::P2PTransportChannel(const std::string& content_name,
                                          int component,
                                          P2PTransport* transport,
-                                         PortAllocator *allocator) :
-    TransportChannelImpl(content_name, component),
-    transport_(transport),
-    allocator_(allocator),
-    worker_thread_(rtc::Thread::Current()),
-    incoming_only_(false),
-    waiting_for_signaling_(false),
-    error_(0),
-    best_connection_(NULL),
-    pending_best_connection_(NULL),
-    sort_dirty_(false),
-    was_writable_(false),
-    remote_ice_mode_(ICEMODE_FULL),
-    ice_role_(ICEROLE_UNKNOWN),
-    tiebreaker_(0),
-    remote_candidate_generation_(0),
-    check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5),
-    receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50) {
-}
+                                         PortAllocator* allocator)
+    : TransportChannelImpl(content_name, component),
+      transport_(transport),
+      allocator_(allocator),
+      worker_thread_(rtc::Thread::Current()),
+      incoming_only_(false),
+      waiting_for_signaling_(false),
+      error_(0),
+      best_connection_(nullptr),
+      pending_best_connection_(nullptr),
+      next_connection_to_ping_(nullptr),
+      sort_dirty_(false),
+      was_writable_(false),
+      remote_ice_mode_(ICEMODE_FULL),
+      ice_role_(ICEROLE_UNKNOWN),
+      tiebreaker_(0),
+      remote_candidate_generation_(0),
+      check_receiving_delay_(MIN_CHECK_RECEIVING_DELAY * 5),
+      receiving_timeout_(MIN_CHECK_RECEIVING_DELAY * 50),
+      ping_delay_(SHORT_PING_DELAY),
+      last_ping_sent_(0) {}
 
 P2PTransportChannel::~P2PTransportChannel() {
   ASSERT(worker_thread_ == rtc::Thread::Current());
@@ -221,6 +248,7 @@ void P2PTransportChannel::AddAllocatorSession(PortAllocatorSession* session) {
 void P2PTransportChannel::AddConnection(Connection* connection) {
   connections_.push_back(connection);
   connection->set_remote_ice_mode(remote_ice_mode_);
+  connection->set_receiving_timeout(receiving_timeout_);
   connection->SignalReadPacket.connect(
       this, &P2PTransportChannel::OnReadPacket);
   connection->SignalReadyToSend.connect(
@@ -342,6 +370,9 @@ void P2PTransportChannel::SetReceivingTimeout(int receiving_timeout_ms) {
       std::max(MIN_CHECK_RECEIVING_DELAY, receiving_timeout_ / 10);
   LOG(LS_VERBOSE) << "Set ICE receiving timeout to " << receiving_timeout_
                   << " milliseconds";
+  for (Connection* connection : connections_) {
+    connection->set_receiving_timeout(receiving_timeout_);
+  }
 }
 
 // Go into the state of processing candidates, and running in general
@@ -358,10 +389,7 @@ void P2PTransportChannel::Connect() {
   Allocate();
 
   // Start pinging as the ports come in.
-  thread()->Post(this, MSG_PING);
-
-  thread()->PostDelayed(
-      check_receiving_delay_, this, MSG_CHECK_RECEIVING);
+  thread()->Post(this, MSG_CHECK_AND_PING);
 }
 
 // A new port is available, attempt to make connections for it
@@ -400,7 +428,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession *session,
   std::vector<RemoteCandidate>::iterator iter;
   for (iter = remote_candidates_.begin(); iter != remote_candidates_.end();
        ++iter) {
-    CreateConnection(port, *iter, iter->origin_port(), false);
+    CreateConnection(port, *iter, iter->origin_port());
   }
 
   SortConnections();
@@ -616,7 +644,7 @@ void P2PTransportChannel::OnCandidate(const Candidate& candidate) {
   }
 
   // Create connections to this remote candidate.
-  CreateConnections(candidate, NULL, false);
+  CreateConnections(candidate, NULL);
 
   // Resort the connections list, which may have new elements.
   SortConnections();
@@ -626,8 +654,7 @@ void P2PTransportChannel::OnCandidate(const Candidate& candidate) {
 // remote candidate.  The return value is true if we created a connection from
 // the origin port.
 bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
-                                            PortInterface* origin_port,
-                                            bool readable) {
+                                            PortInterface* origin_port) {
   ASSERT(worker_thread_ == rtc::Thread::Current());
 
   Candidate new_remote_candidate(remote_candidate);
@@ -665,7 +692,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
   bool created = false;
   std::vector<PortInterface *>::reverse_iterator it;
   for (it = ports_.rbegin(); it != ports_.rend(); ++it) {
-    if (CreateConnection(*it, new_remote_candidate, origin_port, readable)) {
+    if (CreateConnection(*it, new_remote_candidate, origin_port)) {
       if (*it == origin_port)
         created = true;
     }
@@ -673,8 +700,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
 
   if ((origin_port != NULL) &&
       std::find(ports_.begin(), ports_.end(), origin_port) == ports_.end()) {
-    if (CreateConnection(
-        origin_port, new_remote_candidate, origin_port, readable))
+    if (CreateConnection(origin_port, new_remote_candidate, origin_port))
       created = true;
   }
 
@@ -688,8 +714,7 @@ bool P2PTransportChannel::CreateConnections(const Candidate& remote_candidate,
 // And then listen to connection object for changes.
 bool P2PTransportChannel::CreateConnection(PortInterface* port,
                                            const Candidate& remote_candidate,
-                                           PortInterface* origin_port,
-                                           bool readable) {
+                                           PortInterface* origin_port) {
   // Look for an existing connection with this remote address.  If one is not
   // found, then we can create a new connection for this address.
   Connection* connection = port->GetConnection(remote_candidate.address());
@@ -724,11 +749,6 @@ bool P2PTransportChannel::CreateConnection(PortInterface* port,
                          << connections_.size() << " total)";
   }
 
-  // If we are readable, it is because we are creating this in response to a
-  // ping from the other side.  This will cause the state to become readable.
-  if (readable)
-    connection->ReceivedPing();
-
   return true;
 }
 
@@ -853,8 +873,7 @@ bool P2PTransportChannel::GetStats(ConnectionInfos *infos) {
     Connection *connection = *it;
     ConnectionInfo info;
     info.best_connection = (best_connection_ == connection);
-    info.readable =
-        (connection->read_state() == Connection::STATE_READABLE);
+    info.receiving = connection->receiving();
     info.writable =
         (connection->write_state() == Connection::STATE_WRITABLE);
     info.timeout =
@@ -895,10 +914,9 @@ void P2PTransportChannel::Allocate() {
 
 // Monitor connection states.
 void P2PTransportChannel::UpdateConnectionStates() {
-  uint32 now = rtc::Time();
-
   // We need to copy the list of connections since some may delete themselves
   // when we call UpdateState.
+  uint32 now = rtc::Time();
   for (uint32 i = 0; i < connections_.size(); ++i)
     connections_[i]->UpdateState(now);
 }
@@ -937,13 +955,24 @@ void P2PTransportChannel::SortConnections() {
   Connection* top_connection =
       (connections_.size() > 0) ? connections_[0] : nullptr;
 
-  // If necessary, switch to the new choice.
-  // Note that |top_connection| doesn't have to be writable to become the best
-  // connection although it will have higher priority if it is writable.
-  // The controlled side can switch the best connection only if the current
-  // |best connection_| has not been nominated by the controlling side yet.
-  if ((ice_role_ == ICEROLE_CONTROLLING || !best_nominated_connection()) &&
-      ShouldSwitch(best_connection_, top_connection)) {
+  if (top_connection != best_connection_ && best_connection_->writable() &&
+      !top_connection->writable()) {
+    // This is a very special case where the best_connection becomes
+    // not receiving but the top connection is not writable.

[pthatcher1, 2015/09/17 05:58:41]

It seems like, instead, that we should always go into "high speed pinging"
whenever we are "weak" in our connectivity, rather than wait until we have a top
connection to ping.  I think that would be a lot more simple.

[honghaiz3, 2015/09/17 19:47:56]

Yes. That is what is done. 
Combining the comments, I am removing this extra part and sort connections based
on write state first and then receiving. This is also safer.  

+    // We don't switch but check the writability of the |top_connection|.
+    // If we are in the long ping delay, reschedule it so that it will be
+    // executed much sooner. If we are in the short ping delay, just make
+    // it the next one to be pinged, so that we do not overflow the channel
+    // with ping messages.
+    if (ping_delay_ == LONG_PING_DELAY) {
+      thread()->Clear(this, MSG_CHECK_AND_PING);
+      thread()->Post(this, MSG_CHECK_AND_PING);
+    }
+    next_connection_to_ping_ = top_connection;

[pthatcher1, 2015/09/17 05:58:42]

Shouldn't the faster pinging work fine without storing next_connection_to_ping_?

[honghaiz3, 2015/09/17 19:47:56]

Yes. We don't need the extra speedup of the ping request.  

[pthatcher1, 2015/09/17 22:01:17]

So are you going to remove next_connection_to_ping_?

+  } else if (ShouldSwitch(best_connection_, top_connection, ice_role_)) {
+    // If necessary, switch to the new choice.
+    // Note that |top_connection| doesn't have to be writable to become the best
+    // connection although it will have higher priority if it is writable.
     LOG(LS_INFO) << "Switching best connection: " << top_connection->ToString();
     SwitchBestConnectionTo(top_connection);
   }
@@ -993,7 +1022,10 @@ void P2PTransportChannel::PruneConnections() {
   // resources and they may represent very distinct paths over which we can
   // switch. If the |primier| connection is not connected, we may be
   // reconnecting a TCP connection and temporarily do not prune connections in
-  // this network. See the big comment in CompareConnections.
+  // this network. See the big comment in CompareConnections. If the |primier|
+  // connection is not receiving, we do not prune the connections in the network
+  // because either some connection will become receiving or all connections
+  // in the network will time out and be deleted.

[pthatcher1, 2015/09/17 05:58:42]

I think this would be more clear as "once on connection is receiving, we begin
pruning other connections on the same network".

[honghaiz3, 2015/09/17 19:47:55]

Removed the comments as we change back to check writable.

 
   // Get a list of the networks that we are using.
   std::set<rtc::Network*> networks;
@@ -1002,12 +1034,13 @@ void P2PTransportChannel::PruneConnections() {
   }
   for (rtc::Network* network : networks) {
     Connection* primier = GetBestConnectionOnNetwork(network);
-    if (!(primier && primier->writable() && primier->connected())) {
+    if (!(primier && primier->receiving() && primier->connected())) {

[pthatcher1, 2015/09/17 05:58:41]

Why don't we want to keep it as writable()?  Shouldn't we wait until a
connection is writable before pruning other connections?

[honghaiz3, 2015/09/17 19:47:56]

Yes. especially now that we put writable with higher priority then receiving

       continue;
     }
 
     for (Connection* conn : connections_) {
       if ((conn != primier) && (conn->port()->Network() == network) &&
+          (conn != pending_best_connection_) &&
           (CompareConnectionCandidates(primier, conn) >= 0)) {
         conn->Prune();
       }
@@ -1039,21 +1072,13 @@ void P2PTransportChannel::SwitchBestConnectionTo(Connection* conn) {
 void P2PTransportChannel::UpdateChannelState() {
   // The Handle* functions already set the writable state.  We'll just double-
   // check it here.
-  bool writable = ((best_connection_ != NULL)  &&
-      (best_connection_->write_state() ==
-      Connection::STATE_WRITABLE));
+  bool writable = best_connection_ && best_connection_->writable();
   ASSERT(writable == this->writable());
   if (writable != this->writable())
     LOG(LS_ERROR) << "UpdateChannelState: writable state mismatch";
 
-  bool readable = false;
-  for (uint32 i = 0; i < connections_.size(); ++i) {
-    if (connections_[i]->read_state() == Connection::STATE_READABLE) {
-      readable = true;
-      break;
-    }
-  }
-  set_readable(readable);
+  bool receiving = best_connection_ && best_connection_->receiving();

[pthatcher1, 2015/09/17 05:58:42]

Shouldn't the logic here be the same as the previous logic:  if *any* connection
is receiving, the channel is?

[honghaiz3, 2015/09/17 19:47:56]

Done. 
I was thinking that we should use the best_connection's receiving to decide
certain actions (notify upper layer and switch to the fast ping mode) but I also
see the reasons for checking any connection.

+  set_receiving(receiving);
 }
 
 // We checked the status of our connections and we had at least one that
@@ -1109,11 +1134,8 @@ void P2PTransportChannel::OnMessage(rtc::Message *pmsg) {
     case MSG_SORT:
       OnSort();
       break;
-    case MSG_PING:
-      OnPing();
-      break;
-    case MSG_CHECK_RECEIVING:
-      OnCheckReceiving();
+    case MSG_CHECK_AND_PING:
+      OnCheckAndPing();
       break;
     default:
       ASSERT(false);
@@ -1127,33 +1149,20 @@ void P2PTransportChannel::OnSort() {
   SortConnections();
 }
 
-// Handle queued up ping request
-void P2PTransportChannel::OnPing() {
-  // Make sure the states of the connections are up-to-date (since this affects
-  // which ones are pingable).
+void P2PTransportChannel::OnCheckAndPing() {
   UpdateConnectionStates();
-
-  // Find the oldest pingable connection and have it do a ping.
-  Connection* conn = FindNextPingableConnection();
-  if (conn)
-    PingConnection(conn);
-
-  // Post ourselves a message to perform the next ping.
-  uint32 delay = writable() ? WRITABLE_DELAY : UNWRITABLE_DELAY;
-  thread()->PostDelayed(delay, this, MSG_PING);
-}
-
-void P2PTransportChannel::OnCheckReceiving() {
-  // Check receiving only if the best connection has received data packets
-  // because we want to detect not receiving any packets only after the media
-  // have started flowing.
-  if (best_connection_ && best_connection_->recv_total_bytes() > 0) {
-    bool receiving = rtc::Time() <=
-        best_connection_->last_received() + receiving_timeout_;
-    set_receiving(receiving);
+  // When the best connection is not receiving or writable, switch to short ping
+  // delay.

[pthatcher1, 2015/09/17 05:58:41]

not receiving or writable => not receiving or not writable

[honghaiz3, 2015/09/17 19:47:56]

Done with neither nor. 

+  ping_delay_ = writable() && receiving() ? LONG_PING_DELAY : SHORT_PING_DELAY;

[pthatcher1, 2015/09/17 05:58:41]

We might even consider making a method 

bool weak() { return !writable() && !receiving(); }

Then you'd have delay = weak() ? WEAK_CONNECTION_PING_DELAY :
STRONG_CONNECTION_PING_DELAY;

[honghaiz3, 2015/09/17 19:47:56]

Done.

+  uint32 now = rtc::Time();
+  if (now >= last_ping_sent_ + ping_delay_) {

[pthatcher1, 2015/09/17 05:58:41]

Might be more readable as:

uint32 time_since_last_ping = rtc::Time() - last_ping_sent_;
if (time_since_last_ping > delay) {
  ...
}

[honghaiz3, 2015/09/17 19:47:55]

Similar to the other comments. I try to avoid subtraction on two unsigned
number.  

+    Connection* conn = FindNextPingableConnection();
+    if (conn) {
+      PingConnection(conn);
+    }
   }
-
-  thread()->PostDelayed(check_receiving_delay_, this, MSG_CHECK_RECEIVING);
+  uint32 check_delay = std::min(ping_delay_, check_receiving_delay_);
+  thread()->PostDelayed(check_delay, this, MSG_CHECK_AND_PING);

[pthatcher1, 2015/09/17 05:58:41]

I think this was more clear before with two "loops" running.   Was there a good
reason to combine them?

[honghaiz3, 2015/09/17 19:47:55]

For the two reasons I mentioned at the top. 
If you still feel better to use two loops, I will change it back. 

 }
 
 // Is the connection in a state for us to even consider pinging the other side?
@@ -1175,19 +1184,14 @@ bool P2PTransportChannel::IsPingable(Connection* conn) {
     return false;
   }
 

[pthatcher1, 2015/09/17 05:58:41]

Might be more readable as:

// If the channel is weak, then ping pruned candidates.  Otherwise, don't.
return weak() || !conn->pruned();

[honghaiz3, 2015/09/17 19:47:56]

Done.

-  if (writable()) {
-    // If we are writable, then we only want to ping connections that could be
-    // better than this one, i.e., the ones that were not pruned.
-    return (conn->write_state() != Connection::STATE_WRITE_TIMEOUT);
-  } else {
-    // If we are not writable, then we need to try everything that might work.
-    // This includes both connections that do not have write timeout as well as
-    // ones that do not have read timeout.  A connection could be readable but
-    // be in write-timeout if we pruned it before.  Since the other side is
-    // still pinging it, it very well might still work.
-    return (conn->write_state() != Connection::STATE_WRITE_TIMEOUT) ||
-           (conn->read_state() != Connection::STATE_READ_TIMEOUT);
+  if (writable() && receiving()) {
+    // If we are writable and also receiving, then we only want to ping
+    // connections that could be better than this one, i.e., the ones that
+    // were not pruned.
+    return !conn->pruned();
   }
+  // If we are not writable, then we need to try everything that might work.
+  return true;
 }
 
 // Returns the next pingable connection to ping.  This will be the oldest
@@ -1197,9 +1201,14 @@ bool P2PTransportChannel::IsPingable(Connection* conn) {
 // reconnecting. The newly created connection should be selected as the ping
 // target to become writable instead. See the big comment in CompareConnections.
 Connection* P2PTransportChannel::FindNextPingableConnection() {
+  if (next_connection_to_ping_ && !next_connection_to_ping_->writable()) {
+    Connection* conn = next_connection_to_ping_;
+    next_connection_to_ping_ = nullptr;
+    return conn;
+  }
   uint32 now = rtc::Time();
   if (best_connection_ && best_connection_->connected() &&
-      (best_connection_->write_state() == Connection::STATE_WRITABLE) &&
+      best_connection_->writable() && best_connection_->receiving() &&
       (best_connection_->last_ping_sent() + MAX_CURRENT_WRITABLE_DELAY <=
        now)) {
     return best_connection_;
@@ -1260,7 +1269,8 @@ void P2PTransportChannel::PingConnection(Connection* conn) {
     use_candidate = best_connection_->writable();
   }
   conn->set_use_candidate_attr(use_candidate);
-  conn->Ping(rtc::Time());
+  last_ping_sent_ = rtc::Time();
+  conn->Ping(last_ping_sent_);
 }
 
 // When a connection's state changes, we need to figure out who to use as