From 18586d38bcc90fa47f76e0bb54881dd889751167 Mon Sep 17 00:00:00 2001 From: "mallinath@webrtc.org" Date: Mon, 27 Jan 2014 22:00:57 +0000 Subject: [PATCH] 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 git-svn-id: http://webrtc.googlecode.com/svn/trunk@5444 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, 84 insertions(+), 61 deletions(-) diff --git a/talk/media/webrtc/fakewebrtcvideocapturemodule.h b/talk/media/webrtc/fakewebrtcvideocapturemodule.h index a53f31b3d4..b823bc18fe 100644 --- a/talk/media/webrtc/fakewebrtcvideocapturemodule.h +++ b/talk/media/webrtc/fakewebrtcvideocapturemodule.h @@ -59,16 +59,21 @@ class FakeWebRtcVideoCaptureModule : public webrtc::VideoCaptureModule { id_ = id; return 0; } - virtual void RegisterCaptureDataCallback( + virtual int32_t RegisterCaptureDataCallback( webrtc::VideoCaptureDataCallback& callback) { callback_ = &callback; + return 0; } - virtual void DeRegisterCaptureDataCallback() { callback_ = NULL; } - virtual void RegisterCaptureCallback(webrtc::VideoCaptureFeedBack& callback) { - // Not implemented. + virtual int32_t DeRegisterCaptureDataCallback() { + callback_ = NULL; + return 0; } - virtual void DeRegisterCaptureCallback() { - // Not implemented. + virtual int32_t RegisterCaptureCallback( + webrtc::VideoCaptureFeedBack& callback) { + return -1; // not implemented + } + virtual int32_t DeRegisterCaptureCallback() { + return 0; } virtual int32_t StartCapture( const webrtc::VideoCaptureCapability& cap) { @@ -93,8 +98,13 @@ class FakeWebRtcVideoCaptureModule : public webrtc::VideoCaptureModule { settings = cap_; return 0; } - virtual void SetCaptureDelay(int32_t delay) { delay_ = delay; } - virtual int32_t CaptureDelay() { return delay_; } + virtual int32_t SetCaptureDelay(int32_t delay) { + delay_ = delay; + return 0; + } + virtual int32_t CaptureDelay() { + return delay_; + } virtual int32_t SetCaptureRotation( webrtc::VideoCaptureRotation rotation) { return -1; // not implemented @@ -103,11 +113,11 @@ class FakeWebRtcVideoCaptureModule : public webrtc::VideoCaptureModule { const webrtc::VideoCodec& codec) { return NULL; // not implemented } - virtual void EnableFrameRateCallback(const bool enable) { - // not implemented + virtual int32_t EnableFrameRateCallback(const bool enable) { + return -1; // not implemented } - virtual void EnableNoPictureAlarm(const bool enable) { - // not implemented + virtual int32_t EnableNoPictureAlarm(const bool enable) { + return -1; // not implemented } virtual int32_t AddRef() { return 0; diff --git a/talk/media/webrtc/webrtcvideocapturer.cc b/talk/media/webrtc/webrtcvideocapturer.cc index 1ce4d77517..6b05b991ed 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(); - module_->RegisterCaptureDataCallback(*this); - if (module_->StartCapture(cap) != 0) { + if (module_->RegisterCaptureDataCallback(*this) != 0 || + 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 5340cb7311..6b6247c277 100644 --- a/webrtc/modules/video_capture/include/video_capture.h +++ b/webrtc/modules/video_capture/include/video_capture.h @@ -105,17 +105,18 @@ class VideoCaptureModule: public RefCountedModule { }; // Register capture data callback - virtual void RegisterCaptureDataCallback( + virtual int32_t RegisterCaptureDataCallback( VideoCaptureDataCallback& dataCallback) = 0; // Remove capture data callback - virtual void DeRegisterCaptureDataCallback() = 0; + virtual int32_t DeRegisterCaptureDataCallback() = 0; // Register capture callback. - virtual void RegisterCaptureCallback(VideoCaptureFeedBack& callBack) = 0; + virtual int32_t RegisterCaptureCallback( + VideoCaptureFeedBack& callBack) = 0; // Remove capture callback. - virtual void DeRegisterCaptureCallback() = 0; + virtual int32_t DeRegisterCaptureCallback() = 0; // Start capture device virtual int32_t StartCapture( @@ -132,7 +133,7 @@ class VideoCaptureModule: public RefCountedModule { // Gets the current configuration. virtual int32_t CaptureSettings(VideoCaptureCapability& settings) = 0; - virtual void SetCaptureDelay(int32_t delayMS) = 0; + virtual int32_t SetCaptureDelay(int32_t delayMS) = 0; // Returns the current CaptureDelay. Only valid when the camera is running. virtual int32_t CaptureDelay() = 0; @@ -148,8 +149,8 @@ class VideoCaptureModule: public RefCountedModule { virtual VideoCaptureEncodeInterface* GetEncodeInterface( const VideoCodec& codec) = 0; - virtual void EnableFrameRateCallback(const bool enable) = 0; - virtual void EnableNoPictureAlarm(const bool enable) = 0; + virtual int32_t EnableFrameRateCallback(const bool enable) = 0; + virtual int32_t 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 98b6aa61a0..0f8052ebd6 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()); - module->RegisterCaptureDataCallback(*callback); + EXPECT_EQ(0, module->RegisterCaptureDataCallback(*callback)); return module; } @@ -408,10 +408,11 @@ class VideoCaptureExternalTest : public testing::Test { memset(test_frame_.buffer(webrtc::kVPlane), 127, ((kTestWidth + 1) / 2) * ((kTestHeight + 1) / 2)); - capture_module_->RegisterCaptureDataCallback(capture_callback_); - capture_module_->RegisterCaptureCallback(capture_feedback_); - capture_module_->EnableFrameRateCallback(true); - capture_module_->EnableNoPictureAlarm(true); + 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)); } void TearDown() { diff --git a/webrtc/modules/video_capture/video_capture_impl.cc b/webrtc/modules/video_capture/video_capture_impl.cc index 06b4a20896..6689fd132c 100644 --- a/webrtc/modules/video_capture/video_capture_impl.cc +++ b/webrtc/modules/video_capture/video_capture_impl.cc @@ -187,33 +187,45 @@ VideoCaptureImpl::~VideoCaptureImpl() delete[] _deviceUniqueId; } -void VideoCaptureImpl::RegisterCaptureDataCallback( - VideoCaptureDataCallback& dataCallBack) { +int32_t VideoCaptureImpl::RegisterCaptureDataCallback( + VideoCaptureDataCallback& dataCallBack) +{ CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _dataCallBack = &dataCallBack; + + return 0; } -void VideoCaptureImpl::DeRegisterCaptureDataCallback() { +int32_t VideoCaptureImpl::DeRegisterCaptureDataCallback() +{ CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _dataCallBack = NULL; + return 0; } -void VideoCaptureImpl::RegisterCaptureCallback(VideoCaptureFeedBack& callBack) { +int32_t VideoCaptureImpl::RegisterCaptureCallback(VideoCaptureFeedBack& callBack) +{ CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _captureCallBack = &callBack; + return 0; } -void VideoCaptureImpl::DeRegisterCaptureCallback() { +int32_t VideoCaptureImpl::DeRegisterCaptureCallback() +{ CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _captureCallBack = NULL; + return 0; + } -void VideoCaptureImpl::SetCaptureDelay(int32_t delayMS) { +int32_t VideoCaptureImpl::SetCaptureDelay(int32_t delayMS) +{ CriticalSectionScoped cs(&_apiCs); _captureDelay = delayMS; + return 0; } int32_t VideoCaptureImpl::CaptureDelay() { @@ -377,7 +389,8 @@ int32_t VideoCaptureImpl::SetCaptureRotation(VideoCaptureRotation rotation) { return 0; } -void VideoCaptureImpl::EnableFrameRateCallback(const bool enable) { +int32_t VideoCaptureImpl::EnableFrameRateCallback(const bool enable) +{ CriticalSectionScoped cs(&_apiCs); CriticalSectionScoped cs2(&_callBackCs); _frameRateCallBack = enable; @@ -385,12 +398,15 @@ void VideoCaptureImpl::EnableFrameRateCallback(const bool enable) { { _lastFrameRateCallbackTime = TickTime::Now(); } + return 0; } -void VideoCaptureImpl::EnableNoPictureAlarm(const bool enable) { +int32_t 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 f3a4c64cbd..80d5e67862 100644 --- a/webrtc/modules/video_capture/video_capture_impl.h +++ b/webrtc/modules/video_capture/video_capture_impl.h @@ -62,18 +62,17 @@ public: virtual int32_t ChangeUniqueId(const int32_t id); //Call backs - virtual void RegisterCaptureDataCallback( - VideoCaptureDataCallback& dataCallback); - virtual void DeRegisterCaptureDataCallback(); - virtual void RegisterCaptureCallback(VideoCaptureFeedBack& callBack); - virtual void DeRegisterCaptureCallback(); + virtual int32_t RegisterCaptureDataCallback(VideoCaptureDataCallback& dataCallback); + virtual int32_t DeRegisterCaptureDataCallback(); + virtual int32_t RegisterCaptureCallback(VideoCaptureFeedBack& callBack); + virtual int32_t DeRegisterCaptureCallback(); - virtual void SetCaptureDelay(int32_t delayMS); + virtual int32_t SetCaptureDelay(int32_t delayMS); virtual int32_t CaptureDelay(); virtual int32_t SetCaptureRotation(VideoCaptureRotation rotation); - virtual void EnableFrameRateCallback(const bool enable); - virtual void EnableNoPictureAlarm(const bool enable); + virtual int32_t EnableFrameRateCallback(const bool enable); + virtual int32_t 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 8b96277736..6ec5653c2e 100644 --- a/webrtc/video_engine/vie_capturer.cc +++ b/webrtc/video_engine/vie_capturer.cc @@ -279,8 +279,7 @@ void ViECapturer::CpuOveruseMeasures(int* capture_jitter_ms, } int32_t ViECapturer::SetCaptureDelay(int32_t delay_ms) { - capture_module_->SetCaptureDelay(delay_ms); - return 0; + return capture_module_->SetCaptureDelay(delay_ms); } int32_t ViECapturer::SetRotateCapturedFrames( @@ -639,31 +638,29 @@ 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; - } - observer_ = 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; } - 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 21644e20c6..1fa2b53df1 100644 --- a/webrtc/video_engine/vie_capturer.h +++ b/webrtc/video_engine/vie_capturer.h @@ -20,7 +20,6 @@ #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" @@ -186,7 +185,7 @@ class ViECapturer // Statistics observer. scoped_ptr observer_cs_; - ViECaptureObserver* observer_ GUARDED_BY(observer_cs_.get()); + ViECaptureObserver* observer_; CaptureCapability requested_capability_;