[{"id":33246,"web_url":"https://patchwork.libcamera.org/comment/33246/","msgid":"<173840433874.2802919.8653419058521097752@ping.linuxembedded.co.uk>","date":"2025-02-01T10:05:38","subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the proper\n\tthread","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-01-31 19:59:28)\n> inputBufferReady ready signal in the simple pipeline is handled in the\n> pipeline handler thread. outputBufferReady and ispStatsReady signals\n> should be handled there too.\n> \n> Rather than relying on the user of the SoftwareIsp instance, let\n> SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the\n> signals above are emitted from signal handlers. This means that if\n> SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp\n> thread. Which is the camera manager thread because the SoftwareIsp\n> instance is created there.\n> \n> Signed-off-by: Milan Zamazal \n> Reviewed-by: Laurent Pinchart \n> ---\n> .../internal/software_isp/software_isp.h | 3 ++-\n> src/libcamera/pipeline/simple/simple.cpp | 16 +---------------\n> 2 files changed, 3 insertions(+), 16 deletions(-)\n> \n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> index d51b03fd..440a296d 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -18,6 +18,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -43,7 +44,7 @@ struct StreamConfiguration;\n> \n> LOG_DECLARE_CATEGORY(SoftwareIsp)\n> \n> -class SoftwareIsp\n> +class SoftwareIsp : public Object\n> {\n> public:\n> SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 8ac24e6e..6e039bf3 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -537,21 +537,7 @@ int SimpleCameraData::init()\n> << \"Failed to create software ISP, disabling software debayering\";\n> swIsp_.reset();\n> } else {\n> - /*\n> - * The inputBufferReady signal is emitted from the soft ISP thread,\n> - * and needs to be handled in the pipeline handler thread. Signals\n> - * implement queued delivery, but this works transparently only if\n> - * the receiver is bound to the target thread. As the\n> - * SimpleCameraData class doesn't inherit from the Object class, it\n> - * is not bound to any thread, and the signal would be delivered\n> - * synchronously. Instead, connect the signal to a lambda function\n> - * bound explicitly to the pipe, which is bound to the pipeline\n> - * handler thread. The function then simply forwards the call to\n> - * conversionInputDone().\n> - */\n> - swIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {\n> - this->conversionInputDone(buffer);\n> - });\n> + swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n\nThis looks like it was easier than I thought it was going to be!\n\n\nReviewed-by: Kieran Bingham \n\n> swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n> swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n> swIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);\n> -- \n> 2.48.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9AAF2C0F1B\n\tfor ;\n\tSat, 1 Feb 2025 10:05:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1A7B68568;\n\tSat, 1 Feb 2025 11:05:43 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 31EA068559\n\tfor ;\n\tSat, 1 Feb 2025 11:05:42 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 11AA26DC;\n\tSat, 1 Feb 2025 11:04:32 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"izwdmCvc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1738404272;\n\tbh=XvZTYxuJW/eFthq7KCPg3gGzg4vyZCzPmLJLX2zlL6g=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=izwdmCvcddX9mhNkiMH6hJzHVUIEIYFA2b+Jli39B/01dB1ArhU2G606gnis5IwA6\n\tLm5gDUMLhyyM2OPs9kF+mZDFs3A0YgbP85Yjk+fTCTA5d2mPkXBwbVZ39nYzqa0p1M\n\tcbsa+7wPhwbDD15fBJNe0emtE1s+nyvMfgbBbSDM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250131195928.57070-1-mzamazal@redhat.com>","References":"<20250131195928.57070-1-mzamazal@redhat.com>","Subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the proper\n\tthread","From":"Kieran Bingham ","Cc":"Milan Zamazal , Laurent Pinchart\n\t, Stanislaw Gruszka\n\t, =?utf-8?b?QmFybmFiw6FzIFDFkWN6?=\n\t=?utf-8?q?e?= ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Sat, 01 Feb 2025 10:05:38 +0000","Message-ID":"<173840433874.2802919.8653419058521097752@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33355,"web_url":"https://patchwork.libcamera.org/comment/33355/","msgid":"","date":"2025-02-14T08:42:11","subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the proper\n\tthread","submitter":{"id":211,"url":"https://patchwork.libcamera.org/api/people/211/","name":"Stanislaw Gruszka","email":"stanislaw.gruszka@linux.intel.com"},"content":"Hi Milan,\n\nthanks for working on this.\n\nOn Fri, Jan 31, 2025 at 08:59:28PM +0100, Milan Zamazal wrote:\n> inputBufferReady ready signal in the simple pipeline is handled in the\n> pipeline handler thread. outputBufferReady and ispStatsReady signals\n> should be handled there too.\n> \n> Rather than relying on the user of the SoftwareIsp instance, let\n> SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the\n> signals above are emitted from signal handlers. This means that if\n> SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp\n> thread. Which is the camera manager thread because the SoftwareIsp\n> instance is created there.\n> \n> Signed-off-by: Milan Zamazal \n> Reviewed-by: Laurent Pinchart \n\nUnfortunately I have assertion failures like below with the change.\nReproducible approximately 1 per 4 times when stopping qcam.\n\nCould you please take a look? I can dig more into it,\nbut I think it could be easier for you :-) Can provide\nmore data if needed.\n\nThanks\nStanislaw\n\n2:12:50.912442715] [11753] FATAL default soft_ipa_proxy.cpp:456 assertion \"state_ == ProxyRunning\" failed in processStatsThread()\nBacktrace:\nlibcamera::ipa::soft::IPAProxySoft::processStatsThread(unsigned int, unsigned int, libcamera::ControlList const&)+0xd6 (src/libcamera/proxy/soft_ipa_proxy.cpp:457)\nlibcamera::ipa::soft::IPAProxySoft::processStats(unsigned int, unsigned int, libcamera::ControlList const&)+0x57 (src/libcamera/proxy/soft_ipa_proxy.cpp:449)\nlibcamera::SoftwareIsp::processStats(unsigned int, unsigned int, libcamera::ControlList const&)+0x117 (../src/libcamera/software_isp/software_isp.cpp:174)\nlibcamera::SimpleCameraData::ispStatsReady(unsigned int, unsigned int)+0x76 (../src/libcamera/pipeline/simple/simple.cpp:894)\nlibcamera::BoundMethodMember::activate(unsigned int, unsigned int, bool)+0xa4 (../include/libcamera/base/bound_method.h:180)\nlibcamera::Signal::emit(unsigned int, unsigned int)+0x99 (../include/libcamera/base/signal.h:152)\nlibcamera::SoftwareIsp::statsReady(unsigned int, unsigned int)+0x31 (../src/libcamera/software_isp/software_isp.cpp:360)\nlibcamera::BoundMethodMember::invoke(unsigned int, unsigned int)+0x80 (../include/libcamera/base/bound_method.h:191)\nstd::enable_if::value, void>::type libcamera::BoundMethodArgs::invokePack<0ul, 1ul, void>(libcamera::BoundMethodPackBase*, std::integer_sequence)+0x5f (../include/libcamera/base/bound_method.h:116)\nlibcamera::BoundMethodArgs::invokePack(libcamera::BoundMethodPackBase*)+0x27 (../include/libcamera/base/bound_method.h:125)\nlibcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)\nlibcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)\nlibcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317 (../src/libcamera/base/thread.cpp:650)\nlibcamera::ipa::soft::IPAProxySoft::stopThread()+0x167 (src/libcamera/proxy/soft_ipa_proxy.cpp:285)\nlibcamera::ipa::soft::IPAProxySoft::stop()+0x39 (src/libcamera/proxy/soft_ipa_proxy.cpp:268)\nlibcamera::SoftwareIsp::stop()+0x64 (../src/libcamera/software_isp/software_isp.cpp:332)\nlibcamera::SimplePipelineHandler::stopDevice(libcamera::Camera*)+0xb6 (../src/libcamera/pipeline/simple/simple.cpp:1396)\nlibcamera::PipelineHandler::stop(libcamera::Camera*)+0x54 (../src/libcamera/pipeline_handler.cpp:367)\nlibcamera::BoundMethodMember::invoke(libcamera::Camera*)+0x7f (../include/libcamera/base/bound_method.h:191)\nstd::enable_if::value, void>::type libcamera::BoundMethodArgs::invokePack<0ul, void>(libcamera::BoundMethodPackBase*, std::integer_sequence)+0x4a (../include/libcamera/base/bound_method.h:116)\nlibcamera::BoundMethodArgs::invokePack(libcamera::BoundMethodPackBase*)+0x27 (../include/libcamera/base/bound_method.h:125)\nlibcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)\nlibcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)\nlibcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317 (../src/libcamera/base/thread.cpp:650)\nlibcamera::EventDispatcherPoll::processEvents()+0x38 (../src/libcamera/base/event_dispatcher_poll.cpp:149)\nlibcamera::Thread::exec()+0x70 (../src/libcamera/base/thread.cpp:310)\nlibcamera::CameraManager::Private::run()+0x121 (../src/libcamera/camera_manager.cpp:91)\nlibcamera::Thread::startThread()+0xf2 (../src/libcamera/base/thread.cpp:290)\nvoid std::__invoke_impl(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x6a (/usr/include/c++/13/bits/invoke.h:74)\nstd::__invoke_result::type std::__invoke(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x3b (/usr/include/c++/13/bits/invoke.h:97)\nvoid std::thread::_Invoker >::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>)+0x47 (/usr/include/c++/13/bits/std_thread.h:292)\nstd::thread::_Invoker >::operator()()+0x1c (/usr/include/c++/13/bits/std_thread.h:299)\nstd::thread::_State_impl > >::_M_run()+0x20 (/usr/include/c++/13/bits/std_thread.h:244)\nexecute_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)\nstart_thread+0x384 (./nptl/pthread_create.c:447)\n__clone3+0x2c (../sysdeps/unix/sysv/linux/x86_64/clone3.S:80)\nAborted (core dumped)\n\n> ---\n> .../internal/software_isp/software_isp.h | 3 ++-\n> src/libcamera/pipeline/simple/simple.cpp | 16 +---------------\n> 2 files changed, 3 insertions(+), 16 deletions(-)\n> \n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> index d51b03fd..440a296d 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -18,6 +18,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -43,7 +44,7 @@ struct StreamConfiguration;\n> \n> LOG_DECLARE_CATEGORY(SoftwareIsp)\n> \n> -class SoftwareIsp\n> +class SoftwareIsp : public Object\n> {\n> public:\n> \tSoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 8ac24e6e..6e039bf3 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -537,21 +537,7 @@ int SimpleCameraData::init()\n> \t\t\t\t<< \"Failed to create software ISP, disabling software debayering\";\n> \t\t\tswIsp_.reset();\n> \t\t} else {\n> -\t\t\t/*\n> -\t\t\t * The inputBufferReady signal is emitted from the soft ISP thread,\n> -\t\t\t * and needs to be handled in the pipeline handler thread. Signals\n> -\t\t\t * implement queued delivery, but this works transparently only if\n> -\t\t\t * the receiver is bound to the target thread. As the\n> -\t\t\t * SimpleCameraData class doesn't inherit from the Object class, it\n> -\t\t\t * is not bound to any thread, and the signal would be delivered\n> -\t\t\t * synchronously. Instead, connect the signal to a lambda function\n> -\t\t\t * bound explicitly to the pipe, which is bound to the pipeline\n> -\t\t\t * handler thread. The function then simply forwards the call to\n> -\t\t\t * conversionInputDone().\n> -\t\t\t */\n> -\t\t\tswIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {\n> -\t\t\t\tthis->conversionInputDone(buffer);\n> -\t\t\t});\n> +\t\t\tswIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n> \t\t\tswIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n> \t\t\tswIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n> \t\t\tswIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);\n> -- \n> 2.48.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5EE0DC32A3\n\tfor ;\n\tFri, 14 Feb 2025 08:42:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1595068648;\n\tFri, 14 Feb 2025 09:42:21 +0100 (CET)","from mgamail.intel.com (mgamail.intel.com [198.175.65.14])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 9787260337\n\tfor ;\n\tFri, 14 Feb 2025 09:42:17 +0100 (CET)","from orviesa003.jf.intel.com ([10.64.159.143])\n\tby orvoesa106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384;\n\t14 Feb 2025 00:42:16 -0800","from sgruszka-mobl.ger.corp.intel.com (HELO localhost)\n\t([10.245.113.53]) by ORVIESA003-auth.jf.intel.com with\n\tESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 14 Feb 2025 00:42:14 -0800"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=intel.com header.i=@intel.com\n\theader.b=\"ByTTnYaY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=intel.com; i=@intel.com; q=dns/txt; s=Intel;\n\tt=1739522538; x=1771058538;\n\th=date:from:to:cc:subject:message-id:references:\n\tmime-version:in-reply-to;\n\tbh=OmT9uK77F6G7an0MB7WMcc6uACE1TfGlQeYAQbFJ5F0=;\n\tb=ByTTnYaYJaVTNuKI4niLMqTA2zBkuhwwaRdpqg7MzeNO78BZeBIDaakn\n\tdVo9eQq6Uggb+k4n/P0BJcp0ICbRB+540h5hciVt70AieAswYO+WPLvAr\n\t+Swk5vgTYqWGh6W4GXvy+aHHMuToXZ6hEHFnEk6QzXn27eh1TWnhMDoxY\n\tLJQADJ3BWPnF7d4OsHMk9DoywFlmK3hL8g7CoQqTSxPIfLrY4pSWGAayi\n\thAzOWKb4OdLDQWcWkkMXDmLvHEM0s8ArpMeVnw4ra2NGPM28CJ4uaspzu\n\t7S6EBWM8NXt6W6qsbZqhJpyM0q+hNW0XfcrB+rziyJ5ENwSuQE3cKVi6l Q==;","X-CSE-ConnectionGUID":["0X1U4hVsQO+qV+Nf7zmEdw==","gHCckw0NTZi+4SIlbwRHbw=="],"X-CSE-MsgGUID":["sw01OVEYTWmgsvASLRF2ww==","bDAWh9yXQo2eEdjInWGcKQ=="],"X-IronPort-AV":["E=McAfee;i=\"6700,10204,11344\"; a=\"44030703\"","E=Sophos;i=\"6.13,285,1732608000\"; d=\"scan'208\";a=\"44030703\"","E=Sophos;i=\"6.11,199,1725346800\"; d=\"scan'208\";a=\"118333536\""],"X-ExtLoop1":"1","Date":"Fri, 14 Feb 2025 09:42:11 +0100","From":"Stanislaw Gruszka ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org, Laurent Pinchart\n\t, =?utf-8?b?QmFybmFiw6FzIFDFkWN6?=\n\t=?utf-8?q?e?= ","Subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the proper\n\tthread","Message-ID":"","References":"<20250131195928.57070-1-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20250131195928.57070-1-mzamazal@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33370,"web_url":"https://patchwork.libcamera.org/comment/33370/","msgid":"<851pw0453h.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-02-14T20:57:22","subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the\n\tproper thread","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Stanislaw,\n\nStanislaw Gruszka writes:\n\n> Hi Milan,\n>\n> thanks for working on this.\n>\n> On Fri, Jan 31, 2025 at 08:59:28PM +0100, Milan Zamazal wrote:\n>> inputBufferReady ready signal in the simple pipeline is handled in the\n>> pipeline handler thread. outputBufferReady and ispStatsReady signals\n>> should be handled there too.\n>> \n>> Rather than relying on the user of the SoftwareIsp instance, let\n>> SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the\n>> signals above are emitted from signal handlers. This means that if\n>> SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp\n>> thread. Which is the camera manager thread because the SoftwareIsp\n>> instance is created there.\n>> \n>> Signed-off-by: Milan Zamazal \n>> Reviewed-by: Laurent Pinchart \n>\n> Unfortunately I have assertion failures like below with the change.\n> Reproducible approximately 1 per 4 times when stopping qcam.\n\nI can reproduce it occasionally with cam too.\n\n> Could you please take a look? \n\nI think what happens there is:\n\n- The ISP stats signal is emitted at the \"right\" moment to cause the\n race and gets queued.\n\n- The pipeline is asked to stop and the IPA proxy state is set to\n ProxyStopping.\n\n- As part of stopping the IPA, pending messages of the given thread are\n invoked.\n\n- The ISP stats message causes a call to IPA to process the stats, which\n fails on the assertion due to ProxyStopping IPA state. The stats\n related IPA call itself is not different from what hardware pipelines\n do but the circumstances when it happens are different, either\n preventing the race in hardware pipelines or making it much less\n likely (I don't know).\n\nNow the question is what can be done about it. Things should be already\nrun in the right thread as discussed previously. The signal emitter\ncannot do anything about the issue. So the pending message should be\neither discarded or the IPA call should be blocked if the IPA state is\nnot ProxyRunning.\n\nThe only way I can see to discard the messages is to destroy the\ncorresponding object, which is not an option in this case.\n\nI can't see no public way to check the IPA state from outside, excluding\nthe option of blocking the IPA call too. I'm not sure how to interpret\nthe fact that ProxyState enum is public while its only use is to declare\na protected class member; does it indicate that the option of dealing\nwith the state from outside is left open?\n\nAt the moment, what can help is tracking the stop action in SoftwareIsp\nclass itself and preventing forwarding the signal from there in such a\ncase. This apparently fixes the issue. I can post a patch next week if\nthere is no better suggestion.\n\n> I can dig more into it, but I think it could be easier for you :-) Can\n> provide more data if needed.\n>\n> Thanks\n> Stanislaw\n>\n> 2:12:50.912442715] [11753] FATAL default soft_ipa_proxy.cpp:456 assertion \"state_ == ProxyRunning\" failed\n> in processStatsThread()\n> Backtrace:\n> libcamera::ipa::soft::IPAProxySoft::processStatsThread(unsigned int, unsigned int, libcamera::ControlList\n> const&)+0xd6 (src/libcamera/proxy/soft_ipa_proxy.cpp:457)\n> libcamera::ipa::soft::IPAProxySoft::processStats(unsigned int, unsigned int, libcamera::ControlList\n> const&)+0x57 (src/libcamera/proxy/soft_ipa_proxy.cpp:449)\n> libcamera::SoftwareIsp::processStats(unsigned int, unsigned int, libcamera::ControlList const&)+0x117\n> (../src/libcamera/software_isp/software_isp.cpp:174)\n> libcamera::SimpleCameraData::ispStatsReady(unsigned int, unsigned int)+0x76\n> (../src/libcamera/pipeline/simple/simple.cpp:894)\n> libcamera::BoundMethodMember int>::activate(unsigned int, unsigned int, bool)+0xa4 (../include/libcamera/base/bound_method.h:180)\n> libcamera::Signal::emit(unsigned int, unsigned int)+0x99\n> (../include/libcamera/base/signal.h:152)\n> libcamera::SoftwareIsp::statsReady(unsigned int, unsigned int)+0x31\n> (../src/libcamera/software_isp/software_isp.cpp:360)\n> libcamera::BoundMethodMember::invoke(unsigned\n> int, unsigned int)+0x80 (../include/libcamera/base/bound_method.h:191)\n> std::enable_if::value, void>::type libcamera::BoundMethodArgs unsigned int>::invokePack<0ul, 1ul, void>(libcamera::BoundMethodPackBase*, std::integer_sequence long, 0ul, 1ul>)+0x5f (../include/libcamera/base/bound_method.h:116)\n> libcamera::BoundMethodArgs int>::invokePack(libcamera::BoundMethodPackBase*)+0x27 (../include/libcamera/base/bound_method.h:125)\n> libcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)\n> libcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)\n> libcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317\n> (../src/libcamera/base/thread.cpp:650)\n> libcamera::ipa::soft::IPAProxySoft::stopThread()+0x167 (src/libcamera/proxy/soft_ipa_proxy.cpp:285)\n> libcamera::ipa::soft::IPAProxySoft::stop()+0x39 (src/libcamera/proxy/soft_ipa_proxy.cpp:268)\n> libcamera::SoftwareIsp::stop()+0x64 (../src/libcamera/software_isp/software_isp.cpp:332)\n> libcamera::SimplePipelineHandler::stopDevice(libcamera::Camera*)+0xb6\n> (../src/libcamera/pipeline/simple/simple.cpp:1396)\n> libcamera::PipelineHandler::stop(libcamera::Camera*)+0x54 (../src/libcamera/pipeline_handler.cpp:367)\n> libcamera::BoundMethodMember libcamera::Camera*>::invoke(libcamera::Camera*)+0x7f (../include/libcamera/base/bound_method.h:191)\n> std::enable_if::value, void>::type libcamera::BoundMethodArgs libcamera::Camera*>::invokePack<0ul, void>(libcamera::BoundMethodPackBase*,\n> std::integer_sequence)+0x4a (../include/libcamera/base/bound_method.h:116)\n> libcamera::BoundMethodArgs::invokePack(libcamera::BoundMethodPackBase*)+0x27\n> (../include/libcamera/base/bound_method.h:125)\n> libcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)\n> libcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)\n> libcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317\n> (../src/libcamera/base/thread.cpp:650)\n> libcamera::EventDispatcherPoll::processEvents()+0x38\n> (../src/libcamera/base/event_dispatcher_poll.cpp:149)\n> libcamera::Thread::exec()+0x70 (../src/libcamera/base/thread.cpp:310)\n> libcamera::CameraManager::Private::run()+0x121 (../src/libcamera/camera_manager.cpp:91)\n> libcamera::Thread::startThread()+0xf2 (../src/libcamera/base/thread.cpp:290)\n> void std::__invoke_impl libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(),\n> libcamera::Thread*&&)+0x6a (/usr/include/c++/13/bits/invoke.h:74)\n> std::__invoke_result::type std::__invoke (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x3b\n> (/usr/include/c++/13/bits/invoke.h:97)\n> void std::thread::_Invoker\n>>::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>)+0x47 (/usr/include/c++/13/bits/std_thread.h:292)\n> std::thread::_Invoker >::operator()()+0x1c\n> (/usr/include/c++/13/bits/std_thread.h:299)\n> std::thread::_State_impl libcamera::Thread*> > >::_M_run()+0x20 (/usr/include/c++/13/bits/std_thread.h:244)\n> execute_native_thread_routine+0x14\n> (/build/gcc-14-ig5ci0/gcc-14-14.2.0/build/x86_64-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h:93)\n> start_thread+0x384 (./nptl/pthread_create.c:447)\n> __clone3+0x2c (../sysdeps/unix/sysv/linux/x86_64/clone3.S:80)\n> Aborted (core dumped)\n>\n>> ---\n>> .../internal/software_isp/software_isp.h | 3 ++-\n>> src/libcamera/pipeline/simple/simple.cpp | 16 +---------------\n>> 2 files changed, 3 insertions(+), 16 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n>> index d51b03fd..440a296d 100644\n>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>> @@ -18,6 +18,7 @@\n>> \n>> #include \n>> #include \n>> +#include \n>> #include \n>> #include \n>> \n>> @@ -43,7 +44,7 @@ struct StreamConfiguration;\n>> \n>> LOG_DECLARE_CATEGORY(SoftwareIsp)\n>> \n>> -class SoftwareIsp\n>> +class SoftwareIsp : public Object\n>> {\n>> public:\n>> \tSoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 8ac24e6e..6e039bf3 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -537,21 +537,7 @@ int SimpleCameraData::init()\n>> \t\t\t\t<< \"Failed to create software ISP, disabling software debayering\";\n>> \t\t\tswIsp_.reset();\n>> \t\t} else {\n>> -\t\t\t/*\n>> -\t\t\t * The inputBufferReady signal is emitted from the soft ISP thread,\n>> -\t\t\t * and needs to be handled in the pipeline handler thread. Signals\n>> -\t\t\t * implement queued delivery, but this works transparently only if\n>> -\t\t\t * the receiver is bound to the target thread. As the\n>> -\t\t\t * SimpleCameraData class doesn't inherit from the Object class, it\n>> -\t\t\t * is not bound to any thread, and the signal would be delivered\n>> -\t\t\t * synchronously. Instead, connect the signal to a lambda function\n>> -\t\t\t * bound explicitly to the pipe, which is bound to the pipeline\n>> -\t\t\t * handler thread. The function then simply forwards the call to\n>> -\t\t\t * conversionInputDone().\n>> -\t\t\t */\n>> -\t\t\tswIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {\n>> -\t\t\t\tthis->conversionInputDone(buffer);\n>> -\t\t\t});\n>> +\t\t\tswIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n>> \t\t\tswIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n>> \t\t\tswIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n>> \t\t\tswIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);\n>> -- \n>> 2.48.1\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EE655C3273\n\tfor ;\n\tFri, 14 Feb 2025 20:57:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 102B668659;\n\tFri, 14 Feb 2025 21:57:34 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EB7596862A\n\tfor ;\n\tFri, 14 Feb 2025 21:57:31 +0100 (CET)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-259-jx7Wx2dvP8uDOtzMF90_XA-1; Fri, 14 Feb 2025 15:57:29 -0500","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-ab7fa2b5be0so311575266b.3\n\tfor ;\n\tFri, 14 Feb 2025 12:57:27 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-abb5781da94sm62051266b.85.2025.02.14.12.57.22\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tFri, 14 Feb 2025 12:57:23 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"Yj+BVNkv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1739566650;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=a1wpXmBnVdriE2yLKjVTU+l6mx6iyGoiI+UnDgfkuoY=;\n\tb=Yj+BVNkvyMwQ5UfowDhQSFO6LaOKfgBodr2jAIBu5rR+oYuOMnEdQ7AiU7pLS7YDnDoHi1\n\tLlY/Vm87ObWePHFI04iOb+7tBt6vUFKCrZRzb3iINI/7LEGOylv94NmWm28pupzgCj9Wy1\n\tetlErfaO0FN1jl2oqzM6Yc1ljP2yIqU=","X-MC-Unique":"jx7Wx2dvP8uDOtzMF90_XA-1","X-Mimecast-MFC-AGG-ID":"jx7Wx2dvP8uDOtzMF90_XA_1739566645","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1739566645; x=1740171445;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=a1wpXmBnVdriE2yLKjVTU+l6mx6iyGoiI+UnDgfkuoY=;\n\tb=C06ze8kLhDveeQ7/YBahG97z6KB8LfBnAkzt7eZW+SYwfcTwg4SKIWLANJ9cbEMIwK\n\tNxu6gQlvmY9l4sDhE13GjcBhzN1t4avkPyviQp2MU3X95pw4nUW/55p9fqVbScL9viPZ\n\tUmrOpwCmaNzzI93ASHZHKSsTKqRhMHii089MofRGdULpwFhvIVzgtUFMe/U5RYjOYAcg\n\tekBfCOFCTJBV1pVMPEvCCCl0hle9M3ugM3jpe/rqY1MmwTKaFuSG2LkvNVqaiWrBBFCH\n\tesU0YfAmbLsXnbdQ3qj85BRJLr7zYfRGCwpQjz/6jAnK5So0ryVyBG6vPINpE7lHMKCA\n\tCGEQ==","X-Gm-Message-State":"AOJu0YxpuUfi6Zv55pFylesylNxfH/A0KGIsMJkkf6smgnY0a83/vecu\n\tqGfkVDpjeHXg5ddLGgSrZia/yrXAQyOLDlTOKDy5liJ2IQ+6X9U4p/8hJBbWIhs8EjtYZshic/b\n\tWEEa0AcxFVcvXW1kdPr/FJSXjIOWWaxD7fjEQ9ywzZPHeUeBKGarL4Cien1TM5nH/m9aG22rhfv\n\tr5bAA=","X-Gm-Gg":"ASbGncuXRAOYJFH6Qud1VJiDuJBpd8bsq1gOAR9qVHjLEK9mqBS+tg/y/CWcJiz9TyY\n\tCe7+HOrVTcL6yZuZ76QOSvsyQa6QsYdX7tXwDE9yCao3EXOBUSa5O/JmdgdFMrWj5fHU1gAAP+k\n\t/LEAlK3X0zBJtlFNtabyFNGpjErjdPqHea+tf0+RroSU3NKXiM52MejhOrmUM54sv5Aca1tAwFA\n\t7BiebGQz+MfjMvDsDx0/+kwPdIAuLbV4jEgcXXycz70npZDPXMpd58a7JeB7GRsc/68lD9Fbu46\n\tfp4kLrOWQgPGUgUzDa5HQXQ1tZ/sIrAMesuh5pikQciSVrrNyT+e2OkaUA==","X-Received":["by 2002:a17:907:7f27:b0:aa6:b473:ea4c with SMTP id\n\ta640c23a62f3a-abb70bfe005mr68015066b.10.1739566644985; \n\tFri, 14 Feb 2025 12:57:24 -0800 (PST)","by 2002:a17:907:7f27:b0:aa6:b473:ea4c with SMTP id\n\ta640c23a62f3a-abb70bfe005mr68012966b.10.1739566644479; \n\tFri, 14 Feb 2025 12:57:24 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHQ5KCZWB5unXcqnJXIMHias84KavatYM3om93v4Skmd7g254zcGPFHXVXUXoIBDuqB4rYfGA==","From":"Milan Zamazal ","To":"Stanislaw Gruszka ","Cc":"libcamera-devel@lists.libcamera.org,\n\tLaurent Pinchart , =?utf-8?q?Barna?=\n\t=?utf-8?b?YsOhcyBQxZFjemU=?= ","Subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the\n\tproper thread","In-Reply-To":" (Stanislaw Gruszka's message\n\tof \"Fri, 14 Feb 2025 09:42:11 +0100\")","References":"<20250131195928.57070-1-mzamazal@redhat.com>\n\t","Date":"Fri, 14 Feb 2025 21:57:22 +0100","Message-ID":"<851pw0453h.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"_v0Miv_kGTEhi69e9MMCXkM8AiHfp0HIJdfaAiJekuo_1739566645","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33372,"web_url":"https://patchwork.libcamera.org/comment/33372/","msgid":"<20250216003227.GG12632@pendragon.ideasonboard.com>","date":"2025-02-16T00:32:27","subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the proper\n\tthread","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Fri, Feb 14, 2025 at 09:57:22PM +0100, Milan Zamazal wrote:\n> Stanislaw Gruszka writes:\n> > On Fri, Jan 31, 2025 at 08:59:28PM +0100, Milan Zamazal wrote:\n> >> inputBufferReady ready signal in the simple pipeline is handled in the\n> >> pipeline handler thread. outputBufferReady and ispStatsReady signals\n> >> should be handled there too.\n> >> \n> >> Rather than relying on the user of the SoftwareIsp instance, let\n> >> SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the\n> >> signals above are emitted from signal handlers. This means that if\n> >> SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp\n> >> thread. Which is the camera manager thread because the SoftwareIsp\n> >> instance is created there.\n> >> \n> >> Signed-off-by: Milan Zamazal \n> >> Reviewed-by: Laurent Pinchart \n> >\n> > Unfortunately I have assertion failures like below with the change.\n> > Reproducible approximately 1 per 4 times when stopping qcam.\n> \n> I can reproduce it occasionally with cam too.\n> \n> > Could you please take a look? \n> \n> I think what happens there is:\n> \n> - The ISP stats signal is emitted at the \"right\" moment to cause the\n> race and gets queued.\n> \n> - The pipeline is asked to stop and the IPA proxy state is set to\n> ProxyStopping.\n> \n> - As part of stopping the IPA, pending messages of the given thread are\n> invoked.\n> \n> - The ISP stats message causes a call to IPA to process the stats, which\n> fails on the assertion due to ProxyStopping IPA state. The stats\n> related IPA call itself is not different from what hardware pipelines\n> do but the circumstances when it happens are different, either\n> preventing the race in hardware pipelines or making it much less\n> likely (I don't know).\n\nWe indeed tried to model the software ISP similarly to hardware devices,\nin that they would run separately from the pipeline handler thread (the\nhardware obviously runs on its own separately from the CPU, and the\nsoftware ISP runs in a separate thread to mimick that), and emit a\nsignal received in the pipeline handler thread. There's a crucial\ndifference though, related to how the signal is delivered.\n\nFor hardware devices, the file descriptor of the kernel device is added\nto the event loop's even sources (through an EventNotifier). When the\npipeline handler thread goes back to event loop processing, it sees an\nevent on the file descriptor, and calls the event notifier's signal\nhandler, in the same thread. For video devices used to capture\nstatistics, that's an instance of the V4L2VideoDevice class, which\ndecodes the event and emits the corresponding signal (e.g. bufferReady).\nThat signal is connected to the pipeline handler's buffer ready event,\nwhich dequeues the statistics buffer. All of this happens synchronously\nin the same thread. When the video device is stopped, the notifier is\ndisabled synchronously with the stop() call. As stop() is called in the\npipeline handler thread, this guarantees that no event will be received\nby the pipeline handler when stop() returns. Races are not possible.\n\nFor the software ISP, the event is delivered by emitting a signal from\nthe ISP thread, in the DebayerCpu::process() function. The signal is\nconnected to a function of the SoftwareIsp class, using queued signal\ndelivery. The function then emits another signal, connected to the\npipeline handler. Queued signal delivery means that emitting a signal\nqueues a message to the receiver's message queue. That message is\nprocessed when the receiver's thread runs the event loop, and calls the\nsignal handler synchronously from there. As for hardware devices, the\nsignal handler is guaranteed to be called in the pipeline handler\nthread. However, because the signal message can be queued before the\nsoftware ISP thread is stopped with SoftwareIsp::stop(), its delivery\ncan occur after the stop() function returns.\n\n> Now the question is what can be done about it. Things should be already\n> run in the right thread as discussed previously. The signal emitter\n> cannot do anything about the issue. So the pending message should be\n> either discarded or the IPA call should be blocked if the IPA state is\n> not ProxyRunning.\n> \n> The only way I can see to discard the messages is to destroy the\n> corresponding object, which is not an option in this case.\n> \n> I can't see no public way to check the IPA state from outside, excluding\n> the option of blocking the IPA call too. I'm not sure how to interpret\n> the fact that ProxyState enum is public while its only use is to declare\n> a protected class member; does it indicate that the option of dealing\n> with the state from outside is left open?\n> \n> At the moment, what can help is tracking the stop action in SoftwareIsp\n> class itself and preventing forwarding the signal from there in such a\n> case. This apparently fixes the issue. I can post a patch next week if\n> there is no better suggestion.\n\nEvent handling for hardware device was designed to avoid race\nconditions, and I think it has served us well. As seen here, handling\nrace conditions with threads is difficult. I think the best approach is\nto design components and APIs to isolate the rest of libcamera from\nhandling those problems. Handling the race condition in pipeline\nhandlers is likely possible, but it will be more error-prone, especially\nif we consider using the software ISP (or other CPU- or GPU-based image\nprocessing) in other pipeline handlers in the future. We should\ntherefore avoid event delivery after SoftwareIsp::stop() returns.\n\nYour proposed solution, of blocking the signal delivery in the\nSoftwareIsp class when stop() is called, seems good to me. It's as close\nas we can get to disabling the event notifier for hardware devices.\n\nThe stop() function is currently implemented as\n\nvoid SoftwareIsp::stop()\n{\n\tispWorkerThread_.exit();\n\tispWorkerThread_.wait();\n\n\tipa_->stop(); \n} \n\nAfter stopping the thread, we know that it will not touch any buffer\nanymore, and won't send any new event. We may have messages queued at\nthat point, but they won't be processed asynchronously. Normally those\nmessages won't get delivered before we return to the event loop, but\nipa_->stop() calls dispatchMessages() to address the exact same issue as\nthe one we're dealing with here. This causes the queued messages from\nthe software ISP thread (if any) to be delivered immediately. Even if\nthat wasn't the case, we would still have an issue, as the messages\nwould get delivered when the pipeline handler thread returns to the\nevent loop. We can set a running state variable to false just before\ncalling ipa_->stop(), and block delivery of the signals when the\nvariable is false.\n\nI think care also needs to be taken to return all queued buffers to\nthe pipeline handler, like we do with hardware devices. Before returning\nto the caller from the stop() function, any buffer that has been queued\nwith SoftwareIsp::queueBuffers() should be returned to the pipeline\nhandler, the same way V4L2VideoDevice::streamOff() will return buffers\nin the FrameCancelled state. The documentation of SoftwareIsp::stop()\nshould explain this.\n\nThere's one thing that makes me slightly uncomfortable with this though.\nBlocking signal delivery based on the running state means that we leave\nmessages in the queue. ipa_->stop() will flush those, but if its\nimplementation changes later, then the messages would still be in the\nqueue when SoftwareIsp::stop() returns. That's mostly fine, control will\nquickly return to the event loop, those messages will be delivered, and\nthe running state being set to false will block their delivery. However,\nif the pipeline handler were to restart the software ISP right after\nstopping it (I'm not sure why it would do so, but let's assume there\nwould be a use case), then the old messages will be delivered when\ncontrol returns to the event loop, as the running variable would be true\nat that point. I'm wondering if it wouldn't be better to drop the\nmessages from the queue just before calling ipa_->stop(), with\nThread::dispatchMessages(). I think the function should be extended with\na 'Object *receiver' parameter, and only handle messages for that\nreceiver. The IPAProxy::stop() function could then limit message\ndispatching to the appropriate receiver.\n\n> > I can dig more into it, but I think it could be easier for you :-) Can\n> > provide more data if needed.\n> >\n> > Thanks\n> > Stanislaw\n> >\n> > 2:12:50.912442715] [11753] FATAL default soft_ipa_proxy.cpp:456 assertion \"state_ == ProxyRunning\" failed\n> > in processStatsThread()\n> > Backtrace:\n> > libcamera::ipa::soft::IPAProxySoft::processStatsThread(unsigned int, unsigned int, libcamera::ControlList\n> > const&)+0xd6 (src/libcamera/proxy/soft_ipa_proxy.cpp:457)\n> > libcamera::ipa::soft::IPAProxySoft::processStats(unsigned int, unsigned int, libcamera::ControlList\n> > const&)+0x57 (src/libcamera/proxy/soft_ipa_proxy.cpp:449)\n> > libcamera::SoftwareIsp::processStats(unsigned int, unsigned int, libcamera::ControlList const&)+0x117\n> > (../src/libcamera/software_isp/software_isp.cpp:174)\n> > libcamera::SimpleCameraData::ispStatsReady(unsigned int, unsigned int)+0x76\n> > (../src/libcamera/pipeline/simple/simple.cpp:894)\n> > libcamera::BoundMethodMember > int>::activate(unsigned int, unsigned int, bool)+0xa4 (../include/libcamera/base/bound_method.h:180)\n> > libcamera::Signal::emit(unsigned int, unsigned int)+0x99\n> > (../include/libcamera/base/signal.h:152)\n> > libcamera::SoftwareIsp::statsReady(unsigned int, unsigned int)+0x31\n> > (../src/libcamera/software_isp/software_isp.cpp:360)\n> > libcamera::BoundMethodMember::invoke(unsigned\n> > int, unsigned int)+0x80 (../include/libcamera/base/bound_method.h:191)\n> > std::enable_if::value, void>::type libcamera::BoundMethodArgs > unsigned int>::invokePack<0ul, 1ul, void>(libcamera::BoundMethodPackBase*, std::integer_sequence > long, 0ul, 1ul>)+0x5f (../include/libcamera/base/bound_method.h:116)\n> > libcamera::BoundMethodArgs > int>::invokePack(libcamera::BoundMethodPackBase*)+0x27 (../include/libcamera/base/bound_method.h:125)\n> > libcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)\n> > libcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)\n> > libcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317\n> > (../src/libcamera/base/thread.cpp:650)\n> > libcamera::ipa::soft::IPAProxySoft::stopThread()+0x167 (src/libcamera/proxy/soft_ipa_proxy.cpp:285)\n> > libcamera::ipa::soft::IPAProxySoft::stop()+0x39 (src/libcamera/proxy/soft_ipa_proxy.cpp:268)\n> > libcamera::SoftwareIsp::stop()+0x64 (../src/libcamera/software_isp/software_isp.cpp:332)\n> > libcamera::SimplePipelineHandler::stopDevice(libcamera::Camera*)+0xb6\n> > (../src/libcamera/pipeline/simple/simple.cpp:1396)\n> > libcamera::PipelineHandler::stop(libcamera::Camera*)+0x54 (../src/libcamera/pipeline_handler.cpp:367)\n> > libcamera::BoundMethodMember > libcamera::Camera*>::invoke(libcamera::Camera*)+0x7f (../include/libcamera/base/bound_method.h:191)\n> > std::enable_if::value, void>::type libcamera::BoundMethodArgs > libcamera::Camera*>::invokePack<0ul, void>(libcamera::BoundMethodPackBase*,\n> > std::integer_sequence)+0x4a (../include/libcamera/base/bound_method.h:116)\n> > libcamera::BoundMethodArgs::invokePack(libcamera::BoundMethodPackBase*)+0x27\n> > (../include/libcamera/base/bound_method.h:125)\n> > libcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)\n> > libcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)\n> > libcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317\n> > (../src/libcamera/base/thread.cpp:650)\n> > libcamera::EventDispatcherPoll::processEvents()+0x38\n> > (../src/libcamera/base/event_dispatcher_poll.cpp:149)\n> > libcamera::Thread::exec()+0x70 (../src/libcamera/base/thread.cpp:310)\n> > libcamera::CameraManager::Private::run()+0x121 (../src/libcamera/camera_manager.cpp:91)\n> > libcamera::Thread::startThread()+0xf2 (../src/libcamera/base/thread.cpp:290)\n> > void std::__invoke_impl > libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(),\n> > libcamera::Thread*&&)+0x6a (/usr/include/c++/13/bits/invoke.h:74)\n> > std::__invoke_result::type std::__invoke > (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x3b\n> > (/usr/include/c++/13/bits/invoke.h:97)\n> > void std::thread::_Invoker\n> >>::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>)+0x47 (/usr/include/c++/13/bits/std_thread.h:292)\n> > std::thread::_Invoker >::operator()()+0x1c\n> > (/usr/include/c++/13/bits/std_thread.h:299)\n> > std::thread::_State_impl > libcamera::Thread*> > >::_M_run()+0x20 (/usr/include/c++/13/bits/std_thread.h:244)\n> > execute_native_thread_routine+0x14\n> > (/build/gcc-14-ig5ci0/gcc-14-14.2.0/build/x86_64-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h:93)\n> > start_thread+0x384 (./nptl/pthread_create.c:447)\n> > __clone3+0x2c (../sysdeps/unix/sysv/linux/x86_64/clone3.S:80)\n> > Aborted (core dumped)\n> >\n> >> ---\n> >> .../internal/software_isp/software_isp.h | 3 ++-\n> >> src/libcamera/pipeline/simple/simple.cpp | 16 +---------------\n> >> 2 files changed, 3 insertions(+), 16 deletions(-)\n> >> \n> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> >> index d51b03fd..440a296d 100644\n> >> --- a/include/libcamera/internal/software_isp/software_isp.h\n> >> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> >> @@ -18,6 +18,7 @@\n> >> \n> >> #include \n> >> #include \n> >> +#include \n> >> #include \n> >> #include \n> >> \n> >> @@ -43,7 +44,7 @@ struct StreamConfiguration;\n> >> \n> >> LOG_DECLARE_CATEGORY(SoftwareIsp)\n> >> \n> >> -class SoftwareIsp\n> >> +class SoftwareIsp : public Object\n> >> {\n> >> public:\n> >> \tSoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> index 8ac24e6e..6e039bf3 100644\n> >> --- a/src/libcamera/pipeline/simple/simple.cpp\n> >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> >> @@ -537,21 +537,7 @@ int SimpleCameraData::init()\n> >> \t\t\t\t<< \"Failed to create software ISP, disabling software debayering\";\n> >> \t\t\tswIsp_.reset();\n> >> \t\t} else {\n> >> -\t\t\t/*\n> >> -\t\t\t * The inputBufferReady signal is emitted from the soft ISP thread,\n> >> -\t\t\t * and needs to be handled in the pipeline handler thread. Signals\n> >> -\t\t\t * implement queued delivery, but this works transparently only if\n> >> -\t\t\t * the receiver is bound to the target thread. As the\n> >> -\t\t\t * SimpleCameraData class doesn't inherit from the Object class, it\n> >> -\t\t\t * is not bound to any thread, and the signal would be delivered\n> >> -\t\t\t * synchronously. Instead, connect the signal to a lambda function\n> >> -\t\t\t * bound explicitly to the pipe, which is bound to the pipeline\n> >> -\t\t\t * handler thread. The function then simply forwards the call to\n> >> -\t\t\t * conversionInputDone().\n> >> -\t\t\t */\n> >> -\t\t\tswIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {\n> >> -\t\t\t\tthis->conversionInputDone(buffer);\n> >> -\t\t\t});\n> >> +\t\t\tswIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n> >> \t\t\tswIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n> >> \t\t\tswIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n> >> \t\t\tswIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 83D44C3213\n\tfor ;\n\tSun, 16 Feb 2025 00:32:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B605A6865A;\n\tSun, 16 Feb 2025 01:32:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 456506185D\n\tfor ;\n\tSun, 16 Feb 2025 01:32:40 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id AD1C022F;\n\tSun, 16 Feb 2025 01:31:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PddAhhp6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1739665879;\n\tbh=Fkv+50NjHO8XOvspy860YfyWc6g0vh/cNl39Je3F73c=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PddAhhp6NxfaAMi5283UzOx6BmqQK8ucbMY2v3tb/D/7wUOb2oFUMcrtVJ021BE8Y\n\tXI2hC90CpkW61QCBaRN79QgrqHMxWkKtmgX1u4VvUAAdmByUXCQ/AjfyULK969/WJU\n\t4l6yCyKir6eOzD0mUtYx/+T+9iCgLczjaViDk+W0=","Date":"Sun, 16 Feb 2025 02:32:27 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"Stanislaw Gruszka ,\n\tlibcamera-devel@lists.libcamera.org, =?utf-8?b?QmFybmFiw6FzIFDFkWN6?=\n\t=?utf-8?q?e?= ","Subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the proper\n\tthread","Message-ID":"<20250216003227.GG12632@pendragon.ideasonboard.com>","References":"<20250131195928.57070-1-mzamazal@redhat.com>\n\t\n\t<851pw0453h.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<851pw0453h.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33383,"web_url":"https://patchwork.libcamera.org/comment/33383/","msgid":"<85bjv0hfcd.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-02-17T13:26:42","subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the\n\tproper thread","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthank you for thorough explanation and clarifications.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> On Fri, Feb 14, 2025 at 09:57:22PM +0100, Milan Zamazal wrote:\n>> Stanislaw Gruszka writes:\n>> > On Fri, Jan 31, 2025 at 08:59:28PM +0100, Milan Zamazal wrote:\n>> >> inputBufferReady ready signal in the simple pipeline is handled in the\n>> >> pipeline handler thread. outputBufferReady and ispStatsReady signals\n>> >> should be handled there too.\n>> >> \n>> >> Rather than relying on the user of the SoftwareIsp instance, let\n>> >> SoftwareIsp inherits Object. SoftwareIsp serves as a signal proxy, the\n>> >> signals above are emitted from signal handlers. This means that if\n>> >> SoftwareIsp inherits Object then the slots are invoked in SoftwareIsp\n>> >> thread. Which is the camera manager thread because the SoftwareIsp\n>> >> instance is created there.\n>> >> \n>> >> Signed-off-by: Milan Zamazal \n>> >> Reviewed-by: Laurent Pinchart \n>> >\n>> > Unfortunately I have assertion failures like below with the change.\n>> > Reproducible approximately 1 per 4 times when stopping qcam.\n>> \n>> I can reproduce it occasionally with cam too.\n>> \n>> > Could you please take a look? \n>> \n>> I think what happens there is:\n>> \n>> - The ISP stats signal is emitted at the \"right\" moment to cause the\n>> race and gets queued.\n>> \n>> - The pipeline is asked to stop and the IPA proxy state is set to\n>> ProxyStopping.\n>> \n>> - As part of stopping the IPA, pending messages of the given thread are\n>> invoked.\n>> \n>> - The ISP stats message causes a call to IPA to process the stats, which\n>> fails on the assertion due to ProxyStopping IPA state. The stats\n>> related IPA call itself is not different from what hardware pipelines\n>> do but the circumstances when it happens are different, either\n>> preventing the race in hardware pipelines or making it much less\n>> likely (I don't know).\n>\n> We indeed tried to model the software ISP similarly to hardware devices,\n> in that they would run separately from the pipeline handler thread (the\n> hardware obviously runs on its own separately from the CPU, and the\n> software ISP runs in a separate thread to mimick that), and emit a\n> signal received in the pipeline handler thread. There's a crucial\n> difference though, related to how the signal is delivered.\n>\n> For hardware devices, the file descriptor of the kernel device is added\n> to the event loop's even sources (through an EventNotifier). When the\n> pipeline handler thread goes back to event loop processing, it sees an\n> event on the file descriptor, and calls the event notifier's signal\n> handler, in the same thread. For video devices used to capture\n> statistics, that's an instance of the V4L2VideoDevice class, which\n> decodes the event and emits the corresponding signal (e.g. bufferReady).\n> That signal is connected to the pipeline handler's buffer ready event,\n> which dequeues the statistics buffer. All of this happens synchronously\n> in the same thread. When the video device is stopped, the notifier is\n> disabled synchronously with the stop() call. As stop() is called in the\n> pipeline handler thread, this guarantees that no event will be received\n> by the pipeline handler when stop() returns. Races are not possible.\n>\n> For the software ISP, the event is delivered by emitting a signal from\n> the ISP thread, in the DebayerCpu::process() function. The signal is\n> connected to a function of the SoftwareIsp class, using queued signal\n> delivery. The function then emits another signal, connected to the\n> pipeline handler. Queued signal delivery means that emitting a signal\n> queues a message to the receiver's message queue. That message is\n> processed when the receiver's thread runs the event loop, and calls the\n> signal handler synchronously from there. As for hardware devices, the\n> signal handler is guaranteed to be called in the pipeline handler\n> thread. However, because the signal message can be queued before the\n> software ISP thread is stopped with SoftwareIsp::stop(), its delivery\n> can occur after the stop() function returns.\n>\n>> Now the question is what can be done about it. Things should be already\n>> run in the right thread as discussed previously. The signal emitter\n>> cannot do anything about the issue. So the pending message should be\n>> either discarded or the IPA call should be blocked if the IPA state is\n>> not ProxyRunning.\n>> \n>> The only way I can see to discard the messages is to destroy the\n>> corresponding object, which is not an option in this case.\n>> \n>> I can't see no public way to check the IPA state from outside, excluding\n>> the option of blocking the IPA call too. I'm not sure how to interpret\n>> the fact that ProxyState enum is public while its only use is to declare\n>> a protected class member; does it indicate that the option of dealing\n>> with the state from outside is left open?\n>> \n>> At the moment, what can help is tracking the stop action in SoftwareIsp\n>> class itself and preventing forwarding the signal from there in such a\n>> case. This apparently fixes the issue. I can post a patch next week if\n>> there is no better suggestion.\n>\n> Event handling for hardware device was designed to avoid race\n> conditions, and I think it has served us well. As seen here, handling\n> race conditions with threads is difficult. I think the best approach is\n> to design components and APIs to isolate the rest of libcamera from\n> handling those problems. Handling the race condition in pipeline\n> handlers is likely possible, but it will be more error-prone, especially\n> if we consider using the software ISP (or other CPU- or GPU-based image\n> processing) in other pipeline handlers in the future. We should\n> therefore avoid event delivery after SoftwareIsp::stop() returns.\n>\n> Your proposed solution, of blocking the signal delivery in the\n> SoftwareIsp class when stop() is called, seems good to me. It's as close\n> as we can get to disabling the event notifier for hardware devices.\n>\n> The stop() function is currently implemented as\n>\n> void SoftwareIsp::stop()\n> {\n> \tispWorkerThread_.exit();\n> \tispWorkerThread_.wait();\n>\n> \tipa_->stop(); \n> } \n>\n> After stopping the thread, we know that it will not touch any buffer\n> anymore, and won't send any new event. We may have messages queued at\n> that point, but they won't be processed asynchronously. Normally those\n> messages won't get delivered before we return to the event loop, but\n> ipa_->stop() calls dispatchMessages() to address the exact same issue as\n> the one we're dealing with here. This causes the queued messages from\n> the software ISP thread (if any) to be delivered immediately. Even if\n> that wasn't the case, we would still have an issue, as the messages\n> would get delivered when the pipeline handler thread returns to the\n> event loop. We can set a running state variable to false just before\n> calling ipa_->stop(), and block delivery of the signals when the\n> variable is false.\n\nYes, this is what I attempted on Friday and it worked fine.\n\n> I think care also needs to be taken to return all queued buffers to\n> the pipeline handler, like we do with hardware devices. Before returning\n> to the caller from the stop() function, any buffer that has been queued\n> with SoftwareIsp::queueBuffers() should be returned to the pipeline\n> handler, the same way V4L2VideoDevice::streamOff() will return buffers\n> in the FrameCancelled state. The documentation of SoftwareIsp::stop()\n> should explain this.\n\nGood point.\n\n> There's one thing that makes me slightly uncomfortable with this though.\n> Blocking signal delivery based on the running state means that we leave\n> messages in the queue. ipa_->stop() will flush those, but if its\n> implementation changes later, then the messages would still be in the\n> queue when SoftwareIsp::stop() returns. That's mostly fine, control will\n> quickly return to the event loop, those messages will be delivered, and\n> the running state being set to false will block their delivery. However,\n> if the pipeline handler were to restart the software ISP right after\n> stopping it (I'm not sure why it would do so, but let's assume there\n> would be a use case), then the old messages will be delivered when\n> control returns to the event loop, as the running variable would be true\n> at that point. I'm wondering if it wouldn't be better to drop the\n> messages from the queue just before calling ipa_->stop(), with\n> Thread::dispatchMessages(). I think the function should be extended with\n> a 'Object *receiver' parameter, and only handle messages for that\n> receiver. The IPAProxy::stop() function could then limit message\n> dispatching to the appropriate receiver.\n\nLet's try and see.\n\n>> > I can dig more into it, but I think it could be easier for you :-) Can\n>> > provide more data if needed.\n>> >\n>> > Thanks\n>> > Stanislaw\n>> >\n>> > 2:12:50.912442715] [11753] FATAL default soft_ipa_proxy.cpp:456 assertion \"state_ == ProxyRunning\" failed\n>> > in processStatsThread()\n>> > Backtrace:\n>> > libcamera::ipa::soft::IPAProxySoft::processStatsThread(unsigned int, unsigned int, libcamera::ControlList\n>> > const&)+0xd6 (src/libcamera/proxy/soft_ipa_proxy.cpp:457)\n>> > libcamera::ipa::soft::IPAProxySoft::processStats(unsigned int, unsigned int, libcamera::ControlList\n>> > const&)+0x57 (src/libcamera/proxy/soft_ipa_proxy.cpp:449)\n>> > libcamera::SoftwareIsp::processStats(unsigned int, unsigned int, libcamera::ControlList const&)+0x117\n>> > (../src/libcamera/software_isp/software_isp.cpp:174)\n>> > libcamera::SimpleCameraData::ispStatsReady(unsigned int, unsigned int)+0x76\n>> > (../src/libcamera/pipeline/simple/simple.cpp:894)\n>> > libcamera::BoundMethodMember> > int>::activate(unsigned int, unsigned int, bool)+0xa4 (../include/libcamera/base/bound_method.h:180)\n>> > libcamera::Signal::emit(unsigned int, unsigned int)+0x99\n>> > (../include/libcamera/base/signal.h:152)\n>> > libcamera::SoftwareIsp::statsReady(unsigned int, unsigned int)+0x31\n>> > (../src/libcamera/software_isp/software_isp.cpp:360)\n>> > libcamera::BoundMethodMember::invoke(unsigned\n>> > int, unsigned int)+0x80 (../include/libcamera/base/bound_method.h:191)\n>> > std::enable_if::value, void>::type libcamera::BoundMethodArgs> > unsigned int>::invokePack<0ul, 1ul, void>(libcamera::BoundMethodPackBase*, std::integer_sequence> > long, 0ul, 1ul>)+0x5f (../include/libcamera/base/bound_method.h:116)\n>> > libcamera::BoundMethodArgs> > int>::invokePack(libcamera::BoundMethodPackBase*)+0x27 (../include/libcamera/base/bound_method.h:125)\n>> > libcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)\n>> > libcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)\n>> > libcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317\n>> > (../src/libcamera/base/thread.cpp:650)\n>> > libcamera::ipa::soft::IPAProxySoft::stopThread()+0x167 (src/libcamera/proxy/soft_ipa_proxy.cpp:285)\n>> > libcamera::ipa::soft::IPAProxySoft::stop()+0x39 (src/libcamera/proxy/soft_ipa_proxy.cpp:268)\n>> > libcamera::SoftwareIsp::stop()+0x64 (../src/libcamera/software_isp/software_isp.cpp:332)\n>> > libcamera::SimplePipelineHandler::stopDevice(libcamera::Camera*)+0xb6\n>> > (../src/libcamera/pipeline/simple/simple.cpp:1396)\n>> > libcamera::PipelineHandler::stop(libcamera::Camera*)+0x54 (../src/libcamera/pipeline_handler.cpp:367)\n>> > libcamera::BoundMethodMember> > libcamera::Camera*>::invoke(libcamera::Camera*)+0x7f (../include/libcamera/base/bound_method.h:191)\n>> > std::enable_if::value, void>::type libcamera::BoundMethodArgs> > libcamera::Camera*>::invokePack<0ul, void>(libcamera::BoundMethodPackBase*,\n>> > std::integer_sequence)+0x4a (../include/libcamera/base/bound_method.h:116)\n>> > libcamera::BoundMethodArgs::invokePack(libcamera::BoundMethodPackBase*)+0x27\n>> > (../include/libcamera/base/bound_method.h:125)\n>> > libcamera::InvokeMessage::invoke()+0x46 (../src/libcamera/base/message.cpp:154)\n>> > libcamera::Object::message(libcamera::Message*)+0x7a (../src/libcamera/base/object.cpp:213)\n>> > libcamera::Thread::dispatchMessages(libcamera::Message::Type)+0x317\n>> > (../src/libcamera/base/thread.cpp:650)\n>> > libcamera::EventDispatcherPoll::processEvents()+0x38\n>> > (../src/libcamera/base/event_dispatcher_poll.cpp:149)\n>> > libcamera::Thread::exec()+0x70 (../src/libcamera/base/thread.cpp:310)\n>> > libcamera::CameraManager::Private::run()+0x121 (../src/libcamera/camera_manager.cpp:91)\n>> > libcamera::Thread::startThread()+0xf2 (../src/libcamera/base/thread.cpp:290)\n>> > void std::__invoke_impl> > libcamera::Thread*>(std::__invoke_memfun_deref, void (libcamera::Thread::*&&)(),\n>> > libcamera::Thread*&&)+0x6a (/usr/include/c++/13/bits/invoke.h:74)\n>> > std::__invoke_result::type std::__invoke> > (libcamera::Thread::*)(), libcamera::Thread*>(void (libcamera::Thread::*&&)(), libcamera::Thread*&&)+0x3b\n>> > (/usr/include/c++/13/bits/invoke.h:97)\n>> > void std::thread::_Invoker\n>> >>::_M_invoke<0ul, 1ul>(std::_Index_tuple<0ul, 1ul>)+0x47 (/usr/include/c++/13/bits/std_thread.h:292)\n>> > std::thread::_Invoker >::operator()()+0x1c\n>> > (/usr/include/c++/13/bits/std_thread.h:299)\n>> > std::thread::_State_impl> > libcamera::Thread*> > >::_M_run()+0x20 (/usr/include/c++/13/bits/std_thread.h:244)\n>> > execute_native_thread_routine+0x14\n>> > (/build/gcc-14-ig5ci0/gcc-14-14.2.0/build/x86_64-linux-gnu/libstdc++-v3/include/bits/unique_ptr.h:93)\n>> > start_thread+0x384 (./nptl/pthread_create.c:447)\n>> > __clone3+0x2c (../sysdeps/unix/sysv/linux/x86_64/clone3.S:80)\n>> > Aborted (core dumped)\n>> >\n>> >> ---\n>> >> .../internal/software_isp/software_isp.h | 3 ++-\n>> >> src/libcamera/pipeline/simple/simple.cpp | 16 +---------------\n>> >> 2 files changed, 3 insertions(+), 16 deletions(-)\n>> >> \n>> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n>> >> index d51b03fd..440a296d 100644\n>> >> --- a/include/libcamera/internal/software_isp/software_isp.h\n>> >> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>> >> @@ -18,6 +18,7 @@\n>> >> \n>> >> #include \n>> >> #include \n>> >> +#include \n>> >> #include \n>> >> #include \n>> >> \n>> >> @@ -43,7 +44,7 @@ struct StreamConfiguration;\n>> >> \n>> >> LOG_DECLARE_CATEGORY(SoftwareIsp)\n>> >> \n>> >> -class SoftwareIsp\n>> >> +class SoftwareIsp : public Object\n>> >> {\n>> >> public:\n>> >> \tSoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> >> index 8ac24e6e..6e039bf3 100644\n>> >> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> >> @@ -537,21 +537,7 @@ int SimpleCameraData::init()\n>> >> \t\t\t\t<< \"Failed to create software ISP, disabling software debayering\";\n>> >> \t\t\tswIsp_.reset();\n>> >> \t\t} else {\n>> >> -\t\t\t/*\n>> >> -\t\t\t * The inputBufferReady signal is emitted from the soft ISP thread,\n>> >> -\t\t\t * and needs to be handled in the pipeline handler thread. Signals\n>> >> -\t\t\t * implement queued delivery, but this works transparently only if\n>> >> -\t\t\t * the receiver is bound to the target thread. As the\n>> >> -\t\t\t * SimpleCameraData class doesn't inherit from the Object class, it\n>> >> -\t\t\t * is not bound to any thread, and the signal would be delivered\n>> >> -\t\t\t * synchronously. Instead, connect the signal to a lambda function\n>> >> -\t\t\t * bound explicitly to the pipe, which is bound to the pipeline\n>> >> -\t\t\t * handler thread. The function then simply forwards the call to\n>> >> -\t\t\t * conversionInputDone().\n>> >> -\t\t\t */\n>> >> -\t\t\tswIsp_->inputBufferReady.connect(pipe, [this](FrameBuffer *buffer) {\n>> >> -\t\t\t\tthis->conversionInputDone(buffer);\n>> >> -\t\t\t});\n>> >> +\t\t\tswIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n>> >> \t\t\tswIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n>> >> \t\t\tswIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n>> >> \t\t\tswIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A1719BE08B\n\tfor ;\n\tMon, 17 Feb 2025 13:26:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 93F5F6866D;\n\tMon, 17 Feb 2025 14:26:51 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 892CD61865\n\tfor ;\n\tMon, 17 Feb 2025 14:26:49 +0100 (CET)","from mail-ed1-f71.google.com (mail-ed1-f71.google.com\n\t[209.85.208.71]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-218-vqSG5jKCMeywirlBHflBdw-1; Mon, 17 Feb 2025 08:26:46 -0500","by mail-ed1-f71.google.com with SMTP id\n\t4fb4d7f45d1cf-5dc5b397109so3667736a12.2\n\tfor ;\n\tMon, 17 Feb 2025 05:26:46 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5dece1c43a0sm7250274a12.28.2025.02.17.05.26.42\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tMon, 17 Feb 2025 05:26:43 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"eNMY5sjw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1739798808;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=8VbsQv6KZnCbgujtUVkM76g+E5LtviKciFnCi3X733g=;\n\tb=eNMY5sjw2KvGANbsYC/YsHOyq/RRh7D2ED+kfeKdNaYW91B47sOkWjuEYianAQKOpquBWx\n\tRNTthTzAVsxtZdnXezILU+3WZuzB+EE2msk0EeadabBGELvF01yl/JHHsA/UuYWEgZbzpV\n\tLzP71CdsKrOvJCgpc45LFvFOZK6zt+Y=","X-MC-Unique":"vqSG5jKCMeywirlBHflBdw-1","X-Mimecast-MFC-AGG-ID":"vqSG5jKCMeywirlBHflBdw_1739798805","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1739798805; x=1740403605;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=8VbsQv6KZnCbgujtUVkM76g+E5LtviKciFnCi3X733g=;\n\tb=cQEdmsMYvAlYy44Tzi24wjbuak7KrZgIPUPpZUeDFdFTHL5+7eCPoPOYNDjaA4wqRV\n\txKBq9SQCz3hSJNTYtSNl/w16fI/g9tlt6IWxxS7/ENeC8KdNgPkCpm++ZlExWQpyzE9p\n\t90soYsBTRoKFUbzVSIbZQypAWHRibio356YwVmlFC1HhCG+xPn77A5ZIGZiBjsRJE5LF\n\tKJ2TFkidCQDKOV/3+U+D3Mb0c8R/D/O2QyXY60aABFRi10/XrZvvLfUn6XBCGhZQN9go\n\tSld3x6NWPMn5pDbl9f6+NdfdWD91yUza53ve57qgv1+tgzVIP/2Ff6nkWpB6kWXU1jeS\n\tWfYQ==","X-Forwarded-Encrypted":"i=1;\n\tAJvYcCWat1xiuyHpBLaFcmR9GJrJpxextNQknIczKUDjrtY1soQNF88hYs/Jc4W1rcQa9rAREC9Sb9rd11GecAPLFWE=@lists.libcamera.org","X-Gm-Message-State":"AOJu0YzHiQUBlxDpNYMWgs7MOsQhhu113BuTjDXFrJF5GGDWryJ9BZ3m\n\t1rs8z7jR1uLEcVBPNX0G2cLyl5fLPkxpFIlrCe2NNhBFFSkOZTpfGrBg1uQEEwzGmGUaCIOAtEY\n\tAV6hEWsxnacnU5R9ZyseqXu+HHlNpkw+YkDYgGvSQgwzNR7PZoAiINSArpz2yUieBOBRi9wA=","X-Gm-Gg":"ASbGncuOhYTee1c7cp2VIaNejP/iab6rlKail/e1NjsBk1m4eCO7l2JubDH8HzxJyMm\n\tyLj2SQzJv4i29iZ5Le4yI/byw6EqYahqoSrzUR+lK8fCy8DglNr2y2vIP+xaUJ5arszBvknwGWC\n\t6ydLdNrsZTqSLTCzBbKIyFeFk7v9PmoqiqzjtOlCX24iOfKab5qAYEubMpS/whNSqDOF+Oe8h5b\n\tu47W6jI4qCSgeU3/l7MeVhyQQ14cDwInfk9Xol1W7lTyQPn+XVtwIYFb7tw26C54w4eRJuH3wKi\n\tQT8VkiqPNOBZAjceAcnbnG+/EDNT4cWyEzoIDuBflEz3BtcKo0CWVwXE5Q==","X-Received":["by 2002:a05:6402:13cf:b0:5e0:7199:5495 with SMTP id\n\t4fb4d7f45d1cf-5e071995c7cmr259962a12.6.1739798805404; \n\tMon, 17 Feb 2025 05:26:45 -0800 (PST)","by 2002:a05:6402:13cf:b0:5e0:7199:5495 with SMTP id\n\t4fb4d7f45d1cf-5e071995c7cmr259932a12.6.1739798804842; \n\tMon, 17 Feb 2025 05:26:44 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IHTdNsuUmwX2EzTVrlDqsDz4HI+8hguSwHHGwFf1JOhuUlc6r8PveD2E6af1poicU0k2+yvgw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"Stanislaw Gruszka ,\n\tlibcamera-devel@lists.libcamera.org, =?utf-8?b?QmFybmFiw6FzIFDFkWN6?=\n\t=?utf-8?q?e?= ","Subject":"Re: [PATCH v3] libcamera: software_isp: Handle signals in the\n\tproper thread","In-Reply-To":"<20250216003227.GG12632@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Sun, 16 Feb 2025 02:32:27 +0200\")","References":"<20250131195928.57070-1-mzamazal@redhat.com>\n\t\n\t<851pw0453h.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>\n\t<20250216003227.GG12632@pendragon.ideasonboard.com>","Date":"Mon, 17 Feb 2025 14:26:42 +0100","Message-ID":"<85bjv0hfcd.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"caFNoD7c58n8yH-CPkjpuqOrdCHAEnVR4tiTKryaf9o_1739798805","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]