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

[Laurent Pinchart]

Hello,

On Fri, Sep 10, 2021 at 06:04:15PM +0530, Umang Jain wrote:
> 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,

s/class/calls/

> > 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();

stop() is called in configureStreams() and in close(). close() already
has a streams_.clear() call, which can't be removed (yet ?) as there's
no guarantee close() will be called with state_ == State::Stopped. I'm
thus not sure what this patch brings, given that the streams_.clear()
call here is not "centralized", it's only meant for the flush() case.
Maybe I'll have a better understanding when reading the next patches.

> >   		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;
> >   	}

Blank line.

> > +	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);
> >   

-- 
Regards,

Laurent Pinchart