[PATCH v2] libcamera: software_isp: Handle signals in the proper thread

[Laurent Pinchart]

Hi Milan,

Thank you for the patch.

On Fri, Jan 31, 2025 at 08:18:38PM +0100, Milan Zamazal wrote:
> inputBufferReady ready signal in the simple pipeline is handled in the
> pipeline handler thread.  outputBufferReady and ispStatsReady signals
> should be handled there too.
> 
> Rather than relying on the user of the SoftwareIsp instance, let
> SoftwareIsp inherits Object.  SoftwareIsp serves as a signal proxy, the
> signals above are emitted from signal handlers.  This means that if
> SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp
> thread.  Which is the camera manager thread because the SoftwareIsp
> instance is created there.
> 
> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> ---
>  .../internal/software_isp/software_isp.h      |  3 ++-
>  src/libcamera/pipeline/simple/simple.cpp      | 19 ++++++-------------
>  2 files changed, 8 insertions(+), 14 deletions(-)
> 
> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
> index d51b03fd..440a296d 100644
> --- a/include/libcamera/internal/software_isp/software_isp.h
> +++ b/include/libcamera/internal/software_isp/software_isp.h
> @@ -18,6 +18,7 @@
>  
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/object.h>
>  #include <libcamera/base/signal.h>
>  #include <libcamera/base/thread.h>
>  
> @@ -43,7 +44,7 @@ struct StreamConfiguration;
>  
>  LOG_DECLARE_CATEGORY(SoftwareIsp)
>  
> -class SoftwareIsp
> +class SoftwareIsp : public Object
>  {
>  public:
>  	SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 8ac24e6e..fade8fda 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -538,20 +538,13 @@ int SimpleCameraData::init()
>  			swIsp_.reset();
>  		} else {
>  			/*
> -			 * The inputBufferReady signal is emitted from the soft ISP thread,
> -			 * and needs to be handled in the pipeline handler thread. Signals
> -			 * implement queued delivery, but this works transparently only if
> -			 * the receiver is bound to the target thread. As the
> -			 * SimpleCameraData class doesn't inherit from the Object class, it
> -			 * is not bound to any thread, and the signal would be delivered
> -			 * synchronously. Instead, connect the signal to a lambda function
> -			 * bound explicitly to the pipe, which is bound to the pipeline
> -			 * handler thread. The function then simply forwards the call to
> -			 * conversionInputDone().
> +			 * The connected signals should be handled by the camera manager
> +			 * thread. This method is called in the camera manager thread and
> +			 * instantiates the SoftwareIsp instance, which inherits Object and
> +			 * emits the signals from the instance's own signal handlers; thus
> +			 * the slots here are invoked in the camera manager thread.
>  			 */

I think you can drop this comment complete. With that,

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> -			swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {
> -				this->conversionInputDone(buffer);
> -			});
> +			swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);
>  			swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);
>  			swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);
>  			swIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);

-- 
Regards,

Laurent Pinchart