[{"id":33737,"web_url":"https://patchwork.libcamera.org/comment/33737/","msgid":"<174302148371.1701483.16204592275585512132@ping.linuxembedded.co.uk>","date":"2025-03-26T20:38:03","subject":"Re: [PATCH v5 2/6] ipa: simple: softisp: Extend to pass metadata","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-03-26 12:48:51)\n> From: Kieran Bingham \n> \n> Extend the Simple IPA IPC to support returning a metadata ControlList\n> when the process call has completed.\n> \n> A new signal from the IPA is introduced to report the metadata,\n> similarly to what the hardware pipelines do.\n> \n> Merge the metadata reported by the ISP into any completing request to\n> provide to the application. Completion of a request is delayed until\n> this is done; this doesn't apply to canceled requests.\n> \n> Signed-off-by: Kieran Bingham \n\nI might have started this, but I think Milan has looked after it well,\n\n> Signed-off-by: Milan Zamazal \n> ---\n> .../internal/software_isp/software_isp.h | 2 ++\n> include/libcamera/ipa/soft.mojom | 1 +\n> src/ipa/simple/soft_simple.cpp | 7 +---\n> src/libcamera/pipeline/simple/simple.cpp | 33 ++++++++++++++++++-\n> src/libcamera/software_isp/software_isp.cpp | 9 +++++\n> 5 files changed, 45 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> index 0026cec2..1ee37dc7 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -85,12 +85,14 @@ public:\n> Signal inputBufferReady;\n> Signal outputBufferReady;\n> Signal ispStatsReady;\n> + Signal metadataReady;\n> Signal setSensorControls;\n> \n> private:\n> void saveIspParams();\n> void setSensorCtrls(const ControlList &sensorControls);\n> void statsReady(uint32_t frame, uint32_t bufferId);\n> + void saveMetadata(uint32_t frame, const ControlList &metadata);\n> void inputReady(FrameBuffer *input);\n> void outputReady(FrameBuffer *output);\n> \n> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> index ede74413..a8e6ecda 100644\n> --- a/include/libcamera/ipa/soft.mojom\n> +++ b/include/libcamera/ipa/soft.mojom\n> @@ -33,4 +33,5 @@ interface IPASoftInterface {\n> interface IPASoftEventInterface {\n> setSensorControls(libcamera.ControlList sensorControls);\n> setIspParams();\n> + metadataReady(uint32 frame, libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index a87c6cdd..4ef67c43 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -299,15 +299,10 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> int32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> frameContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;\n> \n> - /*\n> - * Software ISP currently does not produce any metadata. Use an empty\n> - * ControlList for now.\n> - *\n> - * \\todo Implement proper metadata handling\n> - */\n> ControlList metadata(controls::controls);\n> for (auto const &algo : algorithms())\n> algo->process(context_, frame, frameContext, stats_, metadata);\n> + metadataReady.emit(frame, metadata);\n> \n> /* Sanity check */\n> if (!sensorControls.contains(V4L2_CID_EXPOSURE) ||\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 2281c0a6..f34df926 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -184,6 +184,7 @@ class SimplePipelineHandler;\n> struct SimpleFrameInfo {\n> uint32_t frame;\n> Request *request;\n> + bool metadataProcessed;\n> };\n> \n> class SimpleFrames\n> @@ -207,6 +208,7 @@ void SimpleFrames::create(Request *request)\n> ASSERT(inserted);\n> it->second.frame = frame;\n> it->second.request = request;\n> + it->second.metadataProcessed = false;\n> }\n> \n> void SimpleFrames::destroy(uint32_t frame)\n> @@ -357,11 +359,13 @@ private:\n> void tryPipeline(unsigned int code, const Size &size);\n> static std::vector routedSourcePads(MediaPad *sink);\n> \n> + void tryCompleteRequest(Request *request);\n> void completeRequest(Request *request);\n> void conversionInputDone(FrameBuffer *buffer);\n> void conversionOutputDone(FrameBuffer *buffer);\n> \n> void ispStatsReady(uint32_t frame, uint32_t bufferId);\n> + void metadataReady(uint32_t frame, const ControlList &metadata);\n> void setSensorControls(const ControlList &sensorControls);\n> };\n> \n> @@ -600,6 +604,7 @@ int SimpleCameraData::init()\n> swIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n> swIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n> swIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n> + swIsp_->metadataReady.connect(this, &SimpleCameraData::metadataReady);\n> swIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);\n> }\n> }\n> @@ -932,6 +937,21 @@ void SimpleCameraData::clearIncompleteRequests()\n> }\n> }\n> \n> +void SimpleCameraData::tryCompleteRequest(Request *request)\n> +{\n> + if (request->hasPendingBuffers())\n> + return;\n> +\n> + SimpleFrameInfo *info = frameInfo_.find(request);\n> + if (!info)\n> + return;\n> +\n> + if (!info->metadataProcessed)\n> + return;\n> +\n> + completeRequest(request);\n> +}\n> +\n> void SimpleCameraData::completeRequest(Request *request)\n> {\n> SimpleFrameInfo *info = frameInfo_.find(request);\n> @@ -957,7 +977,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> /* Complete the buffer and the request. */\n> Request *request = buffer->request();\n> if (pipe->completeBuffer(request, buffer))\n> - completeRequest(request);\n> + tryCompleteRequest(request);\n> }\n> \n> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> @@ -966,6 +986,17 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> delayedCtrls_->get(frame));\n> }\n> \n> +void SimpleCameraData::metadataReady(uint32_t frame, const ControlList &metadata)\n> +{\n> + SimpleFrameInfo *info = frameInfo_.find(frame);\n> + if (!info)\n> + return;\n> +\n> + info->request->metadata().merge(metadata);\n> + info->metadataProcessed = true;\n> + tryCompleteRequest(info->request);\n> +}\n> +\n> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> {\n> delayedCtrls_->push(sensorControls);\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index 4a74dcb6..6baa3fec 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -55,6 +55,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> * \\brief A signal emitted when the statistics for IPA are ready\n> */\n> \n> +/**\n> + * \\var SoftwareIsp::metadataReady\n> + * \\brief A signal emitted when the metadata for IPA is ready\n> + */\n> +\n> /**\n> * \\var SoftwareIsp::setSensorControls\n> * \\brief A signal emitted when the values to write to the sensor controls are\n> @@ -141,6 +146,10 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n> }\n> \n> ipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);\n> + ipa_->metadataReady.connect(this,\n> + [this](uint32_t frame, const ControlList &metadata) {\n> + metadataReady.emit(frame, metadata);\n> + });\n> ipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);\n\nAs far as I can tell, all comments handled from v3:\n\n\nReviewed-by: Kieran Bingham \n\n> \n> debayer_->moveToThread(&ispWorkerThread_);\n> -- \n> 2.49.0\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 6F740C3213\n\tfor ;\n\tWed, 26 Mar 2025 20:38:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BE996896E;\n\tWed, 26 Mar 2025 21:38:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 67A2968950\n\tfor ;\n\tWed, 26 Mar 2025 21:38:06 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 23CEB3A4;\n\tWed, 26 Mar 2025 21:36:18 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Qm1zQyqL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743021378;\n\tbh=i3S47H9aec5cXBSAyR74PoZ5c/OOKqxts0LFMXtzYXw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=Qm1zQyqLr5j+l17RKOzhNJ1qki/y0BFu8/TL2Ex+YK7hLVj04r7jzhoWEs6ZJfL48\n\t3uF61Be0Nl2Ev613k6AXL2LEnyQO7g3HhSuHEZx6KEneXoKHC0JzYM2FEGEoTouB+U\n\tWq203WS+IVtKL3S1OBbWfTxj6wq5YtRNGmTMsOWA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250326124856.75709-3-mzamazal@redhat.com>","References":"<20250326124856.75709-1-mzamazal@redhat.com>\n\t<20250326124856.75709-3-mzamazal@redhat.com>","Subject":"Re: [PATCH v5 2/6] ipa: simple: softisp: Extend to pass metadata","From":"Kieran Bingham ","Cc":"Laurent Pinchart ,\n\tMilan Zamazal ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Wed, 26 Mar 2025 20:38:03 +0000","Message-ID":"<174302148371.1701483.16204592275585512132@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33743,"web_url":"https://patchwork.libcamera.org/comment/33743/","msgid":"<20250326230854.GC21053@pendragon.ideasonboard.com>","date":"2025-03-26T23:08:54","subject":"Re: [PATCH v5 2/6] ipa: simple: softisp: Extend to pass metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Wed, Mar 26, 2025 at 01:48:51PM +0100, Milan Zamazal wrote:\n> From: Kieran Bingham \n> \n> Extend the Simple IPA IPC to support returning a metadata ControlList\n> when the process call has completed.\n> \n> A new signal from the IPA is introduced to report the metadata,\n> similarly to what the hardware pipelines do.\n> \n> Merge the metadata reported by the ISP into any completing request to\n> provide to the application. Completion of a request is delayed until\n> this is done; this doesn't apply to canceled requests.\n> \n> Signed-off-by: Kieran Bingham \n> Signed-off-by: Milan Zamazal \n> ---\n> .../internal/software_isp/software_isp.h | 2 ++\n> include/libcamera/ipa/soft.mojom | 1 +\n> src/ipa/simple/soft_simple.cpp | 7 +---\n> src/libcamera/pipeline/simple/simple.cpp | 33 ++++++++++++++++++-\n> src/libcamera/software_isp/software_isp.cpp | 9 +++++\n> 5 files changed, 45 insertions(+), 7 deletions(-)\n> \n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> index 0026cec2..1ee37dc7 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -85,12 +85,14 @@ public:\n> \tSignal inputBufferReady;\n> \tSignal outputBufferReady;\n> \tSignal ispStatsReady;\n> +\tSignal metadataReady;\n> \tSignal setSensorControls;\n> \n> private:\n> \tvoid saveIspParams();\n> \tvoid setSensorCtrls(const ControlList &sensorControls);\n> \tvoid statsReady(uint32_t frame, uint32_t bufferId);\n> +\tvoid saveMetadata(uint32_t frame, const ControlList &metadata);\n> \tvoid inputReady(FrameBuffer *input);\n> \tvoid outputReady(FrameBuffer *output);\n> \n> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> index ede74413..a8e6ecda 100644\n> --- a/include/libcamera/ipa/soft.mojom\n> +++ b/include/libcamera/ipa/soft.mojom\n> @@ -33,4 +33,5 @@ interface IPASoftInterface {\n> interface IPASoftEventInterface {\n> \tsetSensorControls(libcamera.ControlList sensorControls);\n> \tsetIspParams();\n> +\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index a87c6cdd..4ef67c43 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -299,15 +299,10 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> \tint32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> \tframeContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;\n> \n> -\t/*\n> -\t * Software ISP currently does not produce any metadata. Use an empty\n> -\t * ControlList for now.\n> -\t *\n> -\t * \\todo Implement proper metadata handling\n> -\t */\n> \tControlList metadata(controls::controls);\n> \tfor (auto const &algo : algorithms())\n> \t\talgo->process(context_, frame, frameContext, stats_, metadata);\n> +\tmetadataReady.emit(frame, metadata);\n> \n> \t/* Sanity check */\n> \tif (!sensorControls.contains(V4L2_CID_EXPOSURE) ||\n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 2281c0a6..f34df926 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -184,6 +184,7 @@ class SimplePipelineHandler;\n> struct SimpleFrameInfo {\n> \tuint32_t frame;\n> \tRequest *request;\n> +\tbool metadataProcessed;\n> };\n> \n> class SimpleFrames\n> @@ -207,6 +208,7 @@ void SimpleFrames::create(Request *request)\n> \tASSERT(inserted);\n> \tit->second.frame = frame;\n> \tit->second.request = request;\n> +\tit->second.metadataProcessed = false;\n> }\n> \n> void SimpleFrames::destroy(uint32_t frame)\n> @@ -357,11 +359,13 @@ private:\n> \tvoid tryPipeline(unsigned int code, const Size &size);\n> \tstatic std::vector routedSourcePads(MediaPad *sink);\n> \n> +\tvoid tryCompleteRequest(Request *request);\n> \tvoid completeRequest(Request *request);\n> \tvoid conversionInputDone(FrameBuffer *buffer);\n> \tvoid conversionOutputDone(FrameBuffer *buffer);\n> \n> \tvoid ispStatsReady(uint32_t frame, uint32_t bufferId);\n> +\tvoid metadataReady(uint32_t frame, const ControlList &metadata);\n> \tvoid setSensorControls(const ControlList &sensorControls);\n> };\n> \n> @@ -600,6 +604,7 @@ int SimpleCameraData::init()\n> \t\t\tswIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n> \t\t\tswIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n> \t\t\tswIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n> +\t\t\tswIsp_->metadataReady.connect(this, &SimpleCameraData::metadataReady);\n> \t\t\tswIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);\n> \t\t}\n> \t}\n> @@ -932,6 +937,21 @@ void SimpleCameraData::clearIncompleteRequests()\n> \t}\n> }\n> \n> +void SimpleCameraData::tryCompleteRequest(Request *request)\n> +{\n> +\tif (request->hasPendingBuffers())\n> +\t\treturn;\n> +\n> +\tSimpleFrameInfo *info = frameInfo_.find(request);\n> +\tif (!info)\n> +\t\treturn;\n> +\n> +\tif (!info->metadataProcessed)\n> +\t\treturn;\n> +\n> +\tcompleteRequest(request);\n> +}\n\nI may not have expressed my thoughts correctly in the review of v3. What\nI proposed was having a single function, named tryCompleteRequest(), to\nreplace your current completeRequest() implementation. The function\nwould ensure that all the pieces of data necessary to complete the\nrequest are there, and then proceed to complete it.\n\nAs far as I can tell, the only thing you will need to change in addition\nto merging the completeRequest() function into tryCompleteRequest() is\nto set metadataProcessed to true when the pipeline is not expected to\nproduced metadata (that is, when no soft ISP is used). This could be\ndone for instance when creating the SimpleFrameInfo entry. You should\nalso double-check the cancellation paths, to set metadataProcessed to\ntrue if the frame is cancelled before being processed by the soft ISP.\n\n> +\n> void SimpleCameraData::completeRequest(Request *request)\n> {\n> \tSimpleFrameInfo *info = frameInfo_.find(request);\n> @@ -957,7 +977,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> \t/* Complete the buffer and the request. */\n> \tRequest *request = buffer->request();\n> \tif (pipe->completeBuffer(request, buffer))\n> -\t\tcompleteRequest(request);\n> +\t\ttryCompleteRequest(request);\n> }\n> \n> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> @@ -966,6 +986,17 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> \t\t\t delayedCtrls_->get(frame));\n> }\n> \n> +void SimpleCameraData::metadataReady(uint32_t frame, const ControlList &metadata)\n> +{\n> +\tSimpleFrameInfo *info = frameInfo_.find(frame);\n> +\tif (!info)\n> +\t\treturn;\n> +\n> +\tinfo->request->metadata().merge(metadata);\n> +\tinfo->metadataProcessed = true;\n> +\ttryCompleteRequest(info->request);\n> +}\n> +\n> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> {\n> \tdelayedCtrls_->push(sensorControls);\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index 4a74dcb6..6baa3fec 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -55,6 +55,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> * \\brief A signal emitted when the statistics for IPA are ready\n> */\n> \n> +/**\n> + * \\var SoftwareIsp::metadataReady\n> + * \\brief A signal emitted when the metadata for IPA is ready\n> + */\n> +\n> /**\n> * \\var SoftwareIsp::setSensorControls\n> * \\brief A signal emitted when the values to write to the sensor controls are\n> @@ -141,6 +146,10 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n> \t}\n> \n> \tipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);\n> +\tipa_->metadataReady.connect(this,\n> +\t\t\t\t [this](uint32_t frame, const ControlList &metadata) {\n> +\t\t\t\t\t metadataReady.emit(frame, metadata);\n> +\t\t\t\t });\n> \tipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);\n> \n> \tdebayer_->moveToThread(&ispWorkerThread_);","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 A9ACDC3213\n\tfor ;\n\tWed, 26 Mar 2025 23:09:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D24356896D;\n\tThu, 27 Mar 2025 00:09:19 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C8863600EB\n\tfor ;\n\tThu, 27 Mar 2025 00:09:17 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D1E691D5;\n\tThu, 27 Mar 2025 00:07:28 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"U8TAkyLI\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743030449;\n\tbh=Mn43+5jsKc6LWQR1UzoCWOKgLNavM4sYakLRm2yOMXo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=U8TAkyLIJS3PvNagbm0gUH21uQn0C6mHoED5ArVDG5JcqNRWYTvjBtRKwo236yAn1\n\tIlU8QYmEFrVCjb8v9w5H1iZ3MRsSgIOWWetau2o9Eyno/S69D+eoWU4VaChrFnJjDz\n\tkN7asJOy+RkaHjOTZh1X8e+CySRl2du9K6xnPi0k=","Date":"Thu, 27 Mar 2025 01:08:54 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v5 2/6] ipa: simple: softisp: Extend to pass metadata","Message-ID":"<20250326230854.GC21053@pendragon.ideasonboard.com>","References":"<20250326124856.75709-1-mzamazal@redhat.com>\n\t<20250326124856.75709-3-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250326124856.75709-3-mzamazal@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33749,"web_url":"https://patchwork.libcamera.org/comment/33749/","msgid":"<857c4a2zhp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","date":"2025-03-27T16:46:10","subject":"Re: [PATCH v5 2/6] ipa: simple: softisp: Extend to pass metadata","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Hi Laurent,\n\nthank you for review.\n\nLaurent Pinchart writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Wed, Mar 26, 2025 at 01:48:51PM +0100, Milan Zamazal wrote:\n>> From: Kieran Bingham \n>> \n>> Extend the Simple IPA IPC to support returning a metadata ControlList\n>> when the process call has completed.\n>> \n>> A new signal from the IPA is introduced to report the metadata,\n>> similarly to what the hardware pipelines do.\n>> \n>> Merge the metadata reported by the ISP into any completing request to\n>> provide to the application. Completion of a request is delayed until\n>> this is done; this doesn't apply to canceled requests.\n>> \n>> Signed-off-by: Kieran Bingham \n>> Signed-off-by: Milan Zamazal \n>> ---\n>> .../internal/software_isp/software_isp.h | 2 ++\n>> include/libcamera/ipa/soft.mojom | 1 +\n>> src/ipa/simple/soft_simple.cpp | 7 +---\n>> src/libcamera/pipeline/simple/simple.cpp | 33 ++++++++++++++++++-\n>> src/libcamera/software_isp/software_isp.cpp | 9 +++++\n>> 5 files changed, 45 insertions(+), 7 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n>> index 0026cec2..1ee37dc7 100644\n>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>> @@ -85,12 +85,14 @@ public:\n>> \tSignal inputBufferReady;\n>> \tSignal outputBufferReady;\n>> \tSignal ispStatsReady;\n>> +\tSignal metadataReady;\n>> \tSignal setSensorControls;\n>> \n>> private:\n>> \tvoid saveIspParams();\n>> \tvoid setSensorCtrls(const ControlList &sensorControls);\n>> \tvoid statsReady(uint32_t frame, uint32_t bufferId);\n>> +\tvoid saveMetadata(uint32_t frame, const ControlList &metadata);\n>> \tvoid inputReady(FrameBuffer *input);\n>> \tvoid outputReady(FrameBuffer *output);\n>> \n>> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n>> index ede74413..a8e6ecda 100644\n>> --- a/include/libcamera/ipa/soft.mojom\n>> +++ b/include/libcamera/ipa/soft.mojom\n>> @@ -33,4 +33,5 @@ interface IPASoftInterface {\n>> interface IPASoftEventInterface {\n>> \tsetSensorControls(libcamera.ControlList sensorControls);\n>> \tsetIspParams();\n>> +\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n>> };\n>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n>> index a87c6cdd..4ef67c43 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -299,15 +299,10 @@ void IPASoftSimple::processStats(const uint32_t frame,\n>> \tint32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n>> \tframeContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;\n>> \n>> -\t/*\n>> -\t * Software ISP currently does not produce any metadata. Use an empty\n>> -\t * ControlList for now.\n>> -\t *\n>> -\t * \\todo Implement proper metadata handling\n>> -\t */\n>> \tControlList metadata(controls::controls);\n>> \tfor (auto const &algo : algorithms())\n>> \t\talgo->process(context_, frame, frameContext, stats_, metadata);\n>> +\tmetadataReady.emit(frame, metadata);\n>> \n>> \t/* Sanity check */\n>> \tif (!sensorControls.contains(V4L2_CID_EXPOSURE) ||\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 2281c0a6..f34df926 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -184,6 +184,7 @@ class SimplePipelineHandler;\n>> struct SimpleFrameInfo {\n>> \tuint32_t frame;\n>> \tRequest *request;\n>> +\tbool metadataProcessed;\n>> };\n>> \n>> class SimpleFrames\n>> @@ -207,6 +208,7 @@ void SimpleFrames::create(Request *request)\n>> \tASSERT(inserted);\n>> \tit->second.frame = frame;\n>> \tit->second.request = request;\n>> +\tit->second.metadataProcessed = false;\n>> }\n>> \n>> void SimpleFrames::destroy(uint32_t frame)\n>> @@ -357,11 +359,13 @@ private:\n>> \tvoid tryPipeline(unsigned int code, const Size &size);\n>> \tstatic std::vector routedSourcePads(MediaPad *sink);\n>> \n>> +\tvoid tryCompleteRequest(Request *request);\n>> \tvoid completeRequest(Request *request);\n>> \tvoid conversionInputDone(FrameBuffer *buffer);\n>> \tvoid conversionOutputDone(FrameBuffer *buffer);\n>> \n>> \tvoid ispStatsReady(uint32_t frame, uint32_t bufferId);\n>> +\tvoid metadataReady(uint32_t frame, const ControlList &metadata);\n>> \tvoid setSensorControls(const ControlList &sensorControls);\n>> };\n>> \n>> @@ -600,6 +604,7 @@ int SimpleCameraData::init()\n>> \t\t\tswIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n>> \t\t\tswIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n>> \t\t\tswIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n>> +\t\t\tswIsp_->metadataReady.connect(this, &SimpleCameraData::metadataReady);\n>> \t\t\tswIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);\n>> \t\t}\n>> \t}\n>> @@ -932,6 +937,21 @@ void SimpleCameraData::clearIncompleteRequests()\n>> \t}\n>> }\n>> \n>> +void SimpleCameraData::tryCompleteRequest(Request *request)\n>> +{\n>> +\tif (request->hasPendingBuffers())\n>> +\t\treturn;\n>> +\n>> +\tSimpleFrameInfo *info = frameInfo_.find(request);\n>> +\tif (!info)\n>> +\t\treturn;\n>> +\n>> +\tif (!info->metadataProcessed)\n>> +\t\treturn;\n>> +\n>> +\tcompleteRequest(request);\n>> +}\n>\n> I may not have expressed my thoughts correctly in the review of v3. What\n> I proposed was having a single function, named tryCompleteRequest(), to\n> replace your current completeRequest() implementation. The function\n> would ensure that all the pieces of data necessary to complete the\n> request are there, and then proceed to complete it.\n>\n> As far as I can tell, the only thing you will need to change in addition\n> to merging the completeRequest() function into tryCompleteRequest() \n\nframeInfo_ is used only with software ISP so it would have been handled\nconditionally too. I think this is where we misunderstood each other.\n\nActually, a bug was introduced in v4 of the preceding patch, in that\nframeInfo_ is checked in completeRequest whether it should be present\n(with swisp) or not (without swisp).\n\nLet's try to stop the increasing confusion.\n\n> is to set metadataProcessed to true when the pipeline is not expected\n> to produced metadata (that is, when no soft ISP is used). This could\n> be done for instance when creating the SimpleFrameInfo entry. \n\nOK, let's always create SimpleFrameInfo for a request, whether software\nISP is used or not. This should also avoid further confusion once\nmultiple streams are used (in the patches for raw streams). And instead\nof setting metadataProcessed to true when no metadata is expected, I'd\nintroduce and set an additional metadataRequired; then it is clearer\nwhat is what and it can be handled in tryCompleteRequest. Now a single\ncompletion method, tryCompleteRequest, makes sense.\n\n> You should also double-check the cancellation paths, to set\n> metadataProcessed to true if the frame is cancelled before being\n> processed by the soft ISP.\n\nNot pretty but it is required only in a single place and not much\nconfusing there, so OK. At least until we possibly add more stuff to\nthe frame info and the accompanying completion checks.\n\nI hope we are now getting in sync with each other.\n\n>> +\n>> void SimpleCameraData::completeRequest(Request *request)\n>> {\n>> \tSimpleFrameInfo *info = frameInfo_.find(request);\n>> @@ -957,7 +977,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n>> \t/* Complete the buffer and the request. */\n>> \tRequest *request = buffer->request();\n>> \tif (pipe->completeBuffer(request, buffer))\n>> -\t\tcompleteRequest(request);\n>> +\t\ttryCompleteRequest(request);\n>> }\n>> \n>> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n>> @@ -966,6 +986,17 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n>> \t\t\t delayedCtrls_->get(frame));\n>> }\n>> \n>> +void SimpleCameraData::metadataReady(uint32_t frame, const ControlList &metadata)\n>> +{\n>> +\tSimpleFrameInfo *info = frameInfo_.find(frame);\n>> +\tif (!info)\n>> +\t\treturn;\n>> +\n>> +\tinfo->request->metadata().merge(metadata);\n>> +\tinfo->metadataProcessed = true;\n>> +\ttryCompleteRequest(info->request);\n>> +}\n>> +\n>> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n>> {\n>> \tdelayedCtrls_->push(sensorControls);\n>> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n>> index 4a74dcb6..6baa3fec 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -55,6 +55,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n>> * \\brief A signal emitted when the statistics for IPA are ready\n>> */\n>> \n>> +/**\n>> + * \\var SoftwareIsp::metadataReady\n>> + * \\brief A signal emitted when the metadata for IPA is ready\n>> + */\n>> +\n>> /**\n>> * \\var SoftwareIsp::setSensorControls\n>> * \\brief A signal emitted when the values to write to the sensor controls are\n>> @@ -141,6 +146,10 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>> \t}\n>> \n>> \tipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);\n>> +\tipa_->metadataReady.connect(this,\n>> +\t\t\t\t [this](uint32_t frame, const ControlList &metadata) {\n>> +\t\t\t\t\t metadataReady.emit(frame, metadata);\n>> +\t\t\t\t });\n>> \tipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);\n>> \n>> \tdebayer_->moveToThread(&ispWorkerThread_);","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 DC097C3213\n\tfor ;\n\tThu, 27 Mar 2025 16:46:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D38646894F;\n\tThu, 27 Mar 2025 17:46:20 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 64B6668947\n\tfor ;\n\tThu, 27 Mar 2025 17:46:18 +0100 (CET)","from mail-ed1-f70.google.com (mail-ed1-f70.google.com\n\t[209.85.208.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-672-OkXwMErNNPKebFjdUpU0Cw-1; Thu, 27 Mar 2025 12:46:15 -0400","by mail-ed1-f70.google.com with SMTP id\n\t4fb4d7f45d1cf-5e6caac1488so1265269a12.0\n\tfor ;\n\tThu, 27 Mar 2025 09:46:15 -0700 (PDT)","from mzamazal-thinkpadp1gen7.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5edc16d5077sm65866a12.32.2025.03.27.09.46.11\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 27 Mar 2025 09:46:12 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"FZ1/Jced\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1743093977;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=3IrXJdEdkEbHu7oNszK80gjIwTpCrHpQa0UTef6KRGI=;\n\tb=FZ1/JcedE2KKtJvpEy2VvOLugP319ilSMnGLWdtTpurj+hElFSmtN6WruQf9ZzqrzuD1fV\n\t3EPHZCBb51KHnE573hGQY9VgAR+HPG1CE8xJx/T9aQTtITsPwn8O52pWaeI0hJPeTeL2Tj\n\tfQD4nhGBvS8TZLwJfMbRqncs/6F+hwQ=","X-MC-Unique":"OkXwMErNNPKebFjdUpU0Cw-1","X-Mimecast-MFC-AGG-ID":"OkXwMErNNPKebFjdUpU0Cw_1743093975","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1743093974; x=1743698774;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=3IrXJdEdkEbHu7oNszK80gjIwTpCrHpQa0UTef6KRGI=;\n\tb=S99SdQDSZjiDLc0MaW3uQ5svXv/6IbhbySHYscj2UrTap0Jrchy3ygI5tiv0GQWyTY\n\tCdxKSMYtySYW8mWOXBe8AZh3iXlCHDIk5R3SqClnbfPA3IJ6ZoozCTcEHstLk3wGkK4p\n\tqJ6YYcFEu3lBQnRPF8uuZHzmuUQFD3z6sRVyFH3SMX938JCH0CwyNbqU03e13zoTnGG+\n\twI4voC8jC5AcfoFSptsJa0ADxi0/ut+04pBJotQJGQ//JN+GY3bQ6jhrqKnEeCNiEbM7\n\tud1SJQOjO0y65yYv54El2SBfmIQD+gGJuK3PgfrGuUHLXEt7+yLaC9wc+/AN8GUdrNkP\n\t765Q==","X-Gm-Message-State":"AOJu0YwqFwxPZOnvgDBSSL4M+1Hq6KZn3n4ix2HSvAVnVPDrL7gH116q\n\tRevndxehba3N0LOo9hcfPO3L0BfjzqJzd+KSQiJ78digZylc0HKn6wK5aMlqGY5tThHIbrUlE/9\n\tKBtFL88xxHEafNgjjtYSjzXXLpTm0BwgQ0u64vw+6vU8RfLTmWrb/X6IB9m511JDFoKxPfzCMOt\n\tkqz+hlYA==","X-Gm-Gg":"ASbGncv8HbPirbThe/WRjJNRz8GaLcbuWkFHS/KQCCiN8r2UnSVKxEtrhtqLfF0BkB2\n\tGD4PzAxdf13IDoYyqtQwbwsuGUeGvu82Y54O/yZ3fAXZVm5I3b3MTMnUVPgn48v99L7esVXJIsO\n\tQ6IMt8tdOT7NXZCitB1NJDCesHX7EeuoHWWDmpKEQDOwXSV5yGgYEPFzhaTVxTIw8vfLBpHV2Cg\n\tkVthsVLjht/itjHrcqpXKuGQXfwLq+hd/SNGFMUzzMRL77g281NiO/IJsssBz/ZUR+BV/exL6lj\n\tVFbA273yzHbQI3H8AEPigTnIQ9PmBvPkLmvoNVL+m+RRYs2GCGmhOdOfTWi+lX8S2XZc","X-Received":["by 2002:a05:6402:5205:b0:5ec:6feb:5742 with SMTP id\n\t4fb4d7f45d1cf-5edbf51dd7amr1058279a12.16.1743093974032; \n\tThu, 27 Mar 2025 09:46:14 -0700 (PDT)","by 2002:a05:6402:5205:b0:5ec:6feb:5742 with SMTP id\n\t4fb4d7f45d1cf-5edbf51dd7amr1058231a12.16.1743093973513; \n\tThu, 27 Mar 2025 09:46:13 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IEDjaxusDSL02Q8wMuFk8X+1gwrLlWg40s9U/Oa2t6l2OqUNYZqn1WlCoYn+NSk4qjlWlNrMw==","From":"Milan Zamazal ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t","Subject":"Re: [PATCH v5 2/6] ipa: simple: softisp: Extend to pass metadata","In-Reply-To":"<20250326230854.GC21053@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Thu, 27 Mar 2025 01:08:54 +0200\")","References":"<20250326124856.75709-1-mzamazal@redhat.com>\n\t<20250326124856.75709-3-mzamazal@redhat.com>\n\t<20250326230854.GC21053@pendragon.ideasonboard.com>","Date":"Thu, 27 Mar 2025 17:46:10 +0100","Message-ID":"<857c4a2zhp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"P9a_SqIutu-KoKIpnwLk0xpTsns_T6R-NGw1mcY25hQ_1743093975","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33752,"web_url":"https://patchwork.libcamera.org/comment/33752/","msgid":"<20250327234759.GO4861@pendragon.ideasonboard.com>","date":"2025-03-27T23:47:59","subject":"Re: [PATCH v5 2/6] ipa: simple: softisp: Extend to pass metadata","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nOn Thu, Mar 27, 2025 at 05:46:10PM +0100, Milan Zamazal wrote:\n> Laurent Pinchart writes:\n> > On Wed, Mar 26, 2025 at 01:48:51PM +0100, Milan Zamazal wrote:\n> >> From: Kieran Bingham \n> >> \n> >> Extend the Simple IPA IPC to support returning a metadata ControlList\n> >> when the process call has completed.\n> >> \n> >> A new signal from the IPA is introduced to report the metadata,\n> >> similarly to what the hardware pipelines do.\n> >> \n> >> Merge the metadata reported by the ISP into any completing request to\n> >> provide to the application. Completion of a request is delayed until\n> >> this is done; this doesn't apply to canceled requests.\n> >> \n> >> Signed-off-by: Kieran Bingham \n> >> Signed-off-by: Milan Zamazal \n> >> ---\n> >> .../internal/software_isp/software_isp.h | 2 ++\n> >> include/libcamera/ipa/soft.mojom | 1 +\n> >> src/ipa/simple/soft_simple.cpp | 7 +---\n> >> src/libcamera/pipeline/simple/simple.cpp | 33 ++++++++++++++++++-\n> >> src/libcamera/software_isp/software_isp.cpp | 9 +++++\n> >> 5 files changed, 45 insertions(+), 7 deletions(-)\n> >> \n> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> >> index 0026cec2..1ee37dc7 100644\n> >> --- a/include/libcamera/internal/software_isp/software_isp.h\n> >> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> >> @@ -85,12 +85,14 @@ public:\n> >> \tSignal inputBufferReady;\n> >> \tSignal outputBufferReady;\n> >> \tSignal ispStatsReady;\n> >> +\tSignal metadataReady;\n> >> \tSignal setSensorControls;\n> >> \n> >> private:\n> >> \tvoid saveIspParams();\n> >> \tvoid setSensorCtrls(const ControlList &sensorControls);\n> >> \tvoid statsReady(uint32_t frame, uint32_t bufferId);\n> >> +\tvoid saveMetadata(uint32_t frame, const ControlList &metadata);\n> >> \tvoid inputReady(FrameBuffer *input);\n> >> \tvoid outputReady(FrameBuffer *output);\n> >> \n> >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> >> index ede74413..a8e6ecda 100644\n> >> --- a/include/libcamera/ipa/soft.mojom\n> >> +++ b/include/libcamera/ipa/soft.mojom\n> >> @@ -33,4 +33,5 @@ interface IPASoftInterface {\n> >> interface IPASoftEventInterface {\n> >> \tsetSensorControls(libcamera.ControlList sensorControls);\n> >> \tsetIspParams();\n> >> +\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> >> };\n> >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> >> index a87c6cdd..4ef67c43 100644\n> >> --- a/src/ipa/simple/soft_simple.cpp\n> >> +++ b/src/ipa/simple/soft_simple.cpp\n> >> @@ -299,15 +299,10 @@ void IPASoftSimple::processStats(const uint32_t frame,\n> >> \tint32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get();\n> >> \tframeContext.sensor.gain = camHelper_ ? camHelper_->gain(again) : again;\n> >> \n> >> -\t/*\n> >> -\t * Software ISP currently does not produce any metadata. Use an empty\n> >> -\t * ControlList for now.\n> >> -\t *\n> >> -\t * \\todo Implement proper metadata handling\n> >> -\t */\n> >> \tControlList metadata(controls::controls);\n> >> \tfor (auto const &algo : algorithms())\n> >> \t\talgo->process(context_, frame, frameContext, stats_, metadata);\n> >> +\tmetadataReady.emit(frame, metadata);\n> >> \n> >> \t/* Sanity check */\n> >> \tif (!sensorControls.contains(V4L2_CID_EXPOSURE) ||\n> >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> >> index 2281c0a6..f34df926 100644\n> >> --- a/src/libcamera/pipeline/simple/simple.cpp\n> >> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> >> @@ -184,6 +184,7 @@ class SimplePipelineHandler;\n> >> struct SimpleFrameInfo {\n> >> \tuint32_t frame;\n> >> \tRequest *request;\n> >> +\tbool metadataProcessed;\n> >> };\n> >> \n> >> class SimpleFrames\n> >> @@ -207,6 +208,7 @@ void SimpleFrames::create(Request *request)\n> >> \tASSERT(inserted);\n> >> \tit->second.frame = frame;\n> >> \tit->second.request = request;\n> >> +\tit->second.metadataProcessed = false;\n> >> }\n> >> \n> >> void SimpleFrames::destroy(uint32_t frame)\n> >> @@ -357,11 +359,13 @@ private:\n> >> \tvoid tryPipeline(unsigned int code, const Size &size);\n> >> \tstatic std::vector routedSourcePads(MediaPad *sink);\n> >> \n> >> +\tvoid tryCompleteRequest(Request *request);\n> >> \tvoid completeRequest(Request *request);\n> >> \tvoid conversionInputDone(FrameBuffer *buffer);\n> >> \tvoid conversionOutputDone(FrameBuffer *buffer);\n> >> \n> >> \tvoid ispStatsReady(uint32_t frame, uint32_t bufferId);\n> >> +\tvoid metadataReady(uint32_t frame, const ControlList &metadata);\n> >> \tvoid setSensorControls(const ControlList &sensorControls);\n> >> };\n> >> \n> >> @@ -600,6 +604,7 @@ int SimpleCameraData::init()\n> >> \t\t\tswIsp_->inputBufferReady.connect(this, &SimpleCameraData::conversionInputDone);\n> >> \t\t\tswIsp_->outputBufferReady.connect(this, &SimpleCameraData::conversionOutputDone);\n> >> \t\t\tswIsp_->ispStatsReady.connect(this, &SimpleCameraData::ispStatsReady);\n> >> +\t\t\tswIsp_->metadataReady.connect(this, &SimpleCameraData::metadataReady);\n> >> \t\t\tswIsp_->setSensorControls.connect(this, &SimpleCameraData::setSensorControls);\n> >> \t\t}\n> >> \t}\n> >> @@ -932,6 +937,21 @@ void SimpleCameraData::clearIncompleteRequests()\n> >> \t}\n> >> }\n> >> \n> >> +void SimpleCameraData::tryCompleteRequest(Request *request)\n> >> +{\n> >> +\tif (request->hasPendingBuffers())\n> >> +\t\treturn;\n> >> +\n> >> +\tSimpleFrameInfo *info = frameInfo_.find(request);\n> >> +\tif (!info)\n> >> +\t\treturn;\n> >> +\n> >> +\tif (!info->metadataProcessed)\n> >> +\t\treturn;\n> >> +\n> >> +\tcompleteRequest(request);\n> >> +}\n> >\n> > I may not have expressed my thoughts correctly in the review of v3. What\n> > I proposed was having a single function, named tryCompleteRequest(), to\n> > replace your current completeRequest() implementation. The function\n> > would ensure that all the pieces of data necessary to complete the\n> > request are there, and then proceed to complete it.\n> >\n> > As far as I can tell, the only thing you will need to change in addition\n> > to merging the completeRequest() function into tryCompleteRequest() \n> \n> frameInfo_ is used only with software ISP so it would have been handled\n> conditionally too. I think this is where we misunderstood each other.\n> \n> Actually, a bug was introduced in v4 of the preceding patch, in that\n> frameInfo_ is checked in completeRequest whether it should be present\n> (with swisp) or not (without swisp).\n> \n> Let's try to stop the increasing confusion.\n> \n> > is to set metadataProcessed to true when the pipeline is not expected\n> > to produced metadata (that is, when no soft ISP is used). This could\n> > be done for instance when creating the SimpleFrameInfo entry. \n> \n> OK, let's always create SimpleFrameInfo for a request, whether software\n> ISP is used or not. This should also avoid further confusion once\n> multiple streams are used (in the patches for raw streams). And instead\n> of setting metadataProcessed to true when no metadata is expected, I'd\n> introduce and set an additional metadataRequired; then it is clearer\n> what is what and it can be handled in tryCompleteRequest. Now a single\n> completion method, tryCompleteRequest, makes sense.\n> \n> > You should also double-check the cancellation paths, to set\n> > metadataProcessed to true if the frame is cancelled before being\n> > processed by the soft ISP.\n> \n> Not pretty but it is required only in a single place and not much\n> confusing there, so OK. At least until we possibly add more stuff to\n> the frame info and the accompanying completion checks.\n> \n> I hope we are now getting in sync with each other.\n\nI believe so :-) Thanks for the clarifications. I'll review the latest\nversion of the series.\n\n> >> +\n> >> void SimpleCameraData::completeRequest(Request *request)\n> >> {\n> >> \tSimpleFrameInfo *info = frameInfo_.find(request);\n> >> @@ -957,7 +977,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> >> \t/* Complete the buffer and the request. */\n> >> \tRequest *request = buffer->request();\n> >> \tif (pipe->completeBuffer(request, buffer))\n> >> -\t\tcompleteRequest(request);\n> >> +\t\ttryCompleteRequest(request);\n> >> }\n> >> \n> >> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> >> @@ -966,6 +986,17 @@ void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> >> \t\t\t delayedCtrls_->get(frame));\n> >> }\n> >> \n> >> +void SimpleCameraData::metadataReady(uint32_t frame, const ControlList &metadata)\n> >> +{\n> >> +\tSimpleFrameInfo *info = frameInfo_.find(frame);\n> >> +\tif (!info)\n> >> +\t\treturn;\n> >> +\n> >> +\tinfo->request->metadata().merge(metadata);\n> >> +\tinfo->metadataProcessed = true;\n> >> +\ttryCompleteRequest(info->request);\n> >> +}\n> >> +\n> >> void SimpleCameraData::setSensorControls(const ControlList &sensorControls)\n> >> {\n> >> \tdelayedCtrls_->push(sensorControls);\n> >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> >> index 4a74dcb6..6baa3fec 100644\n> >> --- a/src/libcamera/software_isp/software_isp.cpp\n> >> +++ b/src/libcamera/software_isp/software_isp.cpp\n> >> @@ -55,6 +55,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> >> * \\brief A signal emitted when the statistics for IPA are ready\n> >> */\n> >> \n> >> +/**\n> >> + * \\var SoftwareIsp::metadataReady\n> >> + * \\brief A signal emitted when the metadata for IPA is ready\n> >> + */\n> >> +\n> >> /**\n> >> * \\var SoftwareIsp::setSensorControls\n> >> * \\brief A signal emitted when the values to write to the sensor controls are\n> >> @@ -141,6 +146,10 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n> >> \t}\n> >> \n> >> \tipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);\n> >> +\tipa_->metadataReady.connect(this,\n> >> +\t\t\t\t [this](uint32_t frame, const ControlList &metadata) {\n> >> +\t\t\t\t\t metadataReady.emit(frame, metadata);\n> >> +\t\t\t\t });\n> >> \tipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);\n> >> \n> >> \tdebayer_->moveToThread(&ispWorkerThread_);","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 9CA6FC3213\n\tfor ;\n\tThu, 27 Mar 2025 23:48:25 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E7E09614E7;\n\tFri, 28 Mar 2025 00:48:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7EE62614E7\n\tfor ;\n\tFri, 28 Mar 2025 00:48:23 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 511923DA;\n\tFri, 28 Mar 2025 00:46:34 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"VDhuMgES\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743119194;\n\tbh=IEdQNI1ngYDLEBlhBA7vBrLXB2rsLa3GnOn9WGK/LOY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=VDhuMgESj3FPAjGdYP3nlYInszo1JI681ib1DM6LlFsuQC3hOw0yIrHq0DCypl8Pl\n\tJXt6wiOnaJTsI27/uH07t3nNyLRM3QaIWxOgVh5c++3Sxt2h4f18Yi9md6l/ih93kh\n\ty4groVZ1mSRLrGeG+R5CLs/MfFEuYsgosWkqrzmE=","Date":"Fri, 28 Mar 2025 01:47:59 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v5 2/6] ipa: simple: softisp: Extend to pass metadata","Message-ID":"<20250327234759.GO4861@pendragon.ideasonboard.com>","References":"<20250326124856.75709-1-mzamazal@redhat.com>\n\t<20250326124856.75709-3-mzamazal@redhat.com>\n\t<20250326230854.GC21053@pendragon.ideasonboard.com>\n\t<857c4a2zhp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<857c4a2zhp.fsf@mzamazal-thinkpadp1gen7.tpbc.csb>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]