More minor improvements to BaseChannel/transport code.

Mostly from late comments on this CL:
https://codereview.webrtc.org/2614263002/

Changes SetTransport to DCHECK instead of returning false.
Renames it to SetTransports.
Fixes some possible transport resource leaks.

BUG=None

Review-Url: https://codereview.webrtc.org/2637503003
Cr-Commit-Position: refs/heads/master@{#16130}
This commit is contained in:
deadbeef
2017-01-17 18:32:35 -08:00
committed by Commit bot
parent b308b037e3
commit bad5dadef3
8 changed files with 179 additions and 82 deletions

View File

@ -315,11 +315,11 @@ class RTCStatsCollectorTestHelper : public SetSessionDescriptionObserver {
RTCStatsCollectorTestHelper()
: worker_thread_(rtc::Thread::Current()),
network_thread_(rtc::Thread::Current()),
signaling_thread_(rtc::Thread::Current()),
media_engine_(new cricket::FakeMediaEngine()),
channel_manager_(
new cricket::ChannelManager(media_engine_,
worker_thread_,
network_thread_)),
channel_manager_(new cricket::ChannelManager(media_engine_,
worker_thread_,
network_thread_)),
media_controller_(
MediaControllerInterface::Create(cricket::MediaConfig(),
worker_thread_,
@ -349,6 +349,7 @@ class RTCStatsCollectorTestHelper : public SetSessionDescriptionObserver {
rtc::ScopedFakeClock& fake_clock() { return fake_clock_; }
rtc::Thread* worker_thread() { return worker_thread_; }
rtc::Thread* network_thread() { return network_thread_; }
rtc::Thread* signaling_thread() { return signaling_thread_; }
cricket::FakeMediaEngine* media_engine() { return media_engine_; }
MockWebRtcSession& session() { return session_; }
MockPeerConnection& pc() { return pc_; }
@ -442,6 +443,7 @@ class RTCStatsCollectorTestHelper : public SetSessionDescriptionObserver {
RtcEventLogNullImpl event_log_;
rtc::Thread* const worker_thread_;
rtc::Thread* const network_thread_;
rtc::Thread* const signaling_thread_;
cricket::FakeMediaEngine* media_engine_;
std::unique_ptr<cricket::ChannelManager> channel_manager_;
std::unique_ptr<MediaControllerInterface> media_controller_;
@ -697,15 +699,15 @@ TEST_F(RTCStatsCollectorTest, CollectRTCCertificateStatsSingle) {
TEST_F(RTCStatsCollectorTest, CollectRTCCodecStats) {
MockVoiceMediaChannel* voice_media_channel = new MockVoiceMediaChannel();
cricket::VoiceChannel voice_channel(
test_->worker_thread(), test_->network_thread(), nullptr,
test_->media_engine(), voice_media_channel, "VoiceContentName",
kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
test_->worker_thread(), test_->network_thread(),
test_->signaling_thread(), test_->media_engine(), voice_media_channel,
"VoiceContentName", kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
MockVideoMediaChannel* video_media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(
test_->worker_thread(), test_->network_thread(), nullptr,
video_media_channel, "VideoContentName", kDefaultRtcpMuxRequired,
kDefaultSrtpRequired);
test_->worker_thread(), test_->network_thread(),
test_->signaling_thread(), video_media_channel, "VideoContentName",
kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
// Audio
cricket::VoiceMediaInfo voice_media_info;
@ -1550,9 +1552,9 @@ TEST_F(RTCStatsCollectorTest,
TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Audio) {
MockVoiceMediaChannel* voice_media_channel = new MockVoiceMediaChannel();
cricket::VoiceChannel voice_channel(
test_->worker_thread(), test_->network_thread(), nullptr,
test_->media_engine(), voice_media_channel, "VoiceContentName",
kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
test_->worker_thread(), test_->network_thread(),
test_->signaling_thread(), test_->media_engine(), voice_media_channel,
"VoiceContentName", kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
test_->SetupRemoteTrackAndReceiver(
cricket::MEDIA_TYPE_AUDIO, "RemoteAudioTrackID", 1);
@ -1629,9 +1631,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Audio) {
TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Video) {
MockVideoMediaChannel* video_media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(
test_->worker_thread(), test_->network_thread(), nullptr,
video_media_channel, "VideoContentName", kDefaultRtcpMuxRequired,
kDefaultSrtpRequired);
test_->worker_thread(), test_->network_thread(),
test_->signaling_thread(), video_media_channel, "VideoContentName",
kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
test_->SetupRemoteTrackAndReceiver(
cricket::MEDIA_TYPE_VIDEO, "RemoteVideoTrackID", 1);
@ -1714,9 +1716,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCInboundRTPStreamStats_Video) {
TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Audio) {
MockVoiceMediaChannel* voice_media_channel = new MockVoiceMediaChannel();
cricket::VoiceChannel voice_channel(
test_->worker_thread(), test_->network_thread(), nullptr,
test_->media_engine(), voice_media_channel, "VoiceContentName",
kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
test_->worker_thread(), test_->network_thread(),
test_->signaling_thread(), test_->media_engine(), voice_media_channel,
"VoiceContentName", kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
test_->SetupLocalTrackAndSender(
cricket::MEDIA_TYPE_AUDIO, "LocalAudioTrackID", 1);
@ -1787,9 +1789,9 @@ TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Audio) {
TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Video) {
MockVideoMediaChannel* video_media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(
test_->worker_thread(), test_->network_thread(), nullptr,
video_media_channel, "VideoContentName", kDefaultRtcpMuxRequired,
kDefaultSrtpRequired);
test_->worker_thread(), test_->network_thread(),
test_->signaling_thread(), video_media_channel, "VideoContentName",
kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
test_->SetupLocalTrackAndSender(
cricket::MEDIA_TYPE_VIDEO, "LocalVideoTrackID", 1);
@ -1870,14 +1872,14 @@ TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Video) {
TEST_F(RTCStatsCollectorTest, CollectRTCOutboundRTPStreamStats_Default) {
MockVoiceMediaChannel* voice_media_channel = new MockVoiceMediaChannel();
cricket::VoiceChannel voice_channel(
test_->worker_thread(), test_->network_thread(), nullptr,
test_->media_engine(), voice_media_channel, "VoiceContentName",
kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
test_->worker_thread(), test_->network_thread(),
test_->signaling_thread(), test_->media_engine(), voice_media_channel,
"VoiceContentName", kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
MockVideoMediaChannel* video_media_channel = new MockVideoMediaChannel();
cricket::VideoChannel video_channel(
test_->worker_thread(), test_->network_thread(), nullptr,
video_media_channel, "VideoContentName", kDefaultRtcpMuxRequired,
kDefaultSrtpRequired);
test_->worker_thread(), test_->network_thread(),
test_->signaling_thread(), video_media_channel, "VideoContentName",
kDefaultRtcpMuxRequired, kDefaultSrtpRequired);
cricket::VoiceMediaInfo voice_media_info;
voice_media_info.senders.push_back(cricket::VoiceSenderInfo());

View File

@ -1092,18 +1092,16 @@ bool WebRtcSession::EnableBundle(const cricket::ContentGroup& bundle) {
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
}
if (!ch->SetTransport(rtp_transport, rtcp_transport)) {
LOG(LS_WARNING) << "Failed to enable BUNDLE for " << ch->content_name();
return false;
}
ch->SetTransports(rtp_transport, rtcp_transport);
LOG(LS_INFO) << "Enabled BUNDLE for " << ch->content_name() << " on "
<< transport_name << ".";
DestroyTransport(old_transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
transport_controller_->DestroyTransportChannel(
old_transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
// If the channel needs rtcp, it means that the channel used to have a
// rtcp transport which needs to be deleted now.
if (need_rtcp) {
DestroyTransport(old_transport_name,
cricket::ICE_CANDIDATE_COMPONENT_RTCP);
transport_controller_->DestroyTransportChannel(
old_transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
}
return true;
};
@ -1805,6 +1803,12 @@ bool WebRtcSession::CreateVoiceChannel(const cricket::ContentInfo* content,
transport_controller_->signaling_thread(), content->name,
bundle_transport, require_rtcp_mux, SrtpRequired(), audio_options_));
if (!voice_channel_) {
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
if (rtcp_transport) {
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
}
return false;
}
@ -1842,6 +1846,12 @@ bool WebRtcSession::CreateVideoChannel(const cricket::ContentInfo* content,
bundle_transport, require_rtcp_mux, SrtpRequired(), video_options_));
if (!video_channel_) {
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
if (rtcp_transport) {
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
}
return false;
}
@ -1901,6 +1911,12 @@ bool WebRtcSession::CreateDataChannel(const cricket::ContentInfo* content,
bundle_transport, require_rtcp_mux, SrtpRequired()));
if (!rtp_data_channel_) {
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
if (rtcp_transport) {
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
}
return false;
}
@ -2365,14 +2381,6 @@ const std::string WebRtcSession::GetTransportName(
return channel->transport_name();
}
void WebRtcSession::DestroyTransport(const std::string& transport_name,
int component) {
network_thread_->Invoke<void>(
RTC_FROM_HERE,
rtc::Bind(&cricket::TransportController::DestroyTransportChannel_n,
transport_controller_.get(), transport_name, component));
}
void WebRtcSession::DestroyRtcpTransport_n(const std::string& transport_name) {
RTC_DCHECK(network_thread()->IsCurrent());
transport_controller_->DestroyTransportChannel_n(
@ -2386,9 +2394,11 @@ void WebRtcSession::DestroyVideoChannel() {
transport_name = video_channel_->rtp_transport()->transport_name();
bool need_to_delete_rtcp = (video_channel_->rtcp_transport() != nullptr);
channel_manager_->DestroyVideoChannel(video_channel_.release());
DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
if (need_to_delete_rtcp) {
DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
}
}
@ -2399,9 +2409,11 @@ void WebRtcSession::DestroyVoiceChannel() {
transport_name = voice_channel_->rtp_transport()->transport_name();
bool need_to_delete_rtcp = (voice_channel_->rtcp_transport() != nullptr);
channel_manager_->DestroyVoiceChannel(voice_channel_.release());
DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
if (need_to_delete_rtcp) {
DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
}
}
@ -2412,9 +2424,11 @@ void WebRtcSession::DestroyDataChannel() {
transport_name = rtp_data_channel_->rtp_transport()->transport_name();
bool need_to_delete_rtcp = (rtp_data_channel_->rtcp_transport() != nullptr);
channel_manager_->DestroyRtpDataChannel(rtp_data_channel_.release());
DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTP);
if (need_to_delete_rtcp) {
DestroyTransport(transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
transport_controller_->DestroyTransportChannel(
transport_name, cricket::ICE_CANDIDATE_COMPONENT_RTCP);
}
}
} // namespace webrtc

View File

@ -553,7 +553,6 @@ class WebRtcSession :
const std::string GetTransportName(const std::string& content_name);
void DestroyTransport(const std::string& transport_name, int component);
void DestroyRtcpTransport_n(const std::string& transport_name);
void DestroyVideoChannel();
void DestroyVoiceChannel();

View File

@ -296,6 +296,14 @@ TransportChannel* TransportController::CreateTransportChannel_n(
return dtls;
}
void TransportController::DestroyTransportChannel(
const std::string& transport_name,
int component) {
network_thread_->Invoke<void>(
RTC_FROM_HERE, rtc::Bind(&TransportController::DestroyTransportChannel_n,
this, transport_name, component));
}
void TransportController::DestroyTransportChannel_n(
const std::string& transport_name,
int component) {

View File

@ -121,6 +121,8 @@ class TransportController : public sigslot::has_slots<>,
// Decrements a channel's reference count, and destroys the channel if
// nothing is referencing it.
virtual void DestroyTransportChannel(const std::string& transport_name,
int component);
virtual void DestroyTransportChannel_n(const std::string& transport_name,
int component);

View File

@ -228,11 +228,7 @@ bool BaseChannel::Init_w(TransportChannel* rtp_transport,
bool BaseChannel::InitNetwork_n(TransportChannel* rtp_transport,
TransportChannel* rtcp_transport) {
RTC_DCHECK(network_thread_->IsCurrent());
// const std::string& transport_name =
// (bundle_transport_name ? *bundle_transport_name : content_name());
if (!SetTransport_n(rtp_transport, rtcp_transport)) {
return false;
}
SetTransports_n(rtp_transport, rtcp_transport);
if (!SetDtlsSrtpCryptoSuites_n(rtp_transport_, false)) {
return false;
@ -256,30 +252,27 @@ void BaseChannel::Deinit() {
RTC_FROM_HERE, Bind(&BaseChannel::DisconnectTransportChannels_n, this));
}
bool BaseChannel::SetTransport(TransportChannel* rtp_transport,
TransportChannel* rtcp_transport) {
return network_thread_->Invoke<bool>(
void BaseChannel::SetTransports(TransportChannel* rtp_transport,
TransportChannel* rtcp_transport) {
network_thread_->Invoke<void>(
RTC_FROM_HERE,
Bind(&BaseChannel::SetTransport_n, this, rtp_transport, rtcp_transport));
Bind(&BaseChannel::SetTransports_n, this, rtp_transport, rtcp_transport));
}
bool BaseChannel::SetTransport_n(TransportChannel* rtp_transport,
TransportChannel* rtcp_transport) {
void BaseChannel::SetTransports_n(TransportChannel* rtp_transport,
TransportChannel* rtcp_transport) {
RTC_DCHECK(network_thread_->IsCurrent());
if (!rtp_transport && !rtcp_transport) {
LOG(LS_ERROR) << "Setting nullptr to RTP Transport and RTCP Transport.";
return false;
}
// Verify some assumptions (as described in the comment above SetTransport).
RTC_DCHECK(rtp_transport);
RTC_DCHECK(NeedsRtcpTransport() == (rtcp_transport != nullptr));
if (rtcp_transport) {
RTC_DCHECK(rtp_transport->transport_name() ==
rtcp_transport->transport_name());
RTC_DCHECK(NeedsRtcpTransport());
}
if (rtp_transport->transport_name() == transport_name_) {
// Nothing to do if transport name isn't changing.
return true;
return;
}
transport_name_ = rtp_transport->transport_name();
@ -300,17 +293,13 @@ bool BaseChannel::SetTransport_n(TransportChannel* rtp_transport,
LOG(LS_INFO) << "Setting RTCP Transport for " << content_name() << " on "
<< transport_name() << " transport " << rtcp_transport;
SetTransportChannel_n(true, rtcp_transport);
if (!rtcp_transport_) {
return false;
}
RTC_DCHECK(rtcp_transport_);
}
LOG(LS_INFO) << "Setting non-RTCP Transport for " << content_name() << " on "
<< transport_name() << " transport " << rtp_transport;
SetTransportChannel_n(false, rtp_transport);
if (!rtp_transport_) {
return false;
}
RTC_DCHECK(rtp_transport_);
// Update aggregate writable/ready-to-send state between RTP and RTCP upon
// setting new transport channels.
@ -328,7 +317,6 @@ bool BaseChannel::SetTransport_n(TransportChannel* rtp_transport,
rtp_transport_ && rtp_transport_->writable());
SetTransportChannelReadyToSend(
true, rtcp_transport_ && rtcp_transport_->writable());
return true;
}
void BaseChannel::SetTransportChannel_n(bool rtcp,

View File

@ -107,8 +107,14 @@ class BaseChannel
bool writable() const { return writable_; }
bool SetTransport(TransportChannel* rtp_transport,
TransportChannel* rtcp_transport);
// Set the transport(s), and update writability and "ready-to-send" state.
// |rtp_transport| must be non-null.
// |rtcp_transport| must be supplied if NeedsRtcpTransport() is true (meaning
// RTCP muxing is not fully active yet).
// |rtp_transport| and |rtcp_transport| must share the same transport name as
// well.
void SetTransports(TransportChannel* rtp_transport,
TransportChannel* rtcp_transport);
bool PushdownLocalDescription(const SessionDescription* local_desc,
ContentAction action,
std::string* error_desc);
@ -194,11 +200,8 @@ class BaseChannel
protected:
virtual MediaChannel* media_channel() const { return media_channel_; }
// Sets the |rtp_transport_| (and |rtcp_transport_|, if
// |rtcp_enabled_| is true).
// This method also updates writability and "ready-to-send" state.
bool SetTransport_n(TransportChannel* rtp_transport,
TransportChannel* rtcp_transport);
void SetTransports_n(TransportChannel* rtp_transport,
TransportChannel* rtcp_transport);
// This does not update writability or "ready-to-send" state; it just
// disconnects from the old channel and connects to the new one.

View File

@ -3577,4 +3577,85 @@ TEST_F(RtpDataChannelDoubleThreadTest, TestSendData) {
EXPECT_EQ("foo", media_channel1_->last_sent_data());
}
#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
// Verifies some DCHECKs are in place.
// Uses VoiceChannel, but any BaseChannel subclass would work.
class BaseChannelDeathTest : public testing::Test {
public:
BaseChannelDeathTest()
: // RTCP mux not required, SRTP required.
voice_channel_(
rtc::Thread::Current(),
rtc::Thread::Current(),
rtc::Thread::Current(),
&fake_media_engine_,
new cricket::FakeVoiceMediaChannel(nullptr,
cricket::AudioOptions()),
cricket::CN_AUDIO,
false,
true) {
rtp_transport_ = fake_transport_controller_.CreateTransportChannel(
"foo", cricket::ICE_CANDIDATE_COMPONENT_RTP);
rtcp_transport_ = fake_transport_controller_.CreateTransportChannel(
"foo", cricket::ICE_CANDIDATE_COMPONENT_RTCP);
EXPECT_TRUE(voice_channel_.Init_w(rtp_transport_, rtcp_transport_));
}
protected:
cricket::FakeTransportController fake_transport_controller_;
cricket::FakeMediaEngine fake_media_engine_;
cricket::VoiceChannel voice_channel_;
// Will be cleaned up by FakeTransportController, don't need to worry about
// deleting them in this test.
cricket::TransportChannel* rtp_transport_;
cricket::TransportChannel* rtcp_transport_;
};
TEST_F(BaseChannelDeathTest, SetTransportWithNullRtpTransport) {
cricket::TransportChannel* new_rtcp_transport =
fake_transport_controller_.CreateTransportChannel(
"bar", cricket::ICE_CANDIDATE_COMPONENT_RTCP);
EXPECT_DEATH(voice_channel_.SetTransports(nullptr, new_rtcp_transport), "");
}
TEST_F(BaseChannelDeathTest, SetTransportWithMissingRtcpTransport) {
cricket::TransportChannel* new_rtp_transport =
fake_transport_controller_.CreateTransportChannel(
"bar", cricket::ICE_CANDIDATE_COMPONENT_RTP);
EXPECT_DEATH(voice_channel_.SetTransports(new_rtp_transport, nullptr), "");
}
TEST_F(BaseChannelDeathTest, SetTransportWithUnneededRtcpTransport) {
// Activate RTCP muxing, simulating offer/answer negotiation.
cricket::AudioContentDescription content;
content.set_rtcp_mux(true);
ASSERT_TRUE(voice_channel_.SetLocalContent(&content, CA_OFFER, nullptr));
ASSERT_TRUE(voice_channel_.SetRemoteContent(&content, CA_ANSWER, nullptr));
cricket::TransportChannel* new_rtp_transport =
fake_transport_controller_.CreateTransportChannel(
"bar", cricket::ICE_CANDIDATE_COMPONENT_RTP);
cricket::TransportChannel* new_rtcp_transport =
fake_transport_controller_.CreateTransportChannel(
"bar", cricket::ICE_CANDIDATE_COMPONENT_RTCP);
// After muxing is enabled, no RTCP transport should be passed in here.
EXPECT_DEATH(
voice_channel_.SetTransports(new_rtp_transport, new_rtcp_transport), "");
}
// This test will probably go away if/when we move the transport name out of
// the transport classes and into their parent classes.
TEST_F(BaseChannelDeathTest, SetTransportWithMismatchingTransportNames) {
cricket::TransportChannel* new_rtp_transport =
fake_transport_controller_.CreateTransportChannel(
"bar", cricket::ICE_CANDIDATE_COMPONENT_RTP);
cricket::TransportChannel* new_rtcp_transport =
fake_transport_controller_.CreateTransportChannel(
"baz", cricket::ICE_CANDIDATE_COMPONENT_RTCP);
EXPECT_DEATH(
voice_channel_.SetTransports(new_rtp_transport, new_rtcp_transport), "");
}
#endif // RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID)
// TODO(pthatcher): TestSetReceiver?