From 82f132c90acf79790df4dd2ac9142826309aa362 Mon Sep 17 00:00:00 2001 From: Honghai Zhang Date: Wed, 30 Mar 2016 12:55:14 -0700 Subject: [PATCH] Signal ready-to-send when switching to a writable connection. BUG=webrtc:5705 R=pthatcher@webrtc.org Review URL: https://codereview.webrtc.org/1839803006 . Cr-Commit-Position: refs/heads/master@{#12168} --- webrtc/p2p/base/p2ptransportchannel.cc | 8 ++++++++ .../p2p/base/p2ptransportchannel_unittest.cc | 20 ++++++++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/webrtc/p2p/base/p2ptransportchannel.cc b/webrtc/p2p/base/p2ptransportchannel.cc index 8cb1ffe8bc..fbac24768e 100644 --- a/webrtc/p2p/base/p2ptransportchannel.cc +++ b/webrtc/p2p/base/p2ptransportchannel.cc @@ -1163,6 +1163,14 @@ void P2PTransportChannel::SwitchBestConnectionTo(Connection* conn) { LOG_J(LS_INFO, this) << "New best connection: " << best_connection_->ToString(); SignalRouteChange(this, best_connection_->remote_candidate()); + // This is a temporary, but safe fix to webrtc issue 5705. + // TODO(honghaiz): Make all EWOULDBLOCK error routed through the transport + // channel so that it knows whether the media channel is allowed to + // send; then it will only signal ready-to-send if the media channel + // has been disallowed to send. + if (best_connection_->writable()) { + SignalReadyToSend(this); + } } else { LOG_J(LS_INFO, this) << "No best connection"; } diff --git a/webrtc/p2p/base/p2ptransportchannel_unittest.cc b/webrtc/p2p/base/p2ptransportchannel_unittest.cc index a0855c7605..e2ec44e5ab 100644 --- a/webrtc/p2p/base/p2ptransportchannel_unittest.cc +++ b/webrtc/p2p/base/p2ptransportchannel_unittest.cc @@ -1920,6 +1920,8 @@ class P2PTransportChannelPingTest : public testing::Test, ch->SetIceRole(cricket::ICEROLE_CONTROLLING); ch->SetIceCredentials(kIceUfrag[0], kIcePwd[0]); ch->SetRemoteIceCredentials(kIceUfrag[1], kIcePwd[1]); + ch->SignalReadyToSend.connect(this, + &P2PTransportChannelPingTest::OnReadyToSend); } cricket::Candidate CreateHostCandidate(const std::string& ip, @@ -1969,10 +1971,18 @@ class P2PTransportChannelPingTest : public testing::Test, return conn; } + void OnReadyToSend(cricket::TransportChannel* channel) { + channel_ready_to_send_ = true; + } + + bool channel_ready_to_send() { return channel_ready_to_send_; } + void reset_channel_ready_to_send() { channel_ready_to_send_ = false; } + private: rtc::scoped_ptr pss_; rtc::scoped_ptr vss_; rtc::SocketServerScope ss_scope_; + bool channel_ready_to_send_ = false; }; TEST_F(P2PTransportChannelPingTest, TestTriggeredChecks) { @@ -2162,7 +2172,8 @@ TEST_F(P2PTransportChannelPingTest, TestReceivingStateChange) { // The controlled side will select a connection as the "best connection" based // on priority until the controlling side nominates a connection, at which // point the controlled side will select that connection as the -// "best connection". +// "best connection". Plus, the channel will fire SignalReadyToSend if the new +// best connection is writable. TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { cricket::FakePortAllocator pa(rtc::Thread::Current(), nullptr); cricket::P2PTransportChannel ch("receiving state change", 1, &pa); @@ -2174,6 +2185,8 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { cricket::Connection* conn1 = WaitForConnectionTo(&ch, "1.1.1.1", 1); ASSERT_TRUE(conn1 != nullptr); EXPECT_EQ(conn1, ch.best_connection()); + // Channel is not ready because it is not writable. + EXPECT_FALSE(channel_ready_to_send()); // When a higher priority candidate comes in, the new connection is chosen // as the best connection. @@ -2181,6 +2194,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { cricket::Connection* conn2 = WaitForConnectionTo(&ch, "2.2.2.2", 2); ASSERT_TRUE(conn2 != nullptr); EXPECT_EQ(conn2, ch.best_connection()); + EXPECT_FALSE(channel_ready_to_send()); // If a stun request with use-candidate attribute arrives, the receiving // connection will be set as the best connection, even though @@ -2196,6 +2210,7 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { conn3->set_nominated(true); conn3->SignalNominated(conn3); EXPECT_EQ(conn3, ch.best_connection()); + EXPECT_TRUE(channel_ready_to_send()); // Even if another higher priority candidate arrives, // it will not be set as the best connection because the best connection @@ -2210,9 +2225,12 @@ TEST_F(P2PTransportChannelPingTest, TestSelectConnectionBeforeNomination) { conn4->SignalNominated(conn4); // Not switched yet because conn4 is not writable. EXPECT_EQ(conn3, ch.best_connection()); + reset_channel_ready_to_send(); // The best connection switches after conn4 becomes writable. conn4->ReceivedPingResponse(); EXPECT_EQ(conn4, ch.best_connection()); + // SignalReadyToSend is fired again because conn4 is writable. + EXPECT_TRUE(channel_ready_to_send()); } // The controlled side will select a connection as the "best connection" based