[libcamera-devel] [RFC PATCH 3/6] android: camera_hal_manager: Add thread safety annotation

[Umang Jain]

Hi Hiro

On 10/29/21 9:44 AM, Hirokazu Honda wrote:
> This applies clang thread safety annotation to CameraHalManager.
> Mutex and MutexLocker there are replaced with Mutex2 and
> MutexLocer2.
>
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>


Patch looks good to me

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

> ---
>   src/android/camera_hal_manager.cpp | 10 +++++-----
>   src/android/camera_hal_manager.h   | 14 ++++++--------
>   2 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/src/android/camera_hal_manager.cpp b/src/android/camera_hal_manager.cpp
> index 5f7bfe26..26715a5f 100644
> --- a/src/android/camera_hal_manager.cpp
> +++ b/src/android/camera_hal_manager.cpp
> @@ -75,7 +75,7 @@ int CameraHalManager::init()
>   std::tuple<CameraDevice *, int>
>   CameraHalManager::open(unsigned int id, const hw_module_t *hardwareModule)
>   {
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	if (!callbacks_) {
>   		LOG(HAL, Error) << "Can't open camera before callbacks are set";
> @@ -103,7 +103,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>   	bool isCameraExternal = false;
>   	bool isCameraNew = false;
>   
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	/*
>   	 * Each camera is assigned a unique integer ID when it is seen for the
> @@ -198,7 +198,7 @@ void CameraHalManager::cameraAdded(std::shared_ptr<Camera> cam)
>   
>   void CameraHalManager::cameraRemoved(std::shared_ptr<Camera> cam)
>   {
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	auto iter = std::find_if(cameras_.begin(), cameras_.end(),
>   				 [&cam](const std::unique_ptr<CameraDevice> &camera) {
> @@ -257,7 +257,7 @@ int CameraHalManager::getCameraInfo(unsigned int id, struct camera_info *info)
>   	if (!info)
>   		return -EINVAL;
>   
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	CameraDevice *camera = cameraDeviceFromHalId(id);
>   	if (!camera) {
> @@ -280,7 +280,7 @@ void CameraHalManager::setCallbacks(const camera_module_callbacks_t *callbacks)
>   {
>   	callbacks_ = callbacks;
>   
> -	MutexLocker locker(mutex_);
> +	MutexLocker2 locker(mutex_);
>   
>   	/*
>   	 * Some external cameras may have been identified before the callbacks_
> diff --git a/src/android/camera_hal_manager.h b/src/android/camera_hal_manager.h
> index 3f6d302a..9b8c3a1f 100644
> --- a/src/android/camera_hal_manager.h
> +++ b/src/android/camera_hal_manager.h
> @@ -8,7 +8,6 @@
>   #define __ANDROID_CAMERA_MANAGER_H__
>   
>   #include <map>
> -#include <mutex>
>   #include <stddef.h>
>   #include <tuple>
>   #include <vector>
> @@ -18,6 +17,8 @@
>   #include <system/camera_metadata.h>
>   
>   #include <libcamera/base/class.h>
> +#include <libcamera/base/mutex.h>
> +#include <libcamera/base/thread_annotations.h>
>   
>   #include <libcamera/camera_manager.h>
>   
> @@ -44,9 +45,6 @@ public:
>   private:
>   	LIBCAMERA_DISABLE_COPY_AND_MOVE(CameraHalManager)
>   
> -	using Mutex = std::mutex;
> -	using MutexLocker = std::unique_lock<std::mutex>;
> -
>   	static constexpr unsigned int firstExternalCameraId_ = 1000;
>   
>   	CameraHalManager();
> @@ -56,15 +54,15 @@ private:
>   	void cameraAdded(std::shared_ptr<libcamera::Camera> cam);
>   	void cameraRemoved(std::shared_ptr<libcamera::Camera> cam);
>   
> -	CameraDevice *cameraDeviceFromHalId(unsigned int id);
> +	CameraDevice *cameraDeviceFromHalId(unsigned int id) REQUIRES(mutex_);
>   
>   	std::unique_ptr<libcamera::CameraManager> cameraManager_;
>   	CameraHalConfig halConfig_;
>   
>   	const camera_module_callbacks_t *callbacks_;
> -	std::vector<std::unique_ptr<CameraDevice>> cameras_;
> -	std::map<std::string, unsigned int> cameraIdsMap_;
> -	Mutex mutex_;
> +	std::vector<std::unique_ptr<CameraDevice>> cameras_ GUARDED_BY(mutex_);
> +	std::map<std::string, unsigned int> cameraIdsMap_ GUARDED_BY(mutex_);
> +	libcamera::Mutex2 mutex_;
>   
>   	unsigned int numInternalCameras_;
>   	unsigned int nextExternalCameraId_;