From b2995a1e579ec5937935eb0445f9062a178dfe3c Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Mon, 28 Sep 2020 14:05:35 +0200 Subject: [PATCH] Delete dead signal code in pc/channel.* SignalDtlsSrtpSetupFailure is never fired, so the setup code for it, is dead code. Also removing declarations for methods that have no implementation. For other public signals in BaseChannel I've added an accessor which has revealed a threading problem due to the member variable being public. Bug: webrtc:11994 Change-Id: Iec6046c6a598066b92c956002ba4160708ae7dcc Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/185802 Reviewed-by: Harald Alvestrand Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#32211} --- pc/channel.cc | 15 ++++++++++++++- pc/channel.h | 20 ++++++-------------- pc/channel_unittest.cc | 36 ------------------------------------ pc/peer_connection.cc | 26 +++++--------------------- pc/peer_connection.h | 1 - 5 files changed, 25 insertions(+), 73 deletions(-) diff --git a/pc/channel.cc b/pc/channel.cc index accc233aa1..0f6cdd728b 100644 --- a/pc/channel.cc +++ b/pc/channel.cc @@ -380,6 +380,18 @@ void BaseChannel::OnNetworkRouteChanged( }); } +sigslot::signal1& BaseChannel::SignalFirstPacketReceived() { + RTC_DCHECK_RUN_ON(signaling_thread_); + return SignalFirstPacketReceived_; +} + +sigslot::signal1& BaseChannel::SignalSentPacket() { + // TODO(bugs.webrtc.org/11994): Uncomment this check once callers have been + // fixed to access this variable from the correct thread. + // RTC_DCHECK_RUN_ON(worker_thread_); + return SignalSentPacket_; +} + void BaseChannel::OnTransportReadyToSend(bool ready) { invoker_.AsyncInvoke(RTC_FROM_HERE, worker_thread_, [=] { media_channel_->OnReadyToSend(ready); }); @@ -776,6 +788,7 @@ void BaseChannel::OnMessage(rtc::Message* pmsg) { break; } case MSG_FIRSTPACKETRECEIVED: { + RTC_DCHECK_RUN_ON(signaling_thread_); SignalFirstPacketReceived_(this); break; } @@ -809,7 +822,7 @@ void BaseChannel::SignalSentPacket_n(const rtc::SentPacket& sent_packet) { invoker_.AsyncInvoke(RTC_FROM_HERE, worker_thread_, [this, sent_packet] { RTC_DCHECK_RUN_ON(worker_thread()); - SignalSentPacket(sent_packet); + SignalSentPacket()(sent_packet); }); } diff --git a/pc/channel.h b/pc/channel.h index 15915b4cfe..e99ba2428b 100644 --- a/pc/channel.h +++ b/pc/channel.h @@ -145,22 +145,11 @@ class BaseChannel : public ChannelInterface, return remote_streams_; } - sigslot::signal2 SignalDtlsSrtpSetupFailure; - void SignalDtlsSrtpSetupFailure_n(bool rtcp); - void SignalDtlsSrtpSetupFailure_s(bool rtcp); - // Used for latency measurements. - sigslot::signal1& SignalFirstPacketReceived() override { - return SignalFirstPacketReceived_; - } + sigslot::signal1& SignalFirstPacketReceived() override; // Forward SignalSentPacket to worker thread. - sigslot::signal1 SignalSentPacket; - - // Emitted whenever rtcp-mux is fully negotiated and the rtcp-transport can - // be destroyed. - // Fired on the network thread. - sigslot::signal1 SignalRtcpMuxFullyActive; + sigslot::signal1& SignalSentPacket(); // From RtpTransport - public for testing only void OnTransportReadyToSend(bool ready); @@ -299,7 +288,10 @@ class BaseChannel : public ChannelInterface, rtc::Thread* const network_thread_; rtc::Thread* const signaling_thread_; rtc::AsyncInvoker invoker_; - sigslot::signal1 SignalFirstPacketReceived_; + sigslot::signal1 SignalFirstPacketReceived_ + RTC_GUARDED_BY(signaling_thread_); + sigslot::signal1 SignalSentPacket_ + RTC_GUARDED_BY(worker_thread_); const std::string content_name_; diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index 479340c520..4eef70f4ea 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc @@ -228,10 +228,6 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { rtp_transport1_.get(), flags1); channel2_ = CreateChannel(worker_thread, network_thread_, std::move(ch2), rtp_transport2_.get(), flags2); - channel1_->SignalRtcpMuxFullyActive.connect( - this, &ChannelTest::OnRtcpMuxFullyActive1); - channel2_->SignalRtcpMuxFullyActive.connect( - this, &ChannelTest::OnRtcpMuxFullyActive2); CreateContent(flags1, kPcmuCodec, kH264Codec, &local_media_content1_); CreateContent(flags2, kPcmuCodec, kH264Codec, &local_media_content2_); CopyContent(local_media_content1_, &remote_media_content1_); @@ -490,13 +486,6 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { return false; // overridden in specialized classes } - void OnRtcpMuxFullyActive1(const std::string&) { - rtcp_mux_activated_callbacks1_++; - } - void OnRtcpMuxFullyActive2(const std::string&) { - rtcp_mux_activated_callbacks2_++; - } - cricket::CandidatePairInterface* last_selected_candidate_pair() { return last_selected_candidate_pair_; } @@ -597,29 +586,6 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { EXPECT_TRUE(channel2_->SetRemoteContent(&content, SdpType::kAnswer, NULL)); } - // Test that SetLocalContent and SetRemoteContent properly set RTCP - // mux when a provisional answer is received. - void TestSetContentsRtcpMuxWithPrAnswer() { - CreateChannels(0, 0); - typename T::Content content; - CreateContent(0, kPcmuCodec, kH264Codec, &content); - content.set_rtcp_mux(true); - EXPECT_TRUE(channel1_->SetLocalContent(&content, SdpType::kOffer, NULL)); - EXPECT_TRUE( - channel1_->SetRemoteContent(&content, SdpType::kPrAnswer, NULL)); - // Both sides agree on mux. Should signal RTCP mux as fully activated. - EXPECT_EQ(0, rtcp_mux_activated_callbacks1_); - EXPECT_TRUE(channel1_->SetRemoteContent(&content, SdpType::kAnswer, NULL)); - EXPECT_EQ(1, rtcp_mux_activated_callbacks1_); - // Only initiator supports mux. Should still have a separate RTCP channel. - EXPECT_TRUE(channel2_->SetLocalContent(&content, SdpType::kOffer, NULL)); - content.set_rtcp_mux(false); - EXPECT_TRUE( - channel2_->SetRemoteContent(&content, SdpType::kPrAnswer, NULL)); - EXPECT_TRUE(channel2_->SetRemoteContent(&content, SdpType::kAnswer, NULL)); - EXPECT_EQ(0, rtcp_mux_activated_callbacks2_); - } - // Test that SetLocalContent and SetRemoteContent properly // handles adding and removing StreamParams when the action is a full // SdpType::kOffer / SdpType::kAnswer. @@ -1412,8 +1378,6 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { // The RTP and RTCP packets to send in the tests. rtc::Buffer rtp_packet_; rtc::Buffer rtcp_packet_; - int rtcp_mux_activated_callbacks1_ = 0; - int rtcp_mux_activated_callbacks2_ = 0; cricket::CandidatePairInterface* last_selected_candidate_pair_; rtc::UniqueRandomIdGenerator ssrc_generator_; }; diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index df2fabc93f..4c8a00b1c3 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -100,10 +100,6 @@ const char kSdpWithoutIceUfragPwd[] = "Called with SDP without ice-ufrag and ice-pwd."; const char kSessionError[] = "Session error code: "; const char kSessionErrorDesc[] = "Session error description: "; -const char kDtlsSrtpSetupFailureRtp[] = - "Couldn't set up DTLS-SRTP on RTP channel."; -const char kDtlsSrtpSetupFailureRtcp[] = - "Couldn't set up DTLS-SRTP on RTCP channel."; namespace { @@ -6349,11 +6345,6 @@ void PeerConnection::OnCertificateReady( transport_controller_->SetLocalCertificate(certificate); } -void PeerConnection::OnDtlsSrtpSetupFailure(cricket::BaseChannel*, bool rtcp) { - SetSessionError(SessionError::kTransport, - rtcp ? kDtlsSrtpSetupFailureRtcp : kDtlsSrtpSetupFailureRtp); -} - void PeerConnection::OnTransportControllerConnectionState( cricket::IceConnectionState state) { switch (state) { @@ -6680,10 +6671,8 @@ cricket::VoiceChannel* PeerConnection::CreateVoiceChannel( if (!voice_channel) { return nullptr; } - voice_channel->SignalDtlsSrtpSetupFailure.connect( - this, &PeerConnection::OnDtlsSrtpSetupFailure); - voice_channel->SignalSentPacket.connect(this, - &PeerConnection::OnSentPacket_w); + voice_channel->SignalSentPacket().connect(this, + &PeerConnection::OnSentPacket_w); voice_channel->SetRtpTransport(rtp_transport); return voice_channel; @@ -6704,10 +6693,8 @@ cricket::VideoChannel* PeerConnection::CreateVideoChannel( if (!video_channel) { return nullptr; } - video_channel->SignalDtlsSrtpSetupFailure.connect( - this, &PeerConnection::OnDtlsSrtpSetupFailure); - video_channel->SignalSentPacket.connect(this, - &PeerConnection::OnSentPacket_w); + video_channel->SignalSentPacket().connect(this, + &PeerConnection::OnSentPacket_w); video_channel->SetRtpTransport(rtp_transport); return video_channel; @@ -6737,10 +6724,7 @@ bool PeerConnection::CreateDataChannel(const std::string& mid) { if (!data_channel_controller_.rtp_data_channel()) { return false; } - data_channel_controller_.rtp_data_channel() - ->SignalDtlsSrtpSetupFailure.connect( - this, &PeerConnection::OnDtlsSrtpSetupFailure); - data_channel_controller_.rtp_data_channel()->SignalSentPacket.connect( + data_channel_controller_.rtp_data_channel()->SignalSentPacket().connect( this, &PeerConnection::OnSentPacket_w); data_channel_controller_.rtp_data_channel()->SetRtpTransport( rtp_transport); diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 12775ba232..d92c734608 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -920,7 +920,6 @@ class PeerConnection : public PeerConnectionInternal, // WebRTCSessionDescriptionFactory. Should happen before setLocalDescription. void OnCertificateReady( const rtc::scoped_refptr& certificate); - void OnDtlsSrtpSetupFailure(cricket::BaseChannel*, bool rtcp); // Non-const versions of local_description()/remote_description(), for use // internally.