From 5b5de21accfd29e21cba2d6f38e3087e1f731be6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Wed, 28 Oct 2020 17:18:56 +0100 Subject: [PATCH] Replace RWLockWrapper --> Mutex in DeviceInfoImpl Reader-writer locks helps performance only when there are many concurrent readers, and I would expect that isn't the case for this class. Using a plain mutex reduces complexity. Bug: webrtc:12102 Change-Id: I07c315bcbfc38f1d8befe5395c9ece54c673aeb7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/190722 Reviewed-by: Magnus Flodman Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#32547} --- modules/video_capture/device_info_impl.cc | 29 ++++--------------- modules/video_capture/device_info_impl.h | 14 +++++---- .../video_capture/linux/device_info_linux.h | 5 ++-- .../video_capture/windows/device_info_ds.cc | 8 ++--- .../video_capture/windows/device_info_ds.h | 3 +- 5 files changed, 22 insertions(+), 37 deletions(-) diff --git a/modules/video_capture/device_info_impl.cc b/modules/video_capture/device_info_impl.cc index 91a72326cf..846977e68f 100644 --- a/modules/video_capture/device_info_impl.cc +++ b/modules/video_capture/device_info_impl.cc @@ -25,34 +25,25 @@ namespace webrtc { namespace videocapturemodule { DeviceInfoImpl::DeviceInfoImpl() - : _apiLock(*RWLockWrapper::CreateRWLock()), - _lastUsedDeviceName(NULL), - _lastUsedDeviceNameLength(0) {} + : _lastUsedDeviceName(NULL), _lastUsedDeviceNameLength(0) {} DeviceInfoImpl::~DeviceInfoImpl(void) { - _apiLock.AcquireLockExclusive(); + MutexLock lock(&_apiLock); free(_lastUsedDeviceName); - _apiLock.ReleaseLockExclusive(); - - delete &_apiLock; } int32_t DeviceInfoImpl::NumberOfCapabilities(const char* deviceUniqueIdUTF8) { if (!deviceUniqueIdUTF8) return -1; - _apiLock.AcquireLockShared(); + MutexLock lock(&_apiLock); // Is it the same device that is asked for again. if (absl::EqualsIgnoreCase( deviceUniqueIdUTF8, absl::string_view(_lastUsedDeviceName, _lastUsedDeviceNameLength))) { - _apiLock.ReleaseLockShared(); return static_cast(_captureCapabilities.size()); } - // Need to get exclusive rights to create the new capability map. - _apiLock.ReleaseLockShared(); - WriteLockScoped cs2(_apiLock); int32_t ret = CreateCapabilityMap(deviceUniqueIdUTF8); return ret; @@ -63,20 +54,14 @@ int32_t DeviceInfoImpl::GetCapability(const char* deviceUniqueIdUTF8, VideoCaptureCapability& capability) { assert(deviceUniqueIdUTF8 != NULL); - ReadLockScoped cs(_apiLock); + MutexLock lock(&_apiLock); if (!absl::EqualsIgnoreCase( deviceUniqueIdUTF8, absl::string_view(_lastUsedDeviceName, _lastUsedDeviceNameLength))) { - _apiLock.ReleaseLockShared(); - _apiLock.AcquireLockExclusive(); if (-1 == CreateCapabilityMap(deviceUniqueIdUTF8)) { - _apiLock.ReleaseLockExclusive(); - _apiLock.AcquireLockShared(); return -1; } - _apiLock.ReleaseLockExclusive(); - _apiLock.AcquireLockShared(); } // Make sure the number is valid @@ -98,17 +83,13 @@ int32_t DeviceInfoImpl::GetBestMatchedCapability( if (!deviceUniqueIdUTF8) return -1; - ReadLockScoped cs(_apiLock); + MutexLock lock(&_apiLock); if (!absl::EqualsIgnoreCase( deviceUniqueIdUTF8, absl::string_view(_lastUsedDeviceName, _lastUsedDeviceNameLength))) { - _apiLock.ReleaseLockShared(); - _apiLock.AcquireLockExclusive(); if (-1 == CreateCapabilityMap(deviceUniqueIdUTF8)) { return -1; } - _apiLock.ReleaseLockExclusive(); - _apiLock.AcquireLockShared(); } int32_t bestformatIndex = -1; diff --git a/modules/video_capture/device_info_impl.h b/modules/video_capture/device_info_impl.h index 37a457ce8a..4b47389609 100644 --- a/modules/video_capture/device_info_impl.h +++ b/modules/video_capture/device_info_impl.h @@ -18,7 +18,8 @@ #include "api/video/video_rotation.h" #include "modules/video_capture/video_capture.h" #include "modules/video_capture/video_capture_defines.h" -#include "rtc_base/synchronization/rw_lock_wrapper.h" +#include "rtc_base/synchronization/mutex.h" +#include "rtc_base/thread_annotations.h" namespace webrtc { namespace videocapturemodule { @@ -45,15 +46,16 @@ class DeviceInfoImpl : public VideoCaptureModule::DeviceInfo { * Fills the member variable _captureCapabilities with capabilities for the * given device name. */ - virtual int32_t CreateCapabilityMap(const char* deviceUniqueIdUTF8) = 0; + virtual int32_t CreateCapabilityMap(const char* deviceUniqueIdUTF8) + RTC_EXCLUSIVE_LOCKS_REQUIRED(_apiLock) = 0; protected: // Data members typedef std::vector VideoCaptureCapabilities; - VideoCaptureCapabilities _captureCapabilities; - RWLockWrapper& _apiLock; - char* _lastUsedDeviceName; - uint32_t _lastUsedDeviceNameLength; + VideoCaptureCapabilities _captureCapabilities RTC_GUARDED_BY(_apiLock); + Mutex _apiLock; + char* _lastUsedDeviceName RTC_GUARDED_BY(_apiLock); + uint32_t _lastUsedDeviceNameLength RTC_GUARDED_BY(_apiLock); }; } // namespace videocapturemodule } // namespace webrtc diff --git a/modules/video_capture/linux/device_info_linux.h b/modules/video_capture/linux/device_info_linux.h index a320c36fde..304ae71230 100644 --- a/modules/video_capture/linux/device_info_linux.h +++ b/modules/video_capture/linux/device_info_linux.h @@ -33,13 +33,14 @@ class DeviceInfoLinux : public DeviceInfoImpl { * Fills the membervariable _captureCapabilities with capabilites for the * given device name. */ - int32_t CreateCapabilityMap(const char* deviceUniqueIdUTF8) override; + int32_t CreateCapabilityMap(const char* deviceUniqueIdUTF8) override + RTC_EXCLUSIVE_LOCKS_REQUIRED(_apiLock); int32_t DisplayCaptureSettingsDialogBox(const char* /*deviceUniqueIdUTF8*/, const char* /*dialogTitleUTF8*/, void* /*parentWindow*/, uint32_t /*positionX*/, uint32_t /*positionY*/) override; - int32_t FillCapabilities(int fd); + int32_t FillCapabilities(int fd) RTC_EXCLUSIVE_LOCKS_REQUIRED(_apiLock); int32_t Init() override; private: diff --git a/modules/video_capture/windows/device_info_ds.cc b/modules/video_capture/windows/device_info_ds.cc index a163579bf1..f43c508bee 100644 --- a/modules/video_capture/windows/device_info_ds.cc +++ b/modules/video_capture/windows/device_info_ds.cc @@ -99,7 +99,7 @@ int32_t DeviceInfoDS::Init() { return 0; } uint32_t DeviceInfoDS::NumberOfDevices() { - ReadLockScoped cs(_apiLock); + MutexLock lock(&_apiLock); return GetDeviceInfo(0, 0, 0, 0, 0, 0, 0); } @@ -110,7 +110,7 @@ int32_t DeviceInfoDS::GetDeviceName(uint32_t deviceNumber, uint32_t deviceUniqueIdUTF8Length, char* productUniqueIdUTF8, uint32_t productUniqueIdUTF8Length) { - ReadLockScoped cs(_apiLock); + MutexLock lock(&_apiLock); const int32_t result = GetDeviceInfo( deviceNumber, deviceNameUTF8, deviceNameLength, deviceUniqueIdUTF8, deviceUniqueIdUTF8Length, productUniqueIdUTF8, productUniqueIdUTF8Length); @@ -287,7 +287,7 @@ IBaseFilter* DeviceInfoDS::GetDeviceFilter(const char* deviceUniqueIdUTF8, int32_t DeviceInfoDS::GetWindowsCapability( const int32_t capabilityIndex, VideoCaptureCapabilityWindows& windowsCapability) { - ReadLockScoped cs(_apiLock); + MutexLock lock(&_apiLock); if (capabilityIndex < 0 || static_cast(capabilityIndex) >= _captureCapabilitiesWindows.size()) { @@ -584,7 +584,7 @@ int32_t DeviceInfoDS::DisplayCaptureSettingsDialogBox( void* parentWindow, uint32_t positionX, uint32_t positionY) { - ReadLockScoped cs(_apiLock); + MutexLock lock(&_apiLock); HWND window = (HWND)parentWindow; IBaseFilter* filter = GetDeviceFilter(deviceUniqueIdUTF8, NULL, 0); diff --git a/modules/video_capture/windows/device_info_ds.h b/modules/video_capture/windows/device_info_ds.h index d782eb5415..2fda3257f4 100644 --- a/modules/video_capture/windows/device_info_ds.h +++ b/modules/video_capture/windows/device_info_ds.h @@ -85,7 +85,8 @@ class DeviceInfoDS : public DeviceInfoImpl { char* productUniqueIdUTF8, uint32_t productUniqueIdUTF8Length); - int32_t CreateCapabilityMap(const char* deviceUniqueIdUTF8) override; + int32_t CreateCapabilityMap(const char* deviceUniqueIdUTF8) override + RTC_EXCLUSIVE_LOCKS_REQUIRED(_apiLock); private: ICreateDevEnum* _dsDevEnum;