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

[Milan Zamazal]

Hi Laurent,

thank you for review.

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:

> 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,

OK, posted v3 with this change.

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