Update connection states to match spec changes.
These changes simplify the code, and also fix the issue where the peerconnectionstate would sometimes return to "new" during connection setup.
Bug: webrtc:9308
Change-Id: I895cd2f94a2b9688c821cca64d1a077317b99d44
Reviewed-on: https://webrtc-review.googlesource.com/c/111964
Reviewed-by: Magnus Jedvert <magjed@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Jonas Olsson <jonasolsson@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25942}diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn
index 8611a65..ce390bc 100644
--- a/p2p/BUILD.gn
+++ b/p2p/BUILD.gn
@@ -149,6 +149,7 @@
"../rtc_base/third_party/sigslot",
"../test:test_support",
"//third_party/abseil-cpp/absl/memory",
+ "//third_party/abseil-cpp/absl/types:optional",
]
}
diff --git a/p2p/base/fakeicetransport.h b/p2p/base/fakeicetransport.h
index 88afc38..c36dd7f 100644
--- a/p2p/base/fakeicetransport.h
+++ b/p2p/base/fakeicetransport.h
@@ -15,6 +15,7 @@
#include <string>
#include <utility>
+#include "absl/types/optional.h"
#include "p2p/base/icetransportinternal.h"
#include "rtc_base/asyncinvoker.h"
#include "rtc_base/copyonwritebuffer.h"
@@ -69,6 +70,13 @@
}
}
+ void SetTransportState(webrtc::IceTransportState state,
+ IceTransportState legacy_state) {
+ transport_state_ = state;
+ legacy_transport_state_ = legacy_state;
+ SignalIceTransportStateChanged(this);
+ }
+
void SetConnectionCount(size_t connection_count) {
size_t old_connection_count = connection_count_;
connection_count_ = connection_count;
@@ -107,6 +115,10 @@
const std::string& remote_ice_pwd() const { return remote_ice_pwd_; }
IceTransportState GetState() const override {
+ if (legacy_transport_state_) {
+ return *legacy_transport_state_;
+ }
+
if (connection_count_ == 0) {
return had_connection_ ? IceTransportState::STATE_FAILED
: IceTransportState::STATE_INIT;
@@ -120,6 +132,10 @@
}
webrtc::IceTransportState GetIceTransportState() const override {
+ if (transport_state_) {
+ return *transport_state_;
+ }
+
if (connection_count_ == 0) {
return had_connection_ ? webrtc::IceTransportState::kFailed
: webrtc::IceTransportState::kNew;
@@ -293,6 +309,8 @@
std::string remote_ice_pwd_;
IceMode remote_ice_mode_ = ICEMODE_FULL;
size_t connection_count_ = 0;
+ absl::optional<webrtc::IceTransportState> transport_state_;
+ absl::optional<IceTransportState> legacy_transport_state_;
IceGatheringState gathering_state_ = kIceGatheringNew;
bool had_connection_ = false;
bool writable_ = false;
diff --git a/pc/jseptransportcontroller.cc b/pc/jseptransportcontroller.cc
index 03899c3..ea2451d 100644
--- a/pc/jseptransportcontroller.cc
+++ b/pc/jseptransportcontroller.cc
@@ -1368,36 +1368,30 @@
int total_ice = dtls_transports.size();
if (total_ice_failed > 0) {
- // Any of the RTCIceTransports are in the "failed" state.
+ // Any RTCIceTransports are in the "failed" state.
new_ice_connection_state = PeerConnectionInterface::kIceConnectionFailed;
} else if (total_ice_disconnected > 0) {
- // Any of the RTCIceTransports are in the "disconnected" state and none of
- // them are in the "failed" state.
+ // None of the previous states apply and any RTCIceTransports are in the
+ // "disconnected" state.
new_ice_connection_state =
PeerConnectionInterface::kIceConnectionDisconnected;
- } else if (total_ice_checking > 0) {
- // Any of the RTCIceTransports are in the "checking" state and none of them
- // are in the "disconnected" or "failed" state.
+ } else if (total_ice_new + total_ice_closed == total_ice) {
+ // None of the previous states apply and all RTCIceTransports are in the
+ // "new" or "closed" state, or there are no transports.
+ new_ice_connection_state = PeerConnectionInterface::kIceConnectionNew;
+ } else if (total_ice_new + total_ice_checking > 0) {
+ // None of the previous states apply and any RTCIceTransports are in the
+ // "new" or "checking" state.
new_ice_connection_state = PeerConnectionInterface::kIceConnectionChecking;
- } else if (total_ice_completed + total_ice_closed == total_ice &&
- total_ice_completed > 0) {
- // All RTCIceTransports are in the "completed" or "closed" state and at
- // least one of them is in the "completed" state.
+ } else if (total_ice_completed + total_ice_closed == total_ice) {
+ // None of the previous states apply and all RTCIceTransports are in the
+ // "completed" or "closed" state.
new_ice_connection_state = PeerConnectionInterface::kIceConnectionCompleted;
} else if (total_ice_connected + total_ice_completed + total_ice_closed ==
- total_ice &&
- total_ice_connected > 0) {
- // All RTCIceTransports are in the "connected", "completed" or "closed"
- // state and at least one of them is in the "connected" state.
+ total_ice) {
+ // None of the previous states apply and all RTCIceTransports are in the
+ // "connected", "completed" or "closed" state.
new_ice_connection_state = PeerConnectionInterface::kIceConnectionConnected;
- } else if ((total_ice_new > 0 &&
- total_ice_checking + total_ice_disconnected + total_ice_failed ==
- 0) ||
- total_ice == total_ice_closed) {
- // Any of the RTCIceTransports are in the "new" state and none of them are
- // in the "checking", "disconnected" or "failed" state, or all
- // RTCIceTransports are in the "closed" state, or there are no transports.
- new_ice_connection_state = PeerConnectionInterface::kIceConnectionNew;
} else {
RTC_NOTREACHED();
}
@@ -1431,38 +1425,27 @@
if (total_failed > 0) {
// Any of the RTCIceTransports or RTCDtlsTransports are in a "failed" state.
new_combined_state = PeerConnectionInterface::PeerConnectionState::kFailed;
- } else if (total_ice_disconnected > 0 &&
- total_dtls_connecting + total_ice_checking == 0) {
- // Any of the RTCIceTransports or RTCDtlsTransports are in the
- // "disconnected" state and none of them are in the "failed" or "connecting"
- // or "checking" state.
+ } else if (total_ice_disconnected > 0) {
+ // None of the previous states apply and any RTCIceTransports or
+ // RTCDtlsTransports are in the "disconnected" state.
new_combined_state =
PeerConnectionInterface::PeerConnectionState::kDisconnected;
- } else if (total_dtls_connecting + total_ice_checking > 0) {
- // Any of the RTCIceTransports or RTCDtlsTransports are in the "connecting"
- // or "checking" state and none of them is in the "failed" state.
+ } else if (total_new + total_closed == total_transports) {
+ // None of the previous states apply and all RTCIceTransports and
+ // RTCDtlsTransports are in the "new" or "closed" state, or there are no
+ // transports.
+ new_combined_state = PeerConnectionInterface::PeerConnectionState::kNew;
+ } else if (total_new + total_dtls_connecting + total_ice_checking > 0) {
+ // None of the previous states apply and all RTCIceTransports or
+ // RTCDtlsTransports are in the "new", "connecting" or "checking" state.
new_combined_state =
PeerConnectionInterface::PeerConnectionState::kConnecting;
} else if (total_connected + total_ice_completed + total_closed ==
- total_transports &&
- total_connected + total_ice_completed > 0) {
- // All RTCIceTransports and RTCDtlsTransports are in the "connected",
- // "completed" or "closed" state and at least one of them is in the
- // "connected" or "completed" state.
+ total_transports) {
+ // None of the previous states apply and all RTCIceTransports and
+ // RTCDtlsTransports are in the "connected", "completed" or "closed" state.
new_combined_state =
PeerConnectionInterface::PeerConnectionState::kConnected;
- } else if ((total_new > 0 && total_dtls_connecting + total_ice_checking +
- total_failed + total_ice_disconnected ==
- 0) ||
- total_transports == total_closed) {
- // Any of the RTCIceTransports or RTCDtlsTransports are in the "new" state
- // and none of the transports are in the "connecting", "checking", "failed"
- // or "disconnected" state, or all transports are in the "closed" state, or
- // there are no transports.
- //
- // Note that if none of the other conditions hold this is guaranteed to be
- // true.
- new_combined_state = PeerConnectionInterface::PeerConnectionState::kNew;
} else {
RTC_NOTREACHED();
}
diff --git a/pc/jseptransportcontroller_unittest.cc b/pc/jseptransportcontroller_unittest.cc
index fd8a779..3031b8d 100644
--- a/pc/jseptransportcontroller_unittest.cc
+++ b/pc/jseptransportcontroller_unittest.cc
@@ -273,6 +273,8 @@
void OnCombinedConnectionState(
PeerConnectionInterface::PeerConnectionState state) {
+ RTC_LOG(LS_INFO) << "OnCombinedConnectionState: "
+ << static_cast<int>(state);
if (!signaling_thread_->IsCurrent()) {
signaled_on_non_signaling_thread_ = true;
}
@@ -783,10 +785,10 @@
EXPECT_EQ(1, connection_state_signal_count_);
EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed,
ice_connection_state_, kTimeout);
- EXPECT_EQ(1, ice_connection_state_signal_count_);
+ EXPECT_EQ(2, ice_connection_state_signal_count_);
EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed,
combined_connection_state_, kTimeout);
- EXPECT_EQ(1, combined_connection_state_signal_count_);
+ EXPECT_EQ(2, combined_connection_state_signal_count_);
fake_audio_dtls->SetDtlsState(cricket::DTLS_TRANSPORT_CONNECTED);
fake_video_dtls->SetDtlsState(cricket::DTLS_TRANSPORT_CONNECTED);
@@ -798,10 +800,10 @@
EXPECT_EQ(2, connection_state_signal_count_);
EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionConnected,
ice_connection_state_, kTimeout);
- EXPECT_EQ(2, ice_connection_state_signal_count_);
+ EXPECT_EQ(3, ice_connection_state_signal_count_);
EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected,
combined_connection_state_, kTimeout);
- EXPECT_EQ(2, combined_connection_state_signal_count_);
+ EXPECT_EQ(3, combined_connection_state_signal_count_);
}
TEST_F(JsepTransportControllerTest,
@@ -906,37 +908,48 @@
// First, have one transport connect, and another fail, to ensure that
// the first transport connecting didn't trigger a "connected" state signal.
// We should only get a signal when all are connected.
- fake_audio_dtls->fake_ice_transport()->SetConnectionCount(1);
+ fake_audio_dtls->fake_ice_transport()->SetTransportState(
+ IceTransportState::kCompleted,
+ cricket::IceTransportState::STATE_COMPLETED);
fake_audio_dtls->SetWritable(true);
fake_audio_dtls->fake_ice_transport()->SetCandidatesGatheringComplete();
- // Decrease the number of the connection to trigger the signal.
- fake_video_dtls->fake_ice_transport()->SetConnectionCount(1);
- fake_video_dtls->fake_ice_transport()->SetConnectionCount(0);
+
+ EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionChecking,
+ ice_connection_state_, kTimeout);
+ EXPECT_EQ(1, ice_connection_state_signal_count_);
+ EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnecting,
+ combined_connection_state_, kTimeout);
+ EXPECT_EQ(1, combined_connection_state_signal_count_);
+
+ fake_video_dtls->fake_ice_transport()->SetTransportState(
+ IceTransportState::kFailed, cricket::IceTransportState::STATE_FAILED);
fake_video_dtls->fake_ice_transport()->SetCandidatesGatheringComplete();
EXPECT_EQ_WAIT(cricket::kIceConnectionFailed, connection_state_, kTimeout);
EXPECT_EQ(1, connection_state_signal_count_);
EXPECT_EQ_WAIT(PeerConnectionInterface::kIceConnectionFailed,
ice_connection_state_, kTimeout);
- EXPECT_EQ(1, ice_connection_state_signal_count_);
+ EXPECT_EQ(2, ice_connection_state_signal_count_);
EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kFailed,
combined_connection_state_, kTimeout);
- EXPECT_EQ(1, combined_connection_state_signal_count_);
+ EXPECT_EQ(2, combined_connection_state_signal_count_);
fake_audio_dtls->SetDtlsState(cricket::DTLS_TRANSPORT_CONNECTED);
fake_video_dtls->SetDtlsState(cricket::DTLS_TRANSPORT_CONNECTED);
// Set the connection count to be 1 and the cricket::FakeIceTransport will set
// the transport state to be STATE_COMPLETED.
- fake_video_dtls->fake_ice_transport()->SetConnectionCount(1);
+ fake_video_dtls->fake_ice_transport()->SetTransportState(
+ IceTransportState::kCompleted,
+ 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_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
ice_connection_state_, kTimeout);
- EXPECT_EQ(2, ice_connection_state_signal_count_);
+ EXPECT_EQ(3, ice_connection_state_signal_count_);
EXPECT_EQ_WAIT(PeerConnectionInterface::PeerConnectionState::kConnected,
combined_connection_state_, kTimeout);
- EXPECT_EQ(2, combined_connection_state_signal_count_);
+ EXPECT_EQ(3, combined_connection_state_signal_count_);
}
TEST_F(JsepTransportControllerTest, SignalIceGatheringStateGathering) {
diff --git a/pc/peerconnection_integrationtest.cc b/pc/peerconnection_integrationtest.cc
index f0184f1..fda34fe 100644
--- a/pc/peerconnection_integrationtest.cc
+++ b/pc/peerconnection_integrationtest.cc
@@ -3883,8 +3883,6 @@
EXPECT_THAT(
caller()->peer_connection_state_history(),
ElementsAre(PeerConnectionInterface::PeerConnectionState::kConnecting,
- PeerConnectionInterface::PeerConnectionState::kNew,
- PeerConnectionInterface::PeerConnectionState::kConnecting,
PeerConnectionInterface::PeerConnectionState::kConnected));
EXPECT_THAT(caller()->ice_gathering_state_history(),
ElementsAre(PeerConnectionInterface::kIceGatheringGathering,
diff --git a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java
index 2063199..5855322 100644
--- a/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java
+++ b/sdk/android/instrumentationtests/src/org/webrtc/PeerConnectionTest.java
@@ -836,8 +836,6 @@
offeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING);
offeringExpectations.expectIceConnectionChange(IceConnectionState.CONNECTED);
- offeringExpectations.expectConnectionChange(PeerConnectionState.NEW);
- offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING);
offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTED);
// TODO(bemasc): uncomment once delivery of ICECompleted is reliable
// (https://code.google.com/p/webrtc/issues/detail?id=3021).
@@ -1071,8 +1069,6 @@
offeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING);
offeringExpectations.expectIceConnectionChange(IceConnectionState.CONNECTED);
- offeringExpectations.expectConnectionChange(PeerConnectionState.NEW);
- offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING);
offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTED);
// TODO(bemasc): uncomment once delivery of ICECompleted is reliable
// (https://code.google.com/p/webrtc/issues/detail?id=3021).
@@ -1255,8 +1251,6 @@
offeringExpectations.expectIceConnectionChange(IceConnectionState.CHECKING);
offeringExpectations.expectIceConnectionChange(IceConnectionState.CONNECTED);
- offeringExpectations.expectConnectionChange(PeerConnectionState.NEW);
- offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTING);
offeringExpectations.expectConnectionChange(PeerConnectionState.CONNECTED);
// TODO(bemasc): uncomment once delivery of ICECompleted is reliable
// (https://code.google.com/p/webrtc/issues/detail?id=3021).
