[{"id":33191,"web_url":"https://patchwork.libcamera.org/comment/33191/","msgid":"<20250127065659.GA11289@pendragon.ideasonboard.com>","date":"2025-01-27T06:56:59","subject":"Re: [PATCH v3 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 Mon, Jan 13, 2025 at 02:34:01PM +0100, Milan Zamazal wrote:\n> From: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 <kieran.bingham@ideasonboard.com>\n> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>\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      | 35 +++++++++++++++----\n>  src/libcamera/software_isp/software_isp.cpp   | 11 ++++++\n>  5 files changed, 43 insertions(+), 13 deletions(-)\n> \n> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n> index d51b03fd..3d248aba 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -83,12 +83,14 @@ public:\n>  \tSignal<FrameBuffer *> inputBufferReady;\n>  \tSignal<FrameBuffer *> outputBufferReady;\n>  \tSignal<uint32_t, uint32_t> ispStatsReady;\n> +\tSignal<uint32_t, const ControlList &> metadataReady;\n>  \tSignal<const ControlList &> 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 d52e6f1a..934a6fd1 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 b26e4e15..944c644e 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -295,15 +295,10 @@ void IPASoftSimple::processStats(const uint32_t frame,\n>  \tint32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\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 123179b6..bb0ee23e 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,12 @@ private:\n>  \tvoid tryPipeline(unsigned int code, const Size &size);\n>  \tstatic std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);\n>  \n> -\tvoid completeRequest(Request *request);\n> +\tvoid completeRequest(Request *request, bool checkCompleted);\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> @@ -614,6 +617,7 @@ int SimpleCameraData::init()\n>  \t\t\t});\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> @@ -859,7 +863,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\tcompleteRequest(request, false);\n>  \t\t\treturn;\n>  \t\t}\n>  \n> @@ -877,7 +881,7 @@ 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\tcompleteRequest(outputs.request, false);\n>  \t\tconversionQueue_.pop();\n>  \n>  \t\treturn;\n> @@ -935,7 +939,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>  \n>  \t/* Otherwise simply complete the request. */\n>  \tpipe->completeBuffer(request, buffer);\n> -\tcompleteRequest(request);\n> +\tcompleteRequest(request, false);\n>  }\n>  \n>  void SimpleCameraData::clearIncompleteRequests()\n> @@ -946,11 +950,17 @@ void SimpleCameraData::clearIncompleteRequests()\n>  \t}\n>  }\n>  \n> -void SimpleCameraData::completeRequest(Request *request)\n> +void SimpleCameraData::completeRequest(Request *request, bool checkCompleted)\n\nI would name the function tryCompleteRequest() to match the rkisp1\npipeline handler.\n\nWhy do you need a checkCompleted argument ? This seems fairly error\nprone. Can't you follow the same logic as rkisp1 ?\n\nNote that we plan to completely refactor the request completion logic,\nbut that will likely be done after this series gets merged.\n\n>  {\n> +\tif (checkCompleted && request->hasPendingBuffers())\n> +\t\treturn;\n> +\n>  \tSimpleFrameInfo *info = frameInfo_.find(request);\n> -\tif (info)\n> +\tif (info) {\n> +\t\tif (checkCompleted && !info->metadataProcessed)\n> +\t\t\treturn;\n>  \t\tframeInfo_.destroy(info->frame);\n> +\t}\n>  \tpipe()->completeRequest(request);\n>  }\n>  \n> @@ -967,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\tcompleteRequest(request, true);\n>  }\n>  \n>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> @@ -976,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> +\tcompleteRequest(info->request, true);\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 2bea64d9..bd4c25cc 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -51,6 +51,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 are ready\n> + */\n> +\n>  /**\n>   * \\var SoftwareIsp::setSensorControls\n>   * \\brief A signal emitted when the values to write to the sensor controls are\n> @@ -136,6 +141,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>  \t}\n>  \n>  \tipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);\n> +\tipa_->metadataReady.connect(this, &SoftwareIsp::saveMetadata);\n>  \tipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);\n>  \n>  \tdebayer_->moveToThread(&ispWorkerThread_);\n> @@ -357,6 +363,11 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)\n>  \tispStatsReady.emit(frame, bufferId);\n>  }\n>  \n> +void SoftwareIsp::saveMetadata(uint32_t frame, const ControlList &metadata)\n\nThis function is a bit of a misnomer, as it doesn't save metadata.\n\nYou could rename the function, or drop it and use a lambda when\nconnecting the signal in the SoftwareIsp constructor.\n\n> +{\n> +\tmetadataReady.emit(frame, metadata);\n> +}\n> +\n>  void SoftwareIsp::inputReady(FrameBuffer *input)\n>  {\n>  \tinputBufferReady.emit(input);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 31A70BDCBF\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 27 Jan 2025 06:57:12 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2472F6855D;\n\tMon, 27 Jan 2025 07:57:11 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 46A8161875\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 27 Jan 2025 07:57:10 +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 CBA2CAB5;\n\tMon, 27 Jan 2025 07:56:03 +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=\"HTRKnYtd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1737960963;\n\tbh=ftVmwZpSA5bfJCKQf/8CuWnM0akevRBppblm2NstheU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HTRKnYtdSgUtrORag85f+QbK4E1uURmlLol9ocrNl9JsD1s8F4HoQmJKAvuOYow6L\n\tHKbRMyePQ/hBfqOHGJsJmMljgqeMPUi9AuibjRsIsadkaVzC5p6GHWSb1HSYakCi8q\n\tinu5X1aT0wWsRsKzJG6GQ1dJv+77hPXIjmqnYFf8=","Date":"Mon, 27 Jan 2025 08:56:59 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Milan Zamazal <mzamazal@redhat.com>","Cc":"libcamera-devel@lists.libcamera.org, Kieran Bingham\n\t<kieran.bingham@ideasonboard.com>, =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?=\n\t<pobrn@protonmail.com>","Subject":"Re: [PATCH v3 2/6] ipa: simple: softisp: Extend to pass metadata","Message-ID":"<20250127065659.GA11289@pendragon.ideasonboard.com>","References":"<20250113133405.12167-1-mzamazal@redhat.com>\n\t<20250113133405.12167-3-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250113133405.12167-3-mzamazal@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":33234,"web_url":"https://patchwork.libcamera.org/comment/33234/","msgid":"<85jzach9ak.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","date":"2025-01-30T10:42:59","subject":"Re: [PATCH v3 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 <laurent.pinchart@ideasonboard.com> writes:\n\n> Hi Milan,\n>\n> Thank you for the patch.\n>\n> On Mon, Jan 13, 2025 at 02:34:01PM +0100, Milan Zamazal wrote:\n>> From: Kieran Bingham <kieran.bingham@ideasonboard.com>\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 <kieran.bingham@ideasonboard.com>\n>> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>\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      | 35 +++++++++++++++----\n>>  src/libcamera/software_isp/software_isp.cpp   | 11 ++++++\n>>  5 files changed, 43 insertions(+), 13 deletions(-)\n>> \n>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h\n>> index d51b03fd..3d248aba 100644\n>> --- a/include/libcamera/internal/software_isp/software_isp.h\n>> +++ b/include/libcamera/internal/software_isp/software_isp.h\n>> @@ -83,12 +83,14 @@ public:\n>>  \tSignal<FrameBuffer *> inputBufferReady;\n>>  \tSignal<FrameBuffer *> outputBufferReady;\n>>  \tSignal<uint32_t, uint32_t> ispStatsReady;\n>> +\tSignal<uint32_t, const ControlList &> metadataReady;\n>>  \tSignal<const ControlList &> 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 d52e6f1a..934a6fd1 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 b26e4e15..944c644e 100644\n>> --- a/src/ipa/simple/soft_simple.cpp\n>> +++ b/src/ipa/simple/soft_simple.cpp\n>> @@ -295,15 +295,10 @@ void IPASoftSimple::processStats(const uint32_t frame,\n>>  \tint32_t again = sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>();\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 123179b6..bb0ee23e 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,12 @@ private:\n>>  \tvoid tryPipeline(unsigned int code, const Size &size);\n>>  \tstatic std::vector<const MediaPad *> routedSourcePads(MediaPad *sink);\n>>  \n>> -\tvoid completeRequest(Request *request);\n>> +\tvoid completeRequest(Request *request, bool checkCompleted);\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>> @@ -614,6 +617,7 @@ int SimpleCameraData::init()\n>>  \t\t\t});\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>> @@ -859,7 +863,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\tcompleteRequest(request, false);\n>>  \t\t\treturn;\n>>  \t\t}\n>>  \n>> @@ -877,7 +881,7 @@ 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\tcompleteRequest(outputs.request, false);\n>>  \t\tconversionQueue_.pop();\n>>  \n>>  \t\treturn;\n>> @@ -935,7 +939,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n>>  \n>>  \t/* Otherwise simply complete the request. */\n>>  \tpipe->completeBuffer(request, buffer);\n>> -\tcompleteRequest(request);\n>> +\tcompleteRequest(request, false);\n>>  }\n>>  \n>>  void SimpleCameraData::clearIncompleteRequests()\n>> @@ -946,11 +950,17 @@ void SimpleCameraData::clearIncompleteRequests()\n>>  \t}\n>>  }\n>>  \n>> -void SimpleCameraData::completeRequest(Request *request)\n>> +void SimpleCameraData::completeRequest(Request *request, bool checkCompleted)\n>\n> I would name the function tryCompleteRequest() to match the rkisp1\n> pipeline handler.\n>\n> Why do you need a checkCompleted argument ? This seems fairly error\n> prone. \n\nProbably just to save some lines.\n\n> Can't you follow the same logic as rkisp1 ?\n\nI'll split the function to two, tryCompleteRequest and completeRequest,\nit should help.\n\n> Note that we plan to completely refactor the request completion logic,\n> but that will likely be done after this series gets merged.\n>\n>>  {\n>> +\tif (checkCompleted && request->hasPendingBuffers())\n>> +\t\treturn;\n>> +\n>>  \tSimpleFrameInfo *info = frameInfo_.find(request);\n>> -\tif (info)\n>> +\tif (info) {\n>> +\t\tif (checkCompleted && !info->metadataProcessed)\n>> +\t\t\treturn;\n>>  \t\tframeInfo_.destroy(info->frame);\n>> +\t}\n>>  \tpipe()->completeRequest(request);\n>>  }\n>>  \n>> @@ -967,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\tcompleteRequest(request, true);\n>>  }\n>>  \n>>  void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n>> @@ -976,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>> +\tcompleteRequest(info->request, true);\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 2bea64d9..bd4c25cc 100644\n>> --- a/src/libcamera/software_isp/software_isp.cpp\n>> +++ b/src/libcamera/software_isp/software_isp.cpp\n>> @@ -51,6 +51,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 are ready\n>> + */\n>> +\n>>  /**\n>>   * \\var SoftwareIsp::setSensorControls\n>>   * \\brief A signal emitted when the values to write to the sensor controls are\n>> @@ -136,6 +141,7 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor,\n>>  \t}\n>>  \n>>  \tipa_->setIspParams.connect(this, &SoftwareIsp::saveIspParams);\n>> +\tipa_->metadataReady.connect(this, &SoftwareIsp::saveMetadata);\n>>  \tipa_->setSensorControls.connect(this, &SoftwareIsp::setSensorCtrls);\n>>  \n>>  \tdebayer_->moveToThread(&ispWorkerThread_);\n>> @@ -357,6 +363,11 @@ void SoftwareIsp::statsReady(uint32_t frame, uint32_t bufferId)\n>>  \tispStatsReady.emit(frame, bufferId);\n>>  }\n>>  \n>> +void SoftwareIsp::saveMetadata(uint32_t frame, const ControlList &metadata)\n>\n> This function is a bit of a misnomer, as it doesn't save metadata.\n>\n> You could rename the function, or drop it and use a lambda when\n> connecting the signal in the SoftwareIsp constructor.\n\nOK, I'll use lambda to avoid naming issues. :-)\n\n>> +{\n>> +\tmetadataReady.emit(frame, metadata);\n>> +}\n>> +\n>>  void SoftwareIsp::inputReady(FrameBuffer *input)\n>>  {\n>>  \tinputBufferReady.emit(input);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 1498ABD808\n\tfor <parsemail@patchwork.libcamera.org>;\n\tThu, 30 Jan 2025 10:43:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2A9DE68563;\n\tThu, 30 Jan 2025 11:43:09 +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 8A32B61876\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 30 Jan 2025 11:43:07 +0100 (CET)","from mail-ed1-f72.google.com (mail-ed1-f72.google.com\n\t[209.85.208.72]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-604-ADwlm7SpOSip6QjEwCGZXQ-1; Thu, 30 Jan 2025 05:43:02 -0500","by mail-ed1-f72.google.com with SMTP id\n\t4fb4d7f45d1cf-5d9fcb4a122so635206a12.0\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 30 Jan 2025 02:43:02 -0800 (PST)","from mzamazal-thinkpadp1gen3.tpbc.csb\n\t(ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t4fb4d7f45d1cf-5dc724c93d9sm865966a12.68.2025.01.30.02.42.59\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tThu, 30 Jan 2025 02:42:59 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"cE4+dGNC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1738233786;\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=XP53Ht7cFNI/vHoA2IB5NaTRuPlbId6j/R9F3T/DvV4=;\n\tb=cE4+dGNCqDdZx7YMXcf1oUEeT6iyyAzBxNrtgVpvopBtqwoNxihFlMx+EVVkmEFn+g7bah\n\t6tijx9+MISHYv+lEZlXCh7II8eVEbxmuyqcW+wBgVstwbPTDU0WSw8tKQn7KeYqn7piyMy\n\tbZfL9ocaf5KpZ3cIIWh6mdcWdz5ItOk=","X-MC-Unique":"ADwlm7SpOSip6QjEwCGZXQ-1","X-Mimecast-MFC-AGG-ID":"ADwlm7SpOSip6QjEwCGZXQ","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1738233781; x=1738838581;\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=XP53Ht7cFNI/vHoA2IB5NaTRuPlbId6j/R9F3T/DvV4=;\n\tb=ba08bYt9IdWHiQjJFNbvHctIT6mgrQZtbXmQMgZjK+7TlkW/wssYblVqSvkFpI0LyN\n\tGWqNAhzwd601mTjzx6ufmjXvFxVP5k6GRsVqrJk+oUXyGK2U/P/vd4EMSBHzvlMQNMnJ\n\tJOUF8vTSQDlxkOzIVfJDqu+OPg1IUwVF/CZz2Cx7eFUt4l2OuXKK49DesH2l3CIm40us\n\t4lgUQBTik0Jg1jjks7KVw2gIezn3LgTmCnm8pwan1/6g/fZJLsgEi/TkZw8jvapaAaHL\n\t33/w/S5rX2m/eZxNDVCBfkcjb+ZCmu/1gviobYtgamOa9tG/itqfKBH326K1V3CxY7/9\n\tefaw==","X-Gm-Message-State":"AOJu0YwZJGoW6+twk7wh0L5iLcqCbSIGQlLpXwQsRtuXiMuqGLlKQoRS\n\t1FvjQDlQ52GJG8PmHkZyOLheowJ4hynJl6ubedsi+Sr9/LNPCBDBNlEmUILOM90uP+3/Xs1D9W4\n\t8mLLNCLAD5S3eI/iLn5fMQlw+tW10z6WS8ztGxLF3CKKkOSF05M08IEE28jHUZqqlqJjv25c=","X-Gm-Gg":"ASbGncvzbs3CZJfdpuX/OgKLCCxGItreIzn9XpV+7E/HdBg6j0ehed+VFfa1i8T2t+p\n\tmJ2f/OOM15eE1NKP8qnFqzGVOqxoE104hBMKmcYDokNUEllQG1mVU+HuJKgccZoM/6LT8US796L\n\tjYtm8zNN8CkT5rcRcv0qJAdWdPthEWjbJuVZdASb6Mfe88rHhTtsiexEMLu27k1E1aD4/UFJBsl\n\t8vaBRm5YNTLZYZA2pGnAX6XgdfhWjZGcZFEYJ8W0vKGo8whzszF0hKzHFVr7ni+LsB6CY3bK2nT\n\tySm7ueX6aoAg+KMekF2qlyJqS4516gpyUQ00qcF3cjZhg8cRI+JIGEYb+Q==","X-Received":["by 2002:a05:6402:2390:b0:5dc:72e1:63ee with SMTP id\n\t4fb4d7f45d1cf-5dc72e1647cmr1212973a12.6.1738233781452; \n\tThu, 30 Jan 2025 02:43:01 -0800 (PST)","by 2002:a05:6402:2390:b0:5dc:72e1:63ee with SMTP id\n\t4fb4d7f45d1cf-5dc72e1647cmr1212958a12.6.1738233780719; \n\tThu, 30 Jan 2025 02:43:00 -0800 (PST)"],"X-Google-Smtp-Source":"AGHT+IEX5GtZVPKlo+LFgjb5wXPE5HOwml1JJlm33bR16EneeYwRxXlxaow3DV5CnQ6sUuSnpUWugQ==","From":"Milan Zamazal <mzamazal@redhat.com>","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham <kieran.bingham@ideasonboard.com>, =?utf-8?q?Barnab?=\n\t=?utf-8?b?w6FzIFDFkWN6ZQ==?=  <pobrn@protonmail.com>","Subject":"Re: [PATCH v3 2/6] ipa: simple: softisp: Extend to pass metadata","In-Reply-To":"<20250127065659.GA11289@pendragon.ideasonboard.com> (Laurent\n\tPinchart's message of \"Mon, 27 Jan 2025 08:56:59 +0200\")","References":"<20250113133405.12167-1-mzamazal@redhat.com>\n\t<20250113133405.12167-3-mzamazal@redhat.com>\n\t<20250127065659.GA11289@pendragon.ideasonboard.com>","Date":"Thu, 30 Jan 2025 11:42:59 +0100","Message-ID":"<85jzach9ak.fsf@mzamazal-thinkpadp1gen3.tpbc.csb>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-MFC-PROC-ID":"ke82t2SfNg1EVmvOnhzDfMCmTTQ9dxTto01USwME8wI_1738233781","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":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]