admin/webrtc_m130
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Clean up error handling in ChannelManager.

Browse Source 
This also deletes unused method has_channels() and moves us closer
to having the ChannelManager just be a factory class. Once we get there
the ownership of the channels themselves can be with the classes that
hold pointers to them. Today the initialization and teardown of those
classes need to be synchronized with ChannelManager. But there's no
real value in keeping the channel pointers owned elsewhere.

Places where we have naked un-owned channel pointers:
* RtpTransceiver for voice and video
* PeerConnection::data_channel_controller_ (rtp data channel)

Bug: webrtc:11994
Change-Id: Id6df27414cc57b6ecf0f7f769fdb9603ed114bfd
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/214440
Reviewed-by: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33654}
This commit is contained in:
Tomas Gunnarsson 2021-04-08 14:39:47 +02:00 committed by Commit Bot
parent e9dad5f053
commit 00f4fd9b1a
3 changed files with 42 additions and 63 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

70
pc/channel_manager.cc
View File

@ -159,6 +159,8 @@ VoiceChannel* ChannelManager::CreateVoiceChannel(
const webrtc::CryptoOptions& crypto_options,
rtc::UniqueRandomIdGenerator* ssrc_generator,
const AudioOptions& options) {
RTC_DCHECK(call);
RTC_DCHECK(media_engine_);
// TODO(bugs.webrtc.org/11992): Remove this workaround after updates in
// PeerConnection and add the expectation that we're already on the right
// thread.
@ -171,10 +173,6 @@ VoiceChannel* ChannelManager::CreateVoiceChannel(
}
RTC_DCHECK_RUN_ON(worker_thread_);
RTC_DCHECK(call);
if (!media_engine_) {
return nullptr;
}
VoiceMediaChannel* media_channel = media_engine_->voice().CreateMediaChannel(
call, media_config, options, crypto_options);
@ -196,9 +194,8 @@ VoiceChannel* ChannelManager::CreateVoiceChannel(
void ChannelManager::DestroyVoiceChannel(VoiceChannel* voice_channel) {
TRACE_EVENT0("webrtc", "ChannelManager::DestroyVoiceChannel");
if (!voice_channel) {
return;
}
RTC_DCHECK(voice_channel);
if (!worker_thread_->IsCurrent()) {
worker_thread_->Invoke<void>(RTC_FROM_HERE,
[&] { DestroyVoiceChannel(voice_channel); });
@ -206,16 +203,11 @@ void ChannelManager::DestroyVoiceChannel(VoiceChannel* voice_channel) {
}
RTC_DCHECK_RUN_ON(worker_thread_);
auto it = absl::c_find_if(voice_channels_,
[&](const std::unique_ptr<VoiceChannel>& p) {
return p.get() == voice_channel;
});
RTC_DCHECK(it != voice_channels_.end());
if (it == voice_channels_.end()) {
return;
}
voice_channels_.erase(it);
voice_channels_.erase(absl::c_find_if(
voice_channels_, [&](const std::unique_ptr<VoiceChannel>& p) {
return p.get() == voice_channel;
}));
}
VideoChannel* ChannelManager::CreateVideoChannel(
@ -229,6 +221,8 @@ VideoChannel* ChannelManager::CreateVideoChannel(
rtc::UniqueRandomIdGenerator* ssrc_generator,
const VideoOptions& options,
webrtc::VideoBitrateAllocatorFactory* video_bitrate_allocator_factory) {
RTC_DCHECK(call);
RTC_DCHECK(media_engine_);
// TODO(bugs.webrtc.org/11992): Remove this workaround after updates in
// PeerConnection and add the expectation that we're already on the right
// thread.
@ -242,10 +236,6 @@ VideoChannel* ChannelManager::CreateVideoChannel(
}
RTC_DCHECK_RUN_ON(worker_thread_);
RTC_DCHECK(call);
if (!media_engine_) {
return nullptr;
}
VideoMediaChannel* media_channel = media_engine_->video().CreateMediaChannel(
call, media_config, options, crypto_options,
@ -268,9 +258,8 @@ VideoChannel* ChannelManager::CreateVideoChannel(
void ChannelManager::DestroyVideoChannel(VideoChannel* video_channel) {
TRACE_EVENT0("webrtc", "ChannelManager::DestroyVideoChannel");
if (!video_channel) {
return;
}
RTC_DCHECK(video_channel);
if (!worker_thread_->IsCurrent()) {
worker_thread_->Invoke<void>(RTC_FROM_HERE,
[&] { DestroyVideoChannel(video_channel); });
@ -278,16 +267,10 @@ void ChannelManager::DestroyVideoChannel(VideoChannel* video_channel) {
}
RTC_DCHECK_RUN_ON(worker_thread_);
auto it = absl::c_find_if(video_channels_,
[&](const std::unique_ptr<VideoChannel>& p) {
video_channels_.erase(absl::c_find_if(
video_channels_, [&](const std::unique_ptr<VideoChannel>& p) {
return p.get() == video_channel;
});
RTC_DCHECK(it != video_channels_.end());
if (it == video_channels_.end()) {
return;
}
video_channels_.erase(it);
}));
}
RtpDataChannel* ChannelManager::CreateRtpDataChannel(
@ -330,9 +313,8 @@ RtpDataChannel* ChannelManager::CreateRtpDataChannel(
void ChannelManager::DestroyRtpDataChannel(RtpDataChannel* data_channel) {
TRACE_EVENT0("webrtc", "ChannelManager::DestroyRtpDataChannel");
if (!data_channel) {
return;
}
RTC_DCHECK(data_channel);
if (!worker_thread_->IsCurrent()) {
worker_thread_->Invoke<void>(
RTC_FROM_HERE, [&] { return DestroyRtpDataChannel(data_channel); });
@ -340,22 +322,10 @@ void ChannelManager::DestroyRtpDataChannel(RtpDataChannel* data_channel) {
}
RTC_DCHECK_RUN_ON(worker_thread_);
auto it = absl::c_find_if(data_channels_,
[&](const std::unique_ptr<RtpDataChannel>& p) {
data_channels_.erase(absl::c_find_if(
data_channels_, [&](const std::unique_ptr<RtpDataChannel>& p) {
return p.get() == data_channel;
});
RTC_DCHECK(it != data_channels_.end());
if (it == data_channels_.end()) {
return;
}
data_channels_.erase(it);
}
bool ChannelManager::has_channels() const {
RTC_DCHECK_RUN_ON(worker_thread_);
return (!voice_channels_.empty() || !video_channels_.empty() ||
!data_channels_.empty());
}));
}
bool ChannelManager::StartAecDump(webrtc::FileWrapper file,

3
pc/channel_manager.h
View File

@ -121,9 +121,6 @@ class ChannelManager final {
// Destroys a data channel created by CreateRtpDataChannel.
void DestroyRtpDataChannel(RtpDataChannel* data_channel);
// Indicates whether any channels exist.
bool has_channels() const;
// Starts AEC dump using existing file, with a specified maximum file size in
// bytes. When the limit is reached, logging will stop and the file will be
// closed. If max_size_bytes is set to <= 0, no limit will be used.

28
pc/sdp_offer_answer.cc
View File

@ -4614,6 +4614,9 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) {
cricket::VoiceChannel* SdpOfferAnswerHandler::CreateVoiceChannel(
const std::string& mid) {
RTC_DCHECK_RUN_ON(signaling_thread());
if (!channel_manager()->media_engine())
return nullptr;
RtpTransportInternal* rtp_transport = pc_->GetRtpTransport(mid);
// TODO(bugs.webrtc.org/11992): CreateVoiceChannel internally switches to the
@ -4636,6 +4639,9 @@ cricket::VoiceChannel* SdpOfferAnswerHandler::CreateVoiceChannel(
cricket::VideoChannel* SdpOfferAnswerHandler::CreateVideoChannel(
const std::string& mid) {
RTC_DCHECK_RUN_ON(signaling_thread());
if (!channel_manager()->media_engine())
return nullptr;
// NOTE: This involves a non-ideal hop (Invoke) over to the network thread.
RtpTransportInternal* rtp_transport = pc_->GetRtpTransport(mid);
@ -4659,15 +4665,19 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) {
RTC_DCHECK_RUN_ON(signaling_thread());
switch (pc_->data_channel_type()) {
case cricket::DCT_SCTP:
if (pc_->network_thread()->Invoke<bool>(RTC_FROM_HERE, [this, &mid] {
if (!pc_->network_thread()->Invoke<bool>(RTC_FROM_HERE, [this, &mid] {
RTC_DCHECK_RUN_ON(pc_->network_thread());
return pc_->SetupDataChannelTransport_n(mid);
})) {
pc_->SetSctpDataMid(mid);
} else {
return false;
}
return true;
// TODO(tommi): Is this necessary? SetupDataChannelTransport_n() above
// will have queued up updating the transport name on the signaling thread
// and could update the mid at the same time. This here is synchronous
// though, but it changes the state of PeerConnection and makes it be
// out of sync (transport name not set while the mid is set).
pc_->SetSctpDataMid(mid);
break;
case cricket::DCT_RTP:
default:
RtpTransportInternal* rtp_transport = pc_->GetRtpTransport(mid);
@ -4684,9 +4694,9 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) {
pc_->SetupRtpDataChannelTransport_n(data_channel);
});
have_pending_rtp_data_channel_ = true;
return true;
break;
}
return false;
return true;
}
void SdpOfferAnswerHandler::DestroyTransceiverChannel(
@ -4741,12 +4751,14 @@ void SdpOfferAnswerHandler::DestroyDataChannelTransport() {
void SdpOfferAnswerHandler::DestroyChannelInterface(
cricket::ChannelInterface* channel) {
RTC_DCHECK_RUN_ON(signaling_thread());
RTC_DCHECK(channel_manager()->media_engine());
RTC_DCHECK(channel);
// TODO(bugs.webrtc.org/11992): All the below methods should be called on the
// worker thread. (they switch internally anyway). Change
// DestroyChannelInterface to either be called on the worker thread, or do
// this asynchronously on the worker.
RTC_DCHECK(channel);
RTC_LOG_THREAD_BLOCK_COUNT();
switch (channel->media_type()) {