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

[Hirokazu Honda]

Hi Jacopo, thank you for the patch.

On Thu, May 13, 2021 at 6:22 PM Jacopo Mondi <jacopo at 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 at jmondi.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210517/419c9897/attachment.htm>