[{"id":35349,"web_url":"https://patchwork.libcamera.org/comment/35349/","msgid":"<20250811170532.GC30054@pendragon.ideasonboard.com>","date":"2025-08-11T17:05:32","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nThank you for the patch.\n\nOn Mon, Aug 11, 2025 at 11:49:26AM +0200, Barnabás Pőcze wrote:\n> Currently the virtual pipeline generates the images synchronously. This is not\n> ideal because it blocks the camera manager's internal thread, and because its\n> behaviour is different from other existing pipeline handlers, all of which\n> complete requests asynchronously.\n> \n> So move the image generation to a separate thread by deriving `VirtualCameraData`\n> from `Thread`, as well as `Object` and using the existing asynchronous signal\n> and method call mechanism.\n> \n> Signed-off-by: Barnabás Pőcze \n> ---\n> src/libcamera/pipeline/virtual/virtual.cpp | 80 ++++++++++++++--------\n> src/libcamera/pipeline/virtual/virtual.h | 8 ++-\n> 2 files changed, 58 insertions(+), 30 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> index 049ebcba5..2ad32ec0a 100644\n> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> @@ -129,6 +129,38 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> \n> \t/* \\todo Support multiple streams and pass multi_stream_test */\n> \tstreamConfigs_.resize(kMaxStream);\n> +\n> +\tmoveToThread(this);\n> +}\n> +\n> +void VirtualCameraData::queueRequest(Request *request)\n> +{\n> +\tfor (auto const &[stream, buffer] : request->buffers()) {\n> +\t\tbool found = false;\n> +\t\t/* map buffer and fill test patterns */\n> +\t\tfor (auto &streamConfig : streamConfigs_) {\n> +\t\t\tif (stream == &streamConfig.stream) {\n> +\t\t\t\tFrameMetadata &fmd = buffer->_d()->metadata();\n> +\n> +\t\t\t\tfmd.status = FrameMetadata::Status::FrameSuccess;\n> +\t\t\t\tfmd.sequence = streamConfig.seq++;\n> +\t\t\t\tfmd.timestamp = currentTimestamp();\n> +\n> +\t\t\t\tfor (const auto [i, p] : utils::enumerate(buffer->planes()))\n> +\t\t\t\t\tfmd.planes()[i].bytesused = p.length;\n> +\n> +\t\t\t\tfound = true;\n> +\n> +\t\t\t\tif (streamConfig.frameGenerator->generateFrame(\n> +\t\t\t\t\t stream->configuration().size, buffer))\n> +\t\t\t\t\tfmd.status = FrameMetadata::Status::FrameError;\n> +\n> +\t\t\t\tbufferCompleted.emit(request, buffer);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\t\tASSERT(found);\n> +\t}\n> }\n> \n> VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> @@ -291,11 +323,28 @@ int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> \tfor (auto &s : data->streamConfigs_)\n> \t\ts.seq = 0;\n> \n> +\tdata->bufferCompleted.connect(this, [&](Request *request, FrameBuffer *buffer) {\n> +\t\tif (completeBuffer(request, buffer))\n> +\t\t\tcompleteRequest(request);\n> +\t});\n\nI'm not a big fan of lambdas in such cases, I find they hinder\nreadability compared to member functions. The the PipelineHandlerVirtual\nclass is not exposed outside of the pipeline handler, adding a private\nmember function shouldn't be a big issue.\n\n> +\n> +\tdata->start();\n> +\n> \treturn 0;\n> }\n> \n> -void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> +void PipelineHandlerVirtual::stopDevice(Camera *camera)\n> {\n> +\tVirtualCameraData *data = cameraData(camera);\n> +\n> +\t/* Flush all work. */\n> +\tdata->invokeMethod([] { }, ConnectionTypeBlocking);\n\nDoes this mean the Thread class API is lacking ? In particular, I don't\nsee a similar call in the SoftwareIsp::stop() function. Is it needed\nhere because we rely solely on the queue of messages to keep track the\nrequests ? If so, I think we should keep track of the queued requests in\nthe VirtualCameraData class, and cancel all requests not processed after\nthe thread exits. This will make stopping the camera faster, and will\nalso mimick better the behaviour of hardware cameras.\n\nI know it's a bit more work, hopefully it's just freshening up the yak\nand not fully shaving it :-)\n\n> +\tdata->exit();\n> +\tdata->wait();\n> +\n> +\t/* Process queued `bufferCompleted` signal emissions. */\n> +\tThread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n> +\tdata->bufferCompleted.disconnect(this);\n> }\n> \n> int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> @@ -304,35 +353,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> \tVirtualCameraData *data = cameraData(camera);\n> \tconst auto timestamp = currentTimestamp();\n> \n> -\tfor (auto const &[stream, buffer] : request->buffers()) {\n> -\t\tbool found = false;\n> -\t\t/* map buffer and fill test patterns */\n> -\t\tfor (auto &streamConfig : data->streamConfigs_) {\n> -\t\t\tif (stream == &streamConfig.stream) {\n> -\t\t\t\tFrameMetadata &fmd = buffer->_d()->metadata();\n> -\n> -\t\t\t\tfmd.status = FrameMetadata::Status::FrameSuccess;\n> -\t\t\t\tfmd.sequence = streamConfig.seq++;\n> -\t\t\t\tfmd.timestamp = timestamp;\n> -\n> -\t\t\t\tfor (const auto [i, p] : utils::enumerate(buffer->planes()))\n> -\t\t\t\t\tfmd.planes()[i].bytesused = p.length;\n> -\n> -\t\t\t\tfound = true;\n> -\n> -\t\t\t\tif (streamConfig.frameGenerator->generateFrame(\n> -\t\t\t\t\t stream->configuration().size, buffer))\n> -\t\t\t\t\tfmd.status = FrameMetadata::Status::FrameError;\n> -\n> -\t\t\t\tcompleteBuffer(request, buffer);\n> -\t\t\t\tbreak;\n> -\t\t\t}\n> -\t\t}\n> -\t\tASSERT(found);\n> -\t}\n> -\n> \trequest->metadata().set(controls::SensorTimestamp, timestamp);\n> -\tcompleteRequest(request);\n> +\tdata->invokeMethod(&VirtualCameraData::queueRequest, ConnectionTypeQueued, request);\n> \n> \treturn 0;\n> }\n> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> index 683cb82b4..0832fd80c 100644\n> --- a/src/libcamera/pipeline/virtual/virtual.h\n> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> \n> +#include \n\nYou should also include object.h.\n\n> #include \n> #include \n> \n> @@ -25,7 +26,9 @@ namespace libcamera {\n> \n> using VirtualFrame = std::variant;\n> \n> -class VirtualCameraData : public Camera::Private\n> +class VirtualCameraData : public Camera::Private,\n> +\t\t\t public Thread,\n> +\t\t\t public Object\n> {\n> public:\n> \tconst static unsigned int kMaxStream = 3;\n> @@ -54,9 +57,12 @@ public:\n> \n> \t~VirtualCameraData() = default;\n> \n> +\tvoid queueRequest(Request *request);\n> +\n> \tConfiguration config_;\n> \n> \tstd::vector streamConfigs_;\n> +\tSignal bufferCompleted;\n> };\n> \n> } /* namespace libcamera */","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 8158ABEFBE\n\tfor ;\n\tMon, 11 Aug 2025 17:06:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 823C76923C;\n\tMon, 11 Aug 2025 19:06:01 +0200 (CEST)","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 8047C69233\n\tfor ;\n\tMon, 11 Aug 2025 19:05:50 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 007EB82A;\n\tMon, 11 Aug 2025 19:04:57 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"fYftEk6m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754931898;\n\tbh=KE/Nq0bzuIMMu33N0HnPiyrRnKu4DMJF1mIf+I1FlVI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=fYftEk6mtWL7Uw0+vXdH2tsi02IjZg1zznSSBGFAv4T2gX0XdldPBXxNHYXIfy3Hn\n\t3jhneBsFq+RNbWF/w8eTBQ4mh0cgTQRL0z0qAfSGLM1L4on5mj7GTFuD2mFDMD+r9M\n\tVXUlirmqZdXw6m7P7WdW77Bl14M3I+LsAh2bq1n0=","Date":"Mon, 11 Aug 2025 20:05:32 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","Message-ID":"<20250811170532.GC30054@pendragon.ideasonboard.com>","References":"<20250811094926.1308259-1-barnabas.pocze@ideasonboard.com>\n\t<20250811094926.1308259-3-barnabas.pocze@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20250811094926.1308259-3-barnabas.pocze@ideasonboard.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":35351,"web_url":"https://patchwork.libcamera.org/comment/35351/","msgid":"<1a21d4dc-6201-4b1f-8f3e-ce94ffb6f1a9@ideasonboard.com>","date":"2025-08-11T17:51:21","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 08. 11. 19:05 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> Thank you for the patch.\n> \n> On Mon, Aug 11, 2025 at 11:49:26AM +0200, Barnabás Pőcze wrote:\n>> Currently the virtual pipeline generates the images synchronously. This is not\n>> ideal because it blocks the camera manager's internal thread, and because its\n>> behaviour is different from other existing pipeline handlers, all of which\n>> complete requests asynchronously.\n>>\n>> So move the image generation to a separate thread by deriving `VirtualCameraData`\n>> from `Thread`, as well as `Object` and using the existing asynchronous signal\n>> and method call mechanism.\n>>\n>> Signed-off-by: Barnabás Pőcze \n>> ---\n>> src/libcamera/pipeline/virtual/virtual.cpp | 80 ++++++++++++++--------\n>> src/libcamera/pipeline/virtual/virtual.h | 8 ++-\n>> 2 files changed, 58 insertions(+), 30 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n>> index 049ebcba5..2ad32ec0a 100644\n>> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n>> @@ -129,6 +129,38 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n>> \n>> \t/* \\todo Support multiple streams and pass multi_stream_test */\n>> \tstreamConfigs_.resize(kMaxStream);\n>> +\n>> +\tmoveToThread(this);\n>> +}\n>> +\n>> +void VirtualCameraData::queueRequest(Request *request)\n>> +{\n>> +\tfor (auto const &[stream, buffer] : request->buffers()) {\n>> +\t\tbool found = false;\n>> +\t\t/* map buffer and fill test patterns */\n>> +\t\tfor (auto &streamConfig : streamConfigs_) {\n>> +\t\t\tif (stream == &streamConfig.stream) {\n>> +\t\t\t\tFrameMetadata &fmd = buffer->_d()->metadata();\n>> +\n>> +\t\t\t\tfmd.status = FrameMetadata::Status::FrameSuccess;\n>> +\t\t\t\tfmd.sequence = streamConfig.seq++;\n>> +\t\t\t\tfmd.timestamp = currentTimestamp();\n>> +\n>> +\t\t\t\tfor (const auto [i, p] : utils::enumerate(buffer->planes()))\n>> +\t\t\t\t\tfmd.planes()[i].bytesused = p.length;\n>> +\n>> +\t\t\t\tfound = true;\n>> +\n>> +\t\t\t\tif (streamConfig.frameGenerator->generateFrame(\n>> +\t\t\t\t\t stream->configuration().size, buffer))\n>> +\t\t\t\t\tfmd.status = FrameMetadata::Status::FrameError;\n>> +\n>> +\t\t\t\tbufferCompleted.emit(request, buffer);\n>> +\t\t\t\tbreak;\n>> +\t\t\t}\n>> +\t\t}\n>> +\t\tASSERT(found);\n>> +\t}\n>> }\n>> \n>> VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n>> @@ -291,11 +323,28 @@ int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n>> \tfor (auto &s : data->streamConfigs_)\n>> \t\ts.seq = 0;\n>> \n>> +\tdata->bufferCompleted.connect(this, [&](Request *request, FrameBuffer *buffer) {\n>> +\t\tif (completeBuffer(request, buffer))\n>> +\t\t\tcompleteRequest(request);\n>> +\t});\n> \n> I'm not a big fan of lambdas in such cases, I find they hinder\n> readability compared to member functions. The the PipelineHandlerVirtual\n> class is not exposed outside of the pipeline handler, adding a private\n> member function shouldn't be a big issue.\n\nInteresting, I have the exact opposite view. I find that member functions\nusually involve more work and they don't allow for the same level of \"scoping\"\nas lambdas.\n\n\n> \n>> +\n>> +\tdata->start();\n>> +\n>> \treturn 0;\n>> }\n>> \n>> -void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n>> +void PipelineHandlerVirtual::stopDevice(Camera *camera)\n>> {\n>> +\tVirtualCameraData *data = cameraData(camera);\n>> +\n>> +\t/* Flush all work. */\n>> +\tdata->invokeMethod([] { }, ConnectionTypeBlocking);\n> \n> Does this mean the Thread class API is lacking ? In particular, I don't\n> see a similar call in the SoftwareIsp::stop() function. Is it needed\n> here because we rely solely on the queue of messages to keep track the\n> requests ? If so, I think we should keep track of the queued requests in\n> the VirtualCameraData class, and cancel all requests not processed after\n\nIt's already tracked in `Camera::Private::queuedRequests_`, so that part seems easy.\n\nThere is a need for flushing/cancelling asynchronous method invocations queued on\nthe worker thread(s), otherwise they will be executed when the thread is restarted\n(or am I missing something that prevents this?). So we need a `cancelMessages()`\nfunction in `Thread` then, it seems?\n\n\n> the thread exits. This will make stopping the camera faster, and will\n> also mimick better the behaviour of hardware cameras.\n> \n> I know it's a bit more work, hopefully it's just freshening up the yak\n> and not fully shaving it :-)\n> \n>> +\tdata->exit();\n>> +\tdata->wait();\n>> +\n>> +\t/* Process queued `bufferCompleted` signal emissions. */\n>> +\tThread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n>> +\tdata->bufferCompleted.disconnect(this);\n>> }\n>> \n>> int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n>> @@ -304,35 +353,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n>> \tVirtualCameraData *data = cameraData(camera);\n>> \tconst auto timestamp = currentTimestamp();\n>> \n>> -\tfor (auto const &[stream, buffer] : request->buffers()) {\n>> -\t\tbool found = false;\n>> -\t\t/* map buffer and fill test patterns */\n>> -\t\tfor (auto &streamConfig : data->streamConfigs_) {\n>> -\t\t\tif (stream == &streamConfig.stream) {\n>> -\t\t\t\tFrameMetadata &fmd = buffer->_d()->metadata();\n>> -\n>> -\t\t\t\tfmd.status = FrameMetadata::Status::FrameSuccess;\n>> -\t\t\t\tfmd.sequence = streamConfig.seq++;\n>> -\t\t\t\tfmd.timestamp = timestamp;\n>> -\n>> -\t\t\t\tfor (const auto [i, p] : utils::enumerate(buffer->planes()))\n>> -\t\t\t\t\tfmd.planes()[i].bytesused = p.length;\n>> -\n>> -\t\t\t\tfound = true;\n>> -\n>> -\t\t\t\tif (streamConfig.frameGenerator->generateFrame(\n>> -\t\t\t\t\t stream->configuration().size, buffer))\n>> -\t\t\t\t\tfmd.status = FrameMetadata::Status::FrameError;\n>> -\n>> -\t\t\t\tcompleteBuffer(request, buffer);\n>> -\t\t\t\tbreak;\n>> -\t\t\t}\n>> -\t\t}\n>> -\t\tASSERT(found);\n>> -\t}\n>> -\n>> \trequest->metadata().set(controls::SensorTimestamp, timestamp);\n>> -\tcompleteRequest(request);\n>> +\tdata->invokeMethod(&VirtualCameraData::queueRequest, ConnectionTypeQueued, request);\n>> \n>> \treturn 0;\n>> }\n>> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n>> index 683cb82b4..0832fd80c 100644\n>> --- a/src/libcamera/pipeline/virtual/virtual.h\n>> +++ b/src/libcamera/pipeline/virtual/virtual.h\n>> @@ -11,6 +11,7 @@\n>> #include \n>> #include \n>> \n>> +#include \n> \n> You should also include object.h.\n> \n>> #include \n>> #include \n>> \n>> @@ -25,7 +26,9 @@ namespace libcamera {\n>> \n>> using VirtualFrame = std::variant;\n>> \n>> -class VirtualCameraData : public Camera::Private\n>> +class VirtualCameraData : public Camera::Private,\n>> +\t\t\t public Thread,\n>> +\t\t\t public Object\n>> {\n>> public:\n>> \tconst static unsigned int kMaxStream = 3;\n>> @@ -54,9 +57,12 @@ public:\n>> \n>> \t~VirtualCameraData() = default;\n>> \n>> +\tvoid queueRequest(Request *request);\n>> +\n>> \tConfiguration config_;\n>> \n>> \tstd::vector streamConfigs_;\n>> +\tSignal bufferCompleted;\n>> };\n>> \n>> } /* namespace libcamera */\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 15067BDCC1\n\tfor ;\n\tMon, 11 Aug 2025 17:51:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 334476923C;\n\tMon, 11 Aug 2025 19:51:26 +0200 (CEST)","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 1A40369233\n\tfor ;\n\tMon, 11 Aug 2025 19:51:25 +0200 (CEST)","from [192.168.33.20] (185.221.140.182.nat.pool.zt.hu\n\t[185.221.140.182])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9669F82A;\n\tMon, 11 Aug 2025 19:50:32 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"udva7Wmy\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754934632;\n\tbh=DCw/7ziazF5OK5mA3kA17LEsGUJeRRH8lYcYHtDpmJ0=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=udva7Wmy0QmUgYCJsx9pMVcKIG+iKg38V+jS1Z45dW5BW3sAzY5Na3wI9b9TSGPsS\n\tZz1eZk6MtOwNFdCTnY0KBitKxau8KVGvhsIpotn8BgbyDpAqJZDZXnzY4Kw3DaWMVr\n\tIw9NWQfiFom5V+pJRlY6R3eJcyamvizjPMBkofc0=","Message-ID":"<1a21d4dc-6201-4b1f-8f3e-ce94ffb6f1a9@ideasonboard.com>","Date":"Mon, 11 Aug 2025 19:51:21 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250811094926.1308259-1-barnabas.pocze@ideasonboard.com>\n\t<20250811094926.1308259-3-barnabas.pocze@ideasonboard.com>\n\t<20250811170532.GC30054@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250811170532.GC30054@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":35352,"web_url":"https://patchwork.libcamera.org/comment/35352/","msgid":"<20250811224735.GE30054@pendragon.ideasonboard.com>","date":"2025-08-11T22:47:35","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Barnabás,\n\nOn Mon, Aug 11, 2025 at 07:51:21PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 11. 19:05 keltezéssel, Laurent Pinchart írta:\n> > On Mon, Aug 11, 2025 at 11:49:26AM +0200, Barnabás Pőcze wrote:\n> >> Currently the virtual pipeline generates the images synchronously. This is not\n> >> ideal because it blocks the camera manager's internal thread, and because its\n> >> behaviour is different from other existing pipeline handlers, all of which\n> >> complete requests asynchronously.\n> >>\n> >> So move the image generation to a separate thread by deriving `VirtualCameraData`\n> >> from `Thread`, as well as `Object` and using the existing asynchronous signal\n> >> and method call mechanism.\n> >>\n> >> Signed-off-by: Barnabás Pőcze \n> >> ---\n> >> src/libcamera/pipeline/virtual/virtual.cpp | 80 ++++++++++++++--------\n> >> src/libcamera/pipeline/virtual/virtual.h | 8 ++-\n> >> 2 files changed, 58 insertions(+), 30 deletions(-)\n> >>\n> >> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> >> index 049ebcba5..2ad32ec0a 100644\n> >> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> >> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> >> @@ -129,6 +129,38 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> >> \n> >> \t/* \\todo Support multiple streams and pass multi_stream_test */\n> >> \tstreamConfigs_.resize(kMaxStream);\n> >> +\n> >> +\tmoveToThread(this);\n> >> +}\n> >> +\n> >> +void VirtualCameraData::queueRequest(Request *request)\n> >> +{\n> >> +\tfor (auto const &[stream, buffer] : request->buffers()) {\n> >> +\t\tbool found = false;\n> >> +\t\t/* map buffer and fill test patterns */\n> >> +\t\tfor (auto &streamConfig : streamConfigs_) {\n> >> +\t\t\tif (stream == &streamConfig.stream) {\n> >> +\t\t\t\tFrameMetadata &fmd = buffer->_d()->metadata();\n> >> +\n> >> +\t\t\t\tfmd.status = FrameMetadata::Status::FrameSuccess;\n> >> +\t\t\t\tfmd.sequence = streamConfig.seq++;\n> >> +\t\t\t\tfmd.timestamp = currentTimestamp();\n> >> +\n> >> +\t\t\t\tfor (const auto [i, p] : utils::enumerate(buffer->planes()))\n> >> +\t\t\t\t\tfmd.planes()[i].bytesused = p.length;\n> >> +\n> >> +\t\t\t\tfound = true;\n> >> +\n> >> +\t\t\t\tif (streamConfig.frameGenerator->generateFrame(\n> >> +\t\t\t\t\t stream->configuration().size, buffer))\n> >> +\t\t\t\t\tfmd.status = FrameMetadata::Status::FrameError;\n> >> +\n> >> +\t\t\t\tbufferCompleted.emit(request, buffer);\n> >> +\t\t\t\tbreak;\n> >> +\t\t\t}\n> >> +\t\t}\n> >> +\t\tASSERT(found);\n> >> +\t}\n> >> }\n> >> \n> >> VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> >> @@ -291,11 +323,28 @@ int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> >> \tfor (auto &s : data->streamConfigs_)\n> >> \t\ts.seq = 0;\n> >> \n> >> +\tdata->bufferCompleted.connect(this, [&](Request *request, FrameBuffer *buffer) {\n> >> +\t\tif (completeBuffer(request, buffer))\n> >> +\t\t\tcompleteRequest(request);\n> >> +\t});\n> > \n> > I'm not a big fan of lambdas in such cases, I find they hinder\n> > readability compared to member functions. The the PipelineHandlerVirtual\n> > class is not exposed outside of the pipeline handler, adding a private\n> > member function shouldn't be a big issue.\n> \n> Interesting, I have the exact opposite view. I find that member functions\n> usually involve more work and they don't allow for the same level of \"scoping\"\n> as lambdas.\n\nFor me it's on a case-by-case basis. I really like lambda as adapters\nbetween two APIs. That's particularly useful when there's a small\nimpedance mismatch between signals and slots. On the other hand, when\nthe slot needs to implement a more complex logic that belongs to the\nreceiver of the signal, I think member functions are better.\n\n> >> +\n> >> +\tdata->start();\n> >> +\n> >> \treturn 0;\n> >> }\n> >> \n> >> -void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> >> +void PipelineHandlerVirtual::stopDevice(Camera *camera)\n> >> {\n> >> +\tVirtualCameraData *data = cameraData(camera);\n> >> +\n> >> +\t/* Flush all work. */\n> >> +\tdata->invokeMethod([] { }, ConnectionTypeBlocking);\n> > \n> > Does this mean the Thread class API is lacking ? In particular, I don't\n> > see a similar call in the SoftwareIsp::stop() function. Is it needed\n> > here because we rely solely on the queue of messages to keep track the\n> > requests ? If so, I think we should keep track of the queued requests in\n> > the VirtualCameraData class, and cancel all requests not processed after\n> \n> It's already tracked in `Camera::Private::queuedRequests_`, so that part seems easy.\n> \n> There is a need for flushing/cancelling asynchronous method invocations queued on\n> the worker thread(s), otherwise they will be executed when the thread is restarted\n> (or am I missing something that prevents this?). So we need a `cancelMessages()`\n> function in `Thread` then, it seems?\n\nWe have Thread::removeMessages() that could possibly be useful, but it\nhas to be called from the thread being stopped.\n\nWe use threads in two places: IPA modules, and software ISP. For IPA\nmodules I think we're safe as the stop function calls\n\n\tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n\nbefore stopping the thread. The software ISP, on the other hand, doesn't\nhave such a blocking call. I wonder if it could be affected by the\nproblem you describe. I remember we've fixed a similar issue in the\npast. Digging in the history, the following commit is related:\n\ncommit 86ffaf936dc663afd48bd3e276134fa720210bd6\nAuthor: Milan Zamazal \nDate: Tue Feb 25 16:06:11 2025 +0100\n\n libcamera: software_isp: Dispatch messages on stop\n\nThis is however not the same issue, that commit addresses messages sent\nby the thread, like you do below.\n\nSo yes, it looks like both the software ISP and the virtual pipeline\nhandler will need to use removeMessages(), or something similar. It's\ngetting a bit too late to think about how the right API should look\nlike.\n\n> > the thread exits. This will make stopping the camera faster, and will\n> > also mimick better the behaviour of hardware cameras.\n> > \n> > I know it's a bit more work, hopefully it's just freshening up the yak\n> > and not fully shaving it :-)\n> > \n> >> +\tdata->exit();\n> >> +\tdata->wait();\n> >> +\n> >> +\t/* Process queued `bufferCompleted` signal emissions. */\n> >> +\tThread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n> >> +\tdata->bufferCompleted.disconnect(this);\n> >> }\n> >> \n> >> int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> >> @@ -304,35 +353,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> >> \tVirtualCameraData *data = cameraData(camera);\n> >> \tconst auto timestamp = currentTimestamp();\n> >> \n> >> -\tfor (auto const &[stream, buffer] : request->buffers()) {\n> >> -\t\tbool found = false;\n> >> -\t\t/* map buffer and fill test patterns */\n> >> -\t\tfor (auto &streamConfig : data->streamConfigs_) {\n> >> -\t\t\tif (stream == &streamConfig.stream) {\n> >> -\t\t\t\tFrameMetadata &fmd = buffer->_d()->metadata();\n> >> -\n> >> -\t\t\t\tfmd.status = FrameMetadata::Status::FrameSuccess;\n> >> -\t\t\t\tfmd.sequence = streamConfig.seq++;\n> >> -\t\t\t\tfmd.timestamp = timestamp;\n> >> -\n> >> -\t\t\t\tfor (const auto [i, p] : utils::enumerate(buffer->planes()))\n> >> -\t\t\t\t\tfmd.planes()[i].bytesused = p.length;\n> >> -\n> >> -\t\t\t\tfound = true;\n> >> -\n> >> -\t\t\t\tif (streamConfig.frameGenerator->generateFrame(\n> >> -\t\t\t\t\t stream->configuration().size, buffer))\n> >> -\t\t\t\t\tfmd.status = FrameMetadata::Status::FrameError;\n> >> -\n> >> -\t\t\t\tcompleteBuffer(request, buffer);\n> >> -\t\t\t\tbreak;\n> >> -\t\t\t}\n> >> -\t\t}\n> >> -\t\tASSERT(found);\n> >> -\t}\n> >> -\n> >> \trequest->metadata().set(controls::SensorTimestamp, timestamp);\n> >> -\tcompleteRequest(request);\n> >> +\tdata->invokeMethod(&VirtualCameraData::queueRequest, ConnectionTypeQueued, request);\n> >> \n> >> \treturn 0;\n> >> }\n> >> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> >> index 683cb82b4..0832fd80c 100644\n> >> --- a/src/libcamera/pipeline/virtual/virtual.h\n> >> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> >> @@ -11,6 +11,7 @@\n> >> #include \n> >> #include \n> >> \n> >> +#include \n> > \n> > You should also include object.h.\n> > \n> >> #include \n> >> #include \n> >> \n> >> @@ -25,7 +26,9 @@ namespace libcamera {\n> >> \n> >> using VirtualFrame = std::variant;\n> >> \n> >> -class VirtualCameraData : public Camera::Private\n> >> +class VirtualCameraData : public Camera::Private,\n> >> +\t\t\t public Thread,\n> >> +\t\t\t public Object\n> >> {\n> >> public:\n> >> \tconst static unsigned int kMaxStream = 3;\n> >> @@ -54,9 +57,12 @@ public:\n> >> \n> >> \t~VirtualCameraData() = default;\n> >> \n> >> +\tvoid queueRequest(Request *request);\n> >> +\n> >> \tConfiguration config_;\n> >> \n> >> \tstd::vector streamConfigs_;\n> >> +\tSignal bufferCompleted;\n> >> };\n> >> \n> >> } /* namespace libcamera */","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 580FFBEFBE\n\tfor ;\n\tMon, 11 Aug 2025 22:47:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 20F7269241;\n\tTue, 12 Aug 2025 00:47:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8465361441\n\tfor ;\n\tTue, 12 Aug 2025 00:47:54 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id B7CB6E92;\n\tTue, 12 Aug 2025 00:47:01 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ugHpp8rT\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754952421;\n\tbh=xVW1ex7QvcuF33p3K+qDMY8NBaar+k5VFBR0oAO+nn4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ugHpp8rTwZspBqDRfRjJx70SPCcdSLwiFDpvy1gchbDpyK7e612040ItnMALG3Ym7\n\t8/hAYv4MI1DdQ4c00aayycQblBmaZWNNHC5Dr0P963cqsLEk47lTklywZzFunYhw0Q\n\tMdbc516Qbraq85JbjJb0GMe4NUv/ugQmPTU9hYlw=","Date":"Tue, 12 Aug 2025 01:47:35 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","Message-ID":"<20250811224735.GE30054@pendragon.ideasonboard.com>","References":"<20250811094926.1308259-1-barnabas.pocze@ideasonboard.com>\n\t<20250811094926.1308259-3-barnabas.pocze@ideasonboard.com>\n\t<20250811170532.GC30054@pendragon.ideasonboard.com>\n\t<1a21d4dc-6201-4b1f-8f3e-ce94ffb6f1a9@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<1a21d4dc-6201-4b1f-8f3e-ce94ffb6f1a9@ideasonboard.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":35353,"web_url":"https://patchwork.libcamera.org/comment/35353/","msgid":"","date":"2025-08-12T07:16:53","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"Hi\n\n2025. 08. 12. 0:47 keltezéssel, Laurent Pinchart írta:\n> Hi Barnabás,\n> \n> On Mon, Aug 11, 2025 at 07:51:21PM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 11. 19:05 keltezéssel, Laurent Pinchart írta:\n>>> On Mon, Aug 11, 2025 at 11:49:26AM +0200, Barnabás Pőcze wrote:\n>>>> Currently the virtual pipeline generates the images synchronously. This is not\n>>>> ideal because it blocks the camera manager's internal thread, and because its\n>>>> behaviour is different from other existing pipeline handlers, all of which\n>>>> complete requests asynchronously.\n>>>>\n>>>> So move the image generation to a separate thread by deriving `VirtualCameraData`\n>>>> from `Thread`, as well as `Object` and using the existing asynchronous signal\n>>>> and method call mechanism.\n>>>>\n>>>> Signed-off-by: Barnabás Pőcze \n>>>> ---\n>>>> src/libcamera/pipeline/virtual/virtual.cpp | 80 ++++++++++++++--------\n>>>> src/libcamera/pipeline/virtual/virtual.h | 8 ++-\n>>>> 2 files changed, 58 insertions(+), 30 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n>>>> index 049ebcba5..2ad32ec0a 100644\n>>>> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n>>>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n>>>> @@ -129,6 +129,38 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n>>>> \n>>>> \t/* \\todo Support multiple streams and pass multi_stream_test */\n>>>> \tstreamConfigs_.resize(kMaxStream);\n>>>> +\n>>>> +\tmoveToThread(this);\n>>>> +}\n>>>> +\n>>>> +void VirtualCameraData::queueRequest(Request *request)\n>>>> +{\n>>>> +\tfor (auto const &[stream, buffer] : request->buffers()) {\n>>>> +\t\tbool found = false;\n>>>> +\t\t/* map buffer and fill test patterns */\n>>>> +\t\tfor (auto &streamConfig : streamConfigs_) {\n>>>> +\t\t\tif (stream == &streamConfig.stream) {\n>>>> +\t\t\t\tFrameMetadata &fmd = buffer->_d()->metadata();\n>>>> +\n>>>> +\t\t\t\tfmd.status = FrameMetadata::Status::FrameSuccess;\n>>>> +\t\t\t\tfmd.sequence = streamConfig.seq++;\n>>>> +\t\t\t\tfmd.timestamp = currentTimestamp();\n>>>> +\n>>>> +\t\t\t\tfor (const auto [i, p] : utils::enumerate(buffer->planes()))\n>>>> +\t\t\t\t\tfmd.planes()[i].bytesused = p.length;\n>>>> +\n>>>> +\t\t\t\tfound = true;\n>>>> +\n>>>> +\t\t\t\tif (streamConfig.frameGenerator->generateFrame(\n>>>> +\t\t\t\t\t stream->configuration().size, buffer))\n>>>> +\t\t\t\t\tfmd.status = FrameMetadata::Status::FrameError;\n>>>> +\n>>>> +\t\t\t\tbufferCompleted.emit(request, buffer);\n>>>> +\t\t\t\tbreak;\n>>>> +\t\t\t}\n>>>> +\t\t}\n>>>> +\t\tASSERT(found);\n>>>> +\t}\n>>>> }\n>>>> \n>>>> VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n>>>> @@ -291,11 +323,28 @@ int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n>>>> \tfor (auto &s : data->streamConfigs_)\n>>>> \t\ts.seq = 0;\n>>>> \n>>>> +\tdata->bufferCompleted.connect(this, [&](Request *request, FrameBuffer *buffer) {\n>>>> +\t\tif (completeBuffer(request, buffer))\n>>>> +\t\t\tcompleteRequest(request);\n>>>> +\t});\n>>>\n>>> I'm not a big fan of lambdas in such cases, I find they hinder\n>>> readability compared to member functions. The the PipelineHandlerVirtual\n>>> class is not exposed outside of the pipeline handler, adding a private\n>>> member function shouldn't be a big issue.\n>>\n>> Interesting, I have the exact opposite view. I find that member functions\n>> usually involve more work and they don't allow for the same level of \"scoping\"\n>> as lambdas.\n> \n> For me it's on a case-by-case basis. I really like lambda as adapters\n> between two APIs. That's particularly useful when there's a small\n> impedance mismatch between signals and slots. On the other hand, when\n> the slot needs to implement a more complex logic that belongs to the\n> receiver of the signal, I think member functions are better.\n> \n>>>> +\n>>>> +\tdata->start();\n>>>> +\n>>>> \treturn 0;\n>>>> }\n>>>> \n>>>> -void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n>>>> +void PipelineHandlerVirtual::stopDevice(Camera *camera)\n>>>> {\n>>>> +\tVirtualCameraData *data = cameraData(camera);\n>>>> +\n>>>> +\t/* Flush all work. */\n>>>> +\tdata->invokeMethod([] { }, ConnectionTypeBlocking);\n>>>\n>>> Does this mean the Thread class API is lacking ? In particular, I don't\n>>> see a similar call in the SoftwareIsp::stop() function. Is it needed\n>>> here because we rely solely on the queue of messages to keep track the\n>>> requests ? If so, I think we should keep track of the queued requests in\n>>> the VirtualCameraData class, and cancel all requests not processed after\n>>\n>> It's already tracked in `Camera::Private::queuedRequests_`, so that part seems easy.\n>>\n>> There is a need for flushing/cancelling asynchronous method invocations queued on\n>> the worker thread(s), otherwise they will be executed when the thread is restarted\n>> (or am I missing something that prevents this?). So we need a `cancelMessages()`\n>> function in `Thread` then, it seems?\n> \n> We have Thread::removeMessages() that could possibly be useful, but it\n> has to be called from the thread being stopped.\n\nLooking at the implementation I think I would have assumed that it can be\ncalled from a different thread since it uses locks and everything.\nI suppose the current issue is that it is `private`.\n\n\n> \n> We use threads in two places: IPA modules, and software ISP. For IPA\n> modules I think we're safe as the stop function calls\n> \n> \tproxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> \n> before stopping the thread. The software ISP, on the other hand, doesn't\n\nYes, that is equivalent to flushing all the invoke messages, which is what\nthis version proposes, but \"stop()\" here is a no-op, hence the empty lambda.\n\n\n> have such a blocking call. I wonder if it could be affected by the\n> problem you describe. I remember we've fixed a similar issue in the\n> past. Digging in the history, the following commit is related:\n\nI briefly looked at it yesterday, and my assumption is yes. I think the `SoftwareIsp`\nclass exhibits the same pattern as the virtual pipeline handler. If we consider\ne.g. `SoftwareIsp::process()`, there is an async call:\n\n debayer_->invokeMethod(&DebayerCpu::process,\n ConnectionTypeQueued, frame, input, output, debayerParams_);\n\nbut then `SoftwareIsp::stop()`:\n\n \tispWorkerThread_.exit();\n\tispWorkerThread_.wait();\n\n\tThread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n\n\tipa_->stop();\n\n\tfor (auto buffer : queuedOutputBuffers_) {\n\t\tFrameMetadata &metadata = buffer->_d()->metadata();\n\t\tmetadata.status = FrameMetadata::FrameCancelled;\n\t\toutputBufferReady.emit(buffer);\n\t}\n\tqueuedOutputBuffers_.clear();\n\n\tfor (auto buffer : queuedInputBuffers_) {\n\t\tFrameMetadata &metadata = buffer->_d()->metadata();\n\t\tmetadata.status = FrameMetadata::FrameCancelled;\n\t\tinputBufferReady.emit(buffer);\n\t}\n\tqueuedInputBuffers_.clear();\n\ndoes not seem to cancel/flush the invoke messages on `ispWorkerThread_`, to which\nthe `*debayer_` object is bound. So it seems to me that such messages might\nremain in the queue of `ispWorkerThread_`. But I will check this later.\n\n\n> \n> commit 86ffaf936dc663afd48bd3e276134fa720210bd6\n> Author: Milan Zamazal \n> Date: Tue Feb 25 16:06:11 2025 +0100\n> \n> libcamera: software_isp: Dispatch messages on stop\n> \n> This is however not the same issue, that commit addresses messages sent\n> by the thread, like you do below.\n> \n> So yes, it looks like both the software ISP and the virtual pipeline\n> handler will need to use removeMessages(), or something similar. It's\n> getting a bit too late to think about how the right API should look\n> like.\n\nMy first idea was to have `cancelMessages(Message::Type, Object * = nullptr)`\nsimilarly to `dispatchMessages()`.\n\n\n\n> \n>>> the thread exits. This will make stopping the camera faster, and will\n>>> also mimick better the behaviour of hardware cameras.\n>>>\n>>> I know it's a bit more work, hopefully it's just freshening up the yak\n>>> and not fully shaving it :-)\n>>>\n>>>> +\tdata->exit();\n>>>> +\tdata->wait();\n>>>> +\n>>>> +\t/* Process queued `bufferCompleted` signal emissions. */\n>>>> +\tThread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n>>>> +\tdata->bufferCompleted.disconnect(this);\n>>>> }\n>>>> \n>>>> int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n>>>> @@ -304,35 +353,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n>>>> \tVirtualCameraData *data = cameraData(camera);\n>>>> \tconst auto timestamp = currentTimestamp();\n>>>> \n>>>> -\tfor (auto const &[stream, buffer] : request->buffers()) {\n>>>> -\t\tbool found = false;\n>>>> -\t\t/* map buffer and fill test patterns */\n>>>> -\t\tfor (auto &streamConfig : data->streamConfigs_) {\n>>>> -\t\t\tif (stream == &streamConfig.stream) {\n>>>> -\t\t\t\tFrameMetadata &fmd = buffer->_d()->metadata();\n>>>> -\n>>>> -\t\t\t\tfmd.status = FrameMetadata::Status::FrameSuccess;\n>>>> -\t\t\t\tfmd.sequence = streamConfig.seq++;\n>>>> -\t\t\t\tfmd.timestamp = timestamp;\n>>>> -\n>>>> -\t\t\t\tfor (const auto [i, p] : utils::enumerate(buffer->planes()))\n>>>> -\t\t\t\t\tfmd.planes()[i].bytesused = p.length;\n>>>> -\n>>>> -\t\t\t\tfound = true;\n>>>> -\n>>>> -\t\t\t\tif (streamConfig.frameGenerator->generateFrame(\n>>>> -\t\t\t\t\t stream->configuration().size, buffer))\n>>>> -\t\t\t\t\tfmd.status = FrameMetadata::Status::FrameError;\n>>>> -\n>>>> -\t\t\t\tcompleteBuffer(request, buffer);\n>>>> -\t\t\t\tbreak;\n>>>> -\t\t\t}\n>>>> -\t\t}\n>>>> -\t\tASSERT(found);\n>>>> -\t}\n>>>> -\n>>>> \trequest->metadata().set(controls::SensorTimestamp, timestamp);\n>>>> -\tcompleteRequest(request);\n>>>> +\tdata->invokeMethod(&VirtualCameraData::queueRequest, ConnectionTypeQueued, request);\n>>>> \n>>>> \treturn 0;\n>>>> }\n>>>> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n>>>> index 683cb82b4..0832fd80c 100644\n>>>> --- a/src/libcamera/pipeline/virtual/virtual.h\n>>>> +++ b/src/libcamera/pipeline/virtual/virtual.h\n>>>> @@ -11,6 +11,7 @@\n>>>> #include \n>>>> #include \n>>>> \n>>>> +#include \n>>>\n>>> You should also include object.h.\n>>>\n>>>> #include \n>>>> #include \n>>>> \n>>>> @@ -25,7 +26,9 @@ namespace libcamera {\n>>>> \n>>>> using VirtualFrame = std::variant;\n>>>> \n>>>> -class VirtualCameraData : public Camera::Private\n>>>> +class VirtualCameraData : public Camera::Private,\n>>>> +\t\t\t public Thread,\n>>>> +\t\t\t public Object\n>>>> {\n>>>> public:\n>>>> \tconst static unsigned int kMaxStream = 3;\n>>>> @@ -54,9 +57,12 @@ public:\n>>>> \n>>>> \t~VirtualCameraData() = default;\n>>>> \n>>>> +\tvoid queueRequest(Request *request);\n>>>> +\n>>>> \tConfiguration config_;\n>>>> \n>>>> \tstd::vector streamConfigs_;\n>>>> +\tSignal bufferCompleted;\n>>>> };\n>>>> \n>>>> } /* namespace libcamera */\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 232CBBDCC1\n\tfor ;\n\tTue, 12 Aug 2025 07:17:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B2D1469240;\n\tTue, 12 Aug 2025 09:16:59 +0200 (CEST)","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 F401361462\n\tfor ;\n\tTue, 12 Aug 2025 09:16:56 +0200 (CEST)","from [192.168.33.15] (185.221.140.182.nat.pool.zt.hu\n\t[185.221.140.182])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 19D6182A;\n\tTue, 12 Aug 2025 09:16:04 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"LFbkfVNZ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1754982964;\n\tbh=+zTyfCY7Gbkr/3KtKzdAWhqyME4M8P67GLgzCYsYQlQ=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=LFbkfVNZzF6MJyDkN5+/4IY329Hxs99i1P9i2hH5NTniIq2zPaNfwhDBcxCubZCNG\n\tDQv5VrW9SX48s9qVf776ZNUHgfnraQ6jluPK3fIeJ8ox/fvWjPxPz+om3pc7qC8Xa9\n\trWu2WrJZAUmeKiQp0eKZFFk/iqewQ3FiGe5mWAmc=","Message-ID":"","Date":"Tue, 12 Aug 2025 09:16:53 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250811094926.1308259-1-barnabas.pocze@ideasonboard.com>\n\t<20250811094926.1308259-3-barnabas.pocze@ideasonboard.com>\n\t<20250811170532.GC30054@pendragon.ideasonboard.com>\n\t<1a21d4dc-6201-4b1f-8f3e-ce94ffb6f1a9@ideasonboard.com>\n\t<20250811224735.GE30054@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250811224735.GE30054@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":35357,"web_url":"https://patchwork.libcamera.org/comment/35357/","msgid":"<87746277-5270-48a2-bf17-7ef5653a3050@ideasonboard.com>","date":"2025-08-12T13:17:43","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 12. 9:16 keltezéssel, Barnabás Pőcze írta:\n> Hi\n> \n> 2025. 08. 12. 0:47 keltezéssel, Laurent Pinchart írta:\n>> Hi Barnabás,\n>>\n>> On Mon, Aug 11, 2025 at 07:51:21PM +0200, Barnabás Pőcze wrote:\n>>> 2025. 08. 11. 19:05 keltezéssel, Laurent Pinchart írta:\n>>>> On Mon, Aug 11, 2025 at 11:49:26AM +0200, Barnabás Pőcze wrote:\n>>>>> Currently the virtual pipeline generates the images synchronously. This is not\n>>>>> ideal because it blocks the camera manager's internal thread, and because its\n>>>>> behaviour is different from other existing pipeline handlers, all of which\n>>>>> complete requests asynchronously.\n>>>>>\n>>>>> So move the image generation to a separate thread by deriving `VirtualCameraData`\n>>>>> from `Thread`, as well as `Object` and using the existing asynchronous signal\n>>>>> and method call mechanism.\n>>>>>\n>>>>> Signed-off-by: Barnabás Pőcze \n>>>>> ---\n>>>>>    src/libcamera/pipeline/virtual/virtual.cpp | 80 ++++++++++++++--------\n>>>>>    src/libcamera/pipeline/virtual/virtual.h   |  8 ++-\n>>>>>    2 files changed, 58 insertions(+), 30 deletions(-)\n>>>>>\n>>>>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n>>>>> index 049ebcba5..2ad32ec0a 100644\n>>>>> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n>>>>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n>>>>> @@ -129,6 +129,38 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n>>>>>        /* \\todo Support multiple streams and pass multi_stream_test */\n>>>>>        streamConfigs_.resize(kMaxStream);\n>>>>> +\n>>>>> +    moveToThread(this);\n>>>>> +}\n>>>>> +\n>>>>> +void VirtualCameraData::queueRequest(Request *request)\n>>>>> +{\n>>>>> +    for (auto const &[stream, buffer] : request->buffers()) {\n>>>>> +        bool found = false;\n>>>>> +        /* map buffer and fill test patterns */\n>>>>> +        for (auto &streamConfig : streamConfigs_) {\n>>>>> +            if (stream == &streamConfig.stream) {\n>>>>> +                FrameMetadata &fmd = buffer->_d()->metadata();\n>>>>> +\n>>>>> +                fmd.status = FrameMetadata::Status::FrameSuccess;\n>>>>> +                fmd.sequence = streamConfig.seq++;\n>>>>> +                fmd.timestamp = currentTimestamp();\n>>>>> +\n>>>>> +                for (const auto [i, p] : utils::enumerate(buffer->planes()))\n>>>>> +                    fmd.planes()[i].bytesused = p.length;\n>>>>> +\n>>>>> +                found = true;\n>>>>> +\n>>>>> +                if (streamConfig.frameGenerator->generateFrame(\n>>>>> +                        stream->configuration().size, buffer))\n>>>>> +                    fmd.status = FrameMetadata::Status::FrameError;\n>>>>> +\n>>>>> +                bufferCompleted.emit(request, buffer);\n>>>>> +                break;\n>>>>> +            }\n>>>>> +        }\n>>>>> +        ASSERT(found);\n>>>>> +    }\n>>>>>    }\n>>>>>    VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n>>>>> @@ -291,11 +323,28 @@ int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n>>>>>        for (auto &s : data->streamConfigs_)\n>>>>>            s.seq = 0;\n>>>>> +    data->bufferCompleted.connect(this, [&](Request *request, FrameBuffer *buffer) {\n>>>>> +        if (completeBuffer(request, buffer))\n>>>>> +            completeRequest(request);\n>>>>> +    });\n>>>>\n>>>> I'm not a big fan of lambdas in such cases, I find they hinder\n>>>> readability compared to member functions. The the PipelineHandlerVirtual\n>>>> class is not exposed outside of the pipeline handler, adding a private\n>>>> member function shouldn't be a big issue.\n>>>\n>>> Interesting, I have the exact opposite view. I find that member functions\n>>> usually involve more work and they don't allow for the same level of \"scoping\"\n>>> as lambdas.\n>>\n>> For me it's on a case-by-case basis. I really like lambda as adapters\n>> between two APIs. That's particularly useful when there's a small\n>> impedance mismatch between signals and slots. On the other hand, when\n>> the slot needs to implement a more complex logic that belongs to the\n>> receiver of the signal, I think member functions are better.\n>>\n>>>>> +\n>>>>> +    data->start();\n>>>>> +\n>>>>>        return 0;\n>>>>>    }\n>>>>> -void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n>>>>> +void PipelineHandlerVirtual::stopDevice(Camera *camera)\n>>>>>    {\n>>>>> +    VirtualCameraData *data = cameraData(camera);\n>>>>> +\n>>>>> +    /* Flush all work. */\n>>>>> +    data->invokeMethod([] { }, ConnectionTypeBlocking);\n>>>>\n>>>> Does this mean the Thread class API is lacking ? In particular, I don't\n>>>> see a similar call in the SoftwareIsp::stop() function. Is it needed\n>>>> here because we rely solely on the queue of messages to keep track the\n>>>> requests ? If so, I think we should keep track of the queued requests in\n>>>> the VirtualCameraData class, and cancel all requests not processed after\n>>>\n>>> It's already tracked in `Camera::Private::queuedRequests_`, so that part seems easy.\n>>>\n>>> There is a need for flushing/cancelling asynchronous method invocations queued on\n>>> the worker thread(s), otherwise they will be executed when the thread is restarted\n>>> (or am I missing something that prevents this?). So we need a `cancelMessages()`\n>>> function in `Thread` then, it seems?\n>>\n>> We have Thread::removeMessages() that could possibly be useful, but it\n>> has to be called from the thread being stopped.\n> \n> Looking at the implementation I think I would have assumed that it can be\n> called from a different thread since it uses locks and everything.\n> I suppose the current issue is that it is `private`.\n> \n> \n>>\n>> We use threads in two places: IPA modules, and software ISP. For IPA\n>> modules I think we're safe as the stop function calls\n>>\n>>     proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n>>\n>> before stopping the thread. The software ISP, on the other hand, doesn't\n> \n> Yes, that is equivalent to flushing all the invoke messages, which is what\n> this version proposes, but \"stop()\" here is a no-op, hence the empty lambda.\n> \n> \n>> have such a blocking call. I wonder if it could be affected by the\n>> problem you describe. I remember we've fixed a similar issue in the\n>> past. Digging in the history, the following commit is related:\n> \n> I briefly looked at it yesterday, and my assumption is yes. I think the `SoftwareIsp`\n> class exhibits the same pattern as the virtual pipeline handler. If we consider\n> e.g. `SoftwareIsp::process()`, there is an async call:\n> \n>   debayer_->invokeMethod(&DebayerCpu::process,\n>                          ConnectionTypeQueued, frame, input, output, debayerParams_);\n> \n> but then `SoftwareIsp::stop()`:\n> \n>       ispWorkerThread_.exit();\n>     ispWorkerThread_.wait();\n> \n>     Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n> \n>     ipa_->stop();\n> \n>     for (auto buffer : queuedOutputBuffers_) {\n>         FrameMetadata &metadata = buffer->_d()->metadata();\n>         metadata.status = FrameMetadata::FrameCancelled;\n>         outputBufferReady.emit(buffer);\n>     }\n>     queuedOutputBuffers_.clear();\n> \n>     for (auto buffer : queuedInputBuffers_) {\n>         FrameMetadata &metadata = buffer->_d()->metadata();\n>         metadata.status = FrameMetadata::FrameCancelled;\n>         inputBufferReady.emit(buffer);\n>     }\n>     queuedInputBuffers_.clear();\n> \n> does not seem to cancel/flush the invoke messages on `ispWorkerThread_`, to which\n> the `*debayer_` object is bound. So it seems to me that such messages might\n> remain in the queue of `ispWorkerThread_`. But I will check this later.\n\nUnfortunately it took more time than expected, but with judicious use of `scheduler-locking`\nand breakpoints, I was able to trigger exactly that scenario. Some invoke messages\nremain in the thread's message queue after `SoftwareIsp::stop()` returns. The window\nof opportunity seems quite small, but definitely possible.\n\nIt seems to me that the shutdown procedure is not robust enough. For example, in this\nstate there is an ubsan warning:\n\n ../src/libcamera/request.cpp:107:25: runtime error: load of value 3200171710, which is not a valid value for type 'Status'\n\nwhen it completes buffers in `SimpleCameraData::imageBufferReady()`:\n\n const RequestOutputs &outputs = conversionQueue_.front();\n for (auto &[stream, buf] : outputs.outputs)\n pipe->completeBuffer(outputs.request, buf);\n\nas the buffer metadata remains uninitialized:\n\n(gdb) p buffer->metadata()\n$32 = (const libcamera::FrameMetadata &) @0x7cefedc0ff68: {\n status = 3200171710,\n sequence = 3200171710,\n timestamp = 13744632839234567870,\n planes_ = std::__debug::vector of length 1, capacity 1 = {{\n bytesused = 0\n }}\n}\n\nand it also does not complete all requests:\n\n387\t\tASSERT(data->queuedRequests_.empty());\n(gdb) p data->queuedRequests_\n$35 = std::__debug::list = {\n [0] = 0x7cafedbe0e00,\n [1] = 0x7cafedbe0f60,\n [2] = 0x7cafedbe10c0,\n [3] = 0x7cafedbe1220\n}\n(gdb) n\n[4:37:02.698049276] [7158] FATAL default pipeline_handler.cpp:387 assertion \"data->queuedRequests_.empty()\" failed in stop()\n\n\nAfter the assertion reaches __pthread_kill_implementation():\n\n\n(gdb) p ((SimpleCameraData *)data)->swIsp_->ispWorkerThread_->data_->messages_.list_\n$40 = std::__debug::list = {\n [0] = std::unique_ptr = {\n get() = 0x7c5fedc06540\n },\n [1] = std::unique_ptr = {\n get() = 0x7c5fedc06600\n },\n [2] = std::unique_ptr = {\n get() = 0x7c5fedc066c0\n }\n}\n(gdb) p *((libcamera::Message*)0x7c5fedc06540)\n$46 = {\n _vptr.Message = 0x7ffff06f8500 ,\n type_ = libcamera::Message::InvokeMessage,\n receiver_ = 0x7e2fedbe1d68,\n static nextUserType_ = std::atomic = { 1000 }\n}\n(gdb) p *((libcamera::Message*)0x7c5fedc06600)\n$47 = {\n _vptr.Message = 0x7ffff06f8500 ,\n type_ = libcamera::Message::InvokeMessage,\n receiver_ = 0x7e2fedbe1d68,\n static nextUserType_ = std::atomic = { 1000 }\n}\n(gdb) p *((libcamera::Message*)0x7c5fedc066c0)\n$48 = {\n _vptr.Message = 0x7ffff06f8500 ,\n type_ = libcamera::Message::InvokeMessage,\n receiver_ = 0x7e2fedbe1d68,\n static nextUserType_ = std::atomic = { 1000 }\n}\n\nNote that the `receiver_` is the same:\n\n(gdb) p *((libcamera::Message*)0x7c5fedc066c0)->receiver_\n$49 = {\n _vptr.Object = 0x7ffff4bdb3f8 ,\n parent_ = 0x0,\n children_ = std::__debug::vector of length 0, capacity 0,\n thread_ = 0x7e2fedbe0280,\n signals_ = empty std::__debug::list,\n pendingMessages_ = 3\n}\n\n\nSo should that camera ever be restarted, those messages would be dispatched,\nreusing old requests and arguments.\n\nI think there are multiple issues here: first, the one we have suspected, that\nthe `ispWorkerThread_` is not appropriately flushed/cancelled; and second, the\nshutdown procedure does not seem robust enough to me if it runs into an assertion\nin this state.\n\n\n> \n> \n>>\n>> commit 86ffaf936dc663afd48bd3e276134fa720210bd6\n>> Author: Milan Zamazal \n>> Date:   Tue Feb 25 16:06:11 2025 +0100\n>>\n>>      libcamera: software_isp: Dispatch messages on stop\n>>\n>> This is however not the same issue, that commit addresses messages sent\n>> by the thread, like you do below.\n>>\n>> So yes, it looks like both the software ISP and the virtual pipeline\n>> handler will need to use removeMessages(), or something similar. It's\n>> getting a bit too late to think about how the right API should look\n>> like.\n> \n> My first idea was to have `cancelMessages(Message::Type, Object * = nullptr)`\n> similarly to `dispatchMessages()`.\n> \n> \n> \n>>\n>>>> the thread exits. This will make stopping the camera faster, and will\n>>>> also mimick better the behaviour of hardware cameras.\n>>>>\n>>>> I know it's a bit more work, hopefully it's just freshening up the yak\n>>>> and not fully shaving it :-)\n>>>>\n>>>>> +    data->exit();\n>>>>> +    data->wait();\n>>>>> +\n>>>>> +    /* Process queued `bufferCompleted` signal emissions. */\n>>>>> +    Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n>>>>> +    data->bufferCompleted.disconnect(this);\n>>>>>    }\n>>>>>    int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n>>>>> @@ -304,35 +353,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n>>>>>        VirtualCameraData *data = cameraData(camera);\n>>>>>        const auto timestamp = currentTimestamp();\n>>>>> -    for (auto const &[stream, buffer] : request->buffers()) {\n>>>>> -        bool found = false;\n>>>>> -        /* map buffer and fill test patterns */\n>>>>> -        for (auto &streamConfig : data->streamConfigs_) {\n>>>>> -            if (stream == &streamConfig.stream) {\n>>>>> -                FrameMetadata &fmd = buffer->_d()->metadata();\n>>>>> -\n>>>>> -                fmd.status = FrameMetadata::Status::FrameSuccess;\n>>>>> -                fmd.sequence = streamConfig.seq++;\n>>>>> -                fmd.timestamp = timestamp;\n>>>>> -\n>>>>> -                for (const auto [i, p] : utils::enumerate(buffer->planes()))\n>>>>> -                    fmd.planes()[i].bytesused = p.length;\n>>>>> -\n>>>>> -                found = true;\n>>>>> -\n>>>>> -                if (streamConfig.frameGenerator->generateFrame(\n>>>>> -                        stream->configuration().size, buffer))\n>>>>> -                    fmd.status = FrameMetadata::Status::FrameError;\n>>>>> -\n>>>>> -                completeBuffer(request, buffer);\n>>>>> -                break;\n>>>>> -            }\n>>>>> -        }\n>>>>> -        ASSERT(found);\n>>>>> -    }\n>>>>> -\n>>>>>        request->metadata().set(controls::SensorTimestamp, timestamp);\n>>>>> -    completeRequest(request);\n>>>>> +    data->invokeMethod(&VirtualCameraData::queueRequest, ConnectionTypeQueued, request);\n>>>>>        return 0;\n>>>>>    }\n>>>>> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n>>>>> index 683cb82b4..0832fd80c 100644\n>>>>> --- a/src/libcamera/pipeline/virtual/virtual.h\n>>>>> +++ b/src/libcamera/pipeline/virtual/virtual.h\n>>>>> @@ -11,6 +11,7 @@\n>>>>>    #include \n>>>>>    #include \n>>>>> +#include \n>>>>\n>>>> You should also include object.h.\n>>>>\n>>>>>    #include \n>>>>>    #include \n>>>>> @@ -25,7 +26,9 @@ namespace libcamera {\n>>>>>    using VirtualFrame = std::variant;\n>>>>> -class VirtualCameraData : public Camera::Private\n>>>>> +class VirtualCameraData : public Camera::Private,\n>>>>> +              public Thread,\n>>>>> +              public Object\n>>>>>    {\n>>>>>    public:\n>>>>>        const static unsigned int kMaxStream = 3;\n>>>>> @@ -54,9 +57,12 @@ public:\n>>>>>        ~VirtualCameraData() = default;\n>>>>> +    void queueRequest(Request *request);\n>>>>> +\n>>>>>        Configuration config_;\n>>>>>        std::vector streamConfigs_;\n>>>>> +    Signal bufferCompleted;\n>>>>>    };\n>>>>>    } /* namespace libcamera */\n>>\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 1BF44BEFBE\n\tfor ;\n\tTue, 12 Aug 2025 13:17:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B644B6921A;\n\tTue, 12 Aug 2025 15:17:50 +0200 (CEST)","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 3712761444\n\tfor ;\n\tTue, 12 Aug 2025 15:17:48 +0200 (CEST)","from [192.168.33.15] (185.221.140.182.nat.pool.zt.hu\n\t[185.221.140.182])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1D8483DC;\n\tTue, 12 Aug 2025 15:16:54 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"uYVkM63M\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755004614;\n\tbh=H1TC9MWR+yl2y5GzjIhnzjOiCXXxDleG9M+kt4PlR4s=;\n\th=Date:Subject:From:To:Cc:References:In-Reply-To:From;\n\tb=uYVkM63M4ChY4YG4qeUPK/SAa3NZwfofM4Qvn3GOe7xE4m3ZB0ERR6khr/RWQFUPR\n\tEvVnAXPaf6YijFNhbrzwPwEVV5p2fErOzx+dtM01DsZq93s3G3Ad2RzVPHI6lXPYyn\n\twhrFqu4R30xq471WTBAuwRakFF0AG8+tQsto1z1U=","Message-ID":"<87746277-5270-48a2-bf17-7ef5653a3050@ideasonboard.com>","Date":"Tue, 12 Aug 2025 15:17:43 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250811094926.1308259-1-barnabas.pocze@ideasonboard.com>\n\t<20250811094926.1308259-3-barnabas.pocze@ideasonboard.com>\n\t<20250811170532.GC30054@pendragon.ideasonboard.com>\n\t<1a21d4dc-6201-4b1f-8f3e-ce94ffb6f1a9@ideasonboard.com>\n\t<20250811224735.GE30054@pendragon.ideasonboard.com>\n\t","Content-Language":"en-US, hu-HU","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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":35358,"web_url":"https://patchwork.libcamera.org/comment/35358/","msgid":"<20250812145549.GO30054@pendragon.ideasonboard.com>","date":"2025-08-12T14:55:49","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Aug 12, 2025 at 03:17:43PM +0200, Barnabás Pőcze wrote:\n> 2025. 08. 12. 9:16 keltezéssel, Barnabás Pőcze írta:\n> > 2025. 08. 12. 0:47 keltezéssel, Laurent Pinchart írta:\n> >> On Mon, Aug 11, 2025 at 07:51:21PM +0200, Barnabás Pőcze wrote:\n> >>> 2025. 08. 11. 19:05 keltezéssel, Laurent Pinchart írta:\n> >>>> On Mon, Aug 11, 2025 at 11:49:26AM +0200, Barnabás Pőcze wrote:\n> >>>>> Currently the virtual pipeline generates the images synchronously. This is not\n> >>>>> ideal because it blocks the camera manager's internal thread, and because its\n> >>>>> behaviour is different from other existing pipeline handlers, all of which\n> >>>>> complete requests asynchronously.\n> >>>>>\n> >>>>> So move the image generation to a separate thread by deriving `VirtualCameraData`\n> >>>>> from `Thread`, as well as `Object` and using the existing asynchronous signal\n> >>>>> and method call mechanism.\n> >>>>>\n> >>>>> Signed-off-by: Barnabás Pőcze \n> >>>>> ---\n> >>>>>    src/libcamera/pipeline/virtual/virtual.cpp | 80 ++++++++++++++--------\n> >>>>>    src/libcamera/pipeline/virtual/virtual.h   |  8 ++-\n> >>>>>    2 files changed, 58 insertions(+), 30 deletions(-)\n> >>>>>\n> >>>>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n> >>>>> index 049ebcba5..2ad32ec0a 100644\n> >>>>> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n> >>>>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n> >>>>> @@ -129,6 +129,38 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n> >>>>>        /* \\todo Support multiple streams and pass multi_stream_test */\n> >>>>>        streamConfigs_.resize(kMaxStream);\n> >>>>> +\n> >>>>> +    moveToThread(this);\n> >>>>> +}\n> >>>>> +\n> >>>>> +void VirtualCameraData::queueRequest(Request *request)\n> >>>>> +{\n> >>>>> +    for (auto const &[stream, buffer] : request->buffers()) {\n> >>>>> +        bool found = false;\n> >>>>> +        /* map buffer and fill test patterns */\n> >>>>> +        for (auto &streamConfig : streamConfigs_) {\n> >>>>> +            if (stream == &streamConfig.stream) {\n> >>>>> +                FrameMetadata &fmd = buffer->_d()->metadata();\n> >>>>> +\n> >>>>> +                fmd.status = FrameMetadata::Status::FrameSuccess;\n> >>>>> +                fmd.sequence = streamConfig.seq++;\n> >>>>> +                fmd.timestamp = currentTimestamp();\n> >>>>> +\n> >>>>> +                for (const auto [i, p] : utils::enumerate(buffer->planes()))\n> >>>>> +                    fmd.planes()[i].bytesused = p.length;\n> >>>>> +\n> >>>>> +                found = true;\n> >>>>> +\n> >>>>> +                if (streamConfig.frameGenerator->generateFrame(\n> >>>>> +                        stream->configuration().size, buffer))\n> >>>>> +                    fmd.status = FrameMetadata::Status::FrameError;\n> >>>>> +\n> >>>>> +                bufferCompleted.emit(request, buffer);\n> >>>>> +                break;\n> >>>>> +            }\n> >>>>> +        }\n> >>>>> +        ASSERT(found);\n> >>>>> +    }\n> >>>>>    }\n> >>>>>    VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n> >>>>> @@ -291,11 +323,28 @@ int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n> >>>>>        for (auto &s : data->streamConfigs_)\n> >>>>>            s.seq = 0;\n> >>>>> +    data->bufferCompleted.connect(this, [&](Request *request, FrameBuffer *buffer) {\n> >>>>> +        if (completeBuffer(request, buffer))\n> >>>>> +            completeRequest(request);\n> >>>>> +    });\n> >>>>\n> >>>> I'm not a big fan of lambdas in such cases, I find they hinder\n> >>>> readability compared to member functions. The the PipelineHandlerVirtual\n> >>>> class is not exposed outside of the pipeline handler, adding a private\n> >>>> member function shouldn't be a big issue.\n> >>>\n> >>> Interesting, I have the exact opposite view. I find that member functions\n> >>> usually involve more work and they don't allow for the same level of \"scoping\"\n> >>> as lambdas.\n> >>\n> >> For me it's on a case-by-case basis. I really like lambda as adapters\n> >> between two APIs. That's particularly useful when there's a small\n> >> impedance mismatch between signals and slots. On the other hand, when\n> >> the slot needs to implement a more complex logic that belongs to the\n> >> receiver of the signal, I think member functions are better.\n> >>\n> >>>>> +\n> >>>>> +    data->start();\n> >>>>> +\n> >>>>>        return 0;\n> >>>>>    }\n> >>>>> -void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n> >>>>> +void PipelineHandlerVirtual::stopDevice(Camera *camera)\n> >>>>>    {\n> >>>>> +    VirtualCameraData *data = cameraData(camera);\n> >>>>> +\n> >>>>> +    /* Flush all work. */\n> >>>>> +    data->invokeMethod([] { }, ConnectionTypeBlocking);\n> >>>>\n> >>>> Does this mean the Thread class API is lacking ? In particular, I don't\n> >>>> see a similar call in the SoftwareIsp::stop() function. Is it needed\n> >>>> here because we rely solely on the queue of messages to keep track the\n> >>>> requests ? If so, I think we should keep track of the queued requests in\n> >>>> the VirtualCameraData class, and cancel all requests not processed after\n> >>>\n> >>> It's already tracked in `Camera::Private::queuedRequests_`, so that part seems easy.\n> >>>\n> >>> There is a need for flushing/cancelling asynchronous method invocations queued on\n> >>> the worker thread(s), otherwise they will be executed when the thread is restarted\n> >>> (or am I missing something that prevents this?). So we need a `cancelMessages()`\n> >>> function in `Thread` then, it seems?\n> >>\n> >> We have Thread::removeMessages() that could possibly be useful, but it\n> >> has to be called from the thread being stopped.\n> > \n> > Looking at the implementation I think I would have assumed that it can be\n> > called from a different thread since it uses locks and everything.\n> > I suppose the current issue is that it is `private`.\n\nA quick look confirms that. The devil is of course in the details, but I\nthink you could try to just make that function public. Maybe we should\nalso add a message type argument.\n\nAnother option would be to automatically remove events when stopping the\nthread. Such a hardcoded policy may be too inflexible though.\n\n> >>\n> >> We use threads in two places: IPA modules, and software ISP. For IPA\n> >> modules I think we're safe as the stop function calls\n> >>\n> >>     proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n> >>\n> >> before stopping the thread. The software ISP, on the other hand, doesn't\n> > \n> > Yes, that is equivalent to flushing all the invoke messages, which is what\n> > this version proposes, but \"stop()\" here is a no-op, hence the empty lambda.\n> > \n> >> have such a blocking call. I wonder if it could be affected by the\n> >> problem you describe. I remember we've fixed a similar issue in the\n> >> past. Digging in the history, the following commit is related:\n> > \n> > I briefly looked at it yesterday, and my assumption is yes. I think the `SoftwareIsp`\n> > class exhibits the same pattern as the virtual pipeline handler. If we consider\n> > e.g. `SoftwareIsp::process()`, there is an async call:\n> > \n> >   debayer_->invokeMethod(&DebayerCpu::process,\n> >                          ConnectionTypeQueued, frame, input, output, debayerParams_);\n> > \n> > but then `SoftwareIsp::stop()`:\n> > \n> >       ispWorkerThread_.exit();\n> >     ispWorkerThread_.wait();\n> > \n> >     Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n> > \n> >     ipa_->stop();\n> > \n> >     for (auto buffer : queuedOutputBuffers_) {\n> >         FrameMetadata &metadata = buffer->_d()->metadata();\n> >         metadata.status = FrameMetadata::FrameCancelled;\n> >         outputBufferReady.emit(buffer);\n> >     }\n> >     queuedOutputBuffers_.clear();\n> > \n> >     for (auto buffer : queuedInputBuffers_) {\n> >         FrameMetadata &metadata = buffer->_d()->metadata();\n> >         metadata.status = FrameMetadata::FrameCancelled;\n> >         inputBufferReady.emit(buffer);\n> >     }\n> >     queuedInputBuffers_.clear();\n> > \n> > does not seem to cancel/flush the invoke messages on `ispWorkerThread_`, to which\n> > the `*debayer_` object is bound. So it seems to me that such messages might\n> > remain in the queue of `ispWorkerThread_`. But I will check this later.\n> \n> Unfortunately it took more time than expected, but with judicious use of `scheduler-locking`\n> and breakpoints, I was able to trigger exactly that scenario. Some invoke messages\n> remain in the thread's message queue after `SoftwareIsp::stop()` returns. The window\n> of opportunity seems quite small, but definitely possible.\n\nThank you for confirming it.\n\n> It seems to me that the shutdown procedure is not robust enough. For example, in this\n> state there is an ubsan warning:\n> \n> ../src/libcamera/request.cpp:107:25: runtime error: load of value 3200171710, which is not a valid value for type 'Status'\n> \n> when it completes buffers in `SimpleCameraData::imageBufferReady()`:\n> \n> const RequestOutputs &outputs = conversionQueue_.front();\n> for (auto &[stream, buf] : outputs.outputs)\n> pipe->completeBuffer(outputs.request, buf);\n> \n> as the buffer metadata remains uninitialized:\n> \n> (gdb) p buffer->metadata()\n> $32 = (const libcamera::FrameMetadata &) @0x7cefedc0ff68: {\n> status = 3200171710,\n> sequence = 3200171710,\n> timestamp = 13744632839234567870,\n> planes_ = std::__debug::vector of length 1, capacity 1 = {{\n> bytesused = 0\n> }}\n> }\n> \n> and it also does not complete all requests:\n> \n> 387\t\tASSERT(data->queuedRequests_.empty());\n> (gdb) p data->queuedRequests_\n> $35 = std::__debug::list = {\n> [0] = 0x7cafedbe0e00,\n> [1] = 0x7cafedbe0f60,\n> [2] = 0x7cafedbe10c0,\n> [3] = 0x7cafedbe1220\n> }\n> (gdb) n\n> [4:37:02.698049276] [7158] FATAL default pipeline_handler.cpp:387 assertion \"data->queuedRequests_.empty()\" failed in stop()\n> \n> \n> After the assertion reaches __pthread_kill_implementation():\n> \n> \n> (gdb) p ((SimpleCameraData *)data)->swIsp_->ispWorkerThread_->data_->messages_.list_\n> $40 = std::__debug::list = {\n> [0] = std::unique_ptr = {\n> get() = 0x7c5fedc06540\n> },\n> [1] = std::unique_ptr = {\n> get() = 0x7c5fedc06600\n> },\n> [2] = std::unique_ptr = {\n> get() = 0x7c5fedc066c0\n> }\n> }\n> (gdb) p *((libcamera::Message*)0x7c5fedc06540)\n> $46 = {\n> _vptr.Message = 0x7ffff06f8500 ,\n> type_ = libcamera::Message::InvokeMessage,\n> receiver_ = 0x7e2fedbe1d68,\n> static nextUserType_ = std::atomic = { 1000 }\n> }\n> (gdb) p *((libcamera::Message*)0x7c5fedc06600)\n> $47 = {\n> _vptr.Message = 0x7ffff06f8500 ,\n> type_ = libcamera::Message::InvokeMessage,\n> receiver_ = 0x7e2fedbe1d68,\n> static nextUserType_ = std::atomic = { 1000 }\n> }\n> (gdb) p *((libcamera::Message*)0x7c5fedc066c0)\n> $48 = {\n> _vptr.Message = 0x7ffff06f8500 ,\n> type_ = libcamera::Message::InvokeMessage,\n> receiver_ = 0x7e2fedbe1d68,\n> static nextUserType_ = std::atomic = { 1000 }\n> }\n> \n> Note that the `receiver_` is the same:\n> \n> (gdb) p *((libcamera::Message*)0x7c5fedc066c0)->receiver_\n> $49 = {\n> _vptr.Object = 0x7ffff4bdb3f8 ,\n> parent_ = 0x0,\n> children_ = std::__debug::vector of length 0, capacity 0,\n> thread_ = 0x7e2fedbe0280,\n> signals_ = empty std::__debug::list,\n> pendingMessages_ = 3\n> }\n> \n> So should that camera ever be restarted, those messages would be dispatched,\n> reusing old requests and arguments.\n> \n> I think there are multiple issues here: first, the one we have suspected, that\n> the `ispWorkerThread_` is not appropriately flushed/cancelled; and second, the\n> shutdown procedure does not seem robust enough to me if it runs into an assertion\n> in this state.\n\nWait, does the assertion kill the thread but not the process ?\n\nIn any case, there should be no assertion failure. The existing failure\ncomes from the failure to flush the messages to the thread. If we fix\nthat, are there still other problems ?\n\n> >> commit 86ffaf936dc663afd48bd3e276134fa720210bd6\n> >> Author: Milan Zamazal \n> >> Date:   Tue Feb 25 16:06:11 2025 +0100\n> >>\n> >>      libcamera: software_isp: Dispatch messages on stop\n> >>\n> >> This is however not the same issue, that commit addresses messages sent\n> >> by the thread, like you do below.\n> >>\n> >> So yes, it looks like both the software ISP and the virtual pipeline\n> >> handler will need to use removeMessages(), or something similar. It's\n> >> getting a bit too late to think about how the right API should look\n> >> like.\n> > \n> > My first idea was to have `cancelMessages(Message::Type, Object * = nullptr)`\n> > similarly to `dispatchMessages()`.\n> > \n> >>>> the thread exits. This will make stopping the camera faster, and will\n> >>>> also mimick better the behaviour of hardware cameras.\n> >>>>\n> >>>> I know it's a bit more work, hopefully it's just freshening up the yak\n> >>>> and not fully shaving it :-)\n> >>>>\n> >>>>> +    data->exit();\n> >>>>> +    data->wait();\n> >>>>> +\n> >>>>> +    /* Process queued `bufferCompleted` signal emissions. */\n> >>>>> +    Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n> >>>>> +    data->bufferCompleted.disconnect(this);\n> >>>>>    }\n> >>>>>    int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> >>>>> @@ -304,35 +353,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n> >>>>>        VirtualCameraData *data = cameraData(camera);\n> >>>>>        const auto timestamp = currentTimestamp();\n> >>>>> -    for (auto const &[stream, buffer] : request->buffers()) {\n> >>>>> -        bool found = false;\n> >>>>> -        /* map buffer and fill test patterns */\n> >>>>> -        for (auto &streamConfig : data->streamConfigs_) {\n> >>>>> -            if (stream == &streamConfig.stream) {\n> >>>>> -                FrameMetadata &fmd = buffer->_d()->metadata();\n> >>>>> -\n> >>>>> -                fmd.status = FrameMetadata::Status::FrameSuccess;\n> >>>>> -                fmd.sequence = streamConfig.seq++;\n> >>>>> -                fmd.timestamp = timestamp;\n> >>>>> -\n> >>>>> -                for (const auto [i, p] : utils::enumerate(buffer->planes()))\n> >>>>> -                    fmd.planes()[i].bytesused = p.length;\n> >>>>> -\n> >>>>> -                found = true;\n> >>>>> -\n> >>>>> -                if (streamConfig.frameGenerator->generateFrame(\n> >>>>> -                        stream->configuration().size, buffer))\n> >>>>> -                    fmd.status = FrameMetadata::Status::FrameError;\n> >>>>> -\n> >>>>> -                completeBuffer(request, buffer);\n> >>>>> -                break;\n> >>>>> -            }\n> >>>>> -        }\n> >>>>> -        ASSERT(found);\n> >>>>> -    }\n> >>>>> -\n> >>>>>        request->metadata().set(controls::SensorTimestamp, timestamp);\n> >>>>> -    completeRequest(request);\n> >>>>> +    data->invokeMethod(&VirtualCameraData::queueRequest, ConnectionTypeQueued, request);\n> >>>>>        return 0;\n> >>>>>    }\n> >>>>> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n> >>>>> index 683cb82b4..0832fd80c 100644\n> >>>>> --- a/src/libcamera/pipeline/virtual/virtual.h\n> >>>>> +++ b/src/libcamera/pipeline/virtual/virtual.h\n> >>>>> @@ -11,6 +11,7 @@\n> >>>>>    #include \n> >>>>>    #include \n> >>>>> +#include \n> >>>>\n> >>>> You should also include object.h.\n> >>>>\n> >>>>>    #include \n> >>>>>    #include \n> >>>>> @@ -25,7 +26,9 @@ namespace libcamera {\n> >>>>>    using VirtualFrame = std::variant;\n> >>>>> -class VirtualCameraData : public Camera::Private\n> >>>>> +class VirtualCameraData : public Camera::Private,\n> >>>>> +              public Thread,\n> >>>>> +              public Object\n> >>>>>    {\n> >>>>>    public:\n> >>>>>        const static unsigned int kMaxStream = 3;\n> >>>>> @@ -54,9 +57,12 @@ public:\n> >>>>>        ~VirtualCameraData() = default;\n> >>>>> +    void queueRequest(Request *request);\n> >>>>> +\n> >>>>>        Configuration config_;\n> >>>>>        std::vector streamConfigs_;\n> >>>>> +    Signal bufferCompleted;\n> >>>>>    };\n> >>>>>    } /* namespace libcamera */","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 20AF6BDCC1\n\tfor ;\n\tTue, 12 Aug 2025 14:56:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 85BF469244;\n\tTue, 12 Aug 2025 16:56:09 +0200 (CEST)","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 9E5336921B\n\tfor ;\n\tTue, 12 Aug 2025 16:56:07 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 80B234A4;\n\tTue, 12 Aug 2025 16:55:14 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"iNReVhEn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755010514;\n\tbh=Wuu0eSfU3UA6b6ziaOdCHEOO1njyVOMNE7g8sKYKsXc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iNReVhEnak6vz/Tjg6YdIcQeeKiwoOeQCTwGE5V48QafuNvBeZygrMrmikTULx3Pt\n\t101SQewX73ah3/To66iPxd14VT0/V85mnAnteDJvZRbduOi8k3tFv2ku6KX3Gy4L+d\n\tzY7XymndF+T8oz6zCq9VR6YoJ47v8vR7S6P7Lxpc=","Date":"Tue, 12 Aug 2025 17:55:49 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","Message-ID":"<20250812145549.GO30054@pendragon.ideasonboard.com>","References":"<20250811094926.1308259-1-barnabas.pocze@ideasonboard.com>\n\t<20250811094926.1308259-3-barnabas.pocze@ideasonboard.com>\n\t<20250811170532.GC30054@pendragon.ideasonboard.com>\n\t<1a21d4dc-6201-4b1f-8f3e-ce94ffb6f1a9@ideasonboard.com>\n\t<20250811224735.GE30054@pendragon.ideasonboard.com>\n\t\n\t<87746277-5270-48a2-bf17-7ef5653a3050@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<87746277-5270-48a2-bf17-7ef5653a3050@ideasonboard.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":35359,"web_url":"https://patchwork.libcamera.org/comment/35359/","msgid":"<44ab80be-0a02-4184-b9e5-8cc8cdf168a7@ideasonboard.com>","date":"2025-08-12T16:08:42","subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","submitter":{"id":216,"url":"https://patchwork.libcamera.org/api/people/216/","name":"Barnabás Pőcze","email":"barnabas.pocze@ideasonboard.com"},"content":"2025. 08. 12. 16:55 keltezéssel, Laurent Pinchart írta:\n> On Tue, Aug 12, 2025 at 03:17:43PM +0200, Barnabás Pőcze wrote:\n>> 2025. 08. 12. 9:16 keltezéssel, Barnabás Pőcze írta:\n>>> 2025. 08. 12. 0:47 keltezéssel, Laurent Pinchart írta:\n>>>> On Mon, Aug 11, 2025 at 07:51:21PM +0200, Barnabás Pőcze wrote:\n>>>>> 2025. 08. 11. 19:05 keltezéssel, Laurent Pinchart írta:\n>>>>>> On Mon, Aug 11, 2025 at 11:49:26AM +0200, Barnabás Pőcze wrote:\n>>>>>>> Currently the virtual pipeline generates the images synchronously. This is not\n>>>>>>> ideal because it blocks the camera manager's internal thread, and because its\n>>>>>>> behaviour is different from other existing pipeline handlers, all of which\n>>>>>>> complete requests asynchronously.\n>>>>>>>\n>>>>>>> So move the image generation to a separate thread by deriving `VirtualCameraData`\n>>>>>>> from `Thread`, as well as `Object` and using the existing asynchronous signal\n>>>>>>> and method call mechanism.\n>>>>>>>\n>>>>>>> Signed-off-by: Barnabás Pőcze \n>>>>>>> ---\n>>>>>>>    src/libcamera/pipeline/virtual/virtual.cpp | 80 ++++++++++++++--------\n>>>>>>>    src/libcamera/pipeline/virtual/virtual.h   |  8 ++-\n>>>>>>>    2 files changed, 58 insertions(+), 30 deletions(-)\n>>>>>>>\n>>>>>>> diff --git a/src/libcamera/pipeline/virtual/virtual.cpp b/src/libcamera/pipeline/virtual/virtual.cpp\n>>>>>>> index 049ebcba5..2ad32ec0a 100644\n>>>>>>> --- a/src/libcamera/pipeline/virtual/virtual.cpp\n>>>>>>> +++ b/src/libcamera/pipeline/virtual/virtual.cpp\n>>>>>>> @@ -129,6 +129,38 @@ VirtualCameraData::VirtualCameraData(PipelineHandler *pipe,\n>>>>>>>        /* \\todo Support multiple streams and pass multi_stream_test */\n>>>>>>>        streamConfigs_.resize(kMaxStream);\n>>>>>>> +\n>>>>>>> +    moveToThread(this);\n>>>>>>> +}\n>>>>>>> +\n>>>>>>> +void VirtualCameraData::queueRequest(Request *request)\n>>>>>>> +{\n>>>>>>> +    for (auto const &[stream, buffer] : request->buffers()) {\n>>>>>>> +        bool found = false;\n>>>>>>> +        /* map buffer and fill test patterns */\n>>>>>>> +        for (auto &streamConfig : streamConfigs_) {\n>>>>>>> +            if (stream == &streamConfig.stream) {\n>>>>>>> +                FrameMetadata &fmd = buffer->_d()->metadata();\n>>>>>>> +\n>>>>>>> +                fmd.status = FrameMetadata::Status::FrameSuccess;\n>>>>>>> +                fmd.sequence = streamConfig.seq++;\n>>>>>>> +                fmd.timestamp = currentTimestamp();\n>>>>>>> +\n>>>>>>> +                for (const auto [i, p] : utils::enumerate(buffer->planes()))\n>>>>>>> +                    fmd.planes()[i].bytesused = p.length;\n>>>>>>> +\n>>>>>>> +                found = true;\n>>>>>>> +\n>>>>>>> +                if (streamConfig.frameGenerator->generateFrame(\n>>>>>>> +                        stream->configuration().size, buffer))\n>>>>>>> +                    fmd.status = FrameMetadata::Status::FrameError;\n>>>>>>> +\n>>>>>>> +                bufferCompleted.emit(request, buffer);\n>>>>>>> +                break;\n>>>>>>> +            }\n>>>>>>> +        }\n>>>>>>> +        ASSERT(found);\n>>>>>>> +    }\n>>>>>>>    }\n>>>>>>>    VirtualCameraConfiguration::VirtualCameraConfiguration(VirtualCameraData *data)\n>>>>>>> @@ -291,11 +323,28 @@ int PipelineHandlerVirtual::start([[maybe_unused]] Camera *camera,\n>>>>>>>        for (auto &s : data->streamConfigs_)\n>>>>>>>            s.seq = 0;\n>>>>>>> +    data->bufferCompleted.connect(this, [&](Request *request, FrameBuffer *buffer) {\n>>>>>>> +        if (completeBuffer(request, buffer))\n>>>>>>> +            completeRequest(request);\n>>>>>>> +    });\n>>>>>>\n>>>>>> I'm not a big fan of lambdas in such cases, I find they hinder\n>>>>>> readability compared to member functions. The the PipelineHandlerVirtual\n>>>>>> class is not exposed outside of the pipeline handler, adding a private\n>>>>>> member function shouldn't be a big issue.\n>>>>>\n>>>>> Interesting, I have the exact opposite view. I find that member functions\n>>>>> usually involve more work and they don't allow for the same level of \"scoping\"\n>>>>> as lambdas.\n>>>>\n>>>> For me it's on a case-by-case basis. I really like lambda as adapters\n>>>> between two APIs. That's particularly useful when there's a small\n>>>> impedance mismatch between signals and slots. On the other hand, when\n>>>> the slot needs to implement a more complex logic that belongs to the\n>>>> receiver of the signal, I think member functions are better.\n>>>>\n>>>>>>> +\n>>>>>>> +    data->start();\n>>>>>>> +\n>>>>>>>        return 0;\n>>>>>>>    }\n>>>>>>> -void PipelineHandlerVirtual::stopDevice([[maybe_unused]] Camera *camera)\n>>>>>>> +void PipelineHandlerVirtual::stopDevice(Camera *camera)\n>>>>>>>    {\n>>>>>>> +    VirtualCameraData *data = cameraData(camera);\n>>>>>>> +\n>>>>>>> +    /* Flush all work. */\n>>>>>>> +    data->invokeMethod([] { }, ConnectionTypeBlocking);\n>>>>>>\n>>>>>> Does this mean the Thread class API is lacking ? In particular, I don't\n>>>>>> see a similar call in the SoftwareIsp::stop() function. Is it needed\n>>>>>> here because we rely solely on the queue of messages to keep track the\n>>>>>> requests ? If so, I think we should keep track of the queued requests in\n>>>>>> the VirtualCameraData class, and cancel all requests not processed after\n>>>>>\n>>>>> It's already tracked in `Camera::Private::queuedRequests_`, so that part seems easy.\n>>>>>\n>>>>> There is a need for flushing/cancelling asynchronous method invocations queued on\n>>>>> the worker thread(s), otherwise they will be executed when the thread is restarted\n>>>>> (or am I missing something that prevents this?). So we need a `cancelMessages()`\n>>>>> function in `Thread` then, it seems?\n>>>>\n>>>> We have Thread::removeMessages() that could possibly be useful, but it\n>>>> has to be called from the thread being stopped.\n>>>\n>>> Looking at the implementation I think I would have assumed that it can be\n>>> called from a different thread since it uses locks and everything.\n>>> I suppose the current issue is that it is `private`.\n> \n> A quick look confirms that. The devil is of course in the details, but I\n> think you could try to just make that function public. Maybe we should\n> also add a message type argument.\n> \n> Another option would be to automatically remove events when stopping the\n> thread. Such a hardcoded policy may be too inflexible though.\n> \n>>>>\n>>>> We use threads in two places: IPA modules, and software ISP. For IPA\n>>>> modules I think we're safe as the stop function calls\n>>>>\n>>>>     proxy_.invokeMethod(&ThreadProxy::stop, ConnectionTypeBlocking);\n>>>>\n>>>> before stopping the thread. The software ISP, on the other hand, doesn't\n>>>\n>>> Yes, that is equivalent to flushing all the invoke messages, which is what\n>>> this version proposes, but \"stop()\" here is a no-op, hence the empty lambda.\n>>>\n>>>> have such a blocking call. I wonder if it could be affected by the\n>>>> problem you describe. I remember we've fixed a similar issue in the\n>>>> past. Digging in the history, the following commit is related:\n>>>\n>>> I briefly looked at it yesterday, and my assumption is yes. I think the `SoftwareIsp`\n>>> class exhibits the same pattern as the virtual pipeline handler. If we consider\n>>> e.g. `SoftwareIsp::process()`, there is an async call:\n>>>\n>>>   debayer_->invokeMethod(&DebayerCpu::process,\n>>>                          ConnectionTypeQueued, frame, input, output, debayerParams_);\n>>>\n>>> but then `SoftwareIsp::stop()`:\n>>>\n>>>       ispWorkerThread_.exit();\n>>>     ispWorkerThread_.wait();\n>>>\n>>>     Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n>>>\n>>>     ipa_->stop();\n>>>\n>>>     for (auto buffer : queuedOutputBuffers_) {\n>>>         FrameMetadata &metadata = buffer->_d()->metadata();\n>>>         metadata.status = FrameMetadata::FrameCancelled;\n>>>         outputBufferReady.emit(buffer);\n>>>     }\n>>>     queuedOutputBuffers_.clear();\n>>>\n>>>     for (auto buffer : queuedInputBuffers_) {\n>>>         FrameMetadata &metadata = buffer->_d()->metadata();\n>>>         metadata.status = FrameMetadata::FrameCancelled;\n>>>         inputBufferReady.emit(buffer);\n>>>     }\n>>>     queuedInputBuffers_.clear();\n>>>\n>>> does not seem to cancel/flush the invoke messages on `ispWorkerThread_`, to which\n>>> the `*debayer_` object is bound. So it seems to me that such messages might\n>>> remain in the queue of `ispWorkerThread_`. But I will check this later.\n>>\n>> Unfortunately it took more time than expected, but with judicious use of `scheduler-locking`\n>> and breakpoints, I was able to trigger exactly that scenario. Some invoke messages\n>> remain in the thread's message queue after `SoftwareIsp::stop()` returns. The window\n>> of opportunity seems quite small, but definitely possible.\n> \n> Thank you for confirming it.\n> \n>> It seems to me that the shutdown procedure is not robust enough. For example, in this\n>> state there is an ubsan warning:\n>>\n>> ../src/libcamera/request.cpp:107:25: runtime error: load of value 3200171710, which is not a valid value for type 'Status'\n>>\n>> when it completes buffers in `SimpleCameraData::imageBufferReady()`:\n>>\n>> const RequestOutputs &outputs = conversionQueue_.front();\n>> for (auto &[stream, buf] : outputs.outputs)\n>> pipe->completeBuffer(outputs.request, buf);\n>>\n>> as the buffer metadata remains uninitialized:\n>>\n>> (gdb) p buffer->metadata()\n>> $32 = (const libcamera::FrameMetadata &) @0x7cefedc0ff68: {\n>> status = 3200171710,\n>> sequence = 3200171710,\n>> timestamp = 13744632839234567870,\n>> planes_ = std::__debug::vector of length 1, capacity 1 = {{\n>> bytesused = 0\n>> }}\n>> }\n>>\n>> and it also does not complete all requests:\n>>\n>> 387\t\tASSERT(data->queuedRequests_.empty());\n>> (gdb) p data->queuedRequests_\n>> $35 = std::__debug::list = {\n>> [0] = 0x7cafedbe0e00,\n>> [1] = 0x7cafedbe0f60,\n>> [2] = 0x7cafedbe10c0,\n>> [3] = 0x7cafedbe1220\n>> }\n>> (gdb) n\n>> [4:37:02.698049276] [7158] FATAL default pipeline_handler.cpp:387 assertion \"data->queuedRequests_.empty()\" failed in stop()\n>>\n>>\n>> After the assertion reaches __pthread_kill_implementation():\n>>\n>>\n>> (gdb) p ((SimpleCameraData *)data)->swIsp_->ispWorkerThread_->data_->messages_.list_\n>> $40 = std::__debug::list = {\n>> [0] = std::unique_ptr = {\n>> get() = 0x7c5fedc06540\n>> },\n>> [1] = std::unique_ptr = {\n>> get() = 0x7c5fedc06600\n>> },\n>> [2] = std::unique_ptr = {\n>> get() = 0x7c5fedc066c0\n>> }\n>> }\n>> (gdb) p *((libcamera::Message*)0x7c5fedc06540)\n>> $46 = {\n>> _vptr.Message = 0x7ffff06f8500 ,\n>> type_ = libcamera::Message::InvokeMessage,\n>> receiver_ = 0x7e2fedbe1d68,\n>> static nextUserType_ = std::atomic = { 1000 }\n>> }\n>> (gdb) p *((libcamera::Message*)0x7c5fedc06600)\n>> $47 = {\n>> _vptr.Message = 0x7ffff06f8500 ,\n>> type_ = libcamera::Message::InvokeMessage,\n>> receiver_ = 0x7e2fedbe1d68,\n>> static nextUserType_ = std::atomic = { 1000 }\n>> }\n>> (gdb) p *((libcamera::Message*)0x7c5fedc066c0)\n>> $48 = {\n>> _vptr.Message = 0x7ffff06f8500 ,\n>> type_ = libcamera::Message::InvokeMessage,\n>> receiver_ = 0x7e2fedbe1d68,\n>> static nextUserType_ = std::atomic = { 1000 }\n>> }\n>>\n>> Note that the `receiver_` is the same:\n>>\n>> (gdb) p *((libcamera::Message*)0x7c5fedc066c0)->receiver_\n>> $49 = {\n>> _vptr.Object = 0x7ffff4bdb3f8 ,\n>> parent_ = 0x0,\n>> children_ = std::__debug::vector of length 0, capacity 0,\n>> thread_ = 0x7e2fedbe0280,\n>> signals_ = empty std::__debug::list,\n>> pendingMessages_ = 3\n>> }\n>>\n>> So should that camera ever be restarted, those messages would be dispatched,\n>> reusing old requests and arguments.\n>>\n>> I think there are multiple issues here: first, the one we have suspected, that\n>> the `ispWorkerThread_` is not appropriately flushed/cancelled; and second, the\n>> shutdown procedure does not seem robust enough to me if it runs into an assertion\n>> in this state.\n> \n> Wait, does the assertion kill the thread but not the process ?\n\ngdb breaks before the process is stopped, the above has been recovered\nfrom that state.\n\n\n> \n> In any case, there should be no assertion failure. The existing failure\n> comes from the failure to flush the messages to the thread. If we fix\n> that, are there still other problems ?\n\nIf the messages are just cancelled, this will still fail the same way at the\nassertion. For the virtual pipeline handler either flushing or cancellation\nworks well. I am not too familiar with the simple pipeline handler, so I can't\nsay what should be done here, there is also the IPA that might complicate things.\nMy impression is that the shutdown logic could (should?) be improved.\n\n\n> \n>>>> commit 86ffaf936dc663afd48bd3e276134fa720210bd6\n>>>> Author: Milan Zamazal \n>>>> Date:   Tue Feb 25 16:06:11 2025 +0100\n>>>>\n>>>>      libcamera: software_isp: Dispatch messages on stop\n>>>>\n>>>> This is however not the same issue, that commit addresses messages sent\n>>>> by the thread, like you do below.\n>>>>\n>>>> So yes, it looks like both the software ISP and the virtual pipeline\n>>>> handler will need to use removeMessages(), or something similar. It's\n>>>> getting a bit too late to think about how the right API should look\n>>>> like.\n>>>\n>>> My first idea was to have `cancelMessages(Message::Type, Object * = nullptr)`\n>>> similarly to `dispatchMessages()`.\n>>>\n>>>>>> the thread exits. This will make stopping the camera faster, and will\n>>>>>> also mimick better the behaviour of hardware cameras.\n>>>>>>\n>>>>>> I know it's a bit more work, hopefully it's just freshening up the yak\n>>>>>> and not fully shaving it :-)\n>>>>>>\n>>>>>>> +    data->exit();\n>>>>>>> +    data->wait();\n>>>>>>> +\n>>>>>>> +    /* Process queued `bufferCompleted` signal emissions. */\n>>>>>>> +    Thread::current()->dispatchMessages(Message::Type::InvokeMessage, this);\n>>>>>>> +    data->bufferCompleted.disconnect(this);\n>>>>>>>    }\n>>>>>>>    int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n>>>>>>> @@ -304,35 +353,8 @@ int PipelineHandlerVirtual::queueRequestDevice([[maybe_unused]] Camera *camera,\n>>>>>>>        VirtualCameraData *data = cameraData(camera);\n>>>>>>>        const auto timestamp = currentTimestamp();\n>>>>>>> -    for (auto const &[stream, buffer] : request->buffers()) {\n>>>>>>> -        bool found = false;\n>>>>>>> -        /* map buffer and fill test patterns */\n>>>>>>> -        for (auto &streamConfig : data->streamConfigs_) {\n>>>>>>> -            if (stream == &streamConfig.stream) {\n>>>>>>> -                FrameMetadata &fmd = buffer->_d()->metadata();\n>>>>>>> -\n>>>>>>> -                fmd.status = FrameMetadata::Status::FrameSuccess;\n>>>>>>> -                fmd.sequence = streamConfig.seq++;\n>>>>>>> -                fmd.timestamp = timestamp;\n>>>>>>> -\n>>>>>>> -                for (const auto [i, p] : utils::enumerate(buffer->planes()))\n>>>>>>> -                    fmd.planes()[i].bytesused = p.length;\n>>>>>>> -\n>>>>>>> -                found = true;\n>>>>>>> -\n>>>>>>> -                if (streamConfig.frameGenerator->generateFrame(\n>>>>>>> -                        stream->configuration().size, buffer))\n>>>>>>> -                    fmd.status = FrameMetadata::Status::FrameError;\n>>>>>>> -\n>>>>>>> -                completeBuffer(request, buffer);\n>>>>>>> -                break;\n>>>>>>> -            }\n>>>>>>> -        }\n>>>>>>> -        ASSERT(found);\n>>>>>>> -    }\n>>>>>>> -\n>>>>>>>        request->metadata().set(controls::SensorTimestamp, timestamp);\n>>>>>>> -    completeRequest(request);\n>>>>>>> +    data->invokeMethod(&VirtualCameraData::queueRequest, ConnectionTypeQueued, request);\n>>>>>>>        return 0;\n>>>>>>>    }\n>>>>>>> diff --git a/src/libcamera/pipeline/virtual/virtual.h b/src/libcamera/pipeline/virtual/virtual.h\n>>>>>>> index 683cb82b4..0832fd80c 100644\n>>>>>>> --- a/src/libcamera/pipeline/virtual/virtual.h\n>>>>>>> +++ b/src/libcamera/pipeline/virtual/virtual.h\n>>>>>>> @@ -11,6 +11,7 @@\n>>>>>>>    #include \n>>>>>>>    #include \n>>>>>>> +#include \n>>>>>>\n>>>>>> You should also include object.h.\n>>>>>>\n>>>>>>>    #include \n>>>>>>>    #include \n>>>>>>> @@ -25,7 +26,9 @@ namespace libcamera {\n>>>>>>>    using VirtualFrame = std::variant;\n>>>>>>> -class VirtualCameraData : public Camera::Private\n>>>>>>> +class VirtualCameraData : public Camera::Private,\n>>>>>>> +              public Thread,\n>>>>>>> +              public Object\n>>>>>>>    {\n>>>>>>>    public:\n>>>>>>>        const static unsigned int kMaxStream = 3;\n>>>>>>> @@ -54,9 +57,12 @@ public:\n>>>>>>>        ~VirtualCameraData() = default;\n>>>>>>> +    void queueRequest(Request *request);\n>>>>>>> +\n>>>>>>>        Configuration config_;\n>>>>>>>        std::vector streamConfigs_;\n>>>>>>> +    Signal bufferCompleted;\n>>>>>>>    };\n>>>>>>>    } /* namespace libcamera */\n> \n> --\n> Regards,\n> \n> Laurent Pinchart","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 4D9B6BEFBE\n\tfor ;\n\tTue, 12 Aug 2025 16:08:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D37D69249;\n\tTue, 12 Aug 2025 18:08:48 +0200 (CEST)","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 499F26921A\n\tfor ;\n\tTue, 12 Aug 2025 18:08:46 +0200 (CEST)","from [192.168.33.15] (185.221.140.182.nat.pool.zt.hu\n\t[185.221.140.182])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3BE59465;\n\tTue, 12 Aug 2025 18:07:53 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"cf+jZ+kc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1755014873;\n\tbh=emjt+apFYQCro0k1e/8LZDE/8ju+npf++Ad2Z49mZ+8=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=cf+jZ+kctNxfbghY1ZJu7wngZ9wENxslvWhRfgPFnrpJhHN0tPReOF+S5fXlvGJIg\n\tKRRIudly0fkNANp1BnQl48o9SlhRrZwGMnIkE8gpzy5nqEvKDkWrtFuV/9vr5hFqp8\n\t0AjSisJzkn4aof10kcQGGZl1URHJLWEszxIohdSw=","Message-ID":"<44ab80be-0a02-4184-b9e5-8cc8cdf168a7@ideasonboard.com>","Date":"Tue, 12 Aug 2025 18:08:42 +0200","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [RFC PATCH v1 2/2] libcamera: pipeline: virtual: Move image\n\tgeneration to separate thread","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20250811094926.1308259-1-barnabas.pocze@ideasonboard.com>\n\t<20250811094926.1308259-3-barnabas.pocze@ideasonboard.com>\n\t<20250811170532.GC30054@pendragon.ideasonboard.com>\n\t<1a21d4dc-6201-4b1f-8f3e-ce94ffb6f1a9@ideasonboard.com>\n\t<20250811224735.GE30054@pendragon.ideasonboard.com>\n\t\n\t<87746277-5270-48a2-bf17-7ef5653a3050@ideasonboard.com>\n\t\n\t<20250812145549.GO30054@pendragon.ideasonboard.com>","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Content-Language":"en-US, hu-HU","In-Reply-To":"<20250812145549.GO30054@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}}]