[libcamera-devel] [RFC PATCH 3/6] android: camera_hal_manager: Add thread safety annotation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 11 23:54:13 CET 2021
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
More information about the libcamera-devel
mailing list