WebRtcVideoCapturer: Getting rid of the |critical_section_stopping_| lock and all of its critical sections.

This avoids a deadlock in WebRtcVideoCapturer.
The deadlock could occur because OnIncomingFrame() has the |critical_section_stopping_| lock, which could block a Stop() on the |start_thread_|. When OnIncomingFrame() then tries to do synchronous invoke on |start_thread_| (before releasing said lock) we have a deadlock.

BUG=4670
R=magjed@webrtc.org, tommi@webrtc.org

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

Cr-Commit-Position: refs/heads/master@{#9294}

talk/media/webrtc/webrtcvideocapturer.cc

@@ -129,17 +129,19 @@ static bool FormatToCapability(const VideoFormat& format,
 
 WebRtcVideoCapturer::WebRtcVideoCapturer()
     : factory_(new WebRtcVcmFactory),
-      module_(NULL),
+      module_(nullptr),
       captured_frames_(0),
-      start_thread_(nullptr) {
+      start_thread_(nullptr),
+      async_invoker_(nullptr) {
   set_frame_factory(new WebRtcVideoFrameFactory());
 }
 
 WebRtcVideoCapturer::WebRtcVideoCapturer(WebRtcVcmFactoryInterface* factory)
     : factory_(factory),
-      module_(NULL),
+      module_(nullptr),
       captured_frames_(0),
-      start_thread_(nullptr) {
+      start_thread_(nullptr),
+      async_invoker_(nullptr) {
   set_frame_factory(new WebRtcVideoFrameFactory());
 }
 
@@ -281,16 +283,17 @@ CaptureState WebRtcVideoCapturer::Start(const VideoFormat& capture_format) {
     LOG(LS_ERROR) << "The capturer has not been initialized";
     return CS_NO_DEVICE;
   }
-
+  if (start_thread_) {
-  rtc::CritScope cs(&critical_section_stopping_);
-  if (IsRunning()) {
     LOG(LS_ERROR) << "The capturer is already running";
+    DCHECK(start_thread_->IsCurrent())
+        << "Trying to start capturer on different threads";
     return CS_FAILED;
   }
 
-  DCHECK(!start_thread_);
-
   start_thread_ = rtc::Thread::Current();
+  DCHECK(!async_invoker_);
+  async_invoker_.reset(new rtc::AsyncInvoker());
+  captured_frames_ = 0;
 
   SetCaptureFormat(&capture_format);
 
@@ -300,44 +303,50 @@ CaptureState WebRtcVideoCapturer::Start(const VideoFormat& capture_format) {
     return CS_FAILED;
   }
 
-  std::string camera_id(GetId());
   uint32 start = rtc::Time();
   module_->RegisterCaptureDataCallback(*this);
   if (module_->StartCapture(cap) != 0) {
-    LOG(LS_ERROR) << "Camera '" << camera_id << "' failed to start";
+    LOG(LS_ERROR) << "Camera '" << GetId() << "' failed to start";
+    module_->DeRegisterCaptureDataCallback();
+    async_invoker_.reset();
+    SetCaptureFormat(nullptr);
     start_thread_ = nullptr;
     return CS_FAILED;
   }
 
-  LOG(LS_INFO) << "Camera '" << camera_id << "' started with format "
+  LOG(LS_INFO) << "Camera '" << GetId() << "' started with format "
                << capture_format.ToString() << ", elapsed time "
                << rtc::TimeSince(start) << " ms";
 
-  captured_frames_ = 0;
   SetCaptureState(CS_RUNNING);
   return CS_STARTING;
 }
 
-// Critical section blocks Stop from shutting down during callbacks from capture
-// thread to OnIncomingCapturedFrame. Note that the crit is try-locked in
-// OnFrameCaptured, as the lock ordering between this and the system component
-// controlling the camera is reversed: system frame -> OnIncomingCapturedFrame;
-// Stop -> system stop camera).
 void WebRtcVideoCapturer::Stop() {
-  rtc::CritScope cs(&critical_section_stopping_);
+  if (!start_thread_) {
-  if (IsRunning()) {
+    LOG(LS_ERROR) << "The capturer is already stopped";
+    return;
+  }
   DCHECK(start_thread_);
-    rtc::Thread::Current()->Clear(this);
+  DCHECK(start_thread_->IsCurrent());
+  DCHECK(async_invoker_);
+  if (IsRunning()) {
+    // The module is responsible for OnIncomingCapturedFrame being called, if
+    // we stop it we will get no further callbacks.
     module_->StopCapture();
+  }
   module_->DeRegisterCaptureDataCallback();
 
   // TODO(juberti): Determine if the VCM exposes any drop stats we can use.
   double drop_ratio = 0.0;
-    std::string camera_id(GetId());
+  LOG(LS_INFO) << "Camera '" << GetId() << "' stopped after capturing "
-    LOG(LS_INFO) << "Camera '" << camera_id << "' stopped after capturing "
                << captured_frames_ << " frames and dropping "
                << drop_ratio << "%";
-  }
+
+  // Clear any pending async invokes (that OnIncomingCapturedFrame may have
+  // caused).
+  async_invoker_.reset();
+
   SetCaptureFormat(NULL);
   start_thread_ = nullptr;
 }
@@ -362,37 +371,22 @@ bool WebRtcVideoCapturer::GetPreferredFourccs(
 void WebRtcVideoCapturer::OnIncomingCapturedFrame(
     const int32_t id,
     const webrtc::I420VideoFrame& sample) {
-  // This would be a normal CritScope, except that it's possible that:
+  // This can only happen between Start() and Stop().
-  // (1) whatever system component producing this frame has taken a lock, and
+  DCHECK(start_thread_);
-  // (2) Stop() probably calls back into that system component, which may take
+  DCHECK(async_invoker_);
-  // the same lock. Due to the reversed order, we have to try-lock in order to
-  // avoid a potential deadlock. Besides, if we can't enter because we're
-  // stopping, we may as well drop the frame.
-  rtc::TryCritScope cs(&critical_section_stopping_);
-  if (!cs.locked() || !IsRunning()) {
-    // Capturer has been stopped or is in the process of stopping.
-    return;
-  }
-
-  ++captured_frames_;
-  // Log the size and pixel aspect ratio of the first captured frame.
-  if (1 == captured_frames_) {
-    LOG(LS_INFO) << "Captured frame size "
-                 << sample.width() << "x" << sample.height()
-                 << ". Expected format " << GetCaptureFormat()->ToString();
-  }
-
   if (start_thread_->IsCurrent()) {
-    SignalFrameCapturedOnStartThread(&sample);
+    SignalFrameCapturedOnStartThread(sample);
   } else {
     // This currently happens on with at least VideoCaptureModuleV4L2 and
     // possibly other implementations of WebRTC's VideoCaptureModule.
     // In order to maintain the threading contract with the upper layers and
     // consistency with other capturers such as in Chrome, we need to do a
     // thread hop.
-    start_thread_->Invoke<void>(
+    // Note that Stop() can cause the async invoke call to be cancelled.
+    async_invoker_->AsyncInvoke<void>(start_thread_,
+        // Note that this results in a shallow copying of the frame.
         rtc::Bind(&WebRtcVideoCapturer::SignalFrameCapturedOnStartThread,
-                  this, &sample));
+                  this, sample));
   }
 }
 
@@ -402,18 +396,30 @@ void WebRtcVideoCapturer::OnCaptureDelayChanged(const int32_t id,
 }
 
 void WebRtcVideoCapturer::SignalFrameCapturedOnStartThread(
-    const webrtc::I420VideoFrame* frame) {
+    const webrtc::I420VideoFrame frame) {
+  // This can only happen between Start() and Stop().
+  DCHECK(start_thread_);
   DCHECK(start_thread_->IsCurrent());
+  DCHECK(async_invoker_);
+
+  ++captured_frames_;
+  // Log the size and pixel aspect ratio of the first captured frame.
+  if (1 == captured_frames_) {
+    LOG(LS_INFO) << "Captured frame size "
+                 << frame.width() << "x" << frame.height()
+                 << ". Expected format " << GetCaptureFormat()->ToString();
+  }
+
   // Signal down stream components on captured frame.
   // The CapturedFrame class doesn't support planes. We have to ExtractBuffer
   // to one block for it.
   size_t length =
-      webrtc::CalcBufferSize(webrtc::kI420, frame->width(), frame->height());
+      webrtc::CalcBufferSize(webrtc::kI420, frame.width(), frame.height());
   capture_buffer_.resize(length);
   // TODO(magjed): Refactor the WebRtcCapturedFrame to avoid memory copy or
   // take over ownership of the buffer held by |frame| if that's possible.
-  webrtc::ExtractBuffer(*frame, length, &capture_buffer_[0]);
+  webrtc::ExtractBuffer(frame, length, &capture_buffer_[0]);
-  WebRtcCapturedFrame webrtc_frame(*frame, &capture_buffer_[0], length);
+  WebRtcCapturedFrame webrtc_frame(frame, &capture_buffer_[0], length);
   SignalFrameCaptured(this, &webrtc_frame);
 }
 

talk/media/webrtc/webrtcvideocapturer.h

@@ -35,8 +35,9 @@
 
 #include "talk/media/base/videocapturer.h"
 #include "talk/media/webrtc/webrtcvideoframe.h"
-#include "webrtc/base/criticalsection.h"
+#include "webrtc/base/asyncinvoker.h"
 #include "webrtc/base/messagehandler.h"
+#include "webrtc/base/scoped_ptr.h"
 #include "webrtc/common_video/libyuv/include/webrtc_libyuv.h"
 #include "webrtc/modules/video_capture/include/video_capture.h"
 
@@ -91,7 +92,7 @@ class WebRtcVideoCapturer : public VideoCapturer,
   // directly from OnIncomingCapturedFrame.
   // TODO(tommi): Remove this workaround when we've updated the WebRTC capturers
   // to follow the same contract.
-  void SignalFrameCapturedOnStartThread(const webrtc::I420VideoFrame* frame);
+  void SignalFrameCapturedOnStartThread(const webrtc::I420VideoFrame frame);
 
   rtc::scoped_ptr<WebRtcVcmFactoryInterface> factory_;
   webrtc::VideoCaptureModule* module_;
@@ -99,8 +100,7 @@ class WebRtcVideoCapturer : public VideoCapturer,
   std::vector<uint8_t> capture_buffer_;
   rtc::Thread* start_thread_;  // Set in Start(), unset in Stop();
 
-  // Critical section to avoid Stop during an OnIncomingCapturedFrame callback.
+  rtc::scoped_ptr<rtc::AsyncInvoker> async_invoker_;
-  rtc::CriticalSection critical_section_stopping_;
 };
 
 struct WebRtcCapturedFrame : public CapturedFrame {