[libcamera-devel,v4,1/2] android: CameraDevice: Add stop()

11785 diff mbox series

This adds CameraDevice::stop(), which cleans up the member
variables of CameraDevice. It is called in CameraDevice::close()
and CameraDevice::configureStreams().

Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
---
 src/android/camera_device.cpp | 19 +++++++++++--------
 src/android/camera_device.h   |  2 ++
 2 files changed, 13 insertions(+), 8 deletions(-)

Hi Hiro,

On 30/03/2021 06:46, Hirokazu Honda wrote:
> This adds CameraDevice::stop(), which cleans up the member
> variables of CameraDevice. It is called in CameraDevice::close()
> and CameraDevice::configureStreams().
> 
> Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> ---
>  src/android/camera_device.cpp | 19 +++++++++++--------
>  src/android/camera_device.h   |  2 ++
>  2 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ae693664..14299429 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -657,11 +657,18 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
>  
>  void CameraDevice::close()
>  {
> -	streams_.clear();

Here CameraDevice::close() no longer clears the stream_, and it's not
added anywhere else.

Is this intentional?

It seems like a functional change which is not mentioned by the commit
message and stands out a bit.

Is it handled automatically elsewhere already?


> +	stop();
> +
> +	camera_->release();
> +}
> +
> +void CameraDevice::stop()
> +{
> +	if (!running_)
> +		return;
>  
>  	worker_.stop();
>  	camera_->stop();
> -	camera_->release();
>  
>  	running_ = false;
>  }
> @@ -1545,12 +1552,8 @@ PixelFormat CameraDevice::toPixelFormat(int format) const
>   */
>  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  {
> -	/* Before any configuration attempt, stop the camera if it's running. */
> -	if (running_) {
> -		worker_.stop();
> -		camera_->stop();
> -		running_ = false;
> -	}
> +	/* Before any configuration attempt, stop the camera. */
> +	stop();
>  
>  	/*
>  	 * Generate an empty configuration, and construct a StreamConfiguration
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 11bdfec8..39cf95ad 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -85,6 +85,8 @@ private:
>  		int androidFormat;
>  	};
>  
> +	void stop();
> +
>  	int initializeStreamConfigurations();
>  	std::vector<libcamera::Size>
>  	getYUVResolutions(libcamera::CameraConfiguration *cameraConfig,
>

Hi Kieran,

On Wed, Mar 31, 2021 at 3:48 AM Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On 30/03/2021 06:46, Hirokazu Honda wrote:
> > This adds CameraDevice::stop(), which cleans up the member
> > variables of CameraDevice. It is called in CameraDevice::close()
> > and CameraDevice::configureStreams().
> >
> > Signed-off-by: Hirokazu Honda <hiroh@chromium.org>
> > ---
> >  src/android/camera_device.cpp | 19 +++++++++++--------
> >  src/android/camera_device.h   |  2 ++
> >  2 files changed, 13 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index ae693664..14299429 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -657,11 +657,18 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
> >
> >  void CameraDevice::close()
> >  {
> > -     streams_.clear();
>
> Here CameraDevice::close() no longer clears the stream_, and it's not
> added anywhere else.
>
> Is this intentional?
>
> It seems like a functional change which is not mentioned by the commit
> message and stands out a bit.
>
> Is it handled automatically elsewhere already?
>

That's definitely my fault. I will add revert it on close() in the next patch.
Thanks for finding.

>
> > +     stop();
> > +
> > +     camera_->release();
> > +}
> > +
> > +void CameraDevice::stop()
> > +{
> > +     if (!running_)
> > +             return;
> >
> >       worker_.stop();
> >       camera_->stop();
> > -     camera_->release();
> >
> >       running_ = false;
> >  }
> > @@ -1545,12 +1552,8 @@ PixelFormat CameraDevice::toPixelFormat(int format) const
> >   */
> >  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> >  {
> > -     /* Before any configuration attempt, stop the camera if it's running. */
> > -     if (running_) {
> > -             worker_.stop();
> > -             camera_->stop();
> > -             running_ = false;
> > -     }
> > +     /* Before any configuration attempt, stop the camera. */
> > +     stop();
> >
> >       /*
> >        * Generate an empty configuration, and construct a StreamConfiguration
> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> > index 11bdfec8..39cf95ad 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -85,6 +85,8 @@ private:
> >               int androidFormat;
> >       };
> >
> > +     void stop();
> > +
> >       int initializeStreamConfigurations();
> >       std::vector<libcamera::Size>
> >       getYUVResolutions(libcamera::CameraConfiguration *cameraConfig,
> >
>
> --
> Regards
> --
> Kieran