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

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Fri, Oct 29, 2021 at 01:14:21PM +0900, Hirokazu Honda wrote:
> This applies clang thread safety annotation to CameraHalManager.
> Mutex and MutexLocker there are replaced with Mutex2 and
> MutexLocer2.

s/Locer/Locker/

> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>

This looks fine, except for the *2 suffix that I'd like to avoid. This
is discussed in 2/6, so for this patch,

Reviewed-by: Laurent Pinchart <laurent.pinchart 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_;

-- 
Regards,

Laurent Pinchart