Make Ice Transports signal failures.
The new iceTransportState depends on the transports to signal when they have disconnected, this change ensures that they do so.
The logic is similar to what the old iceConnectionState did, but it uses the ice transports writable() flag instead of the one from the containing dtls transport.
Bug: webrtc:10199, webrtc:9308
Change-Id: I8a2a71a689b2a7027fe9117c79144811367d2165
Reviewed-on: https://webrtc-review.googlesource.com/c/117565
Commit-Queue: Jonas Olsson <jonasolsson@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26269}diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc
index f1c8d1a..b12bd0c 100644
--- a/p2p/base/p2p_transport_channel.cc
+++ b/p2p/base/p2p_transport_channel.cc
@@ -394,8 +394,6 @@
// Compute the current RTCIceTransportState as described in
// https://www.w3.org/TR/webrtc/#dom-rtcicetransportstate
-// TODO(bugs.webrtc.org/9308): Return IceTransportState::kDisconnected when it
-// makes sense.
// TODO(bugs.webrtc.org/9218): Avoid prematurely signalling kFailed once we have
// implemented end-of-candidates signalling.
webrtc::IceTransportState P2PTransportChannel::ComputeIceTransportState()
@@ -408,6 +406,13 @@
}
}
+ if (!writable() && has_been_writable_) {
+ if (has_connection)
+ return webrtc::IceTransportState::kDisconnected;
+ else
+ return webrtc::IceTransportState::kFailed;
+ }
+
switch (gathering_state_) {
case kIceGatheringComplete:
if (has_connection)
@@ -1878,6 +1883,23 @@
// change, it should be called after all the connection states have changed. For
// example, we call this at the end of SortConnectionsAndUpdateState.
void P2PTransportChannel::UpdateState() {
+ // If our selected connection is "presumed writable" (TURN-TURN with no
+ // CreatePermission required), act like we're already writable to the upper
+ // layers, so they can start media quicker.
+ bool writable =
+ selected_connection_ && (selected_connection_->writable() ||
+ PresumedWritable(selected_connection_));
+ SetWritable(writable);
+
+ bool receiving = false;
+ for (const Connection* connection : connections_) {
+ if (connection->receiving()) {
+ receiving = true;
+ break;
+ }
+ }
+ SetReceiving(receiving);
+
IceTransportState state = ComputeState();
webrtc::IceTransportState current_standardized_state =
ComputeIceTransportState();
@@ -1927,22 +1949,6 @@
standardized_state_ = current_standardized_state;
SignalIceTransportStateChanged(this);
}
- // If our selected connection is "presumed writable" (TURN-TURN with no
- // CreatePermission required), act like we're already writable to the upper
- // layers, so they can start media quicker.
- bool writable =
- selected_connection_ && (selected_connection_->writable() ||
- PresumedWritable(selected_connection_));
- set_writable(writable);
-
- bool receiving = false;
- for (const Connection* connection : connections_) {
- if (connection->receiving()) {
- receiving = true;
- break;
- }
- }
- set_receiving(receiving);
}
void P2PTransportChannel::MaybeStopPortAllocatorSessions() {
@@ -2490,7 +2496,7 @@
}));
}
-void P2PTransportChannel::set_writable(bool writable) {
+void P2PTransportChannel::SetWritable(bool writable) {
if (writable_ == writable) {
return;
}
@@ -2498,12 +2504,13 @@
<< ": Changed writable_ to " << writable;
writable_ = writable;
if (writable_) {
+ has_been_writable_ = true;
SignalReadyToSend(this);
}
SignalWritableState(this);
}
-void P2PTransportChannel::set_receiving(bool receiving) {
+void P2PTransportChannel::SetReceiving(bool receiving) {
if (receiving_ == receiving) {
return;
}
diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h
index 5e15f76..f9b4c7dc 100644
--- a/p2p/base/p2p_transport_channel.h
+++ b/p2p/base/p2p_transport_channel.h
@@ -370,9 +370,9 @@
bool IsRemoteCandidatePruned(const Candidate& cand) const;
// Sets the writable state, signaling if necessary.
- void set_writable(bool writable);
+ void SetWritable(bool writable);
// Sets the receiving state, signaling if necessary.
- void set_receiving(bool receiving);
+ void SetReceiving(bool receiving);
std::string transport_name_;
int component_;
@@ -429,6 +429,7 @@
uint32_t nomination_ = 0;
bool receiving_ = false;
bool writable_ = false;
+ bool has_been_writable_ = false; // if writable_ has ever been true
rtc::AsyncInvoker invoker_;
absl::optional<rtc::NetworkRoute> network_route_;
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index 5163e01..dc05073 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -457,6 +457,8 @@
this, &JsepTransportController::OnTransportRoleConflict_n);
dtls->ice_transport()->SignalStateChanged.connect(
this, &JsepTransportController::OnTransportStateChanged_n);
+ dtls->ice_transport()->SignalIceTransportStateChanged.connect(
+ this, &JsepTransportController::OnTransportStateChanged_n);
return dtls;
}
diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc
index d34c4a9..018140d 100644
--- a/pc/jsep_transport_controller_unittest.cc
+++ b/pc/jsep_transport_controller_unittest.cc
@@ -944,7 +944,7 @@
cricket::IceTransportState::STATE_COMPLETED);
fake_video_dtls->SetWritable(true);
EXPECT_EQ_WAIT(cricket::kIceConnectionCompleted, connection_state_, kTimeout);
- EXPECT_EQ(2, connection_state_signal_count_);
+ EXPECT_EQ(3, connection_state_signal_count_);
EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
ice_connection_state_, kTimeout);
EXPECT_EQ(3, ice_connection_state_signal_count_);
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index 84c5c88..1698eac 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -512,6 +512,11 @@
return pc()->ice_connection_state();
}
+ webrtc::PeerConnectionInterface::IceConnectionState
+ standardized_ice_connection_state() {
+ return pc()->standardized_ice_connection_state();
+ }
+
webrtc::PeerConnectionInterface::IceGatheringState ice_gathering_state() {
return pc()->ice_gathering_state();
}
@@ -3811,6 +3816,8 @@
caller()->ice_gathering_state());
ASSERT_EQ(PeerConnectionInterface::kIceConnectionNew,
caller()->ice_connection_state());
+ ASSERT_EQ(PeerConnectionInterface::kIceConnectionNew,
+ caller()->standardized_ice_connection_state());
// Start the call by creating the offer, setting it as the local description,
// then sending it to the peer who will respond with an answer. This happens
@@ -3818,19 +3825,16 @@
// background.
caller()->CreateAndSetAndSignalOffer();
- ASSERT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
- caller()->ice_connection_state(), kDefaultTimeout);
+ ASSERT_EQ(PeerConnectionInterface::kIceConnectionCompleted,
+ caller()->ice_connection_state());
+ ASSERT_EQ(PeerConnectionInterface::kIceConnectionCompleted,
+ caller()->standardized_ice_connection_state());
// Verify that the observer was notified of the intermediate transitions.
EXPECT_THAT(caller()->ice_connection_state_history(),
ElementsAre(PeerConnectionInterface::kIceConnectionChecking,
PeerConnectionInterface::kIceConnectionConnected,
PeerConnectionInterface::kIceConnectionCompleted));
- // After the ice transport transitions from checking to connected we revert
- // back to new as the standard requires, as at that point the DTLS transport
- // is in the "new" state while no transports are "connecting", "checking",
- // "failed" or disconnected. This is pretty unintuitive, and we might want to
- // amend the spec to handle this case more gracefully.
EXPECT_THAT(
caller()->peer_connection_state_history(),
ElementsAre(PeerConnectionInterface::PeerConnectionState::kConnecting,
@@ -3847,12 +3851,18 @@
RTC_LOG(LS_INFO) << "Firewall rules applied";
ASSERT_EQ_WAIT(PeerConnectionInterface::kIceConnectionDisconnected,
caller()->ice_connection_state(), kDefaultTimeout);
+ ASSERT_EQ_WAIT(PeerConnectionInterface::kIceConnectionDisconnected,
+ caller()->standardized_ice_connection_state(),
+ kDefaultTimeout);
// Let ICE re-establish by removing the firewall rules.
firewall()->ClearRules();
RTC_LOG(LS_INFO) << "Firewall rules cleared";
ASSERT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
caller()->ice_connection_state(), kDefaultTimeout);
+ ASSERT_EQ_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
+ caller()->standardized_ice_connection_state(),
+ kDefaultTimeout);
// According to RFC7675, if there is no response within 30 seconds then the
// peer should consider the other side to have rejected the connection. This
@@ -3864,6 +3874,9 @@
RTC_LOG(LS_INFO) << "Firewall rules applied again";
ASSERT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed,
caller()->ice_connection_state(), kConsentTimeout);
+ ASSERT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed,
+ caller()->standardized_ice_connection_state(),
+ kConsentTimeout);
}
// Tests that the best connection is set to the appropriate IPv4/IPv6 connection