| Message ID | 20251125000848.4103786-2-rui.wang@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Rui On Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote: > Introduce computeExposureIndex() to derive an exposure index value from > the AGC gain (approximately exposureIndex = gain * 100), and > selectExposureIndexBand() to search through a container of exposure index > levels and select the appropriate tuning band for the current exposure > index. > > These helpers provide reusable functionality for exposure-index-banded > tuning in denoising algorithms, enabling more precise algorithm > configuration based on lighting conditions. > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> > --- > src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++ > 1 file changed, 55 insertions(+) > create mode 100644 src/ipa/rkisp1/algorithms/denoise.h > > diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h > new file mode 100644 > index 00000000..8907dc4e > --- /dev/null > +++ b/src/ipa/rkisp1/algorithms/denoise.h > @@ -0,0 +1,55 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2025, Ideas On Board > + * > + * RkISP1 Denoising Algorithms Base Class > + */ > + > +#pragma once > + > +#include "../ipa_context.h" > + > +#include "algorithm.h" > + > +namespace libcamera { > + > +namespace ipa::rkisp1::algorithms { > + > +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm Why a base class ? it seems both the functions implemented here are solely used by the dpf algorithm implementation. I see two ways to handle this, unless I'm missing something 1) Make this a libipa helper 2) Add the two function to the RkISP1 DPF implementation > +{ > +protected: > + DenoiseBaseAlgorithm() = default; > + ~DenoiseBaseAlgorithm() = default; > + > + virtual uint32_t computeExposureIndex(const IPAContext &context, > + const IPAFrameContext &frameContext) const; > + template<typename LevelContainer> > + uint32_t selectExposureIndexBand(unsigned exposureIndex, > + const LevelContainer &levels) const; > +}; > + > +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context, > + const IPAFrameContext &frameContext) const > +{ > + double ag = frameContext.agc.gain ? frameContext.agc.gain > + : context.activeState.agc.automatic.gain; does this depend in the AGC has run before the dpf ? We can't rely on the algorithms declaration order, we need something better indeed (not for this series of course) If this depends on the algorithms ordering I would 1) Try to see if the frameContext.agc.gain field is populated 2) if it's not log a Warn message (possibily just once?) to state that the DPF is running before AGC and results might not be accurate ? Is this necessary in your opinion ? Also, what does guarantee that automatic.gain is valid ? What if AGC is running in manual mode ? > + return static_cast<unsigned>(ag * 100.0 + 0.5); nit: we usually "unsigned int" > +} > + > +template<typename LevelContainer> Is using a template without specifying any restrictions on the templated type desirable ? The only user of this function passes in a vector, why do you need templating ? If we need templating, I guess we should make sure that the LevelContainer template argument implements the interface you use in the below code > +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex, > + const LevelContainer &levels) const > +{ > + if (levels.empty()) > + return -1; Does this gets converted to UINT_MAX as you return a unsigned int ? You call this as: int32_t idx = DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex, exposureIndexLevels_); if (idx >= 0) { config_ = exposureIndexLevels_[idx].dpf; strengthConfig_ = exposureIndexLevels_[idx].strength; lastExposureGainIndex_ = idx; } So I guess the returned value is implicitly casted back to a signed integer, but the function prototype should be changed to reflect that. > + uint32_t idx = 0; if you use std::size_t you can probably spare all the casts ? (my compiler doesn't complain if I remove all casts and idx statys an uint32_t though). > + while (idx < static_cast<uint32_t>(levels.size()) && exposureIndex > levels[idx].maxExposureIndex) 'levels' have to be sorted then ? Also, are you willing to take the index of the 'next larger value' ? In example (using random numbers): vector<unsigned int> levels = { 1, 2, 4, 6, 9, 14, 18 }; exposureIndex = 15; should it return 18 or 14 ? (just checking to make sure my below suggestion is right) > + ++idx; > + if (idx >= static_cast<uint32_t>(levels.size())) > + idx = static_cast<uint32_t>(levels.size()) - 1; > + return idx; auto it = std::partition_point(levels.begin(), levels.end() - 1, [exposureIndex](const auto &level) { return exposureIndex > level.maxExposureIndex; }); return std::distance(levels.begin(), it); should do and it's safer (hand-rolled while() loops always make me a bit unconfortable) (also, you need to include <algorithm> if you take this in). Thanks j > +} > + > +} /* namespace ipa::rkisp1::algorithms */ > + > +} /* namespace libcamera */ > -- > 2.43.0 >
On 2025-11-28 07:10, Jacopo Mondi wrote: > Hi Rui > > On Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote: >> Introduce computeExposureIndex() to derive an exposure index value from >> the AGC gain (approximately exposureIndex = gain * 100), and >> selectExposureIndexBand() to search through a container of exposure index >> levels and select the appropriate tuning band for the current exposure >> index. >> >> These helpers provide reusable functionality for exposure-index-banded >> tuning in denoising algorithms, enabling more precise algorithm >> configuration based on lighting conditions. >> >> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> >> --- >> src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++ >> 1 file changed, 55 insertions(+) >> create mode 100644 src/ipa/rkisp1/algorithms/denoise.h >> >> diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h >> new file mode 100644 >> index 00000000..8907dc4e >> --- /dev/null >> +++ b/src/ipa/rkisp1/algorithms/denoise.h >> @@ -0,0 +1,55 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2025, Ideas On Board >> + * >> + * RkISP1 Denoising Algorithms Base Class >> + */ >> + >> +#pragma once >> + >> +#include "../ipa_context.h" >> + >> +#include "algorithm.h" >> + >> +namespace libcamera { >> + >> +namespace ipa::rkisp1::algorithms { >> + >> +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm > Why a base class ? > it seems both the functions implemented here are solely used by the > dpf algorithm implementation. > > I see two ways to handle this, unless I'm missing something > 1) Make this a libipa helper > 2) Add the two function to the RkISP1 DPF implementation > In future , filter denoise also share almost same architecture and procedure like: auto/manual/off/ mode switching and yaml parser that's why I create a base class for both dpf and filter https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/5 >> +{ >> +protected: >> + DenoiseBaseAlgorithm() = default; >> + ~DenoiseBaseAlgorithm() = default; >> + >> + virtual uint32_t computeExposureIndex(const IPAContext &context, >> + const IPAFrameContext &frameContext) const; >> + template<typename LevelContainer> >> + uint32_t selectExposureIndexBand(unsigned exposureIndex, >> + const LevelContainer &levels) const; >> +}; >> + >> +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context, >> + const IPAFrameContext &frameContext) const >> +{ >> + double ag = frameContext.agc.gain ? frameContext.agc.gain >> + : context.activeState.agc.automatic.gain; > does this depend in the AGC has run before the dpf ? computeExposureIndex is called only at auto mode at each frame in prepare() . DPF will select the group of config from tuning params according to current gain value , so as long as either frameContext.agc.gain or context.activeState.agc.automatic.gain populated ,DPF selector can be updated automaticly. > > We can't rely on the algorithms declaration order, we need something > better indeed (not for this series of course) > > If this depends on the algorithms ordering I would > 1) Try to see if the frameContext.agc.gain field is populated > 2) if it's not log a Warn message (possibily just once?) to state that > the DPF is running before AGC and results might not be accurate ? The auto DPF the scalar range can be set in tuning file base on gain range like band : 100/200/400/800/1600/3200 , which AGC calculate value would mapping to band : ag * 100.0 + 0.5 , so once exposure become convergency , then DPF will be updated following that . but all those depends on frameContext.agc.gain from IPA > > Is this necessary in your opinion ? > > Also, what does guarantee that automatic.gain is valid ? What if AGC > is running in manual mode ? in manual mode or other mode ,it is not rely on this AGC, the DPF config will be read from camshark controls value. > >> + return static_cast<unsigned>(ag * 100.0 + 0.5); > nit: we usually "unsigned int" yes , I will try to change to that i > >> +} >> + >> +template<typename LevelContainer> > Is using a template without specifying any restrictions on the > templated type desirable ? The only user of this function passes in a > vector, why do you need templating ? > > If we need templating, I guess we should make sure that the LevelContainer > template argument implements the interface you use in the below code in filter denoise , it has similiar LevelContainer but different structure , so I choose template to support both two denoise data structure. >> +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex, >> + const LevelContainer &levels) const >> +{ >> + if (levels.empty()) >> + return -1; > Does this gets converted to UINT_MAX as you return a unsigned int ? > > You call this as: > > int32_t idx = > DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex, > exposureIndexLevels_); > if (idx >= 0) { > config_ = exposureIndexLevels_[idx].dpf; > strengthConfig_ = exposureIndexLevels_[idx].strength; > lastExposureGainIndex_ = idx; > } > > So I guess the returned value is implicitly casted back to a signed > integer, but the function prototype should be changed to reflect that. > > >> + uint32_t idx = 0; > if you use std::size_t you can probably spare all the casts ? (my > compiler doesn't complain if I remove all casts and idx statys an > uint32_t though). > > >> + while (idx < static_cast<uint32_t>(levels.size()) && exposureIndex > levels[idx].maxExposureIndex) > 'levels' have to be sorted then ? yes ,when readout those levels tuning data from yaml ,those are sorted with greater > > Also, are you willing to take the index of the 'next larger value' ? > > In example (using random numbers): > > vector<unsigned int> levels = { 1, 2, 4, 6, 9, 14, 18 }; > exposureIndex = 15; > > should it return 18 or 14 ? (just checking to make sure my below > suggestion is right) > > 14 is expected. >> + ++idx; >> + if (idx >= static_cast<uint32_t>(levels.size())) >> + idx = static_cast<uint32_t>(levels.size()) - 1; >> + return idx; > auto it = std::partition_point(levels.begin(), levels.end() - 1, > [exposureIndex](const auto &level) { > return exposureIndex > level.maxExposureIndex; > }); > return std::distance(levels.begin(), it); > > should do and it's safer (hand-rolled while() loops always make me a bit > unconfortable) (also, you need to include <algorithm> if you take this in). > > Thanks > j good suggestion , I will excute it like this in the following patch. > >> +} >> + >> +} /* namespace ipa::rkisp1::algorithms */ >> + >> +} /* namespace libcamera */ >> -- >> 2.43.0 >>
Hi Rui On Fri, Nov 28, 2025 at 05:54:21PM -0500, rui wang wrote: > > On 2025-11-28 07:10, Jacopo Mondi wrote: > > Hi Rui > > > > On Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote: > > > Introduce computeExposureIndex() to derive an exposure index value from > > > the AGC gain (approximately exposureIndex = gain * 100), and > > > selectExposureIndexBand() to search through a container of exposure index > > > levels and select the appropriate tuning band for the current exposure > > > index. > > > > > > These helpers provide reusable functionality for exposure-index-banded > > > tuning in denoising algorithms, enabling more precise algorithm > > > configuration based on lighting conditions. > > > > > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> > > > --- > > > src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++ > > > 1 file changed, 55 insertions(+) > > > create mode 100644 src/ipa/rkisp1/algorithms/denoise.h > > > > > > diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h > > > new file mode 100644 > > > index 00000000..8907dc4e > > > --- /dev/null > > > +++ b/src/ipa/rkisp1/algorithms/denoise.h > > > @@ -0,0 +1,55 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2025, Ideas On Board > > > + * > > > + * RkISP1 Denoising Algorithms Base Class > > > + */ > > > + > > > +#pragma once > > > + > > > +#include "../ipa_context.h" > > > + > > > +#include "algorithm.h" > > > + > > > +namespace libcamera { > > > + > > > +namespace ipa::rkisp1::algorithms { > > > + > > > +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm > > Why a base class ? > > it seems both the functions implemented here are solely used by the > > dpf algorithm implementation. > > > > I see two ways to handle this, unless I'm missing something > > 1) Make this a libipa helper > > 2) Add the two function to the RkISP1 DPF implementation > > > In future , filter denoise also share almost same architecture and procedure > like: auto/manual/off/ mode switching and yaml parser > that's why I create a base class for both dpf and filter > https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/5 > I don't see much code re-use between the two implementation. in denoise.h I see only 4 functions implemented - computeExposureIndex() and selectExposureIndexBand() are these used by Filter too ? - getControlMap() and fillMetadata() these seems specific to Denoise, aren't they ? Overall, I'm still not convinced deriving Filter and Dpf from a base class gives us much. > > > +{ > > > +protected: > > > + DenoiseBaseAlgorithm() = default; > > > + ~DenoiseBaseAlgorithm() = default; > > > + > > > + virtual uint32_t computeExposureIndex(const IPAContext &context, > > > + const IPAFrameContext &frameContext) const; > > > + template<typename LevelContainer> > > > + uint32_t selectExposureIndexBand(unsigned exposureIndex, > > > + const LevelContainer &levels) const; > > > +}; > > > + > > > +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context, > > > + const IPAFrameContext &frameContext) const > > > +{ > > > + double ag = frameContext.agc.gain ? frameContext.agc.gain > > > + : context.activeState.agc.automatic.gain; > > does this depend in the AGC has run before the dpf ? > > computeExposureIndex is called only at auto mode at each frame in prepare() . > DPF will select the group of config from tuning params according to current gain value , so as long as either frameContext.agc.gain or > context.activeState.agc.automatic.gain populated ,DPF selector can be updated automaticly. > Yes, but the behaviour migh be different. If AGC runs before DPF the gain in the frame context will be used If DPF runs first AGC won't have populated frameContext.agc.gain yet and we'll use the active state which reflect the most up-to-date gain value used but which might refere to some frames before. As said, we have a limitation that algorithms are run in order of declaration, so depending on the tuning file structure we might have different behaviours > > > > We can't rely on the algorithms declaration order, we need something > > better indeed (not for this series of course) > > > > If this depends on the algorithms ordering I would > > 1) Try to see if the frameContext.agc.gain field is populated > > 2) if it's not log a Warn message (possibily just once?) to state that > > the DPF is running before AGC and results might not be accurate ? and that's why I suggested warning about that > > The auto DPF the scalar range can be set in tuning file base on gain range > like band : > > 100/200/400/800/1600/3200 , > > which AGC calculate value would mapping to band : ag * 100.0 + 0.5 , > > so once exposure become convergency , then DPF will be updated following > that . but all those depends on > > frameContext.agc.gain from IPA > > > > > Is this necessary in your opinion ? > > > > Also, what does guarantee that automatic.gain is valid ? What if AGC > > is running in manual mode ? > in manual mode or other mode ,it is not rely on this AGC, the DPF config > will be read from camshark controls value. Do you mean that computeExposureIndex will only be called in auto mode? I presume this is DPF auto-mode. Does it imply AGC auto mode ? Or can the DPF run with auto and AGC with manual ? > > > > > > + return static_cast<unsigned>(ag * 100.0 + 0.5); > > nit: we usually "unsigned int" > yes , I will try to change to that i > > > > > +} > > > + > > > +template<typename LevelContainer> > > Is using a template without specifying any restrictions on the > > templated type desirable ? The only user of this function passes in a > > vector, why do you need templating ? > > > > If we need templating, I guess we should make sure that the LevelContainer > > template argument implements the interface you use in the below code > > in filter denoise , it has similiar LevelContainer but different structure , > so I choose template to support both two denoise > > data structure. aren't two overlads better ? Or two templates specializations specific to the two containers you'll have to support. Isn't a template function which makes assumptions on the interface of the template argument without restricting it using traits potentially a cause for compilation errors ? > > > > +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex, > > > + const LevelContainer &levels) const > > > +{ > > > + if (levels.empty()) > > > + return -1; > > Does this gets converted to UINT_MAX as you return a unsigned int ? > > > > You call this as: > > > > int32_t idx = > > DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex, > > exposureIndexLevels_); > > if (idx >= 0) { > > config_ = exposureIndexLevels_[idx].dpf; > > strengthConfig_ = exposureIndexLevels_[idx].strength; > > lastExposureGainIndex_ = idx; > > } > > > > So I guess the returned value is implicitly casted back to a signed > > integer, but the function prototype should be changed to reflect that. > > > > > > > + uint32_t idx = 0; > > if you use std::size_t you can probably spare all the casts ? (my > > compiler doesn't complain if I remove all casts and idx statys an > > uint32_t though). > > > > > > > + while (idx < static_cast<uint32_t>(levels.size()) && exposureIndex > levels[idx].maxExposureIndex) > > 'levels' have to be sorted then ? > yes ,when readout those levels tuning data from yaml ,those are sorted with > greater I would -at least- document this assumption in this function. > > > > Also, are you willing to take the index of the 'next larger value' ? > > > > In example (using random numbers): > > > > vector<unsigned int> levels = { 1, 2, 4, 6, 9, 14, 18 }; > > exposureIndex = 15; > > > > should it return 18 or 14 ? (just checking to make sure my below > > suggestion is right) > > > > 14 is expected. This, which -should- match your code vector<unsigned int> levels = { 1, 2, 4, 6, 9, 14, 18 }; unsigned int exposureIndex = 15; unsigned int idx = 0; while (idx < levels.size() && exposureIndex > levels[idx]) ++idx; if (idx >= levels.size()) idx = levels.size() - 1; cout << "target: " << levels[idx] << endl; Will print 18. Have I reimplemented your code wrong ? > > > + ++idx; V> > > + if (idx >= static_cast<uint32_t>(levels.size())) > > > + idx = static_cast<uint32_t>(levels.size()) - 1; > > > + return idx; > > auto it = std::partition_point(levels.begin(), levels.end() - 1, > > [exposureIndex](const auto &level) { > > return exposureIndex > level.maxExposureIndex; > > }); > > return std::distance(levels.begin(), it); > > > > should do and it's safer (hand-rolled while() loops always make me a bit > > unconfortable) (also, you need to include <algorithm> if you take this in). > > > > Thanks > > j > good suggestion , I will excute it like this in the following patch. > > > > > +} > > > + > > > +} /* namespace ipa::rkisp1::algorithms */ > > > + > > > +} /* namespace libcamera */ > > > -- > > > 2.43.0 > > >
On 2025-12-01 07:07, Jacopo Mondi wrote: > Hi Rui > > On Fri, Nov 28, 2025 at 05:54:21PM -0500, rui wang wrote: >> On 2025-11-28 07:10, Jacopo Mondi wrote: >>> Hi Rui >>> >>> On Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote: >>>> Introduce computeExposureIndex() to derive an exposure index value from >>>> the AGC gain (approximately exposureIndex = gain * 100), and >>>> selectExposureIndexBand() to search through a container of exposure index >>>> levels and select the appropriate tuning band for the current exposure >>>> index. >>>> >>>> These helpers provide reusable functionality for exposure-index-banded >>>> tuning in denoising algorithms, enabling more precise algorithm >>>> configuration based on lighting conditions. >>>> >>>> Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> >>>> --- >>>> src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++ >>>> 1 file changed, 55 insertions(+) >>>> create mode 100644 src/ipa/rkisp1/algorithms/denoise.h >>>> >>>> diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h >>>> new file mode 100644 >>>> index 00000000..8907dc4e >>>> --- /dev/null >>>> +++ b/src/ipa/rkisp1/algorithms/denoise.h >>>> @@ -0,0 +1,55 @@ >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>> +/* >>>> + * Copyright (C) 2025, Ideas On Board >>>> + * >>>> + * RkISP1 Denoising Algorithms Base Class >>>> + */ >>>> + >>>> +#pragma once >>>> + >>>> +#include "../ipa_context.h" >>>> + >>>> +#include "algorithm.h" >>>> + >>>> +namespace libcamera { >>>> + >>>> +namespace ipa::rkisp1::algorithms { >>>> + >>>> +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm >>> Why a base class ? >>> it seems both the functions implemented here are solely used by the >>> dpf algorithm implementation. >>> >>> I see two ways to handle this, unless I'm missing something >>> 1) Make this a libipa helper >>> 2) Add the two function to the RkISP1 DPF implementation >>> >> In future , filter denoise also share almost same architecture and procedure >> like: auto/manual/off/ mode switching and yaml parser >> that's why I create a base class for both dpf and filter >> https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/5 >> > I don't see much code re-use between the two implementation. > > in denoise.h I see only 4 functions implemented > > - computeExposureIndex() and selectExposureIndexBand() > are these used by Filter too ? yes , all those two function used for dpf and filter auto mode , base on exposure index , which can be calculate from current gain value. and selectExposureIndexBand can select specific parameter from yaml file. > > - getControlMap() and fillMetadata() > these seems specific to Denoise, aren't they ? > > Overall, I'm still not convinced deriving Filter and Dpf from a base > class gives us much. > getControlMap() which design for manual mode control, dpf and filter will override its specific controls list. I have another branch to refactory filter.cpp , which is as same code structure as dpf.cpp https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/commit/cbc5346d6518c0aca18faf87c2dacd200572a65f, both dpf and filter would override all those virtual functions respectively. >>>> +{ >>>> +protected: >>>> + DenoiseBaseAlgorithm() = default; >>>> + ~DenoiseBaseAlgorithm() = default; >>>> + >>>> + virtual uint32_t computeExposureIndex(const IPAContext &context, >>>> + const IPAFrameContext &frameContext) const; >>>> + template<typename LevelContainer> >>>> + uint32_t selectExposureIndexBand(unsigned exposureIndex, >>>> + const LevelContainer &levels) const; >>>> +}; >>>> + >>>> +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context, >>>> + const IPAFrameContext &frameContext) const >>>> +{ >>>> + double ag = frameContext.agc.gain ? frameContext.agc.gain >>>> + : context.activeState.agc.automatic.gain; >>> does this depend in the AGC has run before the dpf ? >> computeExposureIndex is called only at auto mode at each frame in prepare() . >> DPF will select the group of config from tuning params according to current gain value , so as long as either frameContext.agc.gain or >> context.activeState.agc.automatic.gain populated ,DPF selector can be updated automaticly. >> > Yes, but the behaviour migh be different. > > If AGC runs before DPF the gain in the frame context will be used > If DPF runs first AGC won't have populated frameContext.agc.gain yet > and we'll use the active state which reflect the most up-to-date gain > value used but which might refere to some frames before. > > As said, we have a limitation that algorithms are run in order of > declaration, so depending on the tuning file structure we might have > different behaviours as metioned above, AGC value only be needed at denoise auto mode . But by default , dpf is disable , it need to be enable from control, yes, it need to up-to-date value from agc . and current exposure index determined by current gain value. gain value is float type , and computeExposureIndex will return like :100/200/400/800/1600/* , the only agc change dramaticly(cross band) will trigger denoise's profile update. doubleag=frameContext.agc.gain?frameContext.agc.gain :context.activeState.agc.automatic.gain; will add Warning for current active agc value denoise.h >>> We can't rely on the algorithms declaration order, we need something >>> better indeed (not for this series of course) >>> >>> If this depends on the algorithms ordering I would >>> 1) Try to see if the frameContext.agc.gain field is populated >>> 2) if it's not log a Warn message (possibily just once?) to state that >>> the DPF is running before AGC and results might not be accurate ? > and that's why I suggested warning about that > >> The auto DPF the scalar range can be set in tuning file base on gain range >> like band : >> >> 100/200/400/800/1600/3200 , >> >> which AGC calculate value would mapping to band : ag * 100.0 + 0.5 , >> >> so once exposure become convergency , then DPF will be updated following >> that . but all those depends on >> >> frameContext.agc.gain from IPA >> >>> Is this necessary in your opinion ? >>> >>> Also, what does guarantee that automatic.gain is valid ? What if AGC >>> is running in manual mode ? >> in manual mode or other mode ,it is not rely on this AGC, the DPF config >> will be read from camshark controls value. > Do you mean that computeExposureIndex will only be called in auto > mode? I presume this is DPF auto-mode. Does it imply AGC auto mode ? > > Or can the DPF run with auto and AGC with manual ? DPF auto mode applies config which determined by current exposure index(compute from current AGC value) . No matter current AGC mode is auto or manual. >>>> + return static_cast<unsigned>(ag * 100.0 + 0.5); >>> nit: we usually "unsigned int" >> yes , I will try to change to that i >>>> +} >>>> + >>>> +template<typename LevelContainer> >>> Is using a template without specifying any restrictions on the >>> templated type desirable ? The only user of this function passes in a >>> vector, why do you need templating ? >>> >>> If we need templating, I guess we should make sure that the LevelContainer >>> template argument implements the interface you use in the below code >> in filter denoise , it has similiar LevelContainer but different structure , >> so I choose template to support both two denoise >> >> data structure. > aren't two overlads better ? Or two templates specializations specific > to the two containers you'll have to support. > > Isn't a template function which makes assumptions on the interface of the > template argument without restricting it using traits potentially a > cause for compilation errors ? > >>>> +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex, >>>> + const LevelContainer &levels) const >>>> +{ >>>> + if (levels.empty()) >>>> + return -1; >>> Does this gets converted to UINT_MAX as you return a unsigned int ? >>> >>> You call this as: >>> >>> int32_t idx = >>> DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex, >>> exposureIndexLevels_); >>> if (idx >= 0) { >>> config_ = exposureIndexLevels_[idx].dpf; >>> strengthConfig_ = exposureIndexLevels_[idx].strength; >>> lastExposureGainIndex_ = idx; >>> } >>> >>> So I guess the returned value is implicitly casted back to a signed >>> integer, but the function prototype should be changed to reflect that. >>> >>> >>>> + uint32_t idx = 0; >>> if you use std::size_t you can probably spare all the casts ? (my >>> compiler doesn't complain if I remove all casts and idx statys an >>> uint32_t though). >>> >>> >>>> + while (idx < static_cast<uint32_t>(levels.size()) && exposureIndex > levels[idx].maxExposureIndex) >>> 'levels' have to be sorted then ? >> yes ,when readout those levels tuning data from yaml ,those are sorted with >> greater > I would -at least- document this assumption in this function. sure , will add those information into desciption. > >>> Also, are you willing to take the index of the 'next larger value' ? >>> >>> In example (using random numbers): >>> >>> vector<unsigned int> levels = { 1, 2, 4, 6, 9, 14, 18 }; >>> exposureIndex = 15; >>> >>> should it return 18 or 14 ? (just checking to make sure my below >>> suggestion is right) >>> >>> 14 is expected. > This, which -should- match your code > > vector<unsigned int> levels = { 1, 2, 4, 6, 9, 14, 18 }; > unsigned int exposureIndex = 15; > unsigned int idx = 0; > while (idx < levels.size() && exposureIndex > levels[idx]) > ++idx; > if (idx >= levels.size()) > idx = levels.size() - 1; > cout << "target: " << levels[idx] << endl; > > Will print 18. > > Have I reimplemented your code wrong ? > > good founding , looks there is a little issue in this implementation. it will return +1 than I expect, > will update this logic in the following patch. >>>> + ++idx; > V> > > + if (idx >= static_cast<uint32_t>(levels.size())) >>>> + idx = static_cast<uint32_t>(levels.size()) - 1; >>>> + return idx; >>> auto it = std::partition_point(levels.begin(), levels.end() - 1, >>> [exposureIndex](const auto &level) { >>> return exposureIndex > level.maxExposureIndex; >>> }); >>> return std::distance(levels.begin(), it); >>> >>> should do and it's safer (hand-rolled while() loops always make me a bit >>> unconfortable) (also, you need to include <algorithm> if you take this in). >>> >>> Thanks >>> j >> good suggestion , I will excute it like this in the following patch. >>>> +} >>>> + >>>> +} /* namespace ipa::rkisp1::algorithms */ >>>> + >>>> +} /* namespace libcamera */ >>>> -- >>>> 2.43.0 >>>>
Hi Rui On Mon, Dec 01, 2025 at 12:40:43PM -0500, rui wang wrote: > > On 2025-12-01 07:07, Jacopo Mondi wrote: > > Hi Rui > > > > On Fri, Nov 28, 2025 at 05:54:21PM -0500, rui wang wrote: > > > On 2025-11-28 07:10, Jacopo Mondi wrote: > > > > Hi Rui > > > > > > > > On Mon, Nov 24, 2025 at 07:08:38PM -0500, Rui Wang wrote: > > > > > Introduce computeExposureIndex() to derive an exposure index value from > > > > > the AGC gain (approximately exposureIndex = gain * 100), and > > > > > selectExposureIndexBand() to search through a container of exposure index > > > > > levels and select the appropriate tuning band for the current exposure > > > > > index. > > > > > > > > > > These helpers provide reusable functionality for exposure-index-banded > > > > > tuning in denoising algorithms, enabling more precise algorithm > > > > > configuration based on lighting conditions. > > > > > > > > > > Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> > > > > > --- > > > > > src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++ > > > > > 1 file changed, 55 insertions(+) > > > > > create mode 100644 src/ipa/rkisp1/algorithms/denoise.h > > > > > > > > > > diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h > > > > > new file mode 100644 > > > > > index 00000000..8907dc4e > > > > > --- /dev/null > > > > > +++ b/src/ipa/rkisp1/algorithms/denoise.h > > > > > @@ -0,0 +1,55 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2025, Ideas On Board > > > > > + * > > > > > + * RkISP1 Denoising Algorithms Base Class > > > > > + */ > > > > > + > > > > > +#pragma once > > > > > + > > > > > +#include "../ipa_context.h" > > > > > + > > > > > +#include "algorithm.h" > > > > > + > > > > > +namespace libcamera { > > > > > + > > > > > +namespace ipa::rkisp1::algorithms { > > > > > + > > > > > +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm > > > > Why a base class ? > > > > it seems both the functions implemented here are solely used by the > > > > dpf algorithm implementation. > > > > > > > > I see two ways to handle this, unless I'm missing something > > > > 1) Make this a libipa helper > > > > 2) Add the two function to the RkISP1 DPF implementation > > > > > > > In future , filter denoise also share almost same architecture and procedure > > > like: auto/manual/off/ mode switching and yaml parser > > > that's why I create a base class for both dpf and filter > > > https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/pulls/5 > > > > > I don't see much code re-use between the two implementation. > > > > in denoise.h I see only 4 functions implemented > > > > - computeExposureIndex() and selectExposureIndexBand() > > are these used by Filter too ? > > yes , all those two function used for dpf and filter auto mode , > > base on exposure index , which can be calculate from current gain value. > > and selectExposureIndexBand can select specific parameter from yaml file. > > > > > - getControlMap() and fillMetadata() > > these seems specific to Denoise, aren't they ? > > > > Overall, I'm still not convinced deriving Filter and Dpf from a base > > class gives us much. > > > > getControlMap() which design for manual mode control, > dpf and filter will override its specific controls list. Looking at that branch: - Both DPF and Filter override getControlMap(). They also both call the base class implementation (which adds 1 single control) so both will add ExposureGainIdex to metadata. The code re-use is minimal The base class function simply add 1 control to the metadata list. > > I have another branch to refactory filter.cpp , which is as same code > structure as dpf.cpp > > https://git.ideasonboard.com/ruiwang2002/libcamera_rui_1/commit/cbc5346d6518c0aca18faf87c2dacd200572a65f, > > > both dpf and filter would override all those virtual functions respectively. > And that's my point. The code re-use is minimal but both Dpf and Filter now have to implement the same interface for (in my opinion) no actual benefit (if not re-using 2 helpers) Anyway, if you feel strongly about it, go ahead with this. - > > > > > +{ > > > > > +protected: > > > > > + DenoiseBaseAlgorithm() = default; > > > > > + ~DenoiseBaseAlgorithm() = default; > > > > > + > > > > > + virtual uint32_t computeExposureIndex(const IPAContext &context, > > > > > + const IPAFrameContext &frameContext) const; > > > > > + template<typename LevelContainer> > > > > > + uint32_t selectExposureIndexBand(unsigned exposureIndex, > > > > > + const LevelContainer &levels) const; > > > > > +}; > > > > > + > > > > > +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context, > > > > > + const IPAFrameContext &frameContext) const > > > > > +{ > > > > > + double ag = frameContext.agc.gain ? frameContext.agc.gain > > > > > + : context.activeState.agc.automatic.gain; > > > > does this depend in the AGC has run before the dpf ? > > > computeExposureIndex is called only at auto mode at each frame in prepare() . > > > DPF will select the group of config from tuning params according to current gain value , so as long as either frameContext.agc.gain or > > > context.activeState.agc.automatic.gain populated ,DPF selector can be updated automaticly. > > > > > Yes, but the behaviour migh be different. > > > > If AGC runs before DPF the gain in the frame context will be used > > If DPF runs first AGC won't have populated frameContext.agc.gain yet > > and we'll use the active state which reflect the most up-to-date gain > > value used but which might refere to some frames before. > > > > As said, we have a limitation that algorithms are run in order of > > declaration, so depending on the tuning file structure we might have > > different behaviours > > as metioned above, AGC value only be needed at denoise auto mode . But by > default , dpf is disable , it need to be enable from control, yes, it need That doesn't matter. An application can start DPF at start() and the algorithms implementation cannot make assumptions on how the algorithm will be used > to up-to-date value from agc . and current exposure index determined by > current gain value. > > gain value is float type , and computeExposureIndex will return like > :100/200/400/800/1600/* , the only agc change dramaticly(cross band) will > trigger denoise's profile update. That I get it. My point is again 1) The ordering in which AGC and DPF are run, which is now relevant 2) The fact you get the gain value from activeState.automatic and the AGC can run in manual mode > > doubleag=frameContext.agc.gain?frameContext.agc.gain > :context.activeState.agc.automatic.gain; > will add Warning for current active agc value denoise.h > > > > We can't rely on the algorithms declaration order, we need something > > > > better indeed (not for this series of course) > > > > > > > > If this depends on the algorithms ordering I would > > > > 1) Try to see if the frameContext.agc.gain field is populated > > > > 2) if it's not log a Warn message (possibily just once?) to state that > > > > the DPF is running before AGC and results might not be accurate ? > > and that's why I suggested warning about that > > > > > The auto DPF the scalar range can be set in tuning file base on gain range > > > like band : > > > > > > 100/200/400/800/1600/3200 , > > > > > > which AGC calculate value would mapping to band : ag * 100.0 + 0.5 , Does the 'gain * 100' thing come from some specification or is it an arbitrary way to index the exposures bandings ? > > > > > > so once exposure become convergency , then DPF will be updated following > > > that . but all those depends on > > > > > > frameContext.agc.gain from IPA > > > > > > > Is this necessary in your opinion ? > > > > > > > > Also, what does guarantee that automatic.gain is valid ? What if AGC > > > > is running in manual mode ? > > > in manual mode or other mode ,it is not rely on this AGC, the DPF config > > > will be read from camshark controls value. > > Do you mean that computeExposureIndex will only be called in auto > > mode? I presume this is DPF auto-mode. Does it imply AGC auto mode ? > > > > Or can the DPF run with auto and AGC with manual ? > DPF auto mode applies config which determined by current exposure > index(compute from current AGC value) . No matter current AGC mode is auto > or manual. It does if you fetch activeState.-automatic- doesn't it ? > > > > > + return static_cast<unsigned>(ag * 100.0 + 0.5); > > > > nit: we usually "unsigned int" > > > yes , I will try to change to that i > > > > > +} > > > > > + > > > > > +template<typename LevelContainer> > > > > Is using a template without specifying any restrictions on the > > > > templated type desirable ? The only user of this function passes in a > > > > vector, why do you need templating ? > > > > > > > > If we need templating, I guess we should make sure that the LevelContainer > > > > template argument implements the interface you use in the below code > > > in filter denoise , it has similiar LevelContainer but different structure , > > > so I choose template to support both two denoise > > > > > > data structure. > > aren't two overlads better ? Or two templates specializations specific > > to the two containers you'll have to support. > > > > Isn't a template function which makes assumptions on the interface of the > > template argument without restricting it using traits potentially a > > cause for compilation errors ? > > > > > > > +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex, > > > > > + const LevelContainer &levels) const > > > > > +{ > > > > > + if (levels.empty()) > > > > > + return -1; > > > > Does this gets converted to UINT_MAX as you return a unsigned int ? > > > > > > > > You call this as: > > > > > > > > int32_t idx = > > > > DenoiseBaseAlgorithm::selectExposureIndexBand(exposureIndex, > > > > exposureIndexLevels_); > > > > if (idx >= 0) { > > > > config_ = exposureIndexLevels_[idx].dpf; > > > > strengthConfig_ = exposureIndexLevels_[idx].strength; > > > > lastExposureGainIndex_ = idx; > > > > } > > > > > > > > So I guess the returned value is implicitly casted back to a signed > > > > integer, but the function prototype should be changed to reflect that. > > > > > > > > > > > > > + uint32_t idx = 0; > > > > if you use std::size_t you can probably spare all the casts ? (my > > > > compiler doesn't complain if I remove all casts and idx statys an > > > > uint32_t though). > > > > > > > > > > > > > + while (idx < static_cast<uint32_t>(levels.size()) && exposureIndex > levels[idx].maxExposureIndex) > > > > 'levels' have to be sorted then ? > > > yes ,when readout those levels tuning data from yaml ,those are sorted with > > > greater > > I would -at least- document this assumption in this function. > sure , will add those information into desciption. > > > > > > Also, are you willing to take the index of the 'next larger value' ? > > > > > > > > In example (using random numbers): > > > > > > > > vector<unsigned int> levels = { 1, 2, 4, 6, 9, 14, 18 }; > > > > exposureIndex = 15; > > > > > > > > should it return 18 or 14 ? (just checking to make sure my below > > > > suggestion is right) > > > > > > > > 14 is expected. > > This, which -should- match your code > > > > vector<unsigned int> levels = { 1, 2, 4, 6, 9, 14, 18 }; > > unsigned int exposureIndex = 15; > > unsigned int idx = 0; > > while (idx < levels.size() && exposureIndex > levels[idx]) > > ++idx; > > if (idx >= levels.size()) > > idx = levels.size() - 1; > > cout << "target: " << levels[idx] << endl; > > > > Will print 18. > > > > Have I reimplemented your code wrong ? > > > > good founding , looks there is a little issue in this implementation. it will return +1 than I expect, > > will update this logic in the following patch. > > > > > + ++idx; > > V> > > + if (idx >= static_cast<uint32_t>(levels.size())) > > > > > + idx = static_cast<uint32_t>(levels.size()) - 1; > > > > > + return idx; > > > > auto it = std::partition_point(levels.begin(), levels.end() - 1, > > > > [exposureIndex](const auto &level) { > > > > return exposureIndex > level.maxExposureIndex; > > > > }); > > > > return std::distance(levels.begin(), it); > > > > > > > > should do and it's safer (hand-rolled while() loops always make me a bit > > > > unconfortable) (also, you need to include <algorithm> if you take this in). > > > > > > > > Thanks > > > > j > > > good suggestion , I will excute it like this in the following patch. > > > > > +} > > > > > + > > > > > +} /* namespace ipa::rkisp1::algorithms */ > > > > > + > > > > > +} /* namespace libcamera */ > > > > > -- > > > > > 2.43.0 > > > > >
diff --git a/src/ipa/rkisp1/algorithms/denoise.h b/src/ipa/rkisp1/algorithms/denoise.h new file mode 100644 index 00000000..8907dc4e --- /dev/null +++ b/src/ipa/rkisp1/algorithms/denoise.h @@ -0,0 +1,55 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2025, Ideas On Board + * + * RkISP1 Denoising Algorithms Base Class + */ + +#pragma once + +#include "../ipa_context.h" + +#include "algorithm.h" + +namespace libcamera { + +namespace ipa::rkisp1::algorithms { + +class DenoiseBaseAlgorithm : public ipa::rkisp1::Algorithm +{ +protected: + DenoiseBaseAlgorithm() = default; + ~DenoiseBaseAlgorithm() = default; + + virtual uint32_t computeExposureIndex(const IPAContext &context, + const IPAFrameContext &frameContext) const; + template<typename LevelContainer> + uint32_t selectExposureIndexBand(unsigned exposureIndex, + const LevelContainer &levels) const; +}; + +inline unsigned DenoiseBaseAlgorithm::computeExposureIndex(const IPAContext &context, + const IPAFrameContext &frameContext) const +{ + double ag = frameContext.agc.gain ? frameContext.agc.gain + : context.activeState.agc.automatic.gain; + return static_cast<unsigned>(ag * 100.0 + 0.5); +} + +template<typename LevelContainer> +uint32_t DenoiseBaseAlgorithm::selectExposureIndexBand(unsigned exposureIndex, + const LevelContainer &levels) const +{ + if (levels.empty()) + return -1; + uint32_t idx = 0; + while (idx < static_cast<uint32_t>(levels.size()) && exposureIndex > levels[idx].maxExposureIndex) + ++idx; + if (idx >= static_cast<uint32_t>(levels.size())) + idx = static_cast<uint32_t>(levels.size()) - 1; + return idx; +} + +} /* namespace ipa::rkisp1::algorithms */ + +} /* namespace libcamera */
Introduce computeExposureIndex() to derive an exposure index value from the AGC gain (approximately exposureIndex = gain * 100), and selectExposureIndexBand() to search through a container of exposure index levels and select the appropriate tuning band for the current exposure index. These helpers provide reusable functionality for exposure-index-banded tuning in denoising algorithms, enabling more precise algorithm configuration based on lighting conditions. Signed-off-by: Rui Wang <rui.wang@ideasonboard.com> --- src/ipa/rkisp1/algorithms/denoise.h | 55 +++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 src/ipa/rkisp1/algorithms/denoise.h