[{"id":33753,"web_url":"https://patchwork.libcamera.org/comment/33753/","msgid":"<20250328002025.GC22295@pendragon.ideasonboard.com>","date":"2025-03-28T00:20:25","subject":"Re: [PATCH v6 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 Thu, Mar 27, 2025 at 07:59:40PM +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> Reviewed-by: Kieran Bingham \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 | 48 ++++++++++++++-----\n> src/libcamera/software_isp/software_isp.cpp | 9 ++++\n> 5 files changed, 49 insertions(+), 18 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\nThis function isn't implemented, probably a leftover ?\n\nReviewed-by: Laurent Pinchart \n\nThe series passed CI, I've dropped the saveMetadata() function and\npushed. One more milestone merged for the soft ISP :-) Thanks for all\nyour work.\n\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 004a8d53..fd0ccdca 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -182,19 +182,21 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n> class SimplePipelineHandler;\n> \n> struct SimpleFrameInfo {\n> -\tSimpleFrameInfo(uint32_t f, Request *r)\n> -\t\t: frame(f), request(r)\n> +\tSimpleFrameInfo(uint32_t f, Request *r, bool m)\n> +\t\t: frame(f), request(r), metadataRequired(m), metadataProcessed(false)\n> \t{\n> \t}\n> \n> \tuint32_t frame;\n> \tRequest *request;\n> +\tbool metadataRequired;\n> +\tbool metadataProcessed;\n> };\n> \n> class SimpleFrames\n> {\n> public:\n> -\tvoid create(Request *request);\n> +\tvoid create(Request *request, bool metadataRequested);\n> \tvoid destroy(uint32_t frame);\n> \tvoid clear();\n> \n> @@ -204,10 +206,10 @@ private:\n> \tstd::map frameInfo_;\n> };\n> \n> -void SimpleFrames::create(Request *request)\n> +void SimpleFrames::create(Request *request, bool metadataRequired)\n> {\n> \tconst uint32_t frame = request->sequence();\n> -\tauto [it, inserted] = frameInfo_.try_emplace(frame, frame, request);\n> +\tauto [it, inserted] = frameInfo_.try_emplace(frame, frame, request, metadataRequired);\n> \tASSERT(inserted);\n> }\n> \n> @@ -347,11 +349,12 @@ private:\n> \tvoid tryPipeline(unsigned int code, const Size &size);\n> \tstatic std::vector routedSourcePads(MediaPad *sink);\n> \n> -\tvoid completeRequest(Request *request);\n> +\tvoid tryCompleteRequest(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> @@ -590,6 +593,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> @@ -835,7 +839,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> \t\t\t/* No conversion, just complete the request. */\n> \t\t\tRequest *request = buffer->request();\n> \t\t\tpipe->completeBuffer(request, buffer);\n> -\t\t\tcompleteRequest(request);\n> +\t\t\ttryCompleteRequest(request);\n> \t\t\treturn;\n> \t\t}\n> \n> @@ -853,7 +857,10 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> \t\tconst RequestOutputs &outputs = conversionQueue_.front();\n> \t\tfor (auto &[stream, buf] : outputs.outputs)\n> \t\t\tpipe->completeBuffer(outputs.request, buf);\n> -\t\tcompleteRequest(outputs.request);\n> +\t\tSimpleFrameInfo *info = frameInfo_.find(outputs.request->sequence());\n> +\t\tif (info)\n> +\t\t\tinfo->metadataRequired = false;\n> +\t\ttryCompleteRequest(outputs.request);\n> \t\tconversionQueue_.pop();\n> \n> \t\treturn;\n> @@ -911,7 +918,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> \n> \t/* Otherwise simply complete the request. */\n> \tpipe->completeBuffer(request, buffer);\n> -\tcompleteRequest(request);\n> +\ttryCompleteRequest(request);\n> }\n> \n> void SimpleCameraData::clearIncompleteRequests()\n> @@ -922,14 +929,20 @@ void SimpleCameraData::clearIncompleteRequests()\n> \t}\n> }\n> \n> -void SimpleCameraData::completeRequest(Request *request)\n> +void SimpleCameraData::tryCompleteRequest(Request *request)\n> {\n> +\tif (request->hasPendingBuffers())\n> +\t\treturn;\n> +\n> \tSimpleFrameInfo *info = frameInfo_.find(request->sequence());\n> \tif (!info) {\n> \t\t/* Something is really wrong, let's return. */\n> \t\treturn;\n> \t}\n> \n> +\tif (info->metadataRequired && !info->metadataProcessed)\n> +\t\treturn;\n> +\n> \tframeInfo_.destroy(info->frame);\n> \tpipe()->completeRequest(request);\n> }\n> @@ -947,7 +960,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> @@ -956,6 +969,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> @@ -1489,7 +1513,7 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> \t\t}\n> \t}\n> \n> -\tdata->frameInfo_.create(request);\n> +\tdata->frameInfo_.create(request, !!data->swIsp_);\n> \tif (data->useConversion_) {\n> \t\tdata->conversionQueue_.push({ request, std::move(buffers) });\n> \t\tif (data->swIsp_)\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 1612BC3213\n\tfor ;\n\tFri, 28 Mar 2025 00:20:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6222068971;\n\tFri, 28 Mar 2025 01:20:50 +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 9F075614E7\n\tfor ;\n\tFri, 28 Mar 2025 01:20:49 +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 7174F3DA;\n\tFri, 28 Mar 2025 01:19:00 +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=\"XwgnWzhv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743121140;\n\tbh=87P6GiTpi43NOBpu59HlWhrZcEjLxTmOMfi+WfIeW7s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XwgnWzhvSDe6QRYJoyx3qx50juI0U85beYEmSSuLItoOoEUgKZ3A5cNDiRc7JSh+6\n\tVWMTr7p3Q4HR8dq7V9uQnvvJPM1/6T1p7ejfZdVTAAvmzGwx6z2tD8c/tlGwinChkf\n\tJ4rpl60loQjSwXIjRPBUbCVoa1XtsXZJm05UiIk4=","Date":"Fri, 28 Mar 2025 02:20:25 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 2/6] ipa: simple: softisp: Extend to pass metadata","Message-ID":"<20250328002025.GC22295@pendragon.ideasonboard.com>","References":"<20250327185945.159481-1-mzamazal@redhat.com>\n\t<20250327185945.159481-3-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250327185945.159481-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\" "}}]