From d60d79a14594cbc8266e4a50391ddbe64ed491f0 Mon Sep 17 00:00:00 2001 From: "pbos@webrtc.org" Date: Wed, 24 Sep 2014 07:10:57 +0000 Subject: [PATCH] Thread annotation of rtc::CriticalSection. Effectively re-lands r5516 which was reverted because talk/-only checkouts existed. This now resides in webrtc/base/, so no talk/-only checkouts should be possible. This change also enables -Wthread-safety for talk/ and fixes a bug in talk/media/webrtc/webrtcvideoengine2.cc where a guarded variable was read without taking the corresponding lock. R=andresp@webrtc.org, mflodman@webrtc.org, pthatcher@webrtc.org BUG= Review URL: https://webrtc-codereview.appspot.com/27569004 git-svn-id: http://webrtc.googlecode.com/svn/trunk@7284 4adac7df-926f-26a2-2b94-8c16560cd09d --- talk/build/common.gypi | 1 + talk/media/webrtc/webrtcvideoengine2.cc | 16 ++++++++-------- talk/media/webrtc/webrtcvideoengine2.h | 8 +++++--- talk/session/media/mediamonitor.h | 3 ++- webrtc/base/criticalsection.h | 23 ++++++++++++----------- webrtc/base/sharedexclusivelock.h | 25 ++++++++++++------------- webrtc/base/signalthread.h | 7 ++++--- 7 files changed, 44 insertions(+), 39 deletions(-) diff --git a/talk/build/common.gypi b/talk/build/common.gypi index 57b21af8a..7ee4224e4 100644 --- a/talk/build/common.gypi +++ b/talk/build/common.gypi @@ -91,6 +91,7 @@ # LateBindingSymbolTable::TableInfo from # latebindingsymboltable.cc.def and remove below flag. '-Wno-address-of-array-temporary', + '-Wthread-safety', ], }], ], diff --git a/talk/media/webrtc/webrtcvideoengine2.cc b/talk/media/webrtc/webrtcvideoengine2.cc index 31f6070b1..26e307978 100644 --- a/talk/media/webrtc/webrtcvideoengine2.cc +++ b/talk/media/webrtc/webrtcvideoengine2.cc @@ -1381,14 +1381,8 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame( << frame->GetHeight(); // Lock before copying, can be called concurrently when swapping input source. rtc::CritScope frame_cs(&frame_lock_); - if (!muted_) { - ConvertToI420VideoFrame(*frame, &video_frame_); - } else { - // Create a black frame to transmit instead. - CreateBlackFrame(&video_frame_, - static_cast(frame->GetWidth()), - static_cast(frame->GetHeight())); - } + ConvertToI420VideoFrame(*frame, &video_frame_); + rtc::CritScope cs(&lock_); if (stream_ == NULL) { LOG(LS_WARNING) << "Capturer inputting frames before send codecs are " @@ -1400,6 +1394,12 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::InputFrame( LOG(LS_VERBOSE) << "VideoFormat 0x0 set, Dropping frame."; return; } + if (muted_) { + // Create a black frame to transmit instead. + CreateBlackFrame(&video_frame_, + static_cast(frame->GetWidth()), + static_cast(frame->GetHeight())); + } // Reconfigure codec if necessary. SetDimensions( video_frame_.width(), video_frame_.height(), capturer->IsScreencast()); diff --git a/talk/media/webrtc/webrtcvideoengine2.h b/talk/media/webrtc/webrtcvideoengine2.h index a19abd58d..18a80d7ad 100644 --- a/talk/media/webrtc/webrtcvideoengine2.h +++ b/talk/media/webrtc/webrtcvideoengine2.h @@ -335,10 +335,12 @@ class WebRtcVideoChannel2 : public rtc::MessageHandler, }; void SetCodecAndOptions(const VideoCodecSettings& codec, - const VideoOptions& options); - void RecreateWebRtcStream(); + const VideoOptions& options) + EXCLUSIVE_LOCKS_REQUIRED(lock_); + void RecreateWebRtcStream() EXCLUSIVE_LOCKS_REQUIRED(lock_); // When |override_max| is false constrain width/height to codec dimensions. - void SetDimensions(int width, int height, bool override_max); + void SetDimensions(int width, int height, bool override_max) + EXCLUSIVE_LOCKS_REQUIRED(lock_); webrtc::Call* const call_; WebRtcVideoEncoderFactory2* const encoder_factory_; diff --git a/talk/session/media/mediamonitor.h b/talk/session/media/mediamonitor.h index d549362ad..89740a8d4 100644 --- a/talk/session/media/mediamonitor.h +++ b/talk/session/media/mediamonitor.h @@ -34,6 +34,7 @@ #include "webrtc/base/criticalsection.h" #include "webrtc/base/sigslot.h" #include "webrtc/base/thread.h" +#include "webrtc/base/thread_annotations.h" namespace cricket { @@ -77,7 +78,7 @@ class MediaMonitorT : public MediaMonitor { media_info_.Clear(); media_channel_->GetStats(&media_info_); } - virtual void Update() { + virtual void Update() EXCLUSIVE_LOCKS_REQUIRED(crit_) { MI stats(media_info_); crit_.Leave(); SignalUpdate(media_channel_, stats); diff --git a/webrtc/base/criticalsection.h b/webrtc/base/criticalsection.h index a950a47f5..a2d9bca0e 100644 --- a/webrtc/base/criticalsection.h +++ b/webrtc/base/criticalsection.h @@ -12,6 +12,7 @@ #define WEBRTC_BASE_CRITICALSECTION_H__ #include "webrtc/base/constructormagic.h" +#include "webrtc/base/thread_annotations.h" #if defined(WEBRTC_WIN) #include "webrtc/base/win32.h" @@ -34,7 +35,7 @@ namespace rtc { #if defined(WEBRTC_WIN) -class CriticalSection { +class LOCKABLE CriticalSection { public: CriticalSection() { InitializeCriticalSection(&crit_); @@ -44,18 +45,18 @@ class CriticalSection { ~CriticalSection() { DeleteCriticalSection(&crit_); } - void Enter() { + void Enter() EXCLUSIVE_LOCK_FUNCTION() { EnterCriticalSection(&crit_); TRACK_OWNER(thread_ = GetCurrentThreadId()); } - bool TryEnter() { + bool TryEnter() EXCLUSIVE_TRYLOCK_FUNCTION(true) { if (TryEnterCriticalSection(&crit_) != FALSE) { TRACK_OWNER(thread_ = GetCurrentThreadId()); return true; } return false; } - void Leave() { + void Leave() UNLOCK_FUNCTION() { TRACK_OWNER(thread_ = 0); LeaveCriticalSection(&crit_); } @@ -71,7 +72,7 @@ class CriticalSection { #endif // WEBRTC_WIN #if defined(WEBRTC_POSIX) -class CriticalSection { +class LOCKABLE CriticalSection { public: CriticalSection() { pthread_mutexattr_t mutex_attribute; @@ -84,18 +85,18 @@ class CriticalSection { ~CriticalSection() { pthread_mutex_destroy(&mutex_); } - void Enter() { + void Enter() EXCLUSIVE_LOCK_FUNCTION() { pthread_mutex_lock(&mutex_); TRACK_OWNER(thread_ = pthread_self()); } - bool TryEnter() { + bool TryEnter() EXCLUSIVE_TRYLOCK_FUNCTION(true) { if (pthread_mutex_trylock(&mutex_) == 0) { TRACK_OWNER(thread_ = pthread_self()); return true; } return false; } - void Leave() { + void Leave() UNLOCK_FUNCTION() { TRACK_OWNER(thread_ = 0); pthread_mutex_unlock(&mutex_); } @@ -111,13 +112,13 @@ class CriticalSection { #endif // WEBRTC_POSIX // CritScope, for serializing execution through a scope. -class CritScope { +class SCOPED_LOCKABLE CritScope { public: - explicit CritScope(CriticalSection *pcrit) { + explicit CritScope(CriticalSection *pcrit) EXCLUSIVE_LOCK_FUNCTION(pcrit) { pcrit_ = pcrit; pcrit_->Enter(); } - ~CritScope() { + ~CritScope() UNLOCK_FUNCTION() { pcrit_->Leave(); } private: diff --git a/webrtc/base/sharedexclusivelock.h b/webrtc/base/sharedexclusivelock.h index f64d7cf50..aaaba3b83 100644 --- a/webrtc/base/sharedexclusivelock.h +++ b/webrtc/base/sharedexclusivelock.h @@ -19,14 +19,14 @@ namespace rtc { // This class provides shared-exclusive lock. It can be used in cases like // multiple-readers/single-writer model. -class SharedExclusiveLock { +class LOCKABLE SharedExclusiveLock { public: SharedExclusiveLock(); // Locking/unlocking methods. It is encouraged to use SharedScope or // ExclusiveScope for protection. - void LockExclusive(); - void UnlockExclusive(); + void LockExclusive() EXCLUSIVE_LOCK_FUNCTION(); + void UnlockExclusive() UNLOCK_FUNCTION(); void LockShared(); void UnlockShared(); @@ -39,15 +39,14 @@ class SharedExclusiveLock { DISALLOW_COPY_AND_ASSIGN(SharedExclusiveLock); }; -class SharedScope { +class SCOPED_LOCKABLE SharedScope { public: - explicit SharedScope(SharedExclusiveLock* lock) : lock_(lock) { + explicit SharedScope(SharedExclusiveLock* lock) SHARED_LOCK_FUNCTION(lock) + : lock_(lock) { lock_->LockShared(); } - ~SharedScope() { - lock_->UnlockShared(); - } + ~SharedScope() UNLOCK_FUNCTION() { lock_->UnlockShared(); } private: SharedExclusiveLock* lock_; @@ -55,15 +54,15 @@ class SharedScope { DISALLOW_COPY_AND_ASSIGN(SharedScope); }; -class ExclusiveScope { +class SCOPED_LOCKABLE ExclusiveScope { public: - explicit ExclusiveScope(SharedExclusiveLock* lock) : lock_(lock) { + explicit ExclusiveScope(SharedExclusiveLock* lock) + EXCLUSIVE_LOCK_FUNCTION(lock) + : lock_(lock) { lock_->LockExclusive(); } - ~ExclusiveScope() { - lock_->UnlockExclusive(); - } + ~ExclusiveScope() UNLOCK_FUNCTION() { lock_->UnlockExclusive(); } private: SharedExclusiveLock* lock_; diff --git a/webrtc/base/signalthread.h b/webrtc/base/signalthread.h index a97bda1af..8e18be61d 100644 --- a/webrtc/base/signalthread.h +++ b/webrtc/base/signalthread.h @@ -115,16 +115,17 @@ class SignalThread DISALLOW_IMPLICIT_CONSTRUCTORS(Worker); }; - class EnterExit { + class SCOPED_LOCKABLE EnterExit { public: - explicit EnterExit(SignalThread* t) : t_(t) { + explicit EnterExit(SignalThread* t) EXCLUSIVE_LOCK_FUNCTION(t->cs_) + : t_(t) { t_->cs_.Enter(); // If refcount_ is zero then the object has already been deleted and we // will be double-deleting it in ~EnterExit()! (shouldn't happen) ASSERT(t_->refcount_ != 0); ++t_->refcount_; } - ~EnterExit() { + ~EnterExit() UNLOCK_FUNCTION() { bool d = (0 == --t_->refcount_); t_->cs_.Leave(); if (d)