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.
     Run libjingle_peerconnection_unittest.

R=stefan@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/1997005

git-svn-id: http://webrtc.googlecode.com/svn/trunk@4523 4adac7df-926f-26a2-2b94-8c16560cd09d

webrtc/modules/video_coding/main/interface/video_coding_defines.h

@@ -98,6 +98,8 @@ class VCMReceiveCallback {
       const uint64_t pictureId) {
     return -1;
   }
+  // Called when the current receive codec changes.
+  virtual void IncomingCodecChanged(const VideoCodec& codec) {}
 
  protected:
   virtual ~VCMReceiveCallback() {

webrtc/modules/video_coding/main/source/codec_database.cc

@@ -510,6 +510,8 @@ VCMGenericDecoder* VCMCodecDataBase::GetDecoder(
   if (!ptr_decoder_) {
     return NULL;
   }
+  VCMReceiveCallback* callback = decoded_frame_callback->UserReceiveCallback();
+  if (callback) callback->IncomingCodecChanged(receive_codec_);
   if (ptr_decoder_->RegisterDecodeCompleteCallback(decoded_frame_callback)
       < 0) {
     ReleaseDecoder(ptr_decoder_);

webrtc/modules/video_coding/main/source/generic_decoder.cc

@@ -41,6 +41,12 @@ void VCMDecodedFrameCallback::SetUserReceiveCallback(
     _receiveCallback = receiveCallback;
 }
 
+VCMReceiveCallback* VCMDecodedFrameCallback::UserReceiveCallback()
+{
+    CriticalSectionScoped cs(_critSect);
+    return _receiveCallback;
+}
+
 int32_t VCMDecodedFrameCallback::Decoded(I420VideoFrame& decodedImage)
 {
     // TODO(holmer): We should improve this so that we can handle multiple

webrtc/modules/video_coding/main/source/generic_decoder.h

@@ -37,6 +37,7 @@ public:
     VCMDecodedFrameCallback(VCMTiming& timing, Clock* clock);
     virtual ~VCMDecodedFrameCallback();
     void SetUserReceiveCallback(VCMReceiveCallback* receiveCallback);
+    VCMReceiveCallback* UserReceiveCallback();
 
     virtual int32_t Decoded(I420VideoFrame& decodedImage);
     virtual int32_t ReceivedDecodedReferenceFrame(const uint64_t pictureId);

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<uint16_t>(video_frame.width());
-        decoder.height = static_cast<uint16_t>(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__);
-      }
+      // The codec set by RegisterReceiveCodec might not be the size we're
+      // actually decoding.
+      receive_codec_.width = static_cast<uint16_t>(video_frame.width());
+      receive_codec_.height = static_cast<uint16_t>(video_frame.height());
+      codec_observer_->IncomingCodecChanged(channel_id_, receive_codec_);
     }
     decoder_reset_ = false;
   }
@@ -1723,6 +1718,11 @@ int32_t ViEChannel::ReceivedDecodedReferenceFrame(
   return rtp_rtcp_->SendRTCPReferencePictureSelection(picture_id);
 }
 
+void ViEChannel::IncomingCodecChanged(const VideoCodec& codec) {
+  CriticalSectionScoped cs(callback_cs_.get());
+  receive_codec_ = codec;
+}
+
 int32_t ViEChannel::StoreReceivedFrame(
   const EncodedVideoData& frame_to_store) {
   return 0;
@@ -1949,9 +1949,8 @@ int32_t ViEChannel::OnInitializeDecoder(
                payload_type, payload_name);
   vcm_.ResetDecoder();
 
-  callback_cs_->Enter();
+  CriticalSectionScoped cs(callback_cs_.get());
   decoder_reset_ = true;
-  callback_cs_->Leave();
   return 0;
 }
 

webrtc/video_engine/vie_channel.h

@@ -290,6 +290,9 @@ class ViEChannel
   virtual int32_t ReceivedDecodedReferenceFrame(
       const uint64_t picture_id);
 
+  // Implements VCMReceiveCallback.
+  virtual void IncomingCodecChanged(const VideoCodec& codec);
+
   // Implements VCM.
   virtual int32_t StoreReceivedFrame(
       const EncodedVideoData& frame_to_store);
@@ -383,6 +386,8 @@ class ViEChannel
   Transport* external_transport_;
 
   bool decoder_reset_;
+  // Current receive codec used for codec change callback.
+  VideoCodec receive_codec_;
   bool wait_for_key_frame_;
   ThreadWrapper* decode_thread_;