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

12268 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>
---
 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 Thu, May 13, 2021 at 6:22 PM Jacopo Mondi <jacopo@jmondi.org> 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>
> ---
>  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 8a4543f079eb..c6cd0b6e8be7 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;
>
> @@ -1915,17 +1916,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 30b364decaab..f263fdae472a 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -120,6 +120,7 @@ private:
>
>         CameraWorker worker_;
>
> +       libcamera::Mutex cameraMutex_; /* Protects access to the camera
> state. */
>

I wonder if this should be used to protect camera_ and worker_.


>         State state_;
>
>         std::shared_ptr<libcamera::Camera> camera_;
> --
> 2.31.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>