diff --git a/talk/libjingle_tests.gyp b/talk/libjingle_tests.gyp index 4a6f909c94..5b70088b5b 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 06f4b83b93..a68c91c489 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 @@ 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 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; diff --git a/talk/media/base/capturemanager.h b/talk/media/base/capturemanager.h index 16e80918ee..680300d51e 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 @@ 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_; }; diff --git a/talk/session/media/channelmanager.cc b/talk/session/media/channelmanager.cc index b507d4be17..eb74df6bf3 100644 --- a/talk/session/media/channelmanager.cc +++ b/talk/session/media/channelmanager.cc @@ -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(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() { diff --git a/talk/session/media/channelmanager.h b/talk/session/media/channelmanager.h index 6bbfc5a1fb..ae9224a578 100644 --- a/talk/session/media/channelmanager.h +++ b/talk/session/media/channelmanager.h @@ -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);