[{"id":30018,"web_url":"https://patchwork.libcamera.org/comment/30018/","msgid":"<171866729088.804094.10949552298675636369@ping.linuxembedded.co.uk>","date":"2024-06-17T23:34:50","subject":"Re: [PATCH 1/3] 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 Kieran Bingham (2024-06-18 00:25:23)\n> Extend the Simple IPA IPC to support returning a metadata controllist\n> when the process call has completed.\n> \n> For efficiency, use the existing signal setIspParams() directly\n> to avoid having an extra async callback to return the metadata.\n> \n> Merge the metadata reported by the ISP into any completing\n> request to provide to the application.\n> \n> This should be further extended or improved to make use of the frame\n> context structures so that the metadata is tied to a specific frame and\n> request completion.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/internal/software_isp/software_isp.h | 5 ++++-\n> include/libcamera/ipa/soft.mojom | 2 +-\n> src/ipa/simple/soft_simple.cpp | 4 +++-\n> src/libcamera/pipeline/simple/simple.cpp | 7 +++++--\n> src/libcamera/software_isp/software_isp.cpp | 6 ++++--\n> 5 files changed, 17 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 c5338c05b99b..2631f40ef22a 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -80,8 +80,10 @@ public:\n> Signal<> ispStatsReady;\n> Signal setSensorControls;\n> \n> + const ControlList &metadata() { return metadata_; }\n> +\n\nAnd of course this fails CI because I failed to document this new\naddition:\n\nhttps://gitlab.freedesktop.org/camera/libcamera/-/jobs/60009854\n\n/builds/camera/libcamera/include/libcamera/internal/software_isp/software_isp.h:83: error: Member metadata() (function) of class libcamera::SoftwareIsp is not documented. (warning treated as error, aborting now)\n\n--\nKieran","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 69702BD87C\n\tfor ;\n\tMon, 17 Jun 2024 23:34:56 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53CC36548E;\n\tTue, 18 Jun 2024 01:34:55 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8B08065456\n\tfor ;\n\tTue, 18 Jun 2024 01:34:53 +0200 (CEST)","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 3A214289\n\tfor ;\n\tTue, 18 Jun 2024 01:34:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"vk53AxH1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718667276;\n\tbh=Bkq7abLZ8JbYJdO7mSDIWjI0m99WOFUi5VzZ3OZEpXo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=vk53AxH1SB+WLFF/CMDvPjdZzYPLtQvhd5Y7kwmbDLl8il2Q8ZLIqkhqL0hQgHRSy\n\tORGr6z8KBcQNJ/iOazMZpICAzspgor9MOC898dJzsHmj9OlUTe/LzgbKG/D1QnidVN\n\tDl98UdocydW4QWSUxjM59h7aSwBITIIC9zuFd0lk=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240617232525.878530-2-kieran.bingham@ideasonboard.com>","References":"<20240617232525.878530-1-kieran.bingham@ideasonboard.com>\n\t<20240617232525.878530-2-kieran.bingham@ideasonboard.com>","Subject":"Re: [PATCH 1/3] ipa: simple: softisp: Extend to pass metadata","From":"Kieran Bingham ","Cc":"","To":"libcamera devel ","Date":"Tue, 18 Jun 2024 00:34:50 +0100","Message-ID":"<171866729088.804094.10949552298675636369@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":30019,"web_url":"https://patchwork.libcamera.org/comment/30019/","msgid":"<20240617233714.GH17726@pendragon.ideasonboard.com>","date":"2024-06-17T23:37:14","subject":"Re: [PATCH 1/3] 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 Kieran,\n\nThank you for the patch.\n\nOn Tue, Jun 18, 2024 at 12:25:23AM +0100, Kieran Bingham wrote:\n> Extend the Simple IPA IPC to support returning a metadata controllist\n\ns/controllist/ControlList/\n\n> when the process call has completed.\n> \n> For efficiency, use the existing signal setIspParams() directly\n> to avoid having an extra async callback to return the metadata.\n> \n> Merge the metadata reported by the ISP into any completing\n> request to provide to the application.\n> \n> This should be further extended or improved to make use of the frame\n> context structures so that the metadata is tied to a specific frame and\n> request completion.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/internal/software_isp/software_isp.h | 5 ++++-\n> include/libcamera/ipa/soft.mojom | 2 +-\n> src/ipa/simple/soft_simple.cpp | 4 +++-\n> src/libcamera/pipeline/simple/simple.cpp | 7 +++++--\n> src/libcamera/software_isp/software_isp.cpp | 6 ++++--\n> 5 files changed, 17 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 c5338c05b99b..2631f40ef22a 100644\n> --- a/include/libcamera/internal/software_isp/software_isp.h\n> +++ b/include/libcamera/internal/software_isp/software_isp.h\n> @@ -80,8 +80,10 @@ public:\n> \tSignal<> ispStatsReady;\n> \tSignal setSensorControls;\n> \n> +\tconst ControlList &metadata() { return metadata_; }\n> +\n> private:\n> -\tvoid saveIspParams();\n> +\tvoid saveIspParams(const ControlList &metadata);\n> \tvoid setSensorCtrls(const ControlList &sensorControls);\n> \tvoid statsReady();\n> \tvoid inputReady(FrameBuffer *input);\n> @@ -92,6 +94,7 @@ private:\n> \tSharedMemObject sharedParams_;\n> \tDebayerParams debayerParams_;\n> \tDmaBufAllocator dmaHeap_;\n> +\tControlList metadata_;\n> \n> \tstd::unique_ptr ipa_;\n> };\n> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> index 3aa2066ebaa0..1a0221df7660 100644\n> --- a/include/libcamera/ipa/soft.mojom\n> +++ b/include/libcamera/ipa/soft.mojom\n> @@ -24,5 +24,5 @@ interface IPASoftInterface {\n> \n> interface IPASoftEventInterface {\n> \tsetSensorControls(libcamera.ControlList sensorControls);\n> -\tsetIspParams();\n> +\tsetIspParams(libcamera.ControlList metadata);\n> };\n> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> index b7746ce09206..09c7b575301e 100644\n> --- a/src/ipa/simple/soft_simple.cpp\n> +++ b/src/ipa/simple/soft_simple.cpp\n> @@ -250,6 +250,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \tSwIspStats::Histogram histogram = stats_->yHistogram;\n> \tif (ignoreUpdates_ > 0)\n> \t\tblackLevel_.update(histogram);\n> +\n> +\tControlList metadata(controls::controls);\n> \tconst uint8_t blackLevel = blackLevel_.get();\n> \n> \t/*\n> @@ -303,7 +305,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> \t\tparams_->blue[i] = gammaTable_[idx];\n> \t}\n> \n> -\tsetIspParams.emit();\n> +\tsetIspParams.emit(metadata);\n> \n> \t/* \\todo Switch to the libipa/algorithm.h API someday. */\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index eb36578e320e..accdb744d61b 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -860,10 +860,13 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> \t\t\treturn;\n> \t\t}\n> \n> -\t\tif (converter_)\n> +\t\tif (converter_) {\n> \t\t\tconverter_->queueBuffers(buffer, conversionQueue_.front());\n> -\t\telse\n> +\t\t} else {\n> \t\t\tswIsp_->queueBuffers(buffer, conversionQueue_.front());\n> +\t\t\tif (request)\n> +\t\t\t\trequest->metadata().merge(swIsp_->metadata());\n\nYou're shoving metadata in a random request here, that really makes me\ncringe. It's not just that there's no frame context tracking on the IPA\nside, the pipeline handler side is wrong too.\n\n> +\t\t}\n> \n> \t\tconversionQueue_.pop();\n> \t\treturn;\n> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> index 20fb6f48fdf9..efbbea2d2279 100644\n> --- a/src/libcamera/software_isp/software_isp.cpp\n> +++ b/src/libcamera/software_isp/software_isp.cpp\n> @@ -68,7 +68,8 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> \t: dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> \t\t DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n> -\t\t DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n> +\t\t DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf),\n> +\t metadata_(controls::controls)\n> {\n> \t/*\n> \t * debayerParams_ must be initialized because the initial value is used for\n> @@ -349,9 +350,10 @@ void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output)\n> \t\t\t ConnectionTypeQueued, input, output, debayerParams_);\n> }\n> \n> -void SoftwareIsp::saveIspParams()\n> +void SoftwareIsp::saveIspParams(const ControlList &metadata)\n> {\n> \tdebayerParams_ = *sharedParams_;\n> +\tmetadata_.merge(metadata, ControlList::MergePolicy::OverwriteExisting);\n> }\n> \n> void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)","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 28162C3237\n\tfor ;\n\tMon, 17 Jun 2024 23:37:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6C7786548E;\n\tTue, 18 Jun 2024 01:37:37 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 91D4165456\n\tfor ;\n\tTue, 18 Jun 2024 01:37:36 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 29095289;\n\tTue, 18 Jun 2024 01:37:19 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"XZl1Tb0d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718667439;\n\tbh=BBkdFEVjer13/tkbu4TMGyWmoAQOold+EMSpHF/uuKA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=XZl1Tb0dUsM3fFqrUVAGYrQRAcddEP9MUqAx9jKShBzUJDlo+nKKs8Gath5HG55GX\n\tIHKScTqDjlnxj6RNdrANgSEviBkW7oYAP6ki265pwtiniLRW/aaoypg6xdFrjjiTGU\n\thADF49A77mfCDI3C3hWofLy4e3RFDSSzwnWU1OIo=","Date":"Tue, 18 Jun 2024 02:37:14 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ","Subject":"Re: [PATCH 1/3] ipa: simple: softisp: Extend to pass metadata","Message-ID":"<20240617233714.GH17726@pendragon.ideasonboard.com>","References":"<20240617232525.878530-1-kieran.bingham@ideasonboard.com>\n\t<20240617232525.878530-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240617232525.878530-2-kieran.bingham@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":30027,"web_url":"https://patchwork.libcamera.org/comment/30027/","msgid":"<171883504426.2768.11717592014519468142@ping.linuxembedded.co.uk>","date":"2024-06-19T22:10:44","subject":"Re: [PATCH 1/3] 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 Laurent Pinchart (2024-06-18 00:37:14)\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Tue, Jun 18, 2024 at 12:25:23AM +0100, Kieran Bingham wrote:\n> > Extend the Simple IPA IPC to support returning a metadata controllist\n> \n> s/controllist/ControlList/\n> \n> > when the process call has completed.\n> > \n> > For efficiency, use the existing signal setIspParams() directly\n> > to avoid having an extra async callback to return the metadata.\n> > \n> > Merge the metadata reported by the ISP into any completing\n> > request to provide to the application.\n> > \n> > This should be further extended or improved to make use of the frame\n> > context structures so that the metadata is tied to a specific frame and\n> > request completion.\n> > \n> > Signed-off-by: Kieran Bingham \n> > ---\n> > include/libcamera/internal/software_isp/software_isp.h | 5 ++++-\n> > include/libcamera/ipa/soft.mojom | 2 +-\n> > src/ipa/simple/soft_simple.cpp | 4 +++-\n> > src/libcamera/pipeline/simple/simple.cpp | 7 +++++--\n> > src/libcamera/software_isp/software_isp.cpp | 6 ++++--\n> > 5 files changed, 17 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 c5338c05b99b..2631f40ef22a 100644\n> > --- a/include/libcamera/internal/software_isp/software_isp.h\n> > +++ b/include/libcamera/internal/software_isp/software_isp.h\n> > @@ -80,8 +80,10 @@ public:\n> > Signal<> ispStatsReady;\n> > Signal setSensorControls;\n> > \n> > + const ControlList &metadata() { return metadata_; }\n> > +\n> > private:\n> > - void saveIspParams();\n> > + void saveIspParams(const ControlList &metadata);\n> > void setSensorCtrls(const ControlList &sensorControls);\n> > void statsReady();\n> > void inputReady(FrameBuffer *input);\n> > @@ -92,6 +94,7 @@ private:\n> > SharedMemObject sharedParams_;\n> > DebayerParams debayerParams_;\n> > DmaBufAllocator dmaHeap_;\n> > + ControlList metadata_;\n> > \n> > std::unique_ptr ipa_;\n> > };\n> > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> > index 3aa2066ebaa0..1a0221df7660 100644\n> > --- a/include/libcamera/ipa/soft.mojom\n> > +++ b/include/libcamera/ipa/soft.mojom\n> > @@ -24,5 +24,5 @@ interface IPASoftInterface {\n> > \n> > interface IPASoftEventInterface {\n> > setSensorControls(libcamera.ControlList sensorControls);\n> > - setIspParams();\n> > + setIspParams(libcamera.ControlList metadata);\n> > };\n> > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> > index b7746ce09206..09c7b575301e 100644\n> > --- a/src/ipa/simple/soft_simple.cpp\n> > +++ b/src/ipa/simple/soft_simple.cpp\n> > @@ -250,6 +250,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> > SwIspStats::Histogram histogram = stats_->yHistogram;\n> > if (ignoreUpdates_ > 0)\n> > blackLevel_.update(histogram);\n> > +\n> > + ControlList metadata(controls::controls);\n> > const uint8_t blackLevel = blackLevel_.get();\n> > \n> > /*\n> > @@ -303,7 +305,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> > params_->blue[i] = gammaTable_[idx];\n> > }\n> > \n> > - setIspParams.emit();\n> > + setIspParams.emit(metadata);\n> > \n> > /* \\todo Switch to the libipa/algorithm.h API someday. */\n> > \n> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > index eb36578e320e..accdb744d61b 100644\n> > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > @@ -860,10 +860,13 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> > return;\n> > }\n> > \n> > - if (converter_)\n> > + if (converter_) {\n> > converter_->queueBuffers(buffer, conversionQueue_.front());\n> > - else\n> > + } else {\n> > swIsp_->queueBuffers(buffer, conversionQueue_.front());\n> > + if (request)\n> > + request->metadata().merge(swIsp_->metadata());\n> \n> You're shoving metadata in a random request here, that really makes me\n> cringe. It's not just that there's no frame context tracking on the IPA\n> side, the pipeline handler side is wrong too.\n\nOh, yes - I'm aware ;-)\n\nThere's lots more to do on the pipeline handler to fix up managing\ncontrols and synchronisation.\n\nI know Milan has a branch with some work that gets closer to this on\nthe IPA side already too, so I'm looking forward to that getting out.\n\nAs I said to Robert on IRC when I sent these, it was a case of 'no point\nthem sitting on my PC when they could be a starting point/reference'.\n\nWhat I want to make obvious or easier - is that we can put more data in\nhere such as timing or measurements of the internal performances to be\nable to give constant monitoring of the 'cost' of the SoftISP.\n\n\n\n> > + }\n> > \n> > conversionQueue_.pop();\n> > return;\n> > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> > index 20fb6f48fdf9..efbbea2d2279 100644\n> > --- a/src/libcamera/software_isp/software_isp.cpp\n> > +++ b/src/libcamera/software_isp/software_isp.cpp\n> > @@ -68,7 +68,8 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> > SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> > : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> > DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n> > - DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n> > + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf),\n> > + metadata_(controls::controls)\n> > {\n> > /*\n> > * debayerParams_ must be initialized because the initial value is used for\n> > @@ -349,9 +350,10 @@ void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output)\n> > ConnectionTypeQueued, input, output, debayerParams_);\n> > }\n> > \n> > -void SoftwareIsp::saveIspParams()\n> > +void SoftwareIsp::saveIspParams(const ControlList &metadata)\n> > {\n> > debayerParams_ = *sharedParams_;\n> > + metadata_.merge(metadata, ControlList::MergePolicy::OverwriteExisting);\n> > }\n> > \n> > void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 200F2BD87C\n\tfor ;\n\tWed, 19 Jun 2024 22:10:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D5D55654A9;\n\tThu, 20 Jun 2024 00:10:49 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5821F61A1F\n\tfor ;\n\tThu, 20 Jun 2024 00:10:47 +0200 (CEST)","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 9F8D38E1;\n\tThu, 20 Jun 2024 00:10:28 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZMjuFhuP\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718835028;\n\tbh=VoSsD/NaF6lJjE8RnluViOhRHdkD5sGqxWD596XFIvY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZMjuFhuPbNpZ5upT4TkdeNaiR4J5GyAmlm6eEMnAw+vmzdGrScxYGnRqjVkul0vcF\n\t33hZaSD4Kjz5cN1qWGZZQXYIvJROAai5wXFvoOHcJpmj7QuffVZmQQYbuHtQw1bG8Y\n\tYIP4bt9el5gfqKDjJRsSSq6IszS1XgmIHgwkJYFY=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240617233714.GH17726@pendragon.ideasonboard.com>","References":"<20240617232525.878530-1-kieran.bingham@ideasonboard.com>\n\t<20240617232525.878530-2-kieran.bingham@ideasonboard.com>\n\t<20240617233714.GH17726@pendragon.ideasonboard.com>","Subject":"Re: [PATCH 1/3] ipa: simple: softisp: Extend to pass metadata","From":"Kieran Bingham ","Cc":"libcamera devel ","To":"Laurent Pinchart ","Date":"Wed, 19 Jun 2024 23:10:44 +0100","Message-ID":"<171883504426.2768.11717592014519468142@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":30028,"web_url":"https://patchwork.libcamera.org/comment/30028/","msgid":"<20240619221649.GI31507@pendragon.ideasonboard.com>","date":"2024-06-19T22:16:49","subject":"Re: [PATCH 1/3] 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":"On Wed, Jun 19, 2024 at 11:10:44PM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2024-06-18 00:37:14)\n> > On Tue, Jun 18, 2024 at 12:25:23AM +0100, Kieran Bingham wrote:\n> > > Extend the Simple IPA IPC to support returning a metadata controllist\n> > \n> > s/controllist/ControlList/\n> > \n> > > when the process call has completed.\n> > > \n> > > For efficiency, use the existing signal setIspParams() directly\n> > > to avoid having an extra async callback to return the metadata.\n> > > \n> > > Merge the metadata reported by the ISP into any completing\n> > > request to provide to the application.\n> > > \n> > > This should be further extended or improved to make use of the frame\n> > > context structures so that the metadata is tied to a specific frame and\n> > > request completion.\n> > > \n> > > Signed-off-by: Kieran Bingham \n> > > ---\n> > > include/libcamera/internal/software_isp/software_isp.h | 5 ++++-\n> > > include/libcamera/ipa/soft.mojom | 2 +-\n> > > src/ipa/simple/soft_simple.cpp | 4 +++-\n> > > src/libcamera/pipeline/simple/simple.cpp | 7 +++++--\n> > > src/libcamera/software_isp/software_isp.cpp | 6 ++++--\n> > > 5 files changed, 17 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 c5338c05b99b..2631f40ef22a 100644\n> > > --- a/include/libcamera/internal/software_isp/software_isp.h\n> > > +++ b/include/libcamera/internal/software_isp/software_isp.h\n> > > @@ -80,8 +80,10 @@ public:\n> > > Signal<> ispStatsReady;\n> > > Signal setSensorControls;\n> > > \n> > > + const ControlList &metadata() { return metadata_; }\n> > > +\n> > > private:\n> > > - void saveIspParams();\n> > > + void saveIspParams(const ControlList &metadata);\n> > > void setSensorCtrls(const ControlList &sensorControls);\n> > > void statsReady();\n> > > void inputReady(FrameBuffer *input);\n> > > @@ -92,6 +94,7 @@ private:\n> > > SharedMemObject sharedParams_;\n> > > DebayerParams debayerParams_;\n> > > DmaBufAllocator dmaHeap_;\n> > > + ControlList metadata_;\n> > > \n> > > std::unique_ptr ipa_;\n> > > };\n> > > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom\n> > > index 3aa2066ebaa0..1a0221df7660 100644\n> > > --- a/include/libcamera/ipa/soft.mojom\n> > > +++ b/include/libcamera/ipa/soft.mojom\n> > > @@ -24,5 +24,5 @@ interface IPASoftInterface {\n> > > \n> > > interface IPASoftEventInterface {\n> > > setSensorControls(libcamera.ControlList sensorControls);\n> > > - setIspParams();\n> > > + setIspParams(libcamera.ControlList metadata);\n> > > };\n> > > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp\n> > > index b7746ce09206..09c7b575301e 100644\n> > > --- a/src/ipa/simple/soft_simple.cpp\n> > > +++ b/src/ipa/simple/soft_simple.cpp\n> > > @@ -250,6 +250,8 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> > > SwIspStats::Histogram histogram = stats_->yHistogram;\n> > > if (ignoreUpdates_ > 0)\n> > > blackLevel_.update(histogram);\n> > > +\n> > > + ControlList metadata(controls::controls);\n> > > const uint8_t blackLevel = blackLevel_.get();\n> > > \n> > > /*\n> > > @@ -303,7 +305,7 @@ void IPASoftSimple::processStats(const ControlList &sensorControls)\n> > > params_->blue[i] = gammaTable_[idx];\n> > > }\n> > > \n> > > - setIspParams.emit();\n> > > + setIspParams.emit(metadata);\n> > > \n> > > /* \\todo Switch to the libipa/algorithm.h API someday. */\n> > > \n> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> > > index eb36578e320e..accdb744d61b 100644\n> > > --- a/src/libcamera/pipeline/simple/simple.cpp\n> > > +++ b/src/libcamera/pipeline/simple/simple.cpp\n> > > @@ -860,10 +860,13 @@ void SimpleCameraData::bufferReady(FrameBuffer *buffer)\n> > > return;\n> > > }\n> > > \n> > > - if (converter_)\n> > > + if (converter_) {\n> > > converter_->queueBuffers(buffer, conversionQueue_.front());\n> > > - else\n> > > + } else {\n> > > swIsp_->queueBuffers(buffer, conversionQueue_.front());\n> > > + if (request)\n> > > + request->metadata().merge(swIsp_->metadata());\n> > \n> > You're shoving metadata in a random request here, that really makes me\n> > cringe. It's not just that there's no frame context tracking on the IPA\n> > side, the pipeline handler side is wrong too.\n> \n> Oh, yes - I'm aware ;-)\n> \n> There's lots more to do on the pipeline handler to fix up managing\n> controls and synchronisation.\n> \n> I know Milan has a branch with some work that gets closer to this on\n> the IPA side already too, so I'm looking forward to that getting out.\n> \n> As I said to Robert on IRC when I sent these, it was a case of 'no point\n> them sitting on my PC when they could be a starting point/reference'.\n> \n> What I want to make obvious or easier - is that we can put more data in\n> here such as timing or measurements of the internal performances to be\n> able to give constant monitoring of the 'cost' of the SoftISP.\n\nI like that :-)\n\n> > > + }\n> > > \n> > > conversionQueue_.pop();\n> > > return;\n> > > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp\n> > > index 20fb6f48fdf9..efbbea2d2279 100644\n> > > --- a/src/libcamera/software_isp/software_isp.cpp\n> > > +++ b/src/libcamera/software_isp/software_isp.cpp\n> > > @@ -68,7 +68,8 @@ LOG_DEFINE_CATEGORY(SoftwareIsp)\n> > > SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor)\n> > > : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap |\n> > > DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap |\n> > > - DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf)\n> > > + DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf),\n> > > + metadata_(controls::controls)\n> > > {\n> > > /*\n> > > * debayerParams_ must be initialized because the initial value is used for\n> > > @@ -349,9 +350,10 @@ void SoftwareIsp::process(FrameBuffer *input, FrameBuffer *output)\n> > > ConnectionTypeQueued, input, output, debayerParams_);\n> > > }\n> > > \n> > > -void SoftwareIsp::saveIspParams()\n> > > +void SoftwareIsp::saveIspParams(const ControlList &metadata)\n> > > {\n> > > debayerParams_ = *sharedParams_;\n> > > + metadata_.merge(metadata, ControlList::MergePolicy::OverwriteExisting);\n> > > }\n> > > \n> > > void SoftwareIsp::setSensorCtrls(const ControlList &sensorControls)","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 1C833BE175\n\tfor ;\n\tWed, 19 Jun 2024 22:17:15 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 476E6654A9;\n\tThu, 20 Jun 2024 00:17:14 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C906661A1F\n\tfor ;\n\tThu, 20 Jun 2024 00:17:11 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 03CCF8E1;\n\tThu, 20 Jun 2024 00:16:52 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"AxupvrKh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1718835413;\n\tbh=RnsP1qZtVGGBX6BcE9PKYg18wdt/FB8ZihG4YDMKs4U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=AxupvrKhAfXIf8e9/s/JYEn+Jn51rvYLY6HJTIP0aV+AV7U1L1LLLXVqFBWSI1iS5\n\tdLZ1mZbwTy7ueDEHAtDGJtapPE97+4tmIfCetk/6YaZv7jnYebscf8kfGJEC2GPtZI\n\txKFuvEu2HCWSTIoI7QecxRqkGfGW8ifE49TUfouc=","Date":"Thu, 20 Jun 2024 01:16:49 +0300","From":"Laurent Pinchart ","To":"Kieran Bingham ","Cc":"libcamera devel ","Subject":"Re: [PATCH 1/3] ipa: simple: softisp: Extend to pass metadata","Message-ID":"<20240619221649.GI31507@pendragon.ideasonboard.com>","References":"<20240617232525.878530-1-kieran.bingham@ideasonboard.com>\n\t<20240617232525.878530-2-kieran.bingham@ideasonboard.com>\n\t<20240617233714.GH17726@pendragon.ideasonboard.com>\n\t<171883504426.2768.11717592014519468142@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<171883504426.2768.11717592014519468142@ping.linuxembedded.co.uk>","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\" "}}]