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

[Milan Zamazal]

Hi Laurent,

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

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

Maybe a stupid question, but how to do it?  The signals are emitted from
the corresponding place/thread asynchronously and there is little choice
of the thread to use.  At least without passing around some Object bound
to the camera manager thread, which wouldn't be nice either I suppose.
What do I miss?