From e7b221f4760af10e29cb4c501e758cc3518f628b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Peter=20Bostr=C3=B6m?= Date: Mon, 13 Apr 2015 15:34:32 +0200 Subject: [PATCH] Remove deadlock in WebRtcVideoEngine2. Acquiring stream_lock_ in WebRtcVideoChannel2 in a callback from Call forms a lock-order inversion between process-thread locks and libjingle locks, manifesting as CPU adaptation requests blocking on stream creation that is blocked on the CPU adaptation request finishing. R=asapersson@webrtc.org, mflodman@webrtc.org BUG=4535,chromium:475065 Review URL: https://webrtc-codereview.appspot.com/50679004 Cr-Commit-Position: refs/heads/master@{#8985} --- talk/media/webrtc/webrtcvideoengine2.cc | 57 +++++++++++++------------ talk/media/webrtc/webrtcvideoengine2.h | 10 +++-- 2 files changed, 37 insertions(+), 30 deletions(-) diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index 939df41fe5..42fe072443 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -633,6 +633,7 @@ WebRtcVideoChannel2::WebRtcVideoChannel2( external_decoder_factory_(external_decoder_factory) { SetDefaultOptions(); options_.SetAll(options); + options_.cpu_overuse_detection.Get(&signal_cpu_adaptation_); webrtc::Call::Config config(this); config.overuse_callback = this; if (voice_engine != NULL) { @@ -1144,13 +1145,15 @@ bool WebRtcVideoChannel2::SetCapturer(uint32 ssrc, VideoCapturer* capturer) { LOG(LS_INFO) << "SetCapturer: " << ssrc << " -> " << (capturer != NULL ? "(capturer)" : "NULL"); assert(ssrc != 0); - rtc::CritScope stream_lock(&stream_crit_); - if (send_streams_.find(ssrc) == send_streams_.end()) { - LOG(LS_ERROR) << "No sending stream on ssrc " << ssrc; - return false; - } - if (!send_streams_[ssrc]->SetCapturer(capturer)) { - return false; + { + rtc::CritScope stream_lock(&stream_crit_); + if (send_streams_.find(ssrc) == send_streams_.end()) { + LOG(LS_ERROR) << "No sending stream on ssrc " << ssrc; + return false; + } + if (!send_streams_[ssrc]->SetCapturer(capturer)) { + return false; + } } if (capturer) { @@ -1158,6 +1161,10 @@ bool WebRtcVideoChannel2::SetCapturer(uint32 ssrc, VideoCapturer* capturer) { !FindHeaderExtension(send_rtp_extensions_, kRtpVideoRotationHeaderExtension)); } + { + rtc::CritScope lock(&capturer_crit_); + capturers_[ssrc] = capturer; + } return true; } @@ -1333,6 +1340,10 @@ bool WebRtcVideoChannel2::SetOptions(const VideoOptions& options) { // No new options to set. return true; } + { + rtc::CritScope lock(&capturer_crit_); + options_.cpu_overuse_detection.Get(&signal_cpu_adaptation_); + } rtc::DiffServCodePoint dscp = options_.dscp.GetWithDefaultIfUnset(false) ? rtc::DSCP_AF41 : rtc::DSCP_DEFAULT; @@ -1372,14 +1383,18 @@ void WebRtcVideoChannel2::OnMessage(rtc::Message* msg) { } void WebRtcVideoChannel2::OnLoadUpdate(Load load) { - rtc::CritScope stream_lock(&stream_crit_); - for (std::map::iterator it = - send_streams_.begin(); - it != send_streams_.end(); - ++it) { - it->second->OnCpuResolutionRequest(load == kOveruse - ? CoordinatedVideoAdapter::DOWNGRADE - : CoordinatedVideoAdapter::UPGRADE); + // OnLoadUpdate can not take any locks that are held while creating streams + // etc. Doing so establishes lock-order inversions between the webrtc process + // thread on stream creation and locks such as stream_crit_ while calling out. + rtc::CritScope stream_lock(&capturer_crit_); + if (!signal_cpu_adaptation_) + return; + for (auto& kv : capturers_) { + if (kv.second != nullptr && kv.second->video_adapter() != nullptr) { + kv.second->video_adapter()->OnCpuResolutionRequest( + load == kOveruse ? CoordinatedVideoAdapter::DOWNGRADE + : CoordinatedVideoAdapter::UPGRADE); + } } } @@ -1962,18 +1977,6 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetMaxBitrateBps( last_dimensions_.width = 0; SetDimensions(width, last_dimensions_.height, last_dimensions_.is_screencast); } -void WebRtcVideoChannel2::WebRtcVideoSendStream::OnCpuResolutionRequest( - CoordinatedVideoAdapter::AdaptRequest adapt_request) { - rtc::CritScope cs(&lock_); - bool adapt_cpu; - parameters_.options.cpu_overuse_detection.Get(&adapt_cpu); - if (!adapt_cpu) - return; - if (capturer_ == NULL || capturer_->video_adapter() == NULL) - return; - - capturer_->video_adapter()->OnCpuResolutionRequest(adapt_request); -} void WebRtcVideoChannel2::WebRtcVideoSendStream::RecreateWebRtcStream() { if (stream_ != NULL) { diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h index 3254015ca9..321614dd3e 100644 --- a/talk/media/webrtc/webrtcvideoengine2.h +++ b/talk/media/webrtc/webrtcvideoengine2.h @@ -295,9 +295,6 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, void SetMaxBitrateBps(int max_bitrate_bps); - void OnCpuResolutionRequest( - CoordinatedVideoAdapter::AdaptRequest adapt_request); - private: // Parameters needed to reconstruct the underlying stream. // webrtc::VideoSendStream doesn't support setting a lot of options on the @@ -497,6 +494,13 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, DefaultUnsignalledSsrcHandler default_unsignalled_ssrc_handler_; UnsignalledSsrcHandler* const unsignalled_ssrc_handler_; + // Separate list of set capturers used to signal CPU adaptation. These should + // not be locked while calling methods that take other locks to prevent + // lock-order inversions. + rtc::CriticalSection capturer_crit_; + bool signal_cpu_adaptation_ GUARDED_BY(capturer_crit_); + std::map capturers_ GUARDED_BY(capturer_crit_); + rtc::CriticalSection stream_crit_; // Using primary-ssrc (first ssrc) as key. std::map send_streams_