Remove rtp_ and rtcp_packet_transport() from the RtpTransport interface.

RtpTransportInternal does not need to expose these.  They are only used
by tests and for setting options.  Instead, it can expose a SetRtpOption
and SetRtcpOption to set options relevant to each of its transports.

Also updates tests to work around no longer having access to internals.

This will simplify the composite needed during negotiation of different
RTP transport types, as we no longer need to have composites of both
RtpTransport and PacketTransport.

Bug: webrtc:9719
Change-Id: I91bfa6e95b7aa384d10497f47e7d2483c2e0bef2
No-Try: True
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/138282
Commit-Queue: Bjorn Mellem <mellem@webrtc.org>
Reviewed-by: Anton Sukhanov <sukhanov@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#28066}

pc/channel.cc

@@ -254,8 +254,7 @@ bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) {
 
   rtp_transport_ = rtp_transport;
   if (rtp_transport_) {
-    RTC_DCHECK(rtp_transport_->rtp_packet_transport());
-    transport_name_ = rtp_transport_->rtp_packet_transport()->transport_name();
+    transport_name_ = rtp_transport_->transport_name();
 
     if (!ConnectToRtpTransport()) {
       RTC_LOG(LS_ERROR) << "Failed to connect to the new RtpTransport.";
@@ -266,13 +265,11 @@ bool BaseChannel::SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) {
 
     // Set the cached socket options.
     for (const auto& pair : socket_options_) {
-      rtp_transport_->rtp_packet_transport()->SetOption(pair.first,
-                                                        pair.second);
+      rtp_transport_->SetRtpOption(pair.first, pair.second);
     }
-    if (rtp_transport_->rtcp_packet_transport()) {
+    if (!rtp_transport_->rtcp_mux_enabled()) {
       for (const auto& pair : rtcp_socket_options_) {
-        rtp_transport_->rtp_packet_transport()->SetOption(pair.first,
-                                                          pair.second);
+        rtp_transport_->SetRtcpOption(pair.first, pair.second);
       }
     }
   }
@@ -348,20 +345,17 @@ int BaseChannel::SetOption_n(SocketType type,
                              int value) {
   RTC_DCHECK(network_thread_->IsCurrent());
   RTC_DCHECK(rtp_transport_);
-  rtc::PacketTransportInternal* transport = nullptr;
   switch (type) {
     case ST_RTP:
-      transport = rtp_transport_->rtp_packet_transport();
       socket_options_.push_back(
           std::pair<rtc::Socket::Option, int>(opt, value));
-      break;
+      return rtp_transport_->SetRtpOption(opt, value);
     case ST_RTCP:
-      transport = rtp_transport_->rtcp_packet_transport();
       rtcp_socket_options_.push_back(
           std::pair<rtc::Socket::Option, int>(opt, value));
-      break;
+      return rtp_transport_->SetRtcpOption(opt, value);
   }
-  return transport ? transport->SetOption(opt, value) : -1;
+  return -1;
 }
 
 void BaseChannel::OnWritableState(bool writable) {

pc/channel.h

@@ -120,6 +120,8 @@ class BaseChannel : public ChannelInterface,
   // internally. It would replace the |SetTransports| and its variants.
   bool SetRtpTransport(webrtc::RtpTransportInternal* rtp_transport) override;
 
+  webrtc::RtpTransportInternal* rtp_transport() const { return rtp_transport_; }
+
   // Channel control
   bool SetLocalContent(const MediaContentDescription* content,
                        webrtc::SdpType type,
@@ -154,20 +156,6 @@ class BaseChannel : public ChannelInterface,
   // Fired on the network thread.
   sigslot::signal1<const std::string&> SignalRtcpMuxFullyActive;
 
-  rtc::PacketTransportInternal* rtp_packet_transport() {
-    if (rtp_transport_) {
-      return rtp_transport_->rtp_packet_transport();
-    }
-    return nullptr;
-  }
-
-  rtc::PacketTransportInternal* rtcp_packet_transport() {
-    if (rtp_transport_) {
-      return rtp_transport_->rtcp_packet_transport();
-    }
-    return nullptr;
-  }
-
   // Returns media transport, can be null if media transport is not available.
   webrtc::MediaTransportInterface* media_transport() {
     return media_transport_config_.media_transport;

pc/channel_manager_unittest.cc

@@ -193,8 +193,7 @@ TEST_F(ChannelManagerTest, CreateDestroyChannels) {
 TEST_F(ChannelManagerTest, CreateDestroyChannelsWithMediaTransport) {
   EXPECT_TRUE(cm_->Init());
   auto rtp_transport = CreateDtlsSrtpTransport();
-  auto media_transport =
-      CreateMediaTransport(rtp_transport->rtcp_packet_transport());
+  auto media_transport = CreateMediaTransport(rtp_dtls_transport_.get());
   TestCreateDestroyChannels(
       rtp_transport.get(), webrtc::MediaTransportConfig(media_transport.get()));
 }

pc/channel_unittest.cc

@@ -971,8 +971,8 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
     CreateChannels(RTCP_MUX, RTCP_MUX);
     EXPECT_TRUE(SendInitiate());
     EXPECT_TRUE(SendAccept());
-    EXPECT_EQ(nullptr, channel1_->rtcp_packet_transport());
-    EXPECT_EQ(nullptr, channel2_->rtcp_packet_transport());
+    EXPECT_TRUE(channel1_->rtp_transport()->rtcp_mux_enabled());
+    EXPECT_TRUE(channel2_->rtp_transport()->rtcp_mux_enabled());
     SendRtp1();
     SendRtp2();
     WaitForThreads();
@@ -1000,8 +1000,8 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
     CreateChannels(0, 0);
     EXPECT_TRUE(SendInitiate());
     EXPECT_TRUE(SendAccept());
-    EXPECT_NE(nullptr, channel1_->rtcp_packet_transport());
-    EXPECT_NE(nullptr, channel2_->rtcp_packet_transport());
+    EXPECT_FALSE(channel1_->rtp_transport()->rtcp_mux_enabled());
+    EXPECT_FALSE(channel2_->rtp_transport()->rtcp_mux_enabled());
     SendRtcp1();
     SendRtcp2();
     WaitForThreads();
@@ -1047,8 +1047,8 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
     EXPECT_TRUE(SendProvisionalAnswer());
     EXPECT_TRUE(channel1_->srtp_active());
     EXPECT_TRUE(channel2_->srtp_active());
-    EXPECT_EQ(nullptr, channel1_->rtcp_packet_transport());
-    EXPECT_EQ(nullptr, channel2_->rtcp_packet_transport());
+    EXPECT_TRUE(channel1_->rtp_transport()->rtcp_mux_enabled());
+    EXPECT_TRUE(channel2_->rtp_transport()->rtcp_mux_enabled());
     WaitForThreads();  // Wait for 'sending' flag go through network thread.
     SendCustomRtcp1(kSsrc1);
     SendCustomRtp1(kSsrc1, ++sequence_number1_1);
@@ -1107,8 +1107,8 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
     CreateChannels(RTCP_MUX, RTCP_MUX);
     EXPECT_TRUE(SendInitiate());
     EXPECT_TRUE(SendAccept());
-    EXPECT_EQ(nullptr, channel1_->rtcp_packet_transport());
-    EXPECT_EQ(nullptr, channel2_->rtcp_packet_transport());
+    EXPECT_TRUE(channel1_->rtp_transport()->rtcp_mux_enabled());
+    EXPECT_TRUE(channel2_->rtp_transport()->rtcp_mux_enabled());
     SendRtp1();
     SendRtp2();
     WaitForThreads();
@@ -1343,8 +1343,8 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
     CreateChannels(0, 0);
     EXPECT_TRUE(SendInitiate());
     EXPECT_TRUE(SendAccept());
-    EXPECT_NE(nullptr, channel1_->rtcp_packet_transport());
-    EXPECT_NE(nullptr, channel2_->rtcp_packet_transport());
+    EXPECT_FALSE(channel1_->rtp_transport()->rtcp_mux_enabled());
+    EXPECT_FALSE(channel2_->rtp_transport()->rtcp_mux_enabled());
 
     // Send RTCP1 from a different thread.
     ScopedCallThread send_rtcp([this] { SendRtcp1(); });
@@ -1422,19 +1422,15 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
                          rtc::Socket::Option::OPT_RCVBUF, kRcvBufSize);
 
     new_rtp_transport_ = CreateDtlsSrtpTransport(
-        static_cast<DtlsTransportInternal*>(channel2_->rtp_packet_transport()),
-        static_cast<DtlsTransportInternal*>(
-            channel2_->rtcp_packet_transport()));
+        fake_rtp_dtls_transport2_.get(), fake_rtcp_dtls_transport2_.get());
     channel1_->SetRtpTransport(new_rtp_transport_.get());
 
     int option_val;
-    ASSERT_TRUE(
-        static_cast<DtlsTransportInternal*>(channel1_->rtp_packet_transport())
-            ->GetOption(rtc::Socket::Option::OPT_SNDBUF, &option_val));
+    ASSERT_TRUE(fake_rtp_dtls_transport2_->GetOption(
+        rtc::Socket::Option::OPT_SNDBUF, &option_val));
     EXPECT_EQ(kSndBufSize, option_val);
-    ASSERT_TRUE(
-        static_cast<DtlsTransportInternal*>(channel1_->rtp_packet_transport())
-            ->GetOption(rtc::Socket::Option::OPT_RCVBUF, &option_val));
+    ASSERT_TRUE(fake_rtp_dtls_transport2_->GetOption(
+        rtc::Socket::Option::OPT_RCVBUF, &option_val));
     EXPECT_EQ(kRcvBufSize, option_val);
   }
 

pc/jsep_transport_controller_unittest.cc

@@ -1662,7 +1662,6 @@ TEST_F(JsepTransportControllerTest, MultipleMediaSectionsOfSameTypeWithBundle) {
   // Verify the DtlsTransport for the SCTP data channel is reset correctly.
   auto it2 = changed_dtls_transport_by_mid_.find(kDataMid1);
   ASSERT_TRUE(it2 != changed_dtls_transport_by_mid_.end());
-  EXPECT_EQ(transport1->rtp_packet_transport(), it2->second);
 }
 
 // Tests that only a subset of all the m= sections are bundled.

pc/peer_connection_bundle_unittest.cc

@@ -83,14 +83,8 @@ class PeerConnectionWrapperForBundleTest : public PeerConnectionWrapper {
     return false;
   }
 
-  rtc::PacketTransportInternal* voice_rtp_transport() {
-    return (voice_channel() ? voice_channel()->rtp_packet_transport()
-                            : nullptr);
-  }
-
-  rtc::PacketTransportInternal* voice_rtcp_transport() {
-    return (voice_channel() ? voice_channel()->rtcp_packet_transport()
-                            : nullptr);
+  RtpTransportInternal* voice_rtp_transport() {
+    return (voice_channel() ? voice_channel()->rtp_transport() : nullptr);
   }
 
   cricket::VoiceChannel* voice_channel() {
@@ -104,14 +98,8 @@ class PeerConnectionWrapperForBundleTest : public PeerConnectionWrapper {
     return nullptr;
   }
 
-  rtc::PacketTransportInternal* video_rtp_transport() {
-    return (video_channel() ? video_channel()->rtp_packet_transport()
-                            : nullptr);
-  }
-
-  rtc::PacketTransportInternal* video_rtcp_transport() {
-    return (video_channel() ? video_channel()->rtcp_packet_transport()
-                            : nullptr);
+  RtpTransportInternal* video_rtp_transport() {
+    return (video_channel() ? video_channel()->rtp_transport() : nullptr);
   }
 
   cricket::VideoChannel* video_channel() {
@@ -552,14 +540,14 @@ TEST_P(PeerConnectionBundleTest, NeverCreateRtcpTransportWithRtcpMuxRequired) {
 
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
 
-  EXPECT_FALSE(caller->voice_rtcp_transport());
-  EXPECT_FALSE(caller->video_rtcp_transport());
+  EXPECT_FALSE(caller->voice_rtp_transport()->rtcp_mux_enabled());
+  EXPECT_FALSE(caller->video_rtp_transport()->rtcp_mux_enabled());
 
   ASSERT_TRUE(
       caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
 
-  EXPECT_FALSE(caller->voice_rtcp_transport());
-  EXPECT_FALSE(caller->video_rtcp_transport());
+  EXPECT_TRUE(caller->voice_rtp_transport()->rtcp_mux_enabled());
+  EXPECT_TRUE(caller->video_rtp_transport()->rtcp_mux_enabled());
 }
 
 // When negotiating RTCP multiplexing, the PeerConnection makes RTCP transports
@@ -573,14 +561,14 @@ TEST_P(PeerConnectionBundleTest,
 
   ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal()));
 
-  EXPECT_TRUE(caller->voice_rtcp_transport());
-  EXPECT_TRUE(caller->video_rtcp_transport());
+  EXPECT_FALSE(caller->voice_rtp_transport()->rtcp_mux_enabled());
+  EXPECT_FALSE(caller->video_rtp_transport()->rtcp_mux_enabled());
 
   ASSERT_TRUE(
       caller->SetRemoteDescription(callee->CreateAnswerAndSetAsLocal()));
 
-  EXPECT_FALSE(caller->voice_rtcp_transport());
-  EXPECT_FALSE(caller->video_rtcp_transport());
+  EXPECT_TRUE(caller->voice_rtp_transport()->rtcp_mux_enabled());
+  EXPECT_TRUE(caller->video_rtp_transport()->rtcp_mux_enabled());
 }
 
 TEST_P(PeerConnectionBundleTest, FailToSetDescriptionWithBundleAndNoRtcpMux) {

pc/peer_connection_ice_unittest.cc

@@ -216,17 +216,9 @@ class PeerConnectionIceBaseTest : public ::testing::Test {
     PeerConnection* pc = static_cast<PeerConnection*>(pc_proxy->internal());
     for (const auto& transceiver : pc->GetTransceiversInternal()) {
       if (transceiver->media_type() == cricket::MEDIA_TYPE_AUDIO) {
-        // TODO(amithi): This test seems to be using a method that should not
-        // be public |rtp_packet_transport|. Because the test is not mocking
-        // the channels or transceiver, workaround will be to |static_cast|
-        // the channel until the method is rewritten.
-        cricket::BaseChannel* channel = static_cast<cricket::BaseChannel*>(
-            transceiver->internal()->channel());
-        if (channel) {
-          auto dtls_transport = static_cast<cricket::DtlsTransportInternal*>(
-              channel->rtp_packet_transport());
-          return dtls_transport->ice_transport()->GetIceRole();
-        }
+        auto dtls_transport = pc->LookupDtlsTransportByMidInternal(
+            transceiver->internal()->channel()->content_name());
+        return dtls_transport->ice_transport()->internal()->GetIceRole();
       }
     }
     RTC_NOTREACHED();

pc/rtp_transport.cc

@@ -31,6 +31,21 @@ void RtpTransport::SetRtcpMuxEnabled(bool enable) {
   MaybeSignalReadyToSend();
 }
 
+const std::string& RtpTransport::transport_name() const {
+  return rtp_packet_transport_->transport_name();
+}
+
+int RtpTransport::SetRtpOption(rtc::Socket::Option opt, int value) {
+  return rtp_packet_transport_->SetOption(opt, value);
+}
+
+int RtpTransport::SetRtcpOption(rtc::Socket::Option opt, int value) {
+  if (rtcp_packet_transport_) {
+    return rtcp_packet_transport_->SetOption(opt, value);
+  }
+  return -1;
+}
+
 void RtpTransport::SetRtpPacketTransport(
     rtc::PacketTransportInternal* new_packet_transport) {
   if (new_packet_transport == rtp_packet_transport_) {

pc/rtp_transport.h

@@ -39,15 +39,20 @@ class RtpTransport : public RtpTransportInternal {
   bool rtcp_mux_enabled() const override { return rtcp_mux_enabled_; }
   void SetRtcpMuxEnabled(bool enable) override;
 
-  rtc::PacketTransportInternal* rtp_packet_transport() const override {
+  const std::string& transport_name() const override;
+
+  int SetRtpOption(rtc::Socket::Option opt, int value) override;
+  int SetRtcpOption(rtc::Socket::Option opt, int value) override;
+
+  rtc::PacketTransportInternal* rtp_packet_transport() const {
     return rtp_packet_transport_;
   }
-  void SetRtpPacketTransport(rtc::PacketTransportInternal* rtp) override;
+  void SetRtpPacketTransport(rtc::PacketTransportInternal* rtp);
 
-  rtc::PacketTransportInternal* rtcp_packet_transport() const override {
+  rtc::PacketTransportInternal* rtcp_packet_transport() const {
     return rtcp_packet_transport_;
   }
-  void SetRtcpPacketTransport(rtc::PacketTransportInternal* rtcp) override;
+  void SetRtcpPacketTransport(rtc::PacketTransportInternal* rtcp);
 
   bool IsReadyToSend() const override { return ready_to_send_; }
 

pc/rtp_transport_internal.h

@@ -37,17 +37,14 @@ class RtpTransportInternal : public sigslot::has_slots<> {
 
   virtual void SetRtcpMuxEnabled(bool enable) = 0;
 
-  // TODO(zstein): Remove PacketTransport setters. Clients should pass these
-  // in to constructors instead and construct a new RtpTransportInternal instead
-  // of updating them.
+  virtual const std::string& transport_name() const = 0;
+
+  // Sets socket options on the underlying RTP or RTCP transports.
+  virtual int SetRtpOption(rtc::Socket::Option opt, int value) = 0;
+  virtual int SetRtcpOption(rtc::Socket::Option opt, int value) = 0;
+
   virtual bool rtcp_mux_enabled() const = 0;
 
-  virtual rtc::PacketTransportInternal* rtp_packet_transport() const = 0;
-  virtual void SetRtpPacketTransport(rtc::PacketTransportInternal* rtp) = 0;
-
-  virtual rtc::PacketTransportInternal* rtcp_packet_transport() const = 0;
-  virtual void SetRtcpPacketTransport(rtc::PacketTransportInternal* rtcp) = 0;
-
   virtual bool IsReadyToSend() const = 0;
 
   // Called whenever a transport's ready-to-send state changes. The argument