[libcamera-devel,v2,9/9] RFC: Stop PostProcessor when camera is stopped
diff mbox series

Message ID 20210910070638.467294-10-umang.jain@ideasonboard.com
State Superseded
Delegated to: Umang Jain
Headers show
Series
  • Async Post Processor
Related show

Commit Message

Umang Jain Sept. 10, 2021, 7:06 a.m. UTC
Stopping the post-processor thread from CameraDevice is not ideal.
But it temporarily avoids the crash on stopping the camera.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/android/camera_device.cpp | 8 ++++++++
 src/android/camera_stream.h   | 7 +++++++
 2 files changed, 15 insertions(+)

Comments

Laurent Pinchart Sept. 13, 2021, 1:28 a.m. UTC | #1
Hi Umang,

Thank you for the patch.

On Fri, Sep 10, 2021 at 12:36:38PM +0530, Umang Jain wrote:
> Stopping the post-processor thread from CameraDevice is not ideal.

Why is so ? I think that explicit start() and stop() functions for
CameraStream() could make sense.

> But it temporarily avoids the crash on stopping the camera.

What crash ?

> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/android/camera_device.cpp | 8 ++++++++
>  src/android/camera_stream.h   | 7 +++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 73eb5758..fa2db72f 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -425,6 +425,14 @@ int CameraDevice::open(const hw_module_t *hardwareModule)
>  
>  void CameraDevice::close()
>  {
> +	for (CameraStream &cameraStream : streams_) {
> +		/* CameraStream doesn't have user defined destructor as
> +		 * it is MoveConstructible. Stop the post-processor thread

Why does being move-constructible prevent having a user-defined
destructor ? Adding a user-defined destructor will prevent the compiler
from adding an implicitly-declared move constructor, but you could add
an explicit

	CameraStream(CameraStream &&other) = default;

Or better,

	CameraStream(CameraStream &&other);

in camera_device.h, and

CameraStream::CameraStream(CameraStream &&other) = default;

in camera_device.cpp to avoid making it inline.

> +		 * from here before clearing streams_.
> +		 */
> +		cameraStream.stopPostProcessorThread();
> +	}
> +
>  	streams_.clear();
>  
>  	stop();
> diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
> index dbb7eee3..bd37473e 100644
> --- a/src/android/camera_stream.h
> +++ b/src/android/camera_stream.h
> @@ -133,6 +133,13 @@ public:
>  		    const Camera3RequestDescriptor *context);
>  	libcamera::FrameBuffer *getBuffer();
>  	void putBuffer(libcamera::FrameBuffer *buffer);
> +	void stopPostProcessorThread()
> +	{
> +		if (thread_) {
> +			thread_->exit();
> +			thread_->wait();
> +		}
> +	}
>  
>  	enum ProcessStatus {
>  		Succeeded,

Patch
diff mbox series

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 73eb5758..fa2db72f 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -425,6 +425,14 @@  int CameraDevice::open(const hw_module_t *hardwareModule)
 
 void CameraDevice::close()
 {
+	for (CameraStream &cameraStream : streams_) {
+		/* CameraStream doesn't have user defined destructor as
+		 * it is MoveConstructible. Stop the post-processor thread
+		 * from here before clearing streams_.
+		 */
+		cameraStream.stopPostProcessorThread();
+	}
+
 	streams_.clear();
 
 	stop();
diff --git a/src/android/camera_stream.h b/src/android/camera_stream.h
index dbb7eee3..bd37473e 100644
--- a/src/android/camera_stream.h
+++ b/src/android/camera_stream.h
@@ -133,6 +133,13 @@  public:
 		    const Camera3RequestDescriptor *context);
 	libcamera::FrameBuffer *getBuffer();
 	void putBuffer(libcamera::FrameBuffer *buffer);
+	void stopPostProcessorThread()
+	{
+		if (thread_) {
+			thread_->exit();
+			thread_->wait();
+		}
+	}
 
 	enum ProcessStatus {
 		Succeeded,