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 <hta@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#32211}

pc/channel.cc

@@ -380,6 +380,18 @@ void BaseChannel::OnNetworkRouteChanged(
   });
 }
 
+sigslot::signal1<ChannelInterface*>& BaseChannel::SignalFirstPacketReceived() {
+  RTC_DCHECK_RUN_ON(signaling_thread_);
+  return SignalFirstPacketReceived_;
+}
+
+sigslot::signal1<const rtc::SentPacket&>& 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<void>(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<void>(RTC_FROM_HERE, worker_thread_,
                              [this, sent_packet] {
                                RTC_DCHECK_RUN_ON(worker_thread());
-                               SignalSentPacket(sent_packet);
+                               SignalSentPacket()(sent_packet);
                              });
 }
 

pc/channel.h

@@ -145,22 +145,11 @@ class BaseChannel : public ChannelInterface,
     return remote_streams_;
   }
 
-  sigslot::signal2<BaseChannel*, bool> SignalDtlsSrtpSetupFailure;
-  void SignalDtlsSrtpSetupFailure_n(bool rtcp);
-  void SignalDtlsSrtpSetupFailure_s(bool rtcp);
-
   // Used for latency measurements.
-  sigslot::signal1<ChannelInterface*>& SignalFirstPacketReceived() override {
-    return SignalFirstPacketReceived_;
-  }
+  sigslot::signal1<ChannelInterface*>& SignalFirstPacketReceived() override;
 
   // Forward SignalSentPacket to worker thread.
-  sigslot::signal1<const rtc::SentPacket&> SignalSentPacket;
-
-  // Emitted whenever rtcp-mux is fully negotiated and the rtcp-transport can
-  // be destroyed.
-  // Fired on the network thread.
-  sigslot::signal1<const std::string&> SignalRtcpMuxFullyActive;
+  sigslot::signal1<const rtc::SentPacket&>& 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<ChannelInterface*> SignalFirstPacketReceived_;
+  sigslot::signal1<ChannelInterface*> SignalFirstPacketReceived_
+      RTC_GUARDED_BY(signaling_thread_);
+  sigslot::signal1<const rtc::SentPacket&> SignalSentPacket_
+      RTC_GUARDED_BY(worker_thread_);
 
   const std::string content_name_;
 

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<T>::OnRtcpMuxFullyActive1);
-    channel2_->SignalRtcpMuxFullyActive.connect(
-        this, &ChannelTest<T>::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_;
 };

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);

pc/peer_connection.h

@@ -920,7 +920,6 @@ class PeerConnection : public PeerConnectionInternal,
   // WebRTCSessionDescriptionFactory. Should happen before setLocalDescription.
   void OnCertificateReady(
       const rtc::scoped_refptr<rtc::RTCCertificate>& certificate);
-  void OnDtlsSrtpSetupFailure(cricket::BaseChannel*, bool rtcp);
 
   // Non-const versions of local_description()/remote_description(), for use
   // internally.