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

12357 diff mbox series

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@jmondi.org>
Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 23 ++++++++++++++---------
 src/android/camera_device.h   |  1 +
 2 files changed, 15 insertions(+), 9 deletions(-)

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@jmondi.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Hirokazu Honda <hiroh@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@ideasonboard.com>

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