From 00f4fd9b1a2222383c579dece24ef0997ffbdd4c Mon Sep 17 00:00:00 2001 From: Tomas Gunnarsson Date: Thu, 8 Apr 2021 14:39:47 +0200 Subject: [PATCH] Clean up error handling in ChannelManager. 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 Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#33654} --- pc/channel_manager.cc | 74 +++++++++++++----------------------------- pc/channel_manager.h | 3 -- pc/sdp_offer_answer.cc | 28 +++++++++++----- 3 files changed, 42 insertions(+), 63 deletions(-) diff --git a/pc/channel_manager.cc b/pc/channel_manager.cc index 3fc0cd191b..028d31b1e6 100644 --- a/pc/channel_manager.cc +++ b/pc/channel_manager.cc @@ -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(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& 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& 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(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& p) { - return p.get() == video_channel; - }); - RTC_DCHECK(it != video_channels_.end()); - if (it == video_channels_.end()) { - return; - } - - video_channels_.erase(it); + video_channels_.erase(absl::c_find_if( + video_channels_, [&](const std::unique_ptr& p) { + return p.get() == video_channel; + })); } 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( 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& 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()); + data_channels_.erase(absl::c_find_if( + data_channels_, [&](const std::unique_ptr& p) { + return p.get() == data_channel; + })); } bool ChannelManager::StartAecDump(webrtc::FileWrapper file, diff --git a/pc/channel_manager.h b/pc/channel_manager.h index 145bea412c..10be09236a 100644 --- a/pc/channel_manager.h +++ b/pc/channel_manager.h @@ -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. diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc index 11fe1b33e4..53770eaa50 100644 --- a/pc/sdp_offer_answer.cc +++ b/pc/sdp_offer_answer.cc @@ -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(RTC_FROM_HERE, [this, &mid] { + if (!pc_->network_thread()->Invoke(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()) {