[libcamera-devel] [PATCH 2/5] android: camera_device: Clear streams_ in stop()

[Umang Jain]

Hi Jacopo,

On 9/6/21 7:31 PM, Jacopo Mondi wrote:
> The CameraDevice::streams_ class member is populated at
> configureStreams() time with one item per each stream requested to the
> Camera device.
>
> When a new configuration is applied to the Camera HAL the list of
> requested streams has to be re-initialized and the streams_ vector needs
> to be manually cleared in order to be populated from scratch.
>
> As configureStreams() class CameraDevice::stop() at the very beginning,
> centralize clearing the streams_ vector there even in the case the
> camera has been stopped by a previous call to flush().
>
> Suggested-by: Hirokazu Honda <hiroh at chromium.org>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>   src/android/camera_device.cpp | 21 ++++++++++++---------
>   1 file changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index fda77db4540c..30c173a69720 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -448,8 +448,18 @@ void CameraDevice::flush()
>   void CameraDevice::stop()
>   {
>   	MutexLocker stateLock(stateMutex_);
> -	if (state_ == State::Stopped)
> +	if (state_ == State::Stopped) {
> +		/*
> +		 * 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.
> +		 */
> +		streams_.clear();
>   		return;
> +	}


This makes sense to me, but I facing a more fundamental question that 
why does flush() tries to stop the libcamera::Camera instance on it's 
own and setting state_ = State::Stopped; manually. Ideally it should 
happen by calling CameraDevice::stop() inside flush(). Seems I am 
missing something.. need to look at flush() documentation in depth.

As far as I have looked the current configureStreams() behaviour, the 
change and reasons makes sense to me, so

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

>   
>   	worker_.stop();
>   	camera_->stop();
> @@ -544,6 +554,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>   		LOG(HAL, Error) << "No streams in configuration";
>   		return -EINVAL;
>   	}
> +	streams_.reserve(stream_list->num_streams);
>   
>   #if defined(OS_CHROMEOS)
>   	if (!validateCropRotate(*stream_list))
> @@ -560,14 +571,6 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>   		return -EINVAL;
>   	}
>   
> -	/*
> -	 * Clear and remove any existing configuration from previous calls, and
> -	 * ensure the required entries are available without further
> -	 * reallocation.
> -	 */
> -	streams_.clear();
> -	streams_.reserve(stream_list->num_streams);
> -
>   	std::vector<Camera3StreamConfig> streamConfigs;
>   	streamConfigs.reserve(stream_list->num_streams);
>