From 7433a088d2e97993266b66c102b0866aa90b4424 Mon Sep 17 00:00:00 2001 From: "mallinath@webrtc.org" Date: Wed, 29 Jan 2014 00:56:02 +0000 Subject: [PATCH] Revert 5444 "Revert 5421 "Fix deadlock on register/unregister ob..." We reverted the r5421 to allow us roll webrtc to chrome without any modifications to libjingle. Since webrtc is rolled with r5444, we can add back the original CL and changes to libjingle will be upstreamed in the next roll. TBR=andresp@webrtc.org > Revert 5421 "Fix deadlock on register/unregister observer while ..." > > Failure to compile on Chromium Internal bots, because of API changes. > > http://chromegw.corp.google.com/i/internal.chromium.webrtc.fyi/builders/Mac/builds/2805/steps/compile/logs/stdio > > You need to follow the steps mentioned in > https://docs.google.com/a/google.com/document/d/1aHrmXECnu3-Jovc2-zYI267EaQCYz-IclYyBp9iA9Fc/edit that of a API changer. > > Since I will be rolling the libjingle this week, I can push your changes along with libjingle roll, if you prepare the CLs > as mentioned in the doc. > > > Fix deadlock on register/unregister observer while there is a an going callback. > > > > BUG=2835 > > R=mallinath@webrtc.org > > > > Review URL: https://webrtc-codereview.appspot.com/7119005 > > TBR=andresp@webrtc.org > > Review URL: https://webrtc-codereview.appspot.com/7679004 TBR=mallinath@webrtc.org Review URL: https://webrtc-codereview.appspot.com/7729005 git-svn-id: http://webrtc.googlecode.com/svn/trunk@5453 4adac7df-926f-26a2-2b94-8c16560cd09d --- .../webrtc/fakewebrtcvideocapturemodule.h | 34 +++++++------------ talk/media/webrtc/webrtcvideocapturer.cc | 4 +-- .../video_capture/include/video_capture.h | 15 ++++---- .../test/video_capture_unittest.cc | 11 +++--- .../video_capture/video_capture_impl.cc | 32 +++++------------ .../video_capture/video_capture_impl.h | 15 ++++---- webrtc/video_engine/vie_capturer.cc | 31 +++++++++-------- webrtc/video_engine/vie_capturer.h | 3 +- 8 files changed, 61 insertions(+), 84 deletions(-) diff --git a/talk/media/webrtc/fakewebrtcvideocapturemodule.h b/talk/media/webrtc/fakewebrtcvideocapturemodule.h index b823bc18f..a53f31b3d 100644 --- a/talk/media/webrtc/fakewebrtcvideocapturemodule.h +++ b/talk/media/webrtc/fakewebrtcvideocapturemodule.h @@ -59,21 +59,16 @@ class FakeWebRtcVideoCaptureModule : public webrtc::VideoCaptureModule { id_ = id; return 0; } - virtual int32_t RegisterCaptureDataCallback( + virtual void RegisterCaptureDataCallback( webrtc::VideoCaptureDataCallback& callback) { callback_ = &callback; - return 0; } - virtual int32_t DeRegisterCaptureDataCallback() { - callback_ = NULL; - return 0; + virtual void DeRegisterCaptureDataCallback() { callback_ = NULL; } + virtual void RegisterCaptureCallback(webrtc::VideoCaptureFeedBack& callback) { + // Not implemented. } - virtual int32_t RegisterCaptureCallback( - webrtc::VideoCaptureFeedBack& callback) { - return -1; // not implemented - } - virtual int32_t DeRegisterCaptureCallback() { - return 0; + virtual void DeRegisterCaptureCallback() { + // Not implemented. } virtual int32_t StartCapture( const webrtc::VideoCaptureCapability& cap) { @@ -98,13 +93,8 @@ class FakeWebRtcVideoCaptureModule : public webrtc::VideoCaptureModule { settings = cap_; return 0; } - virtual int32_t SetCaptureDelay(int32_t delay) { - delay_ = delay; - return 0; - } - virtual int32_t CaptureDelay() { - return delay_; - } + virtual void SetCaptureDelay(int32_t delay) { delay_ = delay; } + virtual int32_t CaptureDelay() { return delay_; } virtual int32_t SetCaptureRotation( webrtc::VideoCaptureRotation rotation) { return -1; // not implemented @@ -113,11 +103,11 @@ class FakeWebRtcVideoCaptureModule : public webrtc::VideoCaptureModule { const webrtc::VideoCodec& codec) { return NULL; // not implemented } - virtual int32_t EnableFrameRateCallback(const bool enable) { - return -1; // not implemented + virtual void EnableFrameRateCallback(const bool enable) { + // not implemented } - virtual int32_t EnableNoPictureAlarm(const bool enable) { - return -1; // not implemented + virtual void EnableNoPictureAlarm(const bool enable) { + // not implemented } virtual int32_t AddRef() { return 0; diff --git a/talk/media/webrtc/webrtcvideocapturer.cc b/talk/media/webrtc/webrtcvideocapturer.cc index 6b05b991e..1ce4d7751 100644 --- a/talk/media/webrtc/webrtcvideocapturer.cc +++ b/talk/media/webrtc/webrtcvideocapturer.cc @@ -264,8 +264,8 @@ CaptureState WebRtcVideoCapturer::Start(const VideoFormat& capture_format) { std::string camera_id(GetId()); uint32 start = talk_base::Time(); - if (module_->RegisterCaptureDataCallback(*this) != 0 || - module_->StartCapture(cap) != 0) { + module_->RegisterCaptureDataCallback(*this); + if (module_->StartCapture(cap) != 0) { LOG(LS_ERROR) << "Camera '" << camera_id << "' failed to start"; return CS_FAILED; } diff --git a/webrtc/modules/video_capture/include/video_capture.h b/webrtc/modules/video_capture/include/video_capture.h index 6b6247c27..5340cb731 100644 --- a/webrtc/modules/video_capture/include/video_capture.h +++ b/webrtc/modules/video_capture/include/video_capture.h @@ -105,18 +105,17 @@ class VideoCaptureModule: public RefCountedModule { }; // Register capture data callback - virtual int32_t RegisterCaptureDataCallback( + virtual void RegisterCaptureDataCallback( VideoCaptureDataCallback& dataCallback) = 0; // Remove capture data callback - virtual int32_t DeRegisterCaptureDataCallback() = 0; + virtual void DeRegisterCaptureDataCallback() = 0; // Register capture callback. - virtual int32_t RegisterCaptureCallback( - VideoCaptureFeedBack& callBack) = 0; + virtual void RegisterCaptureCallback(VideoCaptureFeedBack& callBack) = 0; // Remove capture callback. - virtual int32_t DeRegisterCaptureCallback() = 0; + virtual void DeRegisterCaptureCallback() = 0; // Start capture device virtual int32_t StartCapture( @@ -133,7 +132,7 @@ class VideoCaptureModule: public RefCountedModule { // Gets the current configuration. virtual int32_t CaptureSettings(VideoCaptureCapability& settings) = 0; - virtual int32_t SetCaptureDelay(int32_t delayMS) = 0; + virtual void SetCaptureDelay(int32_t delayMS) = 0; // Returns the current CaptureDelay. Only valid when the camera is running. virtual int32_t CaptureDelay() = 0; @@ -149,8 +148,8 @@ class VideoCaptureModule: public RefCountedModule { virtual VideoCaptureEncodeInterface* GetEncodeInterface( const VideoCodec& codec) = 0; - virtual int32_t EnableFrameRateCallback(const bool enable) = 0; - virtual int32_t EnableNoPictureAlarm(const bool enable) = 0; + virtual void EnableFrameRateCallback(const bool enable) = 0; + virtual void EnableNoPictureAlarm(const bool enable) = 0; protected: virtual ~VideoCaptureModule() {}; diff --git a/webrtc/modules/video_capture/test/video_capture_unittest.cc b/webrtc/modules/video_capture/test/video_capture_unittest.cc index 0f8052ebd..98b6aa61a 100644 --- a/webrtc/modules/video_capture/test/video_capture_unittest.cc +++ b/webrtc/modules/video_capture/test/video_capture_unittest.cc @@ -252,7 +252,7 @@ class VideoCaptureTest : public testing::Test { EXPECT_FALSE(module->CaptureStarted()); - EXPECT_EQ(0, module->RegisterCaptureDataCallback(*callback)); + module->RegisterCaptureDataCallback(*callback); return module; } @@ -408,11 +408,10 @@ class VideoCaptureExternalTest : public testing::Test { memset(test_frame_.buffer(webrtc::kVPlane), 127, ((kTestWidth + 1) / 2) * ((kTestHeight + 1) / 2)); - EXPECT_EQ(0, capture_module_->RegisterCaptureDataCallback( - capture_callback_)); - EXPECT_EQ(0, capture_module_->RegisterCaptureCallback(capture_feedback_)); - EXPECT_EQ(0, capture_module_->EnableFrameRateCallback(true)); - EXPECT_EQ(0, capture_module_->EnableNoPictureAlarm(true)); + capture_module_->RegisterCaptureDataCallback(capture_callback_); + capture_module_->RegisterCaptureCallback(capture_feedback_); + capture_module_->EnableFrameRateCallback(true); + capture_module_->EnableNoPictureAlarm(true); } void TearDown() { diff --git a/webrtc/modules/video_capture/video_capture_impl.cc b/webrtc/modules/video_capture/video_capture_impl.cc index 6689fd132..06b4a2089 100644 --- a/webrtc/modules/video_capture/video_capture_impl.cc +++ b/webrtc/modules/video_capture/video_capture_impl.cc @@ -187,45 +187,33 @@ VideoCaptureImpl::~VideoCaptureImpl() delete[] _deviceUniqueId; } -int32_t VideoCaptureImpl::RegisterCaptureDataCallback( - VideoCaptureDataCallback& dataCallBack) -{ +void VideoCaptureImpl::RegisterCaptureDataCallback( + VideoCaptureDataCallback& dataCallBack) { CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _dataCallBack = &dataCallBack; - - return 0; } -int32_t VideoCaptureImpl::DeRegisterCaptureDataCallback() -{ +void VideoCaptureImpl::DeRegisterCaptureDataCallback() { CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _dataCallBack = NULL; - return 0; } -int32_t VideoCaptureImpl::RegisterCaptureCallback(VideoCaptureFeedBack& callBack) -{ +void VideoCaptureImpl::RegisterCaptureCallback(VideoCaptureFeedBack& callBack) { CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _captureCallBack = &callBack; - return 0; } -int32_t VideoCaptureImpl::DeRegisterCaptureCallback() -{ +void VideoCaptureImpl::DeRegisterCaptureCallback() { CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _captureCallBack = NULL; - return 0; - } -int32_t VideoCaptureImpl::SetCaptureDelay(int32_t delayMS) -{ +void VideoCaptureImpl::SetCaptureDelay(int32_t delayMS) { CriticalSectionScoped cs(&_apiCs); _captureDelay = delayMS; - return 0; } int32_t VideoCaptureImpl::CaptureDelay() { @@ -389,8 +377,7 @@ int32_t VideoCaptureImpl::SetCaptureRotation(VideoCaptureRotation rotation) { return 0; } -int32_t VideoCaptureImpl::EnableFrameRateCallback(const bool enable) -{ +void VideoCaptureImpl::EnableFrameRateCallback(const bool enable) { CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _frameRateCallBack = enable; @@ -398,15 +385,12 @@ int32_t VideoCaptureImpl::EnableFrameRateCallback(const bool enable) { _lastFrameRateCallbackTime = TickTime::Now(); } - return 0; } -int32_t VideoCaptureImpl::EnableNoPictureAlarm(const bool enable) -{ +void VideoCaptureImpl::EnableNoPictureAlarm(const bool enable) { CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _noPictureAlarmCallBack = enable; - return 0; } void VideoCaptureImpl::UpdateFrameCount() diff --git a/webrtc/modules/video_capture/video_capture_impl.h b/webrtc/modules/video_capture/video_capture_impl.h index 80d5e6786..f3a4c64cb 100644 --- a/webrtc/modules/video_capture/video_capture_impl.h +++ b/webrtc/modules/video_capture/video_capture_impl.h @@ -62,17 +62,18 @@ public: virtual int32_t ChangeUniqueId(const int32_t id); //Call backs - virtual int32_t RegisterCaptureDataCallback(VideoCaptureDataCallback& dataCallback); - virtual int32_t DeRegisterCaptureDataCallback(); - virtual int32_t RegisterCaptureCallback(VideoCaptureFeedBack& callBack); - virtual int32_t DeRegisterCaptureCallback(); + virtual void RegisterCaptureDataCallback( + VideoCaptureDataCallback& dataCallback); + virtual void DeRegisterCaptureDataCallback(); + virtual void RegisterCaptureCallback(VideoCaptureFeedBack& callBack); + virtual void DeRegisterCaptureCallback(); - virtual int32_t SetCaptureDelay(int32_t delayMS); + virtual void SetCaptureDelay(int32_t delayMS); virtual int32_t CaptureDelay(); virtual int32_t SetCaptureRotation(VideoCaptureRotation rotation); - virtual int32_t EnableFrameRateCallback(const bool enable); - virtual int32_t EnableNoPictureAlarm(const bool enable); + virtual void EnableFrameRateCallback(const bool enable); + virtual void EnableNoPictureAlarm(const bool enable); virtual const char* CurrentDeviceName() const; diff --git a/webrtc/video_engine/vie_capturer.cc b/webrtc/video_engine/vie_capturer.cc index 6ec5653c2..8b9627773 100644 --- a/webrtc/video_engine/vie_capturer.cc +++ b/webrtc/video_engine/vie_capturer.cc @@ -279,7 +279,8 @@ void ViECapturer::CpuOveruseMeasures(int* capture_jitter_ms, } int32_t ViECapturer::SetCaptureDelay(int32_t delay_ms) { - return capture_module_->SetCaptureDelay(delay_ms); + capture_module_->SetCaptureDelay(delay_ms); + return 0; } int32_t ViECapturer::SetRotateCapturedFrames( @@ -638,29 +639,31 @@ bool ViECapturer::CaptureCapabilityFixed() { } int32_t ViECapturer::RegisterObserver(ViECaptureObserver* observer) { - CriticalSectionScoped cs(observer_cs_.get()); - if (observer_) { - WEBRTC_TRACE(kTraceError, kTraceVideo, ViEId(engine_id_, capture_id_), - "%s Observer already registered", __FUNCTION__, capture_id_); - return -1; - } - if (capture_module_->RegisterCaptureCallback(*this) != 0) { - return -1; + { + CriticalSectionScoped cs(observer_cs_.get()); + if (observer_) { + WEBRTC_TRACE(kTraceError, + kTraceVideo, + ViEId(engine_id_, capture_id_), + "%s Observer already registered", + __FUNCTION__, + capture_id_); + return -1; + } + observer_ = observer; } + capture_module_->RegisterCaptureCallback(*this); capture_module_->EnableFrameRateCallback(true); capture_module_->EnableNoPictureAlarm(true); - observer_ = observer; return 0; } int32_t ViECapturer::DeRegisterObserver() { - CriticalSectionScoped cs(observer_cs_.get()); - if (!observer_) { - return 0; - } capture_module_->EnableFrameRateCallback(false); capture_module_->EnableNoPictureAlarm(false); capture_module_->DeRegisterCaptureCallback(); + + CriticalSectionScoped cs(observer_cs_.get()); observer_ = NULL; return 0; } diff --git a/webrtc/video_engine/vie_capturer.h b/webrtc/video_engine/vie_capturer.h index 1fa2b53df..21644e20c 100644 --- a/webrtc/video_engine/vie_capturer.h +++ b/webrtc/video_engine/vie_capturer.h @@ -20,6 +20,7 @@ #include "webrtc/modules/video_coding/main/interface/video_coding.h" #include "webrtc/modules/video_processing/main/interface/video_processing.h" #include "webrtc/system_wrappers/interface/scoped_ptr.h" +#include "webrtc/system_wrappers/interface/thread_annotations.h" #include "webrtc/typedefs.h" #include "webrtc/video_engine/include/vie_capture.h" #include "webrtc/video_engine/vie_defines.h" @@ -185,7 +186,7 @@ class ViECapturer // Statistics observer. scoped_ptr observer_cs_; - ViECaptureObserver* observer_; + ViECaptureObserver* observer_ GUARDED_BY(observer_cs_.get()); CaptureCapability requested_capability_;