diff --git a/webrtc/p2p/base/candidatepairinterface.h b/webrtc/p2p/base/candidatepairinterface.h index 91b47f470a..1d11c22a52 100644 --- a/webrtc/p2p/base/candidatepairinterface.h +++ b/webrtc/p2p/base/candidatepairinterface.h @@ -21,8 +21,6 @@ class CandidatePairInterface { virtual const Candidate& local_candidate() const = 0; virtual const Candidate& remote_candidate() const = 0; - - virtual bool ReadyToSendMedia() const = 0; }; } // namespace cricket diff --git a/webrtc/p2p/base/dtlstransportchannel.cc b/webrtc/p2p/base/dtlstransportchannel.cc index e34dfbd3e2..df6e4d0e09 100644 --- a/webrtc/p2p/base/dtlstransportchannel.cc +++ b/webrtc/p2p/base/dtlstransportchannel.cc @@ -665,10 +665,11 @@ void DtlsTransportChannelWrapper::OnRouteChange( void DtlsTransportChannelWrapper::OnSelectedCandidatePairChanged( TransportChannel* channel, CandidatePairInterface* selected_candidate_pair, - int last_sent_packet_id) { + int last_sent_packet_id, + bool ready_to_send) { ASSERT(channel == channel_); SignalSelectedCandidatePairChanged(this, selected_candidate_pair, - last_sent_packet_id); + last_sent_packet_id, ready_to_send); } void DtlsTransportChannelWrapper::OnChannelStateChanged( diff --git a/webrtc/p2p/base/dtlstransportchannel.h b/webrtc/p2p/base/dtlstransportchannel.h index c17962a7ec..6368bf9819 100644 --- a/webrtc/p2p/base/dtlstransportchannel.h +++ b/webrtc/p2p/base/dtlstransportchannel.h @@ -220,7 +220,8 @@ class DtlsTransportChannelWrapper : public TransportChannelImpl { void OnSelectedCandidatePairChanged( TransportChannel* channel, CandidatePairInterface* selected_candidate_pair, - int last_sent_packet_id); + int last_sent_packet_id, + bool ready_to_send); void OnChannelStateChanged(TransportChannelImpl* channel); void Reconnect(); diff --git a/webrtc/p2p/base/faketransportcontroller.h b/webrtc/p2p/base/faketransportcontroller.h index 59b618a003..7b7aec5908 100644 --- a/webrtc/p2p/base/faketransportcontroller.h +++ b/webrtc/p2p/base/faketransportcontroller.h @@ -476,8 +476,6 @@ class FakeCandidatePair : public CandidatePairInterface { return remote_candidate_; } - bool ReadyToSendMedia() const override { return true; } - private: Candidate local_candidate_; Candidate remote_candidate_; diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 61de2491bf..a4bde05d80 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -888,7 +888,9 @@ int P2PTransportChannel::SendPacket(const char *data, size_t len, error_ = EINVAL; return -1; } - if (selected_connection_ == NULL) { + // If we don't think the connection is working yet, return EWOULDBLOCK + // instead of sending a packet that will probably be dropped. + if (!ReadyToSend()) { error_ = EWOULDBLOCK; return -1; } @@ -1228,7 +1230,7 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn) { LOG_J(LS_INFO, this) << "No selected connection"; } SignalSelectedCandidatePairChanged(this, selected_connection_, - last_sent_packet_id_); + last_sent_packet_id_, ReadyToSend()); } // Warning: UpdateState should eventually be called whenever a connection @@ -1322,6 +1324,17 @@ bool P2PTransportChannel::weak() const { return !selected_connection_ || selected_connection_->weak(); } +bool P2PTransportChannel::ReadyToSend() const { + // Note that we allow sending on an unreliable connection, because it's + // possible that it became unreliable simply due to bad chance. + // So this shouldn't prevent attempting to send media. + return selected_connection_ != nullptr && + (selected_connection_->writable() || + PresumedWritable(selected_connection_) || + selected_connection_->write_state() == + Connection::STATE_WRITE_UNRELIABLE); +} + // If we have a selected connection, return it, otherwise return top one in the // list (later we will mark it best). Connection* P2PTransportChannel::GetBestConnectionOnNetwork( diff --git a/webrtc/p2p/base/p2ptransportchannel.h b/webrtc/p2p/base/p2ptransportchannel.h index a0b3a3ef5f..fc22b2089a 100644 --- a/webrtc/p2p/base/p2ptransportchannel.h +++ b/webrtc/p2p/base/p2ptransportchannel.h @@ -212,6 +212,8 @@ class P2PTransportChannel : public TransportChannelImpl, // A transport channel is weak if the current best connection is either // not receiving or not writable, or if there is no best connection at all. bool weak() const; + // Returns true if it's possible to send packets on this channel. + bool ReadyToSend() const; void UpdateConnectionStates(); void RequestSort(); diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index 98b0b8dae5..c51098ad54 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1698,6 +1698,10 @@ TEST_F(P2PTransportChannelTest, TurnToTurnPresumedWritable) { // it has a TURN-TURN pair. EXPECT_TRUE(ep1_ch1()->writable()); EXPECT_TRUE(GetEndpoint(0)->ready_to_send_); + // Also make sure we can immediately send packets. + const char* data = "test"; + int len = static_cast(strlen(data)); + EXPECT_EQ(len, SendData(ep1_ch1(), data, len)); } // Test that a TURN/peer reflexive candidate pair is also presumed writable. @@ -2233,7 +2237,8 @@ class P2PTransportChannelPingTest : public testing::Test, void OnSelectedCandidatePairChanged( TransportChannel* transport_channel, CandidatePairInterface* selected_candidate_pair, - int last_sent_packet_id) { + int last_sent_packet_id, + bool ready_to_send) { last_selected_candidate_pair_ = selected_candidate_pair; last_sent_packet_id_ = last_sent_packet_id; ++selected_candidate_pair_switches_; @@ -2639,7 +2644,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { int last_packet_id = 0; const char* data = "ABCDEFGH"; int len = static_cast(strlen(data)); - SendData(ch, data, len, ++last_packet_id); + EXPECT_EQ(-1, SendData(ch, data, len, ++last_packet_id)); // When a higher priority candidate comes in, the new connection is chosen // as the selected connection. ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "2.2.2.2", 2, 10)); @@ -2647,13 +2652,13 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { ASSERT_TRUE(conn2 != nullptr); EXPECT_EQ(conn2, ch.selected_connection()); EXPECT_EQ(conn2, last_selected_candidate_pair()); - EXPECT_EQ(last_packet_id, last_sent_packet_id()); + EXPECT_EQ(-1, last_sent_packet_id()); EXPECT_FALSE(channel_ready_to_send()); // If a stun request with use-candidate attribute arrives, the receiving // connection will be set as the selected connection, even though // its priority is lower. - SendData(ch, data, len, ++last_packet_id); + EXPECT_EQ(-1, SendData(ch, data, len, ++last_packet_id)); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "3.3.3.3", 3, 1)); Connection* conn3 = WaitForConnectionTo(&ch, "3.3.3.3", 3); ASSERT_TRUE(conn3 != nullptr); @@ -2666,13 +2671,13 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { conn3->SignalNominated(conn3); EXPECT_EQ(conn3, ch.selected_connection()); EXPECT_EQ(conn3, last_selected_candidate_pair()); - EXPECT_EQ(last_packet_id, last_sent_packet_id()); + EXPECT_EQ(-1, last_sent_packet_id()); EXPECT_TRUE(channel_ready_to_send()); // Even if another higher priority candidate arrives, it will not be set as // the selected connection because the selected connection is nominated by // the controlling side. - SendData(ch, data, len, ++last_packet_id); + EXPECT_EQ(len, SendData(ch, data, len, ++last_packet_id)); ch.AddRemoteCandidate(CreateUdpCandidate(LOCAL_PORT_TYPE, "4.4.4.4", 4, 100)); Connection* conn4 = WaitForConnectionTo(&ch, "4.4.4.4", 4); ASSERT_TRUE(conn4 != nullptr); diff --git a/webrtc/p2p/base/port.cc b/webrtc/p2p/base/port.cc index ab7ba09ea5..55d6b876dc 100644 --- a/webrtc/p2p/base/port.cc +++ b/webrtc/p2p/base/port.cc @@ -1525,10 +1525,6 @@ ProxyConnection::ProxyConnection(Port* port, int ProxyConnection::Send(const void* data, size_t size, const rtc::PacketOptions& options) { - if (!ReadyToSendMedia()) { - error_ = EWOULDBLOCK; - return SOCKET_ERROR; - } stats_.sent_total_packets++; int sent = port_->SendTo(data, size, remote_candidate_.address(), options, true); diff --git a/webrtc/p2p/base/port.h b/webrtc/p2p/base/port.h index 5532239656..53056c1569 100644 --- a/webrtc/p2p/base/port.h +++ b/webrtc/p2p/base/port.h @@ -461,10 +461,6 @@ class Connection : public CandidatePairInterface, bool active() const { return write_state_ != STATE_WRITE_TIMEOUT; } - virtual bool ReadyToSendMedia() const { - return write_state_ == STATE_WRITABLE || - write_state_ == STATE_WRITE_UNRELIABLE; - } // A connection is dead if it can be safely deleted. bool dead(int64_t now) const; diff --git a/webrtc/p2p/base/port_unittest.cc b/webrtc/p2p/base/port_unittest.cc index a345e82b61..743cddacdb 100644 --- a/webrtc/p2p/base/port_unittest.cc +++ b/webrtc/p2p/base/port_unittest.cc @@ -2424,11 +2424,11 @@ TEST_F(PortTest, TestWritableState) { ch1.Ping(); WAIT(!ch2.remote_address().IsNil(), kTimeout); - // Data should be unsendable until the connection is accepted. + // Data should be sendable before the connection is accepted. char data[] = "abcd"; int data_size = arraysize(data); rtc::PacketOptions options; - EXPECT_EQ(SOCKET_ERROR, ch1.conn()->Send(data, data_size, options)); + EXPECT_EQ(data_size, ch1.conn()->Send(data, data_size, options)); // Accept the connection to return the binding response, transition to // writable, and allow data to be sent. @@ -2464,8 +2464,9 @@ TEST_F(PortTest, TestWritableState) { 500u); EXPECT_EQ(Connection::STATE_WRITE_TIMEOUT, ch1.conn()->write_state()); - // Now that the connection has completely timed out, data send should fail. - EXPECT_EQ(SOCKET_ERROR, ch1.conn()->Send(data, data_size, options)); + // Even if the connection has timed out, the Connection shouldn't block + // the sending of data. + EXPECT_EQ(data_size, ch1.conn()->Send(data, data_size, options)); ch1.Stop(); ch2.Stop(); diff --git a/webrtc/p2p/base/transportchannel.h b/webrtc/p2p/base/transportchannel.h index 87ed9fdea3..411591c849 100644 --- a/webrtc/p2p/base/transportchannel.h +++ b/webrtc/p2p/base/transportchannel.h @@ -160,7 +160,9 @@ class TransportChannel : public sigslot::has_slots<> { // The first parameter is the transport channel that signals the event. // The second parameter is the new selected candidate pair. The third // parameter is the last packet id sent on the previous candidate pair. - sigslot::signal3 + // The fourth parameter is a boolean which is true if the TransportChannel + // is ready to send with this candidate pair. + sigslot::signal4 SignalSelectedCandidatePairChanged; // Invoked when the channel is being destroyed. diff --git a/webrtc/p2p/base/turnport_unittest.cc b/webrtc/p2p/base/turnport_unittest.cc index ef678b5d73..e82f10f51d 100644 --- a/webrtc/p2p/base/turnport_unittest.cc +++ b/webrtc/p2p/base/turnport_unittest.cc @@ -1019,12 +1019,12 @@ TEST_F(TurnPortTest, TestChannelBindGetErrorResponse) { EXPECT_TRUE_SIMULATED_WAIT(CheckConnectionFailedAndPruned(conn1), kSimulatedRtt, fake_clock_); - // Verify that no packet can be sent after a bind request error. + // Verify that packets are allowed to be sent after a bind request error. + // They'll just use a send indication instead. conn2->SignalReadPacket.connect(static_cast(this), &TurnPortTest::OnUdpReadPacket); conn1->Send(data.data(), data.length(), options); - SIMULATED_WAIT(!udp_packets_.empty(), kSimulatedRtt, fake_clock_); - EXPECT_TRUE(udp_packets_.empty()); + EXPECT_TRUE_SIMULATED_WAIT(!udp_packets_.empty(), kSimulatedRtt, fake_clock_); } // Do a TURN allocation, establish a UDP connection, and send some data. diff --git a/webrtc/p2p/quic/quictransportchannel.cc b/webrtc/p2p/quic/quictransportchannel.cc index 6aafa9e3db..e86fe6a0b2 100644 --- a/webrtc/p2p/quic/quictransportchannel.cc +++ b/webrtc/p2p/quic/quictransportchannel.cc @@ -395,10 +395,10 @@ void QuicTransportChannel::OnRouteChange(TransportChannel* channel, void QuicTransportChannel::OnSelectedCandidatePairChanged( TransportChannel* channel, CandidatePairInterface* selected_candidate_pair, - int last_sent_packet_id) { + int last_sent_packet_id bool ready_to_send) { ASSERT(channel == channel_.get()); SignalSelectedCandidatePairChanged(this, selected_candidate_pair, - last_sent_packet_id); + last_sent_packet_id, ready_to_send); } void QuicTransportChannel::OnChannelStateChanged( diff --git a/webrtc/p2p/quic/quictransportchannel.h b/webrtc/p2p/quic/quictransportchannel.h index 688323f722..22a33eaf0f 100644 --- a/webrtc/p2p/quic/quictransportchannel.h +++ b/webrtc/p2p/quic/quictransportchannel.h @@ -246,7 +246,8 @@ class QuicTransportChannel : public TransportChannelImpl, void OnSelectedCandidatePairChanged( TransportChannel* channel, CandidatePairInterface* selected_candidate_pair, - int last_sent_packet_id); + int last_sent_packet_id, + bool ready_to_send); void OnChannelStateChanged(TransportChannelImpl* channel); // Callbacks for |quic_|. diff --git a/webrtc/pc/channel.cc b/webrtc/pc/channel.cc index 9fe8aefe92..c1909a2829 100644 --- a/webrtc/pc/channel.cc +++ b/webrtc/pc/channel.cc @@ -601,15 +601,15 @@ void BaseChannel::OnDtlsState(TransportChannel* channel, void BaseChannel::OnSelectedCandidatePairChanged( TransportChannel* channel, CandidatePairInterface* selected_candidate_pair, - int last_sent_packet_id) { + int last_sent_packet_id, + bool ready_to_send) { ASSERT(channel == transport_channel_ || channel == rtcp_transport_channel_); RTC_DCHECK(network_thread_->IsCurrent()); std::string transport_name = channel->transport_name(); rtc::NetworkRoute network_route; if (selected_candidate_pair) { network_route = rtc::NetworkRoute( - selected_candidate_pair->ReadyToSendMedia(), - selected_candidate_pair->local_candidate().network_id(), + ready_to_send, selected_candidate_pair->local_candidate().network_id(), selected_candidate_pair->remote_candidate().network_id(), last_sent_packet_id); } diff --git a/webrtc/pc/channel.h b/webrtc/pc/channel.h index 211f810b85..37eee47c79 100644 --- a/webrtc/pc/channel.h +++ b/webrtc/pc/channel.h @@ -222,7 +222,8 @@ class BaseChannel void OnSelectedCandidatePairChanged( TransportChannel* channel, CandidatePairInterface* selected_candidate_pair, - int last_sent_packet_id); + int last_sent_packet_id, + bool ready_to_send); bool PacketIsRtcp(const TransportChannel* channel, const char* data, size_t len); diff --git a/webrtc/pc/channel_unittest.cc b/webrtc/pc/channel_unittest.cc index c57ac76257..492c7d548c 100644 --- a/webrtc/pc/channel_unittest.cc +++ b/webrtc/pc/channel_unittest.cc @@ -915,8 +915,8 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { media_channel1->set_num_network_route_changes(0); network_thread_->Invoke(RTC_FROM_HERE, [transport_channel1] { // The transport channel becomes disconnected. - transport_channel1->SignalSelectedCandidatePairChanged(transport_channel1, - nullptr, -1); + transport_channel1->SignalSelectedCandidatePairChanged( + transport_channel1, nullptr, -1, false); }); WaitForThreads(); EXPECT_EQ(1, media_channel1->num_network_route_changes()); @@ -933,7 +933,7 @@ class ChannelTest : public testing::Test, public sigslot::has_slots<> { transport_controller1_->CreateFakeCandidatePair( local_address, kLocalNetId, remote_address, kRemoteNetId)); transport_channel1->SignalSelectedCandidatePairChanged( - transport_channel1, candidate_pair.get(), kLastPacketId); + transport_channel1, candidate_pair.get(), kLastPacketId, true); }); WaitForThreads(); EXPECT_EQ(1, media_channel1->num_network_route_changes());