Add thread checks to the CaptureManager.
It looks like it is being used single threadedly, except that in some cases it is created and/or destroyed in threads other than the one running its operations. As such, CaptureManager() contains 'thread_checker_.DetachFromThread()' and ~CaptureManager() does not have a DCHECK.
BUG=
R=perkj@webrtc.org, tommi@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/36279004
Cr-Commit-Position: refs/heads/master@{#8498}
git-svn-id: http://webrtc.googlecode.com/svn/trunk@8498 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/talk/libjingle_tests.gyp b/talk/libjingle_tests.gyp
index 4a6f909..5b70088 100755
--- a/talk/libjingle_tests.gyp
+++ b/talk/libjingle_tests.gyp
@@ -92,8 +92,7 @@
'libjingle_unittest_main',
],
'sources': [
- # TODO(ronghuawu): Reenable this test.
- # 'media/base/capturemanager_unittest.cc',
+ 'media/base/capturemanager_unittest.cc',
'media/base/codec_unittest.cc',
'media/base/filemediaengine_unittest.cc',
'media/base/rtpdataengine_unittest.cc',
diff --git a/talk/media/base/capturemanager.cc b/talk/media/base/capturemanager.cc
index 06f4b83..a68c91c 100644
--- a/talk/media/base/capturemanager.cc
+++ b/talk/media/base/capturemanager.cc
@@ -32,6 +32,7 @@
#include "talk/media/base/videocapturer.h"
#include "talk/media/base/videoprocessor.h"
#include "talk/media/base/videorenderer.h"
+#include "webrtc/base/checks.h"
#include "webrtc/base/logging.h"
namespace cricket {
@@ -50,10 +51,19 @@
int IncCaptureStartRef();
int DecCaptureStartRef();
- CaptureRenderAdapter* adapter() { return adapter_.get(); }
- VideoCapturer* GetVideoCapturer() { return adapter()->video_capturer(); }
+ CaptureRenderAdapter* adapter() {
+ DCHECK(thread_checker_.CalledOnValidThread());
+ return adapter_.get();
+ }
+ VideoCapturer* GetVideoCapturer() {
+ DCHECK(thread_checker_.CalledOnValidThread());
+ return adapter()->video_capturer();
+ }
- int start_count() const { return start_count_; }
+ int start_count() const {
+ DCHECK(thread_checker_.CalledOnValidThread());
+ return start_count_;
+ }
private:
struct CaptureResolutionInfo {
@@ -64,6 +74,7 @@
explicit VideoCapturerState(CaptureRenderAdapter* adapter);
+ rtc::ThreadChecker thread_checker_;
rtc::scoped_ptr<CaptureRenderAdapter> adapter_;
int start_count_;
@@ -77,6 +88,7 @@
VideoCapturerState::VideoCapturerState(CaptureRenderAdapter* adapter)
: adapter_(adapter), start_count_(1) {}
+// static
VideoCapturerState* VideoCapturerState::Create(VideoCapturer* video_capturer) {
CaptureRenderAdapter* adapter = CaptureRenderAdapter::Create(video_capturer);
if (!adapter) {
@@ -87,6 +99,7 @@
void VideoCapturerState::AddCaptureResolution(
const VideoFormat& desired_format) {
+ DCHECK(thread_checker_.CalledOnValidThread());
for (CaptureFormats::iterator iter = capture_formats_.begin();
iter != capture_formats_.end(); ++iter) {
if (desired_format == iter->video_format) {
@@ -99,6 +112,7 @@
}
bool VideoCapturerState::RemoveCaptureResolution(const VideoFormat& format) {
+ DCHECK(thread_checker_.CalledOnValidThread());
for (CaptureFormats::iterator iter = capture_formats_.begin();
iter != capture_formats_.end(); ++iter) {
if (format == iter->video_format) {
@@ -114,6 +128,7 @@
VideoFormat VideoCapturerState::GetHighestFormat(
VideoCapturer* video_capturer) const {
+ DCHECK(thread_checker_.CalledOnValidThread());
VideoFormat highest_format(0, 0, VideoFormat::FpsToInterval(1), FOURCC_ANY);
if (capture_formats_.empty()) {
VideoFormat default_format(kDefaultCaptureFormat);
@@ -134,9 +149,13 @@
return highest_format;
}
-int VideoCapturerState::IncCaptureStartRef() { return ++start_count_; }
+int VideoCapturerState::IncCaptureStartRef() {
+ DCHECK(thread_checker_.CalledOnValidThread());
+ return ++start_count_;
+}
int VideoCapturerState::DecCaptureStartRef() {
+ DCHECK(thread_checker_.CalledOnValidThread());
if (start_count_ > 0) {
// Start count may be 0 if a capturer was added but never started.
--start_count_;
@@ -144,25 +163,25 @@
return start_count_;
}
+CaptureManager::CaptureManager() {
+ // Allowing construction of manager in any thread as long as subsequent calls
+ // are all from the same thread.
+ thread_checker_.DetachFromThread();
+}
+
CaptureManager::~CaptureManager() {
+ DCHECK(thread_checker_.CalledOnValidThread());
+
// Since we don't own any of the capturers, all capturers should have been
// cleaned up before we get here. In fact, in the normal shutdown sequence,
// all capturers *will* be shut down by now, so trying to stop them here
// will crash. If we're still tracking any, it's a dangling pointer.
- if (!capture_states_.empty()) {
- ASSERT(false &&
- "CaptureManager destructing while still tracking capturers!");
- // Delete remaining VideoCapturerStates, but don't touch the capturers.
- do {
- CaptureStates::iterator it = capture_states_.begin();
- delete it->second;
- capture_states_.erase(it);
- } while (!capture_states_.empty());
- }
+ CHECK(capture_states_.empty());
}
bool CaptureManager::StartVideoCapture(VideoCapturer* video_capturer,
const VideoFormat& desired_format) {
+ DCHECK(thread_checker_.CalledOnValidThread());
if (desired_format.width == 0 || desired_format.height == 0) {
return false;
}
@@ -195,6 +214,7 @@
bool CaptureManager::StopVideoCapture(VideoCapturer* video_capturer,
const VideoFormat& format) {
+ DCHECK(thread_checker_.CalledOnValidThread());
VideoCapturerState* capture_state = GetCaptureState(video_capturer);
if (!capture_state) {
return false;
@@ -215,6 +235,7 @@
const VideoFormat& previous_format,
const VideoFormat& desired_format,
CaptureManager::RestartOptions options) {
+ DCHECK(thread_checker_.CalledOnValidThread());
if (!IsCapturerRegistered(video_capturer)) {
LOG(LS_ERROR) << "RestartVideoCapture: video_capturer is not registered.";
return false;
@@ -267,6 +288,7 @@
bool CaptureManager::AddVideoRenderer(VideoCapturer* video_capturer,
VideoRenderer* video_renderer) {
+ DCHECK(thread_checker_.CalledOnValidThread());
if (!video_capturer || !video_renderer) {
return false;
}
@@ -279,6 +301,7 @@
bool CaptureManager::RemoveVideoRenderer(VideoCapturer* video_capturer,
VideoRenderer* video_renderer) {
+ DCHECK(thread_checker_.CalledOnValidThread());
if (!video_capturer || !video_renderer) {
return false;
}
@@ -291,6 +314,7 @@
bool CaptureManager::AddVideoProcessor(VideoCapturer* video_capturer,
VideoProcessor* video_processor) {
+ DCHECK(thread_checker_.CalledOnValidThread());
if (!video_capturer || !video_processor) {
return false;
}
@@ -303,6 +327,7 @@
bool CaptureManager::RemoveVideoProcessor(VideoCapturer* video_capturer,
VideoProcessor* video_processor) {
+ DCHECK(thread_checker_.CalledOnValidThread());
if (!video_capturer || !video_processor) {
return false;
}
@@ -313,10 +338,12 @@
}
bool CaptureManager::IsCapturerRegistered(VideoCapturer* video_capturer) const {
+ DCHECK(thread_checker_.CalledOnValidThread());
return GetCaptureState(video_capturer) != NULL;
}
bool CaptureManager::RegisterVideoCapturer(VideoCapturer* video_capturer) {
+ DCHECK(thread_checker_.CalledOnValidThread());
VideoCapturerState* capture_state =
VideoCapturerState::Create(video_capturer);
if (!capture_state) {
@@ -329,6 +356,7 @@
void CaptureManager::UnregisterVideoCapturer(
VideoCapturerState* capture_state) {
+ DCHECK(thread_checker_.CalledOnValidThread());
VideoCapturer* video_capturer = capture_state->GetVideoCapturer();
capture_states_.erase(video_capturer);
delete capture_state;
@@ -351,6 +379,7 @@
bool CaptureManager::StartWithBestCaptureFormat(
VideoCapturerState* capture_state, VideoCapturer* video_capturer) {
+ DCHECK(thread_checker_.CalledOnValidThread());
VideoFormat highest_asked_format =
capture_state->GetHighestFormat(video_capturer);
VideoFormat capture_format;
@@ -377,6 +406,7 @@
VideoCapturerState* CaptureManager::GetCaptureState(
VideoCapturer* video_capturer) const {
+ DCHECK(thread_checker_.CalledOnValidThread());
CaptureStates::const_iterator iter = capture_states_.find(video_capturer);
if (iter == capture_states_.end()) {
return NULL;
@@ -386,6 +416,7 @@
CaptureRenderAdapter* CaptureManager::GetAdapter(
VideoCapturer* video_capturer) const {
+ DCHECK(thread_checker_.CalledOnValidThread());
VideoCapturerState* capture_state = GetCaptureState(video_capturer);
if (!capture_state) {
return NULL;
diff --git a/talk/media/base/capturemanager.h b/talk/media/base/capturemanager.h
index 16e8091..680300d 100644
--- a/talk/media/base/capturemanager.h
+++ b/talk/media/base/capturemanager.h
@@ -49,6 +49,7 @@
#include "talk/media/base/capturerenderadapter.h"
#include "talk/media/base/videocommon.h"
#include "webrtc/base/sigslotrepeater.h"
+#include "webrtc/base/thread_checker.h"
namespace cricket {
@@ -64,7 +65,7 @@
kForceRestart
};
- CaptureManager() {}
+ CaptureManager();
virtual ~CaptureManager();
virtual bool StartVideoCapture(VideoCapturer* video_capturer,
@@ -106,6 +107,7 @@
VideoCapturerState* GetCaptureState(VideoCapturer* video_capturer) const;
CaptureRenderAdapter* GetAdapter(VideoCapturer* video_capturer) const;
+ rtc::ThreadChecker thread_checker_;
CaptureStates capture_states_;
};
diff --git a/talk/session/media/channelmanager.cc b/talk/session/media/channelmanager.cc
index b507d4b..eb74df6 100644
--- a/talk/session/media/channelmanager.cc
+++ b/talk/session/media/channelmanager.cc
@@ -149,10 +149,10 @@
// shutdown.
ShutdownSrtp();
}
- // Always delete the media engine on the worker thread to match how it was
- // created.
+ // Some deletes need to be on the worker thread for thread safe destruction,
+ // this includes the media engine and capture manager.
worker_thread_->Invoke<void>(Bind(
- &ChannelManager::DeleteMediaEngine_w, this));
+ &ChannelManager::DestructorDeletes_w, this));
}
bool ChannelManager::SetVideoRtxEnabled(bool enable) {
@@ -310,9 +310,10 @@
initialized_ = false;
}
-void ChannelManager::DeleteMediaEngine_w() {
+void ChannelManager::DestructorDeletes_w() {
ASSERT(worker_thread_ == rtc::Thread::Current());
media_engine_.reset(NULL);
+ capture_manager_.reset(NULL);
}
void ChannelManager::Terminate_w() {
diff --git a/talk/session/media/channelmanager.h b/talk/session/media/channelmanager.h
index 6bbfc5a..ae9224a 100644
--- a/talk/session/media/channelmanager.h
+++ b/talk/session/media/channelmanager.h
@@ -268,7 +268,7 @@
CaptureManager* cm,
rtc::Thread* worker_thread);
bool InitMediaEngine_w();
- void DeleteMediaEngine_w();
+ void DestructorDeletes_w();
void Terminate_w();
VoiceChannel* CreateVoiceChannel_w(
BaseSession* session, const std::string& content_name, bool rtcp);
