[libcamera-devel,v3,2/2] android: CameraDevice: Add stop()
diff mbox series

Message ID 20210329061119.135513-3-hiroh@chromium.org
State Superseded
Headers show
Series
  • Fix Camera3RequestDescriptor leakage
Related show

Commit Message

Hirokazu Honda March 29, 2021, 6:11 a.m. UTC 
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 | 17 +++++++++--------
 src/android/camera_device.h   |  2 ++
 2 files changed, 11 insertions(+), 8 deletions(-)

Comments

Jacopo Mondi March 29, 2021, 9:13 a.m. UTC | #1 
Hi Hiro,

On Mon, Mar 29, 2021 at 03:11:19PM +0900, 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 | 17 +++++++++--------
>  src/android/camera_device.h   |  2 ++
>  2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 02d3bfb2..d5447d8e 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -659,12 +659,17 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
>
>  void CameraDevice::close()
>  {
> -	streams_.clear();
> +	stop();
> +
> +	camera_->release();
> +}
>
> +void CameraDevice::stop()
> +{
>  	worker_.stop();
>  	camera_->stop();

I think the camera shall be stopped only if (running_). Otherwise
you'll get an error message from the Camera state maching if I'm not
mistaken.

> -	camera_->release();
>
> +	descriptors_.clear();

Ideally, this should be done as:
1) Introduce stop() in CameraDevice
2) Introduce descriptors_ and clear it in stop()

As this bundle two changes in one patch.

A minor though.

The patch is fine, but I think the discussion with Laurent on what has
triggered this issue in first place is still a bit on-going, right ?

>  	running_ = false;
>  }
>
> @@ -1547,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 5a2d22d6..cc12fe20 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -87,6 +87,8 @@ private:
>  		int androidFormat;
>  	};
>
> +	void stop();
> +
>  	int initializeStreamConfigurations();
>  	std::vector<libcamera::Size>
>  	getYUVResolutions(libcamera::CameraConfiguration *cameraConfig,
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hirokazu Honda March 30, 2021, 5:41 a.m. UTC | #2 
Hi, Jacopo, thanks for reviewing.

On Mon, Mar 29, 2021 at 6:13 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Hiro,
>
> On Mon, Mar 29, 2021 at 03:11:19PM +0900, 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 | 17 +++++++++--------
> >  src/android/camera_device.h   |  2 ++
> >  2 files changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 02d3bfb2..d5447d8e 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -659,12 +659,17 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
> >
> >  void CameraDevice::close()
> >  {
> > -     streams_.clear();
> > +     stop();
> > +
> > +     camera_->release();
> > +}
> >
> > +void CameraDevice::stop()
> > +{
> >       worker_.stop();
> >       camera_->stop();
>
> I think the camera shall be stopped only if (running_). Otherwise
> you'll get an error message from the Camera state maching if I'm not
> mistaken.
>
> > -     camera_->release();
> >
> > +     descriptors_.clear();
>
> Ideally, this should be done as:
> 1) Introduce stop() in CameraDevice
> 2) Introduce descriptors_ and clear it in stop()
>
> As this bundle two changes in one patch.
>
> A minor though.
>

Okay, I reorder the patches to first one of adding stop() and the
second one of fixing leakage.


> The patch is fine, but I think the discussion with Laurent on what has
> triggered this issue in first place is still a bit on-going, right ?
>

We agree, regardless of issues, this patch should go as avoiding the leakage.
The issue is trivial, and we are discussing to select the most proper
way of fixing it.
That is, we shall merge this patch without minding the discussion.

Regards,
-Hiro
> >       running_ = false;
> >  }
> >
> > @@ -1547,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 5a2d22d6..cc12fe20 100644
> > --- a/src/android/camera_device.h
> > +++ b/src/android/camera_device.h
> > @@ -87,6 +87,8 @@ private:
> >               int androidFormat;
> >       };
> >
> > +     void stop();
> > +
> >       int initializeStreamConfigurations();
> >       std::vector<libcamera::Size>
> >       getYUVResolutions(libcamera::CameraConfiguration *cameraConfig,
> > --
> > 2.31.0.291.g576ba9dcdaf-goog
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 02d3bfb2..d5447d8e 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -659,12 +659,17 @@  int CameraDevice::open(const hw_module_t *hardwareModule)
 
 void CameraDevice::close()
 {
-	streams_.clear();
+	stop();
+
+	camera_->release();
+}
 
+void CameraDevice::stop()
+{
 	worker_.stop();
 	camera_->stop();
-	camera_->release();
 
+	descriptors_.clear();
 	running_ = false;
 }
 
@@ -1547,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 5a2d22d6..cc12fe20 100644
--- a/src/android/camera_device.h
+++ b/src/android/camera_device.h
@@ -87,6 +87,8 @@  private:
 		int androidFormat;
 	};
 
+	void stop();
+
 	int initializeStreamConfigurations();
 	std::vector<libcamera::Size>
 	getYUVResolutions(libcamera::CameraConfiguration *cameraConfig,