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

[Umang Jain]

Hi Jacopo,

On 9/6/21 7:31 PM, Jacopo Mondi 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. */
> +	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());
> +

Well, the descriptors_ mapped might seem empty but some descriptors  can 
be queued somewhere else for async stuff in future ;-)

anyways looks good to me as per master so,

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>   	streams_.clear();
>   
>   	state_ = State::Stopped;