[{"id":37099,"web_url":"https://patchwork.libcamera.org/comment/37099/","msgid":"","date":"2025-11-28T12:10:48","subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Rui\n\nOn Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote:\n> Introduce computeExposureIndex() to derive an exposure index value from\n> the AGC gain (approximately exposureIndex = gain * 100), and\n> selectExposureIndexBand() to search through a container of exposure index\n> levels and select the appropriate tuning band for the current exposure\n> index.\n>\n> These helpers provide reusable functionality for exposure-index-banded\n> tuning in denoising algorithms, enabling more precise algorithm\n> configuration based on lighting conditions.\n>\n> Signed-off-by: Rui Wang \n> ---\n> src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++\n> 1 file changed, 55 insertions(+)\n> create mode 100644 src/ipa/rkisp1/algorithms/denoise.h\n>\n> diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h\n> new file mode 100644\n> index 00000000..8907dc4e\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/algorithms/denoise.h\n> @@ -0,0 +1,55 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2025, Ideas On Board\n> + *\n> + * RkISP1 Denoising Algorithms Base Class\n> + */\n> +\n> +#pragma once\n> +\n> +#include \"../ipa_context.h\"\n> +\n> +#include \"algorithm.h\"\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::algorithms {\n> +\n> +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm\n\nWhy a base class ?\nit seems both the functions implemented here are solely used by the\ndpf algorithm implementation.\n\nI see two ways to handle this, unless I'm missing something\n1) Make this a libipa helper\n2) Add the two function to the RkISP1 DPF implementation\n\n> +{\n> +protected:\n> +\tDenoiseBaseAlgorithm() = default;\n> +\t~DenoiseBaseAlgorithm() = default;\n> +\n> +\tvirtual uint32_t computeExposureIndex(const IPAContext &context,\n> +\t\t\t\t\t const IPAFrameContext &frameContext) const;\n> +\ttemplate\n> +\tuint32_t selectExposureIndexBand(unsigned exposureIndex,\n> +\t\t\t\t\t const LevelContainer &levels) const;\n> +};\n> +\n> +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context,\n> +\t\t\t\t\t\t\t const IPAFrameContext &frameContext) const\n> +{\n> +\tdouble ag = frameContext.agc.gain ? frameContext.agc.gain\n> +\t\t\t\t\t : context.activeState.agc.automatic.gain;\n\ndoes this depend in the AGC has run before the dpf ?\n\nWe can't rely on the algorithms declaration order, we need something\nbetter indeed (not for this series of course)\n\nIf this depends on the algorithms ordering I would\n1) Try to see if the frameContext.agc.gain field is populated\n2) if it's not log a Warn message (possibily just once?) to state that\nthe DPF is running before AGC and results might not be accurate ?\n\nIs this necessary in your opinion ?\n\nAlso, what does guarantee that automatic.gain is valid ? What if AGC\nis running in manual mode ?\n\n> +\treturn static_cast(ag * 100.0 + 0.5);\n\nnit: we usually \"unsigned int\"\n\n> +}\n> +\n> +template\n\nIs using a template without specifying any restrictions on the\ntemplated type desirable ? The only user of this function passes in a\nvector, why do you need templating ?\n\nIf we need templating, I guess we should make sure that the LevelContainer\ntemplate argument implements the interface you use in the below code\n\n> +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex,\n> +\t\t\t\t\t\t const LevelContainer &levels) const\n> +{\n> +\tif (levels.empty())\n> +\t\treturn -1;\n\nDoes this gets converted to UINT_MAX as you return a unsigned int ?\n\nYou call this as:\n\n int32_t idx =\n DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex,\n exposureIndexLevels_);\n\t\tif (idx >= 0) {\n\t\t\tconfig_ = exposureIndexLevels_[idx].dpf;\n\t\t\tstrengthConfig_ = exposureIndexLevels_[idx].strength;\n\t\t\tlastExposureGainIndex_ = idx;\n\t\t}\n\nSo I guess the returned value is implicitly casted back to a signed\ninteger, but the function prototype should be changed to reflect that.\n\n\n> +\tuint32_t idx = 0;\n\nif you use std::size_t you can probably spare all the casts ? (my\ncompiler doesn't complain if I remove all casts and idx statys an\nuint32_t though).\n\n\n> +\twhile (idx < static_cast(levels.size()) && exposureIndex > levels[idx].maxExposureIndex)\n\n'levels' have to be sorted then ?\n\nAlso, are you willing to take the index of the 'next larger value' ?\n\nIn example (using random numbers):\n\nvector levels = { 1, 2, 4, 6, 9, 14, 18 };\nexposureIndex = 15;\n\nshould it return 18 or 14 ? (just checking to make sure my below\nsuggestion is right)\n\n> +\t\t++idx;\n\n> +\tif (idx >= static_cast(levels.size()))\n> +\t\tidx = static_cast(levels.size()) - 1;\n> +\treturn idx;\n\n\tauto it = std::partition_point(levels.begin(), levels.end() - 1,\n\t\t\t[exposureIndex](const auto &level) {\n\t\t\t\treturn exposureIndex > level.maxExposureIndex;\n\t\t\t});\n\treturn std::distance(levels.begin(), it);\n\nshould do and it's safer (hand-rolled while() loops always make me a bit\nunconfortable) (also, you need to include if you take this in).\n\nThanks\n j\n\n> +}\n> +\n> +} /* namespace ipa::rkisp1::algorithms */\n> +\n> +} /* namespace libcamera */\n> --\n> 2.43.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 3729EC0F2A\n\tfor ;\n\tFri, 28 Nov 2025 12:10:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 37F4560A85;\n\tFri, 28 Nov 2025 13:10:53 +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 AAA386069A\n\tfor ;\n\tFri, 28 Nov 2025 13:10:51 +0100 (CET)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 533274F1;\n\tFri, 28 Nov 2025 13:08:40 +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=\"sXCIJoRd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764331720;\n\tbh=UCfe7a/4e0E3gac1FYUNDN4ClIyotkFz7LkAZNgiwpI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=sXCIJoRdpI4sFVQjrUY9BT0Ubgrzu7HffMLkHRTmWTbZBslug6DU2fzZx9BTndteW\n\tJdPv3q74WimBdJVz254BKxSAIGZjZmkkgOwEAB+BZsNr7xOEBFvSoI9v2pi2MIIzGM\n\t8fDM+21hlCscUnGz7ss9+e/E9NKUc3Mp8x0uQNJA=","Date":"Fri, 28 Nov 2025 13:10:48 +0100","From":"Jacopo Mondi ","To":"Rui Wang ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","Message-ID":"","References":"<20251125000848.4103786-1-rui.wang@ideasonboard.com>\n\t<20251125000848.4103786-2-rui.wang@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20251125000848.4103786-2-rui.wang@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":37106,"web_url":"https://patchwork.libcamera.org/comment/37106/","msgid":"<3ad522e3-b6a6-4715-a8d5-059f2d6170f3@ideasonboard.com>","date":"2025-11-28T22:54:21","subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","submitter":{"id":241,"url":"https://patchwork.libcamera.org/api/people/241/","name":"Rui Wang","email":"rui.wang@ideasonboard.com"},"content":"On 2025-11-28 07:10, Jacopo Mondi wrote:\n> Hi Rui\n>\n> On Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote:\n>> Introduce computeExposureIndex() to derive an exposure index value from\n>> the AGC gain (approximately exposureIndex = gain * 100), and\n>> selectExposureIndexBand() to search through a container of exposure index\n>> levels and select the appropriate tuning band for the current exposure\n>> index.\n>>\n>> These helpers provide reusable functionality for exposure-index-banded\n>> tuning in denoising algorithms, enabling more precise algorithm\n>> configuration based on lighting conditions.\n>>\n>> Signed-off-by: Rui Wang \n>> ---\n>> src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++\n>> 1 file changed, 55 insertions(+)\n>> create mode 100644 src/ipa/rkisp1/algorithms/denoise.h\n>>\n>> diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h\n>> new file mode 100644\n>> index 00000000..8907dc4e\n>> --- /dev/null\n>> +++ b/src/ipa/rkisp1/algorithms/denoise.h\n>> @@ -0,0 +1,55 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2025, Ideas On Board\n>> + *\n>> + * RkISP1 Denoising Algorithms Base Class\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \"../ipa_context.h\"\n>> +\n>> +#include \"algorithm.h\"\n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa::rkisp1::algorithms {\n>> +\n>> +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm\n> Why a base class ?\n> it seems both the functions implemented here are solely used by the\n> dpf algorithm implementation.\n>\n> I see two ways to handle this, unless I'm missing something\n> 1) Make this a libipa helper\n> 2) Add the two function to the RkISP1 DPF implementation\n>\nIn future , filter denoise also share almost same architecture and \nprocedure like: auto/manual/off/ mode  switching and yaml parser\nthat's why I create a base class for both dpf and filter\nhttps://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/5\n\n>> +{\n>> +protected:\n>> +\tDenoiseBaseAlgorithm() = default;\n>> +\t~DenoiseBaseAlgorithm() = default;\n>> +\n>> +\tvirtual uint32_t computeExposureIndex(const IPAContext &context,\n>> +\t\t\t\t\t const IPAFrameContext &frameContext) const;\n>> +\ttemplate\n>> +\tuint32_t selectExposureIndexBand(unsigned exposureIndex,\n>> +\t\t\t\t\t const LevelContainer &levels) const;\n>> +};\n>> +\n>> +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context,\n>> +\t\t\t\t\t\t\t const IPAFrameContext &frameContext) const\n>> +{\n>> +\tdouble ag = frameContext.agc.gain ? frameContext.agc.gain\n>> +\t\t\t\t\t : context.activeState.agc.automatic.gain;\n> does this depend in the AGC has run before the dpf ?\n\ncomputeExposureIndex is called only at auto mode at each frame in prepare() .\nDPF will select the group of config from tuning params according to current gain value , so as long as either frameContext.agc.gain or\ncontext.activeState.agc.automatic.gain populated ,DPF selector can be updated automaticly.\n\n>\n> We can't rely on the algorithms declaration order, we need something\n> better indeed (not for this series of course)\n>\n> If this depends on the algorithms ordering I would\n> 1) Try to see if the frameContext.agc.gain field is populated\n> 2) if it's not log a Warn message (possibily just once?) to state that\n> the DPF is running before AGC and results might not be accurate ?\n\nThe auto DPF the scalar range can be set in tuning file base on gain \nrange like band :\n\n   100/200/400/800/1600/3200  ,\n\nwhich AGC calculate value would mapping to band : ag * 100.0 + 0.5 ,\n\nso once exposure become convergency  , then DPF will be updated \nfollowing that . but all those depends on\n\nframeContext.agc.gain from IPA\n\n>\n> Is this necessary in your opinion ?\n>\n> Also, what does guarantee that automatic.gain is valid ? What if AGC\n> is running in manual mode ?\nin manual mode or other mode ,it is not rely on this AGC, the DPF config \nwill be read from camshark controls value.\n\n>\n>> +\treturn static_cast(ag * 100.0 + 0.5);\n> nit: we usually \"unsigned int\"\nyes , I will try to change to that i\n>\n>> +}\n>> +\n>> +template\n> Is using a template without specifying any restrictions on the\n> templated type desirable ? The only user of this function passes in a\n> vector, why do you need templating ?\n>\n> If we need templating, I guess we should make sure that the LevelContainer\n> template argument implements the interface you use in the below code\n\nin filter denoise , it has similiar LevelContainer but different \nstructure , so I choose template to support both two denoise\n\ndata structure.\n\n>> +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex,\n>> +\t\t\t\t\t\t const LevelContainer &levels) const\n>> +{\n>> +\tif (levels.empty())\n>> +\t\treturn -1;\n> Does this gets converted to UINT_MAX as you return a unsigned int ?\n>\n> You call this as:\n>\n> int32_t idx =\n> DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex,\n> exposureIndexLevels_);\n> \t\tif (idx >= 0) {\n> \t\t\tconfig_ = exposureIndexLevels_[idx].dpf;\n> \t\t\tstrengthConfig_ = exposureIndexLevels_[idx].strength;\n> \t\t\tlastExposureGainIndex_ = idx;\n> \t\t}\n>\n> So I guess the returned value is implicitly casted back to a signed\n> integer, but the function prototype should be changed to reflect that.\n>\n>\n>> +\tuint32_t idx = 0;\n> if you use std::size_t you can probably spare all the casts ? (my\n> compiler doesn't complain if I remove all casts and idx statys an\n> uint32_t though).\n>\n>\n>> +\twhile (idx < static_cast(levels.size()) && exposureIndex > levels[idx].maxExposureIndex)\n> 'levels' have to be sorted then ?\nyes ,when readout those levels tuning data from yaml ,those are sorted \nwith greater\n>\n> Also, are you willing to take the index of the 'next larger value' ?\n>\n> In example (using random numbers):\n>\n> vector levels = { 1, 2, 4, 6, 9, 14, 18 };\n> exposureIndex = 15;\n>\n> should it return 18 or 14 ? (just checking to make sure my below\n> suggestion is right)\n>\n> 14 is expected.\n>> +\t\t++idx;\n>> +\tif (idx >= static_cast(levels.size()))\n>> +\t\tidx = static_cast(levels.size()) - 1;\n>> +\treturn idx;\n> \tauto it = std::partition_point(levels.begin(), levels.end() - 1,\n> \t\t\t[exposureIndex](const auto &level) {\n> \t\t\t\treturn exposureIndex > level.maxExposureIndex;\n> \t\t\t});\n> \treturn std::distance(levels.begin(), it);\n>\n> should do and it's safer (hand-rolled while() loops always make me a bit\n> unconfortable) (also, you need to include if you take this in).\n>\n> Thanks\n> j\ngood suggestion , I will excute it like this in the following patch.\n>\n>> +}\n>> +\n>> +} /* namespace ipa::rkisp1::algorithms */\n>> +\n>> +} /* namespace libcamera */\n>> --\n>> 2.43.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 D6E64C0F2A\n\tfor ;\n\tFri, 28 Nov 2025 22:54:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7DA160A9E;\n\tFri, 28 Nov 2025 23:54:36 +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 EC2316069A\n\tfor ;\n\tFri, 28 Nov 2025 23:54:34 +0100 (CET)","from [192.168.31.114] (unknown [209.216.103.65])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 359AC593;\n\tFri, 28 Nov 2025 23:52:23 +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=\"qhE2T6oH\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764370343;\n\tbh=gU2MpDnbzdESqcjqyM980SE2odn4Ykk1wRmaquHH1gU=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=qhE2T6oHLz7y/x6zKlyRP/N/ZrgCu8JExMRDy9q3TXcj+gIyjvJoTaH4tTggjDKbm\n\thGVaOzVmeOP4Ng1OIxlrbyxiE2VK3o2lN/t4VADGrUoFJJgu3yB9DRApxG1KPCLLLc\n\tT94ydAFGO9YUbJEHbbWJh1tX62rAsGKoR9cNKSfQ=","Message-ID":"<3ad522e3-b6a6-4715-a8d5-059f2d6170f3@ideasonboard.com>","Date":"Fri, 28 Nov 2025 17:54:21 -0500","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20251125000848.4103786-1-rui.wang@ideasonboard.com>\n\t<20251125000848.4103786-2-rui.wang@ideasonboard.com>\n\t","Content-Language":"en-US","From":"rui wang ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37134,"web_url":"https://patchwork.libcamera.org/comment/37134/","msgid":"","date":"2025-12-01T12:07:41","subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Rui\n\nOn Fri, Nov 28, 2025 at 05:54:21PM -0500, rui wang wrote:\n>\n> On 2025-11-28 07:10, Jacopo Mondi wrote:\n> > Hi Rui\n> >\n> > On Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote:\n> > > Introduce computeExposureIndex() to derive an exposure index value from\n> > > the AGC gain (approximately exposureIndex = gain * 100), and\n> > > selectExposureIndexBand() to search through a container of exposure index\n> > > levels and select the appropriate tuning band for the current exposure\n> > > index.\n> > >\n> > > These helpers provide reusable functionality for exposure-index-banded\n> > > tuning in denoising algorithms, enabling more precise algorithm\n> > > configuration based on lighting conditions.\n> > >\n> > > Signed-off-by: Rui Wang \n> > > ---\n> > > src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++\n> > > 1 file changed, 55 insertions(+)\n> > > create mode 100644 src/ipa/rkisp1/algorithms/denoise.h\n> > >\n> > > diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h\n> > > new file mode 100644\n> > > index 00000000..8907dc4e\n> > > --- /dev/null\n> > > +++ b/src/ipa/rkisp1/algorithms/denoise.h\n> > > @@ -0,0 +1,55 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2025, Ideas On Board\n> > > + *\n> > > + * RkISP1 Denoising Algorithms Base Class\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \"../ipa_context.h\"\n> > > +\n> > > +#include \"algorithm.h\"\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace ipa::rkisp1::algorithms {\n> > > +\n> > > +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm\n> > Why a base class ?\n> > it seems both the functions implemented here are solely used by the\n> > dpf algorithm implementation.\n> >\n> > I see two ways to handle this, unless I'm missing something\n> > 1) Make this a libipa helper\n> > 2) Add the two function to the RkISP1 DPF implementation\n> >\n> In future , filter denoise also share almost same architecture and procedure\n> like: auto/manual/off/ mode  switching and yaml parser\n> that's why I create a base class for both dpf and filter\n> https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/5\n>\n\nI don't see much code re-use between the two implementation.\n\nin denoise.h I see only 4 functions implemented\n\n- computeExposureIndex() and selectExposureIndexBand()\n are these used by Filter too ?\n\n- getControlMap() and fillMetadata()\n these seems specific to Denoise, aren't they ?\n\nOverall, I'm still not convinced deriving Filter and Dpf from a base\nclass gives us much.\n\n> > > +{\n> > > +protected:\n> > > +\tDenoiseBaseAlgorithm() = default;\n> > > +\t~DenoiseBaseAlgorithm() = default;\n> > > +\n> > > +\tvirtual uint32_t computeExposureIndex(const IPAContext &context,\n> > > +\t\t\t\t\t const IPAFrameContext &frameContext) const;\n> > > +\ttemplate\n> > > +\tuint32_t selectExposureIndexBand(unsigned exposureIndex,\n> > > +\t\t\t\t\t const LevelContainer &levels) const;\n> > > +};\n> > > +\n> > > +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context,\n> > > +\t\t\t\t\t\t\t const IPAFrameContext &frameContext) const\n> > > +{\n> > > +\tdouble ag = frameContext.agc.gain ? frameContext.agc.gain\n> > > +\t\t\t\t\t : context.activeState.agc.automatic.gain;\n> > does this depend in the AGC has run before the dpf ?\n>\n> computeExposureIndex is called only at auto mode at each frame in prepare() .\n> DPF will select the group of config from tuning params according to current gain value , so as long as either frameContext.agc.gain or\n> context.activeState.agc.automatic.gain populated ,DPF selector can be updated automaticly.\n>\n\nYes, but the behaviour migh be different.\n\nIf AGC runs before DPF the gain in the frame context will be used\nIf DPF runs first AGC won't have populated frameContext.agc.gain yet\nand we'll use the active state which reflect the most up-to-date gain\nvalue used but which might refere to some frames before.\n\nAs said, we have a limitation that algorithms are run in order of\ndeclaration, so depending on the tuning file structure we might have\ndifferent behaviours\n\n> >\n> > We can't rely on the algorithms declaration order, we need something\n> > better indeed (not for this series of course)\n> >\n> > If this depends on the algorithms ordering I would\n> > 1) Try to see if the frameContext.agc.gain field is populated\n> > 2) if it's not log a Warn message (possibily just once?) to state that\n> > the DPF is running before AGC and results might not be accurate ?\n\nand that's why I suggested warning about that\n\n>\n> The auto DPF the scalar range can be set in tuning file base on gain range\n> like band :\n>\n>   100/200/400/800/1600/3200  ,\n>\n> which AGC calculate value would mapping to band : ag * 100.0 + 0.5 ,\n>\n> so once exposure become convergency  , then DPF will be updated following\n> that . but all those depends on\n>\n> frameContext.agc.gain from IPA\n>\n> >\n> > Is this necessary in your opinion ?\n> >\n> > Also, what does guarantee that automatic.gain is valid ? What if AGC\n> > is running in manual mode ?\n> in manual mode or other mode ,it is not rely on this AGC, the DPF config\n> will be read from camshark controls value.\n\nDo you mean that computeExposureIndex will only be called in auto\nmode? I presume this is DPF auto-mode. Does it imply AGC auto mode ?\n\nOr can the DPF run with auto and AGC with manual ?\n\n>\n> >\n> > > +\treturn static_cast(ag * 100.0 + 0.5);\n> > nit: we usually \"unsigned int\"\n> yes , I will try to change to that i\n> >\n> > > +}\n> > > +\n> > > +template\n> > Is using a template without specifying any restrictions on the\n> > templated type desirable ? The only user of this function passes in a\n> > vector, why do you need templating ?\n> >\n> > If we need templating, I guess we should make sure that the LevelContainer\n> > template argument implements the interface you use in the below code\n>\n> in filter denoise , it has similiar LevelContainer but different structure ,\n> so I choose template to support both two denoise\n>\n> data structure.\n\naren't two overlads better ? Or two templates specializations specific\nto the two containers you'll have to support.\n\nIsn't a template function which makes assumptions on the interface of the\ntemplate argument without restricting it using traits potentially a\ncause for compilation errors ?\n\n>\n> > > +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex,\n> > > +\t\t\t\t\t\t const LevelContainer &levels) const\n> > > +{\n> > > +\tif (levels.empty())\n> > > +\t\treturn -1;\n> > Does this gets converted to UINT_MAX as you return a unsigned int ?\n> >\n> > You call this as:\n> >\n> > int32_t idx =\n> > DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex,\n> > exposureIndexLevels_);\n> > \t\tif (idx >= 0) {\n> > \t\t\tconfig_ = exposureIndexLevels_[idx].dpf;\n> > \t\t\tstrengthConfig_ = exposureIndexLevels_[idx].strength;\n> > \t\t\tlastExposureGainIndex_ = idx;\n> > \t\t}\n> >\n> > So I guess the returned value is implicitly casted back to a signed\n> > integer, but the function prototype should be changed to reflect that.\n> >\n> >\n> > > +\tuint32_t idx = 0;\n> > if you use std::size_t you can probably spare all the casts ? (my\n> > compiler doesn't complain if I remove all casts and idx statys an\n> > uint32_t though).\n> >\n> >\n> > > +\twhile (idx < static_cast(levels.size()) && exposureIndex > levels[idx].maxExposureIndex)\n> > 'levels' have to be sorted then ?\n> yes ,when readout those levels tuning data from yaml ,those are sorted with\n> greater\n\nI would -at least- document this assumption in this function.\n\n> >\n> > Also, are you willing to take the index of the 'next larger value' ?\n> >\n> > In example (using random numbers):\n> >\n> > vector levels = { 1, 2, 4, 6, 9, 14, 18 };\n> > exposureIndex = 15;\n> >\n> > should it return 18 or 14 ? (just checking to make sure my below\n> > suggestion is right)\n> >\n> > 14 is expected.\n\nThis, which -should- match your code\n\n\tvector levels = { 1, 2, 4, 6, 9, 14, 18 };\n\tunsigned int exposureIndex = 15;\n\tunsigned int idx = 0;\n\twhile (idx < levels.size() && exposureIndex > levels[idx])\n\t\t++idx;\n\tif (idx >= levels.size())\n\t\tidx = levels.size() - 1;\n\tcout << \"target: \" << levels[idx] << endl;\n\nWill print 18.\n\nHave I reimplemented your code wrong ?\n\n\n> > > +\t\t++idx;\nV> > > +\tif (idx >= static_cast(levels.size()))\n> > > +\t\tidx = static_cast(levels.size()) - 1;\n> > > +\treturn idx;\n> > \tauto it = std::partition_point(levels.begin(), levels.end() - 1,\n> > \t\t\t[exposureIndex](const auto &level) {\n> > \t\t\t\treturn exposureIndex > level.maxExposureIndex;\n> > \t\t\t});\n> > \treturn std::distance(levels.begin(), it);\n> >\n> > should do and it's safer (hand-rolled while() loops always make me a bit\n> > unconfortable) (also, you need to include if you take this in).\n> >\n> > Thanks\n> > j\n> good suggestion , I will excute it like this in the following patch.\n> >\n> > > +}\n> > > +\n> > > +} /* namespace ipa::rkisp1::algorithms */\n> > > +\n> > > +} /* namespace libcamera */\n> > > --\n> > > 2.43.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 DFC9EC0F1B\n\tfor ;\n\tMon, 1 Dec 2025 12:07:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DEA4E60AD0;\n\tMon, 1 Dec 2025 13:07:46 +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 4D33A60A7B\n\tfor ;\n\tMon, 1 Dec 2025 13:07:45 +0100 (CET)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C0D324F1;\n\tMon, 1 Dec 2025 13:05:31 +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=\"E3hvbJI4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764590731;\n\tbh=r5PsVSWiumM34aMruJvYPIdEe0MuhtSfCHyXNGLPp3s=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E3hvbJI4RrmdG0SUAu72p0M/ZUaPuA9CKy4r9fZQ5Kxhpxl8ef3g1O0iqeq304cDJ\n\tsPoQCo3CK+NCaAVmeEwUP9FDyvE74FJw7QYXOJA1SlClNIWDkaBy9K6qtMusivvXAi\n\tXuRX8S8Hhreehia+koJOzHUpBDplYdC8qIWwhiFY=","Date":"Mon, 1 Dec 2025 13:07:41 +0100","From":"Jacopo Mondi ","To":"rui wang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","Message-ID":"","References":"<20251125000848.4103786-1-rui.wang@ideasonboard.com>\n\t<20251125000848.4103786-2-rui.wang@ideasonboard.com>\n\t\n\t<3ad522e3-b6a6-4715-a8d5-059f2d6170f3@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<3ad522e3-b6a6-4715-a8d5-059f2d6170f3@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":37150,"web_url":"https://patchwork.libcamera.org/comment/37150/","msgid":"","date":"2025-12-01T17:40:43","subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","submitter":{"id":241,"url":"https://patchwork.libcamera.org/api/people/241/","name":"Rui Wang","email":"rui.wang@ideasonboard.com"},"content":"On 2025-12-01 07:07, Jacopo Mondi wrote:\n> Hi Rui\n>\n> On Fri, Nov 28, 2025 at 05:54:21PM -0500, rui wang wrote:\n>> On 2025-11-28 07:10, Jacopo Mondi wrote:\n>>> Hi Rui\n>>>\n>>> On Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote:\n>>>> Introduce computeExposureIndex() to derive an exposure index value from\n>>>> the AGC gain (approximately exposureIndex = gain * 100), and\n>>>> selectExposureIndexBand() to search through a container of exposure index\n>>>> levels and select the appropriate tuning band for the current exposure\n>>>> index.\n>>>>\n>>>> These helpers provide reusable functionality for exposure-index-banded\n>>>> tuning in denoising algorithms, enabling more precise algorithm\n>>>> configuration based on lighting conditions.\n>>>>\n>>>> Signed-off-by: Rui Wang \n>>>> ---\n>>>> src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++\n>>>> 1 file changed, 55 insertions(+)\n>>>> create mode 100644 src/ipa/rkisp1/algorithms/denoise.h\n>>>>\n>>>> diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h\n>>>> new file mode 100644\n>>>> index 00000000..8907dc4e\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/rkisp1/algorithms/denoise.h\n>>>> @@ -0,0 +1,55 @@\n>>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>>>> +/*\n>>>> + * Copyright (C) 2025, Ideas On Board\n>>>> + *\n>>>> + * RkISP1 Denoising Algorithms Base Class\n>>>> + */\n>>>> +\n>>>> +#pragma once\n>>>> +\n>>>> +#include \"../ipa_context.h\"\n>>>> +\n>>>> +#include \"algorithm.h\"\n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +namespace ipa::rkisp1::algorithms {\n>>>> +\n>>>> +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm\n>>> Why a base class ?\n>>> it seems both the functions implemented here are solely used by the\n>>> dpf algorithm implementation.\n>>>\n>>> I see two ways to handle this, unless I'm missing something\n>>> 1) Make this a libipa helper\n>>> 2) Add the two function to the RkISP1 DPF implementation\n>>>\n>> In future , filter denoise also share almost same architecture and procedure\n>> like: auto/manual/off/ mode  switching and yaml parser\n>> that's why I create a base class for both dpf and filter\n>> https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/5\n>>\n> I don't see much code re-use between the two implementation.\n>\n> in denoise.h I see only 4 functions implemented\n>\n> - computeExposureIndex() and selectExposureIndexBand()\n> are these used by Filter too ?\n\nyes , all those two function used for dpf and filter auto mode ,\n\n  base on exposure index , which can be calculate from current gain value.\n\nand selectExposureIndexBand can select specific parameter from yaml file.\n\n>\n> - getControlMap() and fillMetadata()\n> these seems specific to Denoise, aren't they ?\n>\n> Overall, I'm still not convinced deriving Filter and Dpf from a base\n> class gives us much.\n>\n\ngetControlMap() which design for manual mode control,\ndpf and filter will override its specific controls list.\n\nI have another branch to refactory filter.cpp , which is as same code \nstructure as dpf.cpp\n\nhttps://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/commit/cbc5346d6518c0aca18faf87c2dacd200572a65f, \n\n\nboth dpf and filter would override all those virtual functions respectively.\n\n>>>> +{\n>>>> +protected:\n>>>> +\tDenoiseBaseAlgorithm() = default;\n>>>> +\t~DenoiseBaseAlgorithm() = default;\n>>>> +\n>>>> +\tvirtual uint32_t computeExposureIndex(const IPAContext &context,\n>>>> +\t\t\t\t\t const IPAFrameContext &frameContext) const;\n>>>> +\ttemplate\n>>>> +\tuint32_t selectExposureIndexBand(unsigned exposureIndex,\n>>>> +\t\t\t\t\t const LevelContainer &levels) const;\n>>>> +};\n>>>> +\n>>>> +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context,\n>>>> +\t\t\t\t\t\t\t const IPAFrameContext &frameContext) const\n>>>> +{\n>>>> +\tdouble ag = frameContext.agc.gain ? frameContext.agc.gain\n>>>> +\t\t\t\t\t : context.activeState.agc.automatic.gain;\n>>> does this depend in the AGC has run before the dpf ?\n>> computeExposureIndex is called only at auto mode at each frame in prepare() .\n>> DPF will select the group of config from tuning params according to current gain value , so as long as either frameContext.agc.gain or\n>> context.activeState.agc.automatic.gain populated ,DPF selector can be updated automaticly.\n>>\n> Yes, but the behaviour migh be different.\n>\n> If AGC runs before DPF the gain in the frame context will be used\n> If DPF runs first AGC won't have populated frameContext.agc.gain yet\n> and we'll use the active state which reflect the most up-to-date gain\n> value used but which might refere to some frames before.\n>\n> As said, we have a limitation that algorithms are run in order of\n> declaration, so depending on the tuning file structure we might have\n> different behaviours\n\nas metioned above, AGC value only be needed at denoise auto mode . But \nby default , dpf is disable , it need to be enable from control, yes, it \nneed to up-to-date value from agc . and current exposure index \ndetermined by current gain value.\n\ngain value is float type , and computeExposureIndex will return like \n:100/200/400/800/1600/* , the only agc change dramaticly(cross band) \nwill trigger denoise's profile update.\n\ndoubleag=frameContext.agc.gain?frameContext.agc.gain\n:context.activeState.agc.automatic.gain;\nwill add Warning for current active agc value denoise.h\n>>> We can't rely on the algorithms declaration order, we need something\n>>> better indeed (not for this series of course)\n>>>\n>>> If this depends on the algorithms ordering I would\n>>> 1) Try to see if the frameContext.agc.gain field is populated\n>>> 2) if it's not log a Warn message (possibily just once?) to state that\n>>> the DPF is running before AGC and results might not be accurate ?\n> and that's why I suggested warning about that\n>\n>> The auto DPF the scalar range can be set in tuning file base on gain range\n>> like band :\n>>\n>>   100/200/400/800/1600/3200  ,\n>>\n>> which AGC calculate value would mapping to band : ag * 100.0 + 0.5 ,\n>>\n>> so once exposure become convergency  , then DPF will be updated following\n>> that . but all those depends on\n>>\n>> frameContext.agc.gain from IPA\n>>\n>>> Is this necessary in your opinion ?\n>>>\n>>> Also, what does guarantee that automatic.gain is valid ? What if AGC\n>>> is running in manual mode ?\n>> in manual mode or other mode ,it is not rely on this AGC, the DPF config\n>> will be read from camshark controls value.\n> Do you mean that computeExposureIndex will only be called in auto\n> mode? I presume this is DPF auto-mode. Does it imply AGC auto mode ?\n>\n> Or can the DPF run with auto and AGC with manual ?\nDPF auto mode applies config which determined by current exposure \nindex(compute from current AGC value) . No matter current AGC mode is \nauto or manual.\n>>>> +\treturn static_cast(ag * 100.0 + 0.5);\n>>> nit: we usually \"unsigned int\"\n>> yes , I will try to change to that i\n>>>> +}\n>>>> +\n>>>> +template\n>>> Is using a template without specifying any restrictions on the\n>>> templated type desirable ? The only user of this function passes in a\n>>> vector, why do you need templating ?\n>>>\n>>> If we need templating, I guess we should make sure that the LevelContainer\n>>> template argument implements the interface you use in the below code\n>> in filter denoise , it has similiar LevelContainer but different structure ,\n>> so I choose template to support both two denoise\n>>\n>> data structure.\n> aren't two overlads better ? Or two templates specializations specific\n> to the two containers you'll have to support.\n>\n> Isn't a template function which makes assumptions on the interface of the\n> template argument without restricting it using traits potentially a\n> cause for compilation errors ?\n>\n>>>> +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex,\n>>>> +\t\t\t\t\t\t const LevelContainer &levels) const\n>>>> +{\n>>>> +\tif (levels.empty())\n>>>> +\t\treturn -1;\n>>> Does this gets converted to UINT_MAX as you return a unsigned int ?\n>>>\n>>> You call this as:\n>>>\n>>> int32_t idx =\n>>> DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex,\n>>> exposureIndexLevels_);\n>>> \t\tif (idx >= 0) {\n>>> \t\t\tconfig_ = exposureIndexLevels_[idx].dpf;\n>>> \t\t\tstrengthConfig_ = exposureIndexLevels_[idx].strength;\n>>> \t\t\tlastExposureGainIndex_ = idx;\n>>> \t\t}\n>>>\n>>> So I guess the returned value is implicitly casted back to a signed\n>>> integer, but the function prototype should be changed to reflect that.\n>>>\n>>>\n>>>> +\tuint32_t idx = 0;\n>>> if you use std::size_t you can probably spare all the casts ? (my\n>>> compiler doesn't complain if I remove all casts and idx statys an\n>>> uint32_t though).\n>>>\n>>>\n>>>> +\twhile (idx < static_cast(levels.size()) && exposureIndex > levels[idx].maxExposureIndex)\n>>> 'levels' have to be sorted then ?\n>> yes ,when readout those levels tuning data from yaml ,those are sorted with\n>> greater\n> I would -at least- document this assumption in this function.\nsure , will add those information into desciption.\n>\n>>> Also, are you willing to take the index of the 'next larger value' ?\n>>>\n>>> In example (using random numbers):\n>>>\n>>> vector levels = { 1, 2, 4, 6, 9, 14, 18 };\n>>> exposureIndex = 15;\n>>>\n>>> should it return 18 or 14 ? (just checking to make sure my below\n>>> suggestion is right)\n>>>\n>>> 14 is expected.\n> This, which -should- match your code\n>\n> \tvector levels = { 1, 2, 4, 6, 9, 14, 18 };\n> \tunsigned int exposureIndex = 15;\n> \tunsigned int idx = 0;\n> \twhile (idx < levels.size() && exposureIndex > levels[idx])\n> \t\t++idx;\n> \tif (idx >= levels.size())\n> \t\tidx = levels.size() - 1;\n> \tcout << \"target: \" << levels[idx] << endl;\n>\n> Will print 18.\n>\n> Have I reimplemented your code wrong ?\n>\n> good founding , looks there is a little issue in this implementation. it will return +1 than I expect,\n> will update this logic in the following patch.\n>>>> +\t\t++idx;\n> V> > > +\tif (idx >= static_cast(levels.size()))\n>>>> +\t\tidx = static_cast(levels.size()) - 1;\n>>>> +\treturn idx;\n>>> \tauto it = std::partition_point(levels.begin(), levels.end() - 1,\n>>> \t\t\t[exposureIndex](const auto &level) {\n>>> \t\t\t\treturn exposureIndex > level.maxExposureIndex;\n>>> \t\t\t});\n>>> \treturn std::distance(levels.begin(), it);\n>>>\n>>> should do and it's safer (hand-rolled while() loops always make me a bit\n>>> unconfortable) (also, you need to include if you take this in).\n>>>\n>>> Thanks\n>>> j\n>> good suggestion , I will excute it like this in the following patch.\n>>>> +}\n>>>> +\n>>>> +} /* namespace ipa::rkisp1::algorithms */\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> --\n>>>> 2.43.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 53EA5BD80A\n\tfor ;\n\tMon, 1 Dec 2025 17:40:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 63DFD60C6F;\n\tMon, 1 Dec 2025 18:40:58 +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 519F5606D5\n\tfor ;\n\tMon, 1 Dec 2025 18:40:57 +0100 (CET)","from [192.168.31.114] (unknown [209.216.103.65])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5D10D6DF;\n\tMon, 1 Dec 2025 18:38:43 +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=\"WsI2jTW+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764610723;\n\tbh=rTH7JWwfmI3p8mxWodKA+tcjCzX99gLrhU05nL/1Ffw=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=WsI2jTW+cgN242wxOou6vKNACIbhsjYM9BvMQSg13g+nD2viPN83vd2L5UUQEyW+W\n\tMPukbRGl+OrI346obHqzNQ0c83HhHTTUmdOV6L/0jtcJPdTonI7BEBq1eT/fvPoWcB\n\tflb8/Dfh9nTCaPDI9xVQZ2HssVE3nB4gwxx2GvDs=","Message-ID":"","Date":"Mon, 1 Dec 2025 12:40:43 -0500","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","References":"<20251125000848.4103786-1-rui.wang@ideasonboard.com>\n\t<20251125000848.4103786-2-rui.wang@ideasonboard.com>\n\t\n\t<3ad522e3-b6a6-4715-a8d5-059f2d6170f3@ideasonboard.com>\n\t","Content-Language":"en-US","From":"rui wang ","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":37156,"web_url":"https://patchwork.libcamera.org/comment/37156/","msgid":"","date":"2025-12-02T08:44:01","subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Rui\n\nOn Mon, Dec 01, 2025 at 12:40:43PM -0500, rui wang wrote:\n>\n> On 2025-12-01 07:07, Jacopo Mondi wrote:\n> > Hi Rui\n> >\n> > On Fri, Nov 28, 2025 at 05:54:21PM -0500, rui wang wrote:\n> > > On 2025-11-28 07:10, Jacopo Mondi wrote:\n> > > > Hi Rui\n> > > >\n> > > > On Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote:\n> > > > > Introduce computeExposureIndex() to derive an exposure index value from\n> > > > > the AGC gain (approximately exposureIndex = gain * 100), and\n> > > > > selectExposureIndexBand() to search through a container of exposure index\n> > > > > levels and select the appropriate tuning band for the current exposure\n> > > > > index.\n> > > > >\n> > > > > These helpers provide reusable functionality for exposure-index-banded\n> > > > > tuning in denoising algorithms, enabling more precise algorithm\n> > > > > configuration based on lighting conditions.\n> > > > >\n> > > > > Signed-off-by: Rui Wang \n> > > > > ---\n> > > > > src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++\n> > > > > 1 file changed, 55 insertions(+)\n> > > > > create mode 100644 src/ipa/rkisp1/algorithms/denoise.h\n> > > > >\n> > > > > diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h\n> > > > > new file mode 100644\n> > > > > index 00000000..8907dc4e\n> > > > > --- /dev/null\n> > > > > +++ b/src/ipa/rkisp1/algorithms/denoise.h\n> > > > > @@ -0,0 +1,55 @@\n> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > > +/*\n> > > > > + * Copyright (C) 2025, Ideas On Board\n> > > > > + *\n> > > > > + * RkISP1 Denoising Algorithms Base Class\n> > > > > + */\n> > > > > +\n> > > > > +#pragma once\n> > > > > +\n> > > > > +#include \"../ipa_context.h\"\n> > > > > +\n> > > > > +#include \"algorithm.h\"\n> > > > > +\n> > > > > +namespace libcamera {\n> > > > > +\n> > > > > +namespace ipa::rkisp1::algorithms {\n> > > > > +\n> > > > > +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm\n> > > > Why a base class ?\n> > > > it seems both the functions implemented here are solely used by the\n> > > > dpf algorithm implementation.\n> > > >\n> > > > I see two ways to handle this, unless I'm missing something\n> > > > 1) Make this a libipa helper\n> > > > 2) Add the two function to the RkISP1 DPF implementation\n> > > >\n> > > In future , filter denoise also share almost same architecture and procedure\n> > > like: auto/manual/off/ mode  switching and yaml parser\n> > > that's why I create a base class for both dpf and filter\n> > > https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/5\n> > >\n> > I don't see much code re-use between the two implementation.\n> >\n> > in denoise.h I see only 4 functions implemented\n> >\n> > - computeExposureIndex() and selectExposureIndexBand()\n> > are these used by Filter too ?\n>\n> yes , all those two function used for dpf and filter auto mode ,\n>\n>  base on exposure index , which can be calculate from current gain value.\n>\n> and selectExposureIndexBand can select specific parameter from yaml file.\n>\n> >\n> > - getControlMap() and fillMetadata()\n> > these seems specific to Denoise, aren't they ?\n> >\n> > Overall, I'm still not convinced deriving Filter and Dpf from a base\n> > class gives us much.\n> >\n>\n> getControlMap() which design for manual mode control,\n> dpf and filter will override its specific controls list.\n\nLooking at that branch:\n- Both DPF and Filter override getControlMap(). They also both call\n the base class implementation (which adds 1 single control) so both\n will add ExposureGainIdex to metadata. The code re-use is minimal\n\n The base class function simply add 1 control to the metadata list.\n\n>\n> I have another branch to refactory filter.cpp , which is as same code\n> structure as dpf.cpp\n>\n> https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/commit/cbc5346d6518c0aca18faf87c2dacd200572a65f,\n>\n>\n> both dpf and filter would override all those virtual functions respectively.\n>\n\nAnd that's my point. The code re-use is minimal but both Dpf and\nFilter now have to implement the same interface for (in my opinion) no\nactual benefit (if not re-using 2 helpers)\n\nAnyway, if you feel strongly about it, go ahead with this.\n\n-\n> > > > > +{\n> > > > > +protected:\n> > > > > +\tDenoiseBaseAlgorithm() = default;\n> > > > > +\t~DenoiseBaseAlgorithm() = default;\n> > > > > +\n> > > > > +\tvirtual uint32_t computeExposureIndex(const IPAContext &context,\n> > > > > +\t\t\t\t\t const IPAFrameContext &frameContext) const;\n> > > > > +\ttemplate\n> > > > > +\tuint32_t selectExposureIndexBand(unsigned exposureIndex,\n> > > > > +\t\t\t\t\t const LevelContainer &levels) const;\n> > > > > +};\n> > > > > +\n> > > > > +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context,\n> > > > > +\t\t\t\t\t\t\t const IPAFrameContext &frameContext) const\n> > > > > +{\n> > > > > +\tdouble ag = frameContext.agc.gain ? frameContext.agc.gain\n> > > > > +\t\t\t\t\t : context.activeState.agc.automatic.gain;\n> > > > does this depend in the AGC has run before the dpf ?\n> > > computeExposureIndex is called only at auto mode at each frame in prepare() .\n> > > DPF will select the group of config from tuning params according to current gain value , so as long as either frameContext.agc.gain or\n> > > context.activeState.agc.automatic.gain populated ,DPF selector can be updated automaticly.\n> > >\n> > Yes, but the behaviour migh be different.\n> >\n> > If AGC runs before DPF the gain in the frame context will be used\n> > If DPF runs first AGC won't have populated frameContext.agc.gain yet\n> > and we'll use the active state which reflect the most up-to-date gain\n> > value used but which might refere to some frames before.\n> >\n> > As said, we have a limitation that algorithms are run in order of\n> > declaration, so depending on the tuning file structure we might have\n> > different behaviours\n>\n> as metioned above, AGC value only be needed at denoise auto mode . But by\n> default , dpf is disable , it need to be enable from control, yes, it need\n\nThat doesn't matter. An application can start DPF at start() and the\nalgorithms implementation cannot make assumptions on how the algorithm\nwill be used\n\n> to up-to-date value from agc . and current exposure index determined by\n> current gain value.\n>\n> gain value is float type , and computeExposureIndex will return like\n> :100/200/400/800/1600/* , the only agc change dramaticly(cross band) will\n> trigger denoise's profile update.\n\nThat I get it.\n\nMy point is again\n1) The ordering in which AGC and DPF are run, which is now relevant\n2) The fact you get the gain value from activeState.automatic and the\nAGC can run in manual mode\n\n>\n> doubleag=frameContext.agc.gain?frameContext.agc.gain\n> :context.activeState.agc.automatic.gain;\n> will add Warning for current active agc value denoise.h\n> > > > We can't rely on the algorithms declaration order, we need something\n> > > > better indeed (not for this series of course)\n> > > >\n> > > > If this depends on the algorithms ordering I would\n> > > > 1) Try to see if the frameContext.agc.gain field is populated\n> > > > 2) if it's not log a Warn message (possibily just once?) to state that\n> > > > the DPF is running before AGC and results might not be accurate ?\n> > and that's why I suggested warning about that\n> >\n> > > The auto DPF the scalar range can be set in tuning file base on gain range\n> > > like band :\n> > >\n> > >   100/200/400/800/1600/3200  ,\n> > >\n> > > which AGC calculate value would mapping to band : ag * 100.0 + 0.5 ,\n\nDoes the 'gain * 100' thing come from some specification or is it an\narbitrary way to index the exposures bandings ?\n\n> > >\n> > > so once exposure become convergency  , then DPF will be updated following\n> > > that . but all those depends on\n> > >\n> > > frameContext.agc.gain from IPA\n> > >\n> > > > Is this necessary in your opinion ?\n> > > >\n> > > > Also, what does guarantee that automatic.gain is valid ? What if AGC\n> > > > is running in manual mode ?\n> > > in manual mode or other mode ,it is not rely on this AGC, the DPF config\n> > > will be read from camshark controls value.\n> > Do you mean that computeExposureIndex will only be called in auto\n> > mode? I presume this is DPF auto-mode. Does it imply AGC auto mode ?\n> >\n> > Or can the DPF run with auto and AGC with manual ?\n> DPF auto mode applies config which determined by current exposure\n> index(compute from current AGC value) . No matter current AGC mode is auto\n> or manual.\n\nIt does if you fetch activeState.-automatic- doesn't it ?\n\n\n> > > > > +\treturn static_cast(ag * 100.0 + 0.5);\n> > > > nit: we usually \"unsigned int\"\n> > > yes , I will try to change to that i\n> > > > > +}\n> > > > > +\n> > > > > +template\n> > > > Is using a template without specifying any restrictions on the\n> > > > templated type desirable ? The only user of this function passes in a\n> > > > vector, why do you need templating ?\n> > > >\n> > > > If we need templating, I guess we should make sure that the LevelContainer\n> > > > template argument implements the interface you use in the below code\n> > > in filter denoise , it has similiar LevelContainer but different structure ,\n> > > so I choose template to support both two denoise\n> > >\n> > > data structure.\n> > aren't two overlads better ? Or two templates specializations specific\n> > to the two containers you'll have to support.\n> >\n> > Isn't a template function which makes assumptions on the interface of the\n> > template argument without restricting it using traits potentially a\n> > cause for compilation errors ?\n> >\n> > > > > +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex,\n> > > > > +\t\t\t\t\t\t const LevelContainer &levels) const\n> > > > > +{\n> > > > > +\tif (levels.empty())\n> > > > > +\t\treturn -1;\n> > > > Does this gets converted to UINT_MAX as you return a unsigned int ?\n> > > >\n> > > > You call this as:\n> > > >\n> > > > int32_t idx =\n> > > > DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex,\n> > > > exposureIndexLevels_);\n> > > > \t\tif (idx >= 0) {\n> > > > \t\t\tconfig_ = exposureIndexLevels_[idx].dpf;\n> > > > \t\t\tstrengthConfig_ = exposureIndexLevels_[idx].strength;\n> > > > \t\t\tlastExposureGainIndex_ = idx;\n> > > > \t\t}\n> > > >\n> > > > So I guess the returned value is implicitly casted back to a signed\n> > > > integer, but the function prototype should be changed to reflect that.\n> > > >\n> > > >\n> > > > > +\tuint32_t idx = 0;\n> > > > if you use std::size_t you can probably spare all the casts ? (my\n> > > > compiler doesn't complain if I remove all casts and idx statys an\n> > > > uint32_t though).\n> > > >\n> > > >\n> > > > > +\twhile (idx < static_cast(levels.size()) && exposureIndex > levels[idx].maxExposureIndex)\n> > > > 'levels' have to be sorted then ?\n> > > yes ,when readout those levels tuning data from yaml ,those are sorted with\n> > > greater\n> > I would -at least- document this assumption in this function.\n> sure , will add those information into desciption.\n> >\n> > > > Also, are you willing to take the index of the 'next larger value' ?\n> > > >\n> > > > In example (using random numbers):\n> > > >\n> > > > vector levels = { 1, 2, 4, 6, 9, 14, 18 };\n> > > > exposureIndex = 15;\n> > > >\n> > > > should it return 18 or 14 ? (just checking to make sure my below\n> > > > suggestion is right)\n> > > >\n> > > > 14 is expected.\n> > This, which -should- match your code\n> >\n> > \tvector levels = { 1, 2, 4, 6, 9, 14, 18 };\n> > \tunsigned int exposureIndex = 15;\n> > \tunsigned int idx = 0;\n> > \twhile (idx < levels.size() && exposureIndex > levels[idx])\n> > \t\t++idx;\n> > \tif (idx >= levels.size())\n> > \t\tidx = levels.size() - 1;\n> > \tcout << \"target: \" << levels[idx] << endl;\n> >\n> > Will print 18.\n> >\n> > Have I reimplemented your code wrong ?\n> >\n> > good founding , looks there is a little issue in this implementation. it will return +1 than I expect,\n> > will update this logic in the following patch.\n> > > > > +\t\t++idx;\n> > V> > > +\tif (idx >= static_cast(levels.size()))\n> > > > > +\t\tidx = static_cast(levels.size()) - 1;\n> > > > > +\treturn idx;\n> > > > \tauto it = std::partition_point(levels.begin(), levels.end() - 1,\n> > > > \t\t\t[exposureIndex](const auto &level) {\n> > > > \t\t\t\treturn exposureIndex > level.maxExposureIndex;\n> > > > \t\t\t});\n> > > > \treturn std::distance(levels.begin(), it);\n> > > >\n> > > > should do and it's safer (hand-rolled while() loops always make me a bit\n> > > > unconfortable) (also, you need to include if you take this in).\n> > > >\n> > > > Thanks\n> > > > j\n> > > good suggestion , I will excute it like this in the following patch.\n> > > > > +}\n> > > > > +\n> > > > > +} /* namespace ipa::rkisp1::algorithms */\n> > > > > +\n> > > > > +} /* namespace libcamera */\n> > > > > --\n> > > > > 2.43.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 5DCEABD80A\n\tfor ;\n\tTue, 2 Dec 2025 08:44:07 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7FCAF60C73;\n\tTue, 2 Dec 2025 09:44:06 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BC3FD609E0\n\tfor ;\n\tTue, 2 Dec 2025 09:44:04 +0100 (CET)","from ideasonboard.com (net-93-65-100-155.cust.vodafonedsl.it\n\t[93.65.100.155])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C6251161;\n\tTue, 2 Dec 2025 09:41:50 +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=\"dO2m8b1m\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1764664910;\n\tbh=mKhN/HttYEt5USCeEiEw4VPqAmtHMF+Sypxd64x7/+o=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=dO2m8b1mnfOUAVsJEb+jR4XdSFc2p8gF6SizO5LUSTHn5dCZy6xhEGAG6x8wbJ/fc\n\tE7HMz8pn/3DMwvAo+EYKvSgF3MOvf3m+CJePgAD1R+yiDuNlJVC/4JFsGj82EXf43H\n\tmaoamX40wFad3KZQ5M02nJ2wpUzbH70BWXxX3WMU=","Date":"Tue, 2 Dec 2025 09:44:01 +0100","From":"Jacopo Mondi ","To":"rui wang ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 01/11] ipa: rkisp1: algorithms: Add exposure index\n\tcomputation helpers","Message-ID":"","References":"<20251125000848.4103786-1-rui.wang@ideasonboard.com>\n\t<20251125000848.4103786-2-rui.wang@ideasonboard.com>\n\t\n\t<3ad522e3-b6a6-4715-a8d5-059f2d6170f3@ideasonboard.com>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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\" "}}]