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

[Laurent Pinchart]

Hi Milan,

On Fri, Jan 03, 2025 at 07:31:54PM +0100, Milan Zamazal wrote:
> Laurent Pinchart writes:
> > 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?

Not a stupid question at all, I should have made this clearer.

Signals are emitted from the thread running when the emit() function is
called. If a signal is connected to an object that does not inherit from
the Object class, the connected slot will be called synchronously, in
the same thread.

If the connected object inherits from the Object class, the behaviour
differs and depends on the connection type, specified when calling the
connect() function on the signal:

- If the connection type is ConnectionTypeDirect, the slot is called
  synchronously, in the same thread as the emit() calls.

- If the connection type is ConnectionTypeQueued, the slot is called
  asynchronously, in the thread of the connected object. The emit() call
  returns immediately, before the slot is called.

- If the connection type is ConnectionTypeBlocking, the slot is called
  synchronously, in the thread of the connected object. The emit() call
  waits until the synchronous call completes in the other thread before
  returning. If the emitter and receiver are in the same thread, this
  behaves like ConnectionTypeDirect.

- If the connection type is ConnectionTypeAuto (the default),
  ConnectionTypeDirect is used if the emitter and receiver are in the
  same thread, and ConnectionTypeQueued is used otherwise.

The SimpleCameraData class does not inherit from Object, so slots are
called synchronously, in the thread of the emitter. This is why the
inputBufferReady signal is handled with a lambda function:

	swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {
		this->conversionInputDone(buffer);
	});

The first argument to the connect() function is the receiver object,
which here inherits from the Object class. As a result, the lambda
function is called asynchronusly in the pipeline handler thread, not the
soft ISP worker thread.

This construct is fragile, as shown by this patch: the outputBufferReady
and ispStatsReady signals were mistakenly connected to the camera data
object, resulting in the slots being called from the wrong thread. I
think it would be better to handle this issue in the soft ISP class, to
avoid depending on the implementation of the receiver. One easy way to
do so is to make the SoftwareIsp class inherit from the Object class.
That way, SoftwareIsp::inputReady() will be called in the thread in
which the SoftwareIsp instance lives, which is the pipeline handler
thread. The inputBufferReady signal that is emitted from there will
always come from the pipeline handler thread.

-- 
Regards,

Laurent Pinchart