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}
diff --git a/talk/media/webrtc/webrtcvideocapturer.cc b/talk/media/webrtc/webrtcvideocapturer.cc
index a6a0176..8d8270a 100644
--- a/talk/media/webrtc/webrtcvideocapturer.cc
+++ b/talk/media/webrtc/webrtcvideocapturer.cc
@@ -129,17 +129,19 @@
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 @@
LOG(LS_ERROR) << "The capturer has not been initialized";
return CS_NO_DEVICE;
}
-
- rtc::CritScope cs(&critical_section_stopping_);
- if (IsRunning()) {
+ if (start_thread_) {
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 @@
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 (IsRunning()) {
- DCHECK(start_thread_);
- rtc::Thread::Current()->Clear(this);
- 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 '" << camera_id << "' stopped after capturing "
- << captured_frames_ << " frames and dropping "
- << drop_ratio << "%";
+ if (!start_thread_) {
+ LOG(LS_ERROR) << "The capturer is already stopped";
+ return;
}
+ DCHECK(start_thread_);
+ 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;
+ LOG(LS_INFO) << "Camera '" << GetId() << "' 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 @@
void WebRtcVideoCapturer::OnIncomingCapturedFrame(
const int32_t id,
const webrtc::I420VideoFrame& sample) {
- // This would be a normal CritScope, except that it's possible that:
- // (1) whatever system component producing this frame has taken a lock, and
- // (2) Stop() probably calls back into that system component, which may take
- // 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();
- }
-
+ // This can only happen between Start() and Stop().
+ DCHECK(start_thread_);
+ DCHECK(async_invoker_);
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::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]);
- WebRtcCapturedFrame webrtc_frame(*frame, &capture_buffer_[0], length);
+ webrtc::ExtractBuffer(frame, length, &capture_buffer_[0]);
+ WebRtcCapturedFrame webrtc_frame(frame, &capture_buffer_[0], length);
SignalFrameCaptured(this, &webrtc_frame);
}
diff --git a/talk/media/webrtc/webrtcvideocapturer.h b/talk/media/webrtc/webrtcvideocapturer.h
index 56896f9..737a7b2 100644
--- a/talk/media/webrtc/webrtcvideocapturer.h
+++ b/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 @@
// 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 @@
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::CriticalSection critical_section_stopping_;
+ rtc::scoped_ptr<rtc::AsyncInvoker> async_invoker_;
};
struct WebRtcCapturedFrame : public CapturedFrame {
