From a717ee9962ac7c72a2fa00d63a83d7d3d4c3a131 Mon Sep 17 00:00:00 2001 From: "wuchengli@chromium.org" Date: Fri, 9 Aug 2013 04:08:38 +0000 Subject: [PATCH] Avoid acquiring VCM::_receiveCritSect during decode callback. When VideoDecoder::Decode, Reset, or Release is called, VideoCodingModuleImpl::_receiveCritSect may have been acquired. Decode callback needs to acquire the same lock in ViEChannel::FrameToRender. It is not a problem for SW decode because decode callback is run on the same WebRTC decoding thread and the lock is re-entrant. But for HW decode, decode callback is run on a thread different from WebRTC decoding thread. Decode callback gets the locks in the opposite order. Deadlock can happen. BUG=http://crbug.com/170345 TEST=Try apprtc.appspot.com/?debug=loopback on ARM Chromebook Daisy. R=stefan@webrtc.org, wu@webrtc.org Review URL: https://webrtc-codereview.appspot.com/1977004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@4509 4adac7df-926f-26a2-2b94-8c16560cd09d --- webrtc/video_engine/vie_channel.cc | 37 ++++++++++++++++-------------- webrtc/video_engine/vie_channel.h | 1 + 2 files changed, 21 insertions(+), 17 deletions(-) diff --git a/webrtc/video_engine/vie_channel.cc b/webrtc/video_engine/vie_channel.cc index 874db7c4c6..4351815a95 100644 --- a/webrtc/video_engine/vie_channel.cc +++ b/webrtc/video_engine/vie_channel.cc @@ -1665,6 +1665,10 @@ CallStatsObserver* ViEChannel::GetStatsObserver() { return stats_observer_.get(); } +// Do not acquire the lock of |vcm_| in this function. Decode callback won't +// necessarily be called from the decoding thread. The decoding thread may have +// held the lock when calling VideoDecoder::Decode, Reset, or Release. Acquiring +// the same lock in the path of decode callback can deadlock. int32_t ViEChannel::FrameToRender( I420VideoFrame& video_frame) { // NOLINT CriticalSectionScoped cs(callback_cs_.get()); @@ -1672,20 +1676,11 @@ int32_t ViEChannel::FrameToRender( if (decoder_reset_) { // Trigger a callback to the user if the incoming codec has changed. if (codec_observer_) { - VideoCodec decoder; - memset(&decoder, 0, sizeof(decoder)); - if (vcm_.ReceiveCodec(&decoder) == VCM_OK) { - // VCM::ReceiveCodec returns the codec set by - // RegisterReceiveCodec, which might not be the size we're - // actually decoding. - decoder.width = static_cast(video_frame.width()); - decoder.height = static_cast(video_frame.height()); - codec_observer_->IncomingCodecChanged(channel_id_, decoder); - } else { - assert(false); - WEBRTC_TRACE(kTraceInfo, kTraceVideo, ViEId(engine_id_, channel_id_), - "%s: Could not get receive codec", __FUNCTION__); - } + // VCM::ReceiveCodec returns the codec set by RegisterReceiveCodec, which + // might not be the size we're actually decoding. + receive_codec_.width = static_cast(video_frame.width()); + receive_codec_.height = static_cast(video_frame.height()); + codec_observer_->IncomingCodecChanged(channel_id_, receive_codec_); } decoder_reset_ = false; } @@ -1949,9 +1944,17 @@ int32_t ViEChannel::OnInitializeDecoder( payload_type, payload_name); vcm_.ResetDecoder(); - callback_cs_->Enter(); - decoder_reset_ = true; - callback_cs_->Leave(); + VideoCodec receive_codec; + memset(&receive_codec, 0, sizeof(receive_codec)); + if (vcm_.ReceiveCodec(&receive_codec) == VCM_OK) { + CriticalSectionScoped cs(callback_cs_.get()); + decoder_reset_ = true; + receive_codec_ = receive_codec; + } else { + assert(false); + WEBRTC_TRACE(kTraceInfo, kTraceVideo, ViEId(engine_id_, channel_id_), + "%s: Could not get receive codec", __FUNCTION__); + } return 0; } diff --git a/webrtc/video_engine/vie_channel.h b/webrtc/video_engine/vie_channel.h index 090582b9cf..6f1a639ffc 100644 --- a/webrtc/video_engine/vie_channel.h +++ b/webrtc/video_engine/vie_channel.h @@ -383,6 +383,7 @@ class ViEChannel Transport* external_transport_; bool decoder_reset_; + VideoCodec receive_codec_; bool wait_for_key_frame_; ThreadWrapper* decode_thread_;