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

[Laurent Pinchart]

Hi Milan,

On Thu, Dec 19, 2024 at 10:13:17PM +0100, Milan Zamazal wrote:
> Barnabás Pőcze writes:
> > 2024. december 19., csütörtök 11:31 keltezéssel, Milan Zamazal írta:
> >
> >> inputBufferReady ready signal in the simple pipeline is handled in the
> >> pipeline handler thread.  Similarly, outputBufferReady and ispStatsReady
> >> signals should be handled there too, everything should be called from
> >> the right thread and not block the callers.
> >> 
> >> Signed-off-by: Milan Zamazal <mzamazal at redhat.com>
> >> ---
> >>  src/libcamera/pipeline/simple/simple.cpp | 9 +++++++--
> >>  1 file changed, 7 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> >> index 8ac24e6e..280112f4 100644
> >> --- a/src/libcamera/pipeline/simple/simple.cpp
> >> +++ b/src/libcamera/pipeline/simple/simple.cpp
> >> @@ -548,12 +548,17 @@ int SimpleCameraData::init()
> >>  			 * bound explicitly to the pipe, which is bound to the pipeline
> >>  			 * handler thread. The function then simply forwards the call to
> >>  			 * conversionInputDone().
> >> +			 * Similarly for outputBufferReady and ispStatsReady signals.
> >>  			 */
> >>  			swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {
> >>  				this->conversionInputDone(buffer);
> >>  			});
> >> -			swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);
> >> -			swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);
> >> +			swIsp_->outputBufferReady.connect(this, [this](FrameBuffer *buffer) {
> >> +				this->conversionOutputDone(buffer);
> >> +			});
> >> +			swIsp_->ispStatsReady.connect(this, [this](uint32_t frame, uint32_t bufferId) {
> >> +				this->ispStatsReady(frame, bufferId);
> >> +			});
> >> [...]
> >
> > The object is still `this`, so I wouldn't expect any difference in behaviour. Is there?
> 
> Ah, right, should be `pipe', will fix it in v2.

I think it would be better to emit the signals from the camera manager
thread, instead of relying on the user of the soft ISP to perform
cross-thread synchronization. That will be less error-prone.

-- 
Regards,

Laurent Pinchart