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}
diff --git a/pc/channel.cc b/pc/channel.cc
index be40a7e..13341e3 100644
--- a/pc/channel.cc
+++ b/pc/channel.cc
@@ -254,8 +254,7 @@
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 @@
// 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 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) {
diff --git a/pc/channel.h b/pc/channel.h
index 9d41419..8a75a1a 100644
--- a/pc/channel.h
+++ b/pc/channel.h
@@ -120,6 +120,8 @@
// 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 @@
// 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;
diff --git a/pc/channel_manager_unittest.cc b/pc/channel_manager_unittest.cc
index 91cd0c7..3d7e01a 100644
--- a/pc/channel_manager_unittest.cc
+++ b/pc/channel_manager_unittest.cc
@@ -193,8 +193,7 @@
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 4a64f7a..e4252b2 100644
--- a/pc/channel_unittest.cc
+++ b/pc/channel_unittest.cc
@@ -971,8 +971,8 @@
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 @@
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 @@
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 @@
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 @@
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 @@
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);
}
diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc
index 346168d..8081df9 100644
--- a/pc/jsep_transport_controller_unittest.cc
+++ b/pc/jsep_transport_controller_unittest.cc
@@ -1662,7 +1662,6 @@
// 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 b369db2..57d059c 100644
--- a/pc/peer_connection_bundle_unittest.cc
+++ b/pc/peer_connection_bundle_unittest.cc
@@ -83,14 +83,8 @@
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 @@
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 @@
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 @@
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 afc8594..3b8b4db 100644
--- a/pc/peer_connection_ice_unittest.cc
+++ b/pc/peer_connection_ice_unittest.cc
@@ -216,17 +216,9 @@
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();
diff --git a/pc/rtp_transport.cc b/pc/rtp_transport.cc
index b7d3a9d..6cfbed9 100644
--- a/pc/rtp_transport.cc
+++ b/pc/rtp_transport.cc
@@ -31,6 +31,21 @@
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 269a61a..57ad9e5 100644
--- a/pc/rtp_transport.h
+++ b/pc/rtp_transport.h
@@ -39,15 +39,20 @@
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 fce1f9b..dfcdbbf 100644
--- a/pc/rtp_transport_internal.h
+++ b/pc/rtp_transport_internal.h
@@ -37,17 +37,14 @@
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
