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

[Stanislaw Gruszka]

Hi Milan,

thanks for working on this.

On Fri, Jan 31, 2025 at 08:59:28PM +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>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Unfortunately I have assertion failures like below with the change.
Reproducible approximately 1 per 4 times when stopping qcam.

Could you please take a look? I can dig more into it,
but I think it could be easier for you :-) Can provide
more data if needed.

Thanks
Stanislaw

2:12:50.912442715] [11753] FATAL default soft_ipa_proxy.cpp:456 assertion "state_ == ProxyRunning" failed in processStatsThread()
Backtrace:
libcamera::ipa::soft::IPAProxySoft::processStatsThread(unsigned int, unsigned int, libcamera::ControlList const&)+0xd6 (src/libcamera/proxy/soft_ipa_proxy.cpp:457)
libcamera::ipa::soft::IPAProxySoft::processStats(unsigned int, unsigned int, libcamera::ControlList const&)+0x57 (src/libcamera/proxy/soft_ipa_proxy.cpp:449)
libcamera::SoftwareIsp::processStats(unsigned int, unsigned int, libcamera::ControlList const&)+0x117 (../src/libcamera/software_isp/software_isp.cpp:174)
libcamera::SimpleCameraData::ispStatsReady(unsigned int, unsigned int)+0x76 (../src/libcamera/pipeline/simple/simple.cpp:894)
libcamera::BoundMethodMember<libcamera::SimpleCameraData, void, unsigned int, unsigned int>::activate(unsigned int, unsigned int, bool)+0xa4 (../include/libcamera/base/bound_method.h:180)
libcamera::Signal<unsigned int, unsigned int>::emit(unsigned int, unsigned int)+0x99 (../include/libcamera/base/signal.h:152)
libcamera::SoftwareIsp::statsReady(unsigned int, unsigned int)+0x31 (../src/libcamera/software_isp/software_isp.cpp:360)
libcamera::BoundMethodMember<libcamera::SoftwareIsp, void, unsigned int, unsigned int>::invoke(unsigned int, unsigned int)+0x80 (../include/libcamera/base/bound_method.h:191)
std::enable_if<std::is_void<void>::value, void>::type libcamera::BoundMethodArgs<void, unsigned int, unsigned int>::invokePack<0ul, 1ul, void>(libcamera::BoundMethodPackBase*, std::integer_sequence<unsigned long, 0ul, 1ul>)+0x5f (../include/libcamera/base/bound_method.h:116)
libcamera::BoundMethodArgs<void, unsigned int, unsigned int>::invokePack(libcamera::BoundMethodPackBase*)+0x27 (../include/libcamera/base/bound_method.h:125)
libcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)
libcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)
libcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317 (../src/libcamera/base/thread.cpp:650)
libcamera::ipa::soft::IPAProxySoft::stopThread()+0x167 (src/libcamera/proxy/soft_ipa_proxy.cpp:285)
libcamera::ipa::soft::IPAProxySoft::stop()+0x39 (src/libcamera/proxy/soft_ipa_proxy.cpp:268)
libcamera::SoftwareIsp::stop()+0x64 (../src/libcamera/software_isp/software_isp.cpp:332)
libcamera::SimplePipelineHandler::stopDevice(libcamera::Camera*)+0xb6 (../src/libcamera/pipeline/simple/simple.cpp:1396)
libcamera::PipelineHandler::stop(libcamera::Camera*)+0x54 (../src/libcamera/pipeline_handler.cpp:367)
libcamera::BoundMethodMember<libcamera::PipelineHandler, void, libcamera::Camera*>::invoke(libcamera::Camera*)+0x7f (../include/libcamera/base/bound_method.h:191)
std::enable_if<std::is_void<void>::value, void>::type libcamera::BoundMethodArgs<void, libcamera::Camera*>::invokePack<0ul, void>(libcamera::BoundMethodPackBase*, std::integer_sequence<unsigned long, 0ul>)+0x4a (../include/libcamera/base/bound_method.h:116)
libcamera::BoundMethodArgs<void, libcamera::Camera*>::invokePack(libcamera::BoundMethodPackBase*)+0x27 (../include/libcamera/base/bound_method.h:125)
libcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)
libcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)
libcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317 (../src/libcamera/base/thread.cpp:650)
libcamera::EventDispatcherPoll::processEvents()+0x38 (../src/libcamera/base/event_dispatcher_poll.cpp:149)
libcamera::Thread::exec()+0x70 (../src/libcamera/base/thread.cpp:310)
libcamera::CameraManager::Private::run()+0x121 (../src/libcamera/camera_manager.cpp:91)
libcamera::Thread::startThread()+0xf2 (../src/libcamera/base/thread.cpp:290)
void std::__invoke_impl<void, void (libcamera::Thread::*)(), libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x6a (/usr/include/c++/13/bits/invoke.h:74)
std::__invoke_result<void (libcamera::Thread::*)(), libcamera::Thread*>::type std::__invoke<void (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x3b (/usr/include/c++/13/bits/invoke.h:97)
void std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>)+0x47 (/usr/include/c++/13/bits/std_thread.h:292)
std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> >::operator()()+0x1c (/usr/include/c++/13/bits/std_thread.h:299)
std::thread::_State_impl<std::thread::_Invoker<std::tuple<void (libcamera::Thread::*)(), libcamera::Thread*> > >::_M_run()+0x20 (/usr/include/c++/13/bits/std_thread.h:244)
execute_native_thread_routine+0x14 (/build/gcc-14-ig5ci0/gcc-14-14.2.0/build/x86_64-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h:93)
start_thread+0x384 (./nptl/pthread_create.c:447)
__clone3+0x2c (../sysdeps/unix/sysv/linux/x86_64/clone3.S:80)
Aborted (core dumped)

> ---
>  .../internal/software_isp/software_isp.h         |  3 ++-
>  src/libcamera/pipeline/simple/simple.cpp         | 16 +---------------
>  2 files changed, 3 insertions(+), 16 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..6e039bf3 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -537,21 +537,7 @@ int SimpleCameraData::init()
>  				<< "Failed to create software ISP, disabling software debayering";
>  			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().
> -			 */
> -			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);
> -- 
> 2.48.1
>