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}
diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc
index 939df41..42fe072 100644
--- a/talk/media/webrtc/webrtcvideoengine2.cc
+++ b/talk/media/webrtc/webrtcvideoengine2.cc
@@ -633,6 +633,7 @@
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 @@
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 @@
!FindHeaderExtension(send_rtp_extensions_,
kRtpVideoRotationHeaderExtension));
}
+ {
+ rtc::CritScope lock(&capturer_crit_);
+ capturers_[ssrc] = capturer;
+ }
return true;
}
@@ -1333,6 +1340,10 @@
// 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::OnLoadUpdate(Load load) {
- rtc::CritScope stream_lock(&stream_crit_);
- for (std::map<uint32, WebRtcVideoSendStream*>::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 @@
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 3254015..321614d 100644
--- a/talk/media/webrtc/webrtcvideoengine2.h
+++ b/talk/media/webrtc/webrtcvideoengine2.h
@@ -295,9 +295,6 @@
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 @@
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<uint32, VideoCapturer*> capturers_ GUARDED_BY(capturer_crit_);
+
rtc::CriticalSection stream_crit_;
// Using primary-ssrc (first ssrc) as key.
std::map<uint32, WebRtcVideoSendStream*> send_streams_
