[libcamera-devel] [PATCH v3 6/8] android: Guard access to the camera state

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, May 21, 2021 at 05:42:25PM +0200, Jacopo Mondi wrote:
> Guard access to the camera state and the start/stop sequences
> with a mutex.
> 
> Currently only stop() and the first call to processCaptureRequest()
> start and stop the camera, and they're not meant to race with each
> other. With the introduction of flush() the camera can be stopped
> concurrently to a processCaptureRequest() call, hence access to the
> camera state will need to be protected.
> 
> Prepare for that by guarding the existing paths with a mutex.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_device.cpp | 23 ++++++++++++++---------
>  src/android/camera_device.h   |  1 +
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index fceae96a8b7c..6645c019410e 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -759,6 +759,7 @@ void CameraDevice::close()
>  
>  void CameraDevice::stop()
>  {
> +	MutexLocker cameraLock(cameraMutex_);
>  	if (state_ == CameraStopped)
>  		return;
>  
> @@ -1844,17 +1845,21 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	if (!isValidRequest(camera3Request))
>  		return -EINVAL;
>  
> -	/* Start the camera if that's the first request we handle. */
> -	if (state_ == CameraStopped) {
> -		worker_.start();
> +	{
> +		MutexLocker cameraLock(cameraMutex_);
>  
> -		int ret = camera_->start();
> -		if (ret) {
> -			LOG(HAL, Error) << "Failed to start camera";
> -			return ret;
> -		}
> +		/* Start the camera if that's the first request we handle. */
> +		if (state_ == CameraStopped) {
> +			worker_.start();
>  
> -		state_ = CameraRunning;
> +			int ret = camera_->start();
> +			if (ret) {
> +				LOG(HAL, Error) << "Failed to start camera";
> +				return ret;
> +			}
> +
> +			state_ = CameraRunning;
> +		}
>  	}
>  
>  	/*
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 995b423c6deb..6fe5454c6535 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -119,6 +119,7 @@ private:
>  
>  	CameraWorker worker_;
>  
> +	libcamera::Mutex cameraMutex_; /* Protects access to the camera state. */

To make it more explicit that it protects the state, I'd prefer naming
the variable stateMutex_.

The patch looks good, but I suppose I'll see how the mutex is really
used in the next patches. Assuming there's no issue there,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  	State state_;
>  
>  	std::shared_ptr<libcamera::Camera> camera_;

-- 
Regards,

Laurent Pinchart