[libcamera-devel] [PATCH 5/5] android: camera_device: Assert descriptors_ is empty

[Hirokazu Honda]

Hi Jacopo, thank you for the patch.

On Mon, Sep 6, 2021 at 11:01 PM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> The CameraDevice::descriptors_ class member holds one item per each
> in-flight request. A new descriptor is added to the map each time a new
> request is queued to the camera and it gets then popped out when the
> request completes or when an error happens before queueing it.
>
> As stopping the camera guarantees that all in-flight requests are
> completed synchronously to the Camera::stop() call, there is no need to
> manually clear the descriptors_ map, on the contrary, it might hides
> issues in the handling of in-flight requests.
>
> Remove it and replace it with an assertion to make sure the underlying
> camera behaves as intended and all Request are completed when the camera
> is stopped.
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  src/android/camera_device.cpp | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 0ce9b699bfae..1591ad98c7d5 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -442,6 +442,9 @@ void CameraDevice::flush()
>         worker_.stop();
>         camera_->stop();
>
> +       /* Make sure all in-flight requests have been completed. */

Shall we say descriptors are flushed in worker.stop()?

Reviewed-by: Hirokazu Honda <hiroh at chromium.org>

-Hiro
> +       ASSERT(descriptors_.empty());
> +
>         MutexLocker stateLock(stateMutex_);
>         state_ = State::Stopped;
>  }
> @@ -454,9 +457,7 @@ void CameraDevice::stop()
>                  * We get here if the list of streams requested by the camera
>                  * service has to be updated but the camera has been stopped
>                  * already, which happens if configureStreams() gets called
> -                * after a flush(). Clear the list of stream configurations,
> -                * no need to clear descriptors as stopping the camera completes
> -                * all the pending requests.
> +                * after a flush().
>                  */
>                 streams_.clear();
>                 return;
> @@ -465,7 +466,9 @@ void CameraDevice::stop()
>         worker_.stop();
>         camera_->stop();
>
> -       descriptors_.clear();
> +       /* Make sure all in-flight requests have been completed. */
> +       ASSERT(descriptors_.empty());
> +
>         streams_.clear();
>
>         state_ = State::Stopped;
> --
> 2.32.0
>