diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index f1c8d1aa98..b12bd0cccc 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -394,8 +394,6 @@ IceTransportState P2PTransportChannel::ComputeState() const { // 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 @@ webrtc::IceTransportState P2PTransportChannel::ComputeIceTransportState() } } + 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 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) { // 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 @@ void P2PTransportChannel::UpdateState() { 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 @@ Connection* P2PTransportChannel::MorePingable(Connection* conn1, })); } -void P2PTransportChannel::set_writable(bool writable) { +void P2PTransportChannel::SetWritable(bool writable) { if (writable_ == writable) { return; } @@ -2498,12 +2504,13 @@ void P2PTransportChannel::set_writable(bool writable) { << ": 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 5e15f76c72..f9b4c7dc93 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -370,9 +370,9 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { 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 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { 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 network_route_; diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 5163e016b3..dc05073c1b 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -457,6 +457,8 @@ JsepTransportController::CreateDtlsTransport(const std::string& transport_name, 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 d34c4a918e..018140d6a7 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/pc/jsep_transport_controller_unittest.cc @@ -944,7 +944,7 @@ TEST_F(JsepTransportControllerTest, SignalConnectionStateComplete) { 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 84c5c886f3..1698eac0f7 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -512,6 +512,11 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver, 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 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyIceStates) { 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 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyIceStates) { // 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 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyIceStates) { 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 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyIceStates) { 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