generic-library/webrtc
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Add thread checks to the CaptureManager.

Browse Source 
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
This commit is contained in:
hbos@webrtc.org 2015-02-25 10:09:05 +00:00
parent 8d350d4bc4
commit 4aef5fef18
5 changed files with 55 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
talk/libjingle_tests.gyp
View File

@ -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',

59
talk/media/base/capturemanager.cc
View File

@ -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 @@ class VideoCapturerState {
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 @@ class VideoCapturerState {
explicit VideoCapturerState(CaptureRenderAdapter* adapter);
rtc::ThreadChecker thread_checker_;
rtc::scoped_ptr<CaptureRenderAdapter> adapter_;
int start_count_;
@ -77,6 +88,7 @@ const VideoFormatPod VideoCapturerState::kDefaultCaptureFormat = {
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 @@ VideoCapturerState* VideoCapturerState::Create(VideoCapturer* video_capturer) {
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 @@ void VideoCapturerState::AddCaptureResolution(
}
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 @@ bool VideoCapturerState::RemoveCaptureResolution(const VideoFormat& format) {
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 @@ VideoFormat VideoCapturerState::GetHighestFormat(
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 @@ int VideoCapturerState::DecCaptureStartRef() {
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::StartVideoCapture(VideoCapturer* video_capturer,
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 @@ bool CaptureManager::RestartVideoCapture(
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::RestartVideoCapture(
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::AddVideoRenderer(VideoCapturer* video_capturer,
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::RemoveVideoRenderer(VideoCapturer* video_capturer,
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::AddVideoProcessor(VideoCapturer* video_capturer,
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::RemoveVideoProcessor(VideoCapturer* video_capturer,
}
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 @@ bool CaptureManager::RegisterVideoCapturer(VideoCapturer* video_capturer) {
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 @@ void CaptureManager::UnregisterVideoCapturer(
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 @@ bool CaptureManager::StartWithBestCaptureFormat(
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 @@ VideoCapturerState* CaptureManager::GetCaptureState(
CaptureRenderAdapter* CaptureManager::GetAdapter(
VideoCapturer* video_capturer) const {
DCHECK(thread_checker_.CalledOnValidThread());
VideoCapturerState* capture_state = GetCaptureState(video_capturer);
if (!capture_state) {
return NULL;

4
talk/media/base/capturemanager.h
View File

@ -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 @@ class CaptureManager : public sigslot::has_slots<> {
kForceRestart
};
CaptureManager() {}
CaptureManager();
virtual ~CaptureManager();
virtual bool StartVideoCapture(VideoCapturer* video_capturer,
@ -106,6 +107,7 @@ class CaptureManager : public sigslot::has_slots<> {
VideoCapturerState* GetCaptureState(VideoCapturer* video_capturer) const;
CaptureRenderAdapter* GetAdapter(VideoCapturer* video_capturer) const;
rtc::ThreadChecker thread_checker_;
CaptureStates capture_states_;
};

9
talk/session/media/channelmanager.cc
View File

@ -149,10 +149,10 @@ ChannelManager::~ChannelManager() {
// 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 @@ void ChannelManager::Terminate() {
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() {

2
talk/session/media/channelmanager.h
View File

@ -268,7 +268,7 @@ class ChannelManager : public rtc::MessageHandler,
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);