From 3a1b92772f661b6b52710fee27c5707bcfdda4a6 Mon Sep 17 00:00:00 2001 From: Bjorn A Mellem Date: Fri, 24 May 2019 16:13:08 -0700 Subject: [PATCH] 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 Reviewed-by: Anton Sukhanov Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#28066} --- pc/channel.cc | 20 +++++-------- pc/channel.h | 16 ++--------- pc/channel_manager_unittest.cc | 3 +- pc/channel_unittest.cc | 34 ++++++++++------------ pc/jsep_transport_controller_unittest.cc | 1 - pc/peer_connection_bundle_unittest.cc | 36 ++++++++---------------- pc/peer_connection_ice_unittest.cc | 14 ++------- pc/rtp_transport.cc | 15 ++++++++++ pc/rtp_transport.h | 13 ++++++--- pc/rtp_transport_internal.h | 15 ++++------ 10 files changed, 70 insertions(+), 97 deletions(-) diff --git a/pc/channel.cc b/pc/channel.cc index be40a7e446..13341e3e1b 100644 --- a/pc/channel.cc +++ b/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(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(opt, value)); - break; + return rtp_transport_->SetRtcpOption(opt, value); } - return transport ? transport->SetOption(opt, value) : -1; + return -1; } void BaseChannel::OnWritableState(bool writable) { diff --git a/pc/channel.h b/pc/channel.h index 9d41419cc7..8a75a1a5f3 100644 --- a/pc/channel.h +++ b/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 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; diff --git a/pc/channel_manager_unittest.cc b/pc/channel_manager_unittest.cc index 91cd0c77e7..3d7e01ab57 100644 --- a/pc/channel_manager_unittest.cc +++ b/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())); } diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index 4a64f7aa7a..e4252b2e8b 100644 --- a/pc/channel_unittest.cc +++ b/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(channel2_->rtp_packet_transport()), - static_cast( - 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(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(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); } diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc index 346168d1fa..8081df9432 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/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. diff --git a/pc/peer_connection_bundle_unittest.cc b/pc/peer_connection_bundle_unittest.cc index b369db23da..57d059c06e 100644 --- a/pc/peer_connection_bundle_unittest.cc +++ b/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) { diff --git a/pc/peer_connection_ice_unittest.cc b/pc/peer_connection_ice_unittest.cc index afc859434f..3b8b4db951 100644 --- a/pc/peer_connection_ice_unittest.cc +++ b/pc/peer_connection_ice_unittest.cc @@ -216,17 +216,9 @@ class PeerConnectionIceBaseTest : public ::testing::Test { PeerConnection* pc = static_cast(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( - transceiver->internal()->channel()); - if (channel) { - auto dtls_transport = static_cast( - 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(); diff --git a/pc/rtp_transport.cc b/pc/rtp_transport.cc index b7d3a9d893..6cfbed9cdd 100644 --- a/pc/rtp_transport.cc +++ b/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_) { diff --git a/pc/rtp_transport.h b/pc/rtp_transport.h index 269a61a09f..57ad9e5fd0 100644 --- a/pc/rtp_transport.h +++ b/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_; } diff --git a/pc/rtp_transport_internal.h b/pc/rtp_transport_internal.h index fce1f9bf71..dfcdbbfc61 100644 --- a/pc/rtp_transport_internal.h +++ b/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