| Message ID | 20251114-exposure-limits-v3-15-b7c07feba026@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
2025. 11. 14. 15:17 keltezéssel, Jacopo Mondi írta: > The AgcMeanLuminance::resetFrameCount() function has to be called > after a call to AgcMeanLuminance::configure(). As the two calls always > happen one after another, do not require each IPA implementation to do > that but fold instead the call to resetFrameCount() in > AgcMeanLuminance::configure(). > > Update the AgcMeanLuminance class documentation accordingly. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > src/ipa/ipu3/algorithms/agc.cpp | 2 -- > src/ipa/libipa/agc_mean_luminance.cpp | 19 ++++++------------- > src/ipa/libipa/agc_mean_luminance.h | 9 ++++----- > src/ipa/mali-c55/algorithms/agc.cpp | 2 -- > src/ipa/rkisp1/algorithms/agc.cpp | 2 -- > 5 files changed, 10 insertions(+), 24 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 002ee574c02b79c25834a9d87a5881a9de52e39e..c9d41f93cff5b81710b76592303f1e0d10697326 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -128,8 +128,6 @@ int Agc::configure(IPAContext &context, > > /* \todo Update AGC limits when FrameDurationLimits is passed in */ > > - resetFrameCount(); > - > return 0; > } > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > index 9fc275ea9e5b81ce107eabe1982be3c44c01479c..725a23ef2f6f612c6d3408701246db7415fd8327 100644 > --- a/src/ipa/libipa/agc_mean_luminance.cpp > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > @@ -171,9 +171,10 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95; > * > * IPA modules that want to use this class to implement their AEGC algorithm > * should derive it and provide an overriding estimateLuminance() function for > - * this class to use. They must call parseTuningData() in init(), and must also > - * call resetFrameCounter() in configure(). They may then use calculateNewEv() > - * in process(). To update the algorithm limits for example, in response to a > + * this class to use. They must call parseTuningData() in init() and the use the > + * sensor configuration data to call AgcMeanLuminance::configure() in their > + * configure() implementation. They may then use calculateNewEv() in process(). > + * To update the algorithm limits for example, in response to a > * FrameDurationLimit control being passed in queueRequest()) then > * setExposureLimits() must be called with the new values. > */ > @@ -379,6 +380,8 @@ void AgcMeanLuminance::configure(const SensorConfiguration &config, > > helper->configure(sensorConfig, sensorHelper); > } > + > + resetFrameCount(); > } > > /** > @@ -692,16 +695,6 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > return exposureModeHelper->splitExposure(newExposureValue); > } > > -/** > - * \fn AgcMeanLuminance::resetFrameCount() > - * \brief Reset the frame counter > - * > - * This function resets the internal frame counter, which exists to help the > - * algorithm decide whether it should respond instantly or not. The expectation > - * is for derived classes to call this function before each camera start call in > - * their configure() function. > - */ > - > } /* namespace ipa */ > > } /* namespace libcamera */ > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > index 12316ca8bbd7d8b5783a948f5e01d5f0f56bfe3a..acbefc4e5765413bc803417eae1dbd0a943bc95e 100644 > --- a/src/ipa/libipa/agc_mean_luminance.h > +++ b/src/ipa/libipa/agc_mean_luminance.h > @@ -87,11 +87,6 @@ public: > > double effectiveYTarget() const; > > - void resetFrameCount() > - { > - frameCount_ = 0; > - } > - > private: > virtual double estimateLuminance(const double gain) const = 0; > > @@ -104,6 +99,10 @@ private: > const Histogram &hist, > double gain); > utils::Duration filterExposure(utils::Duration exposureValue); I'd probably add an empty line here. > + void resetFrameCount() > + { > + frameCount_ = 0; > + } > > double exposureCompensation_; > uint64_t frameCount_; > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > index 731b29ced1030ecb3f44b83ad28a0691dd5d2f0d..91b1438f7e5ca0498373c86fd75b91f9c5a81c3f 100644 > --- a/src/ipa/mali-c55/algorithms/agc.cpp > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > @@ -186,8 +186,6 @@ int Agc::configure(IPAContext &context, > > /* \todo Update AGC limits when FrameDurationLimits is passed in */ > > - resetFrameCount(); > - > return 0; > } > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index c8210e175186a282faf586378c5a0a761612047c..b9a94ba03c910f73420579dd6737d8d46b26e576 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -213,8 +213,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > context.activeState.agc.automatic.yTarget = effectiveYTarget(); > > - resetFrameCount(); > - > return 0; > } > >
Hi Jacopo, Thank you for the patch. Quoting Jacopo Mondi (2025-11-14 15:17:10) > The AgcMeanLuminance::resetFrameCount() function has to be called > after a call to AgcMeanLuminance::configure(). As the two calls always > happen one after another, do not require each IPA implementation to do > that but fold instead the call to resetFrameCount() in > AgcMeanLuminance::configure(). > > Update the AgcMeanLuminance class documentation accordingly. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 2 -- > src/ipa/libipa/agc_mean_luminance.cpp | 19 ++++++------------- > src/ipa/libipa/agc_mean_luminance.h | 9 ++++----- > src/ipa/mali-c55/algorithms/agc.cpp | 2 -- > src/ipa/rkisp1/algorithms/agc.cpp | 2 -- > 5 files changed, 10 insertions(+), 24 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 002ee574c02b79c25834a9d87a5881a9de52e39e..c9d41f93cff5b81710b76592303f1e0d10697326 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -128,8 +128,6 @@ int Agc::configure(IPAContext &context, > > /* \todo Update AGC limits when FrameDurationLimits is passed in */ > > - resetFrameCount(); > - > return 0; > } > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > index 9fc275ea9e5b81ce107eabe1982be3c44c01479c..725a23ef2f6f612c6d3408701246db7415fd8327 100644 > --- a/src/ipa/libipa/agc_mean_luminance.cpp > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > @@ -171,9 +171,10 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95; > * > * IPA modules that want to use this class to implement their AEGC algorithm > * should derive it and provide an overriding estimateLuminance() function for > - * this class to use. They must call parseTuningData() in init(), and must also > - * call resetFrameCounter() in configure(). They may then use calculateNewEv() > - * in process(). To update the algorithm limits for example, in response to a > + * this class to use. They must call parseTuningData() in init() and the use the > + * sensor configuration data to call AgcMeanLuminance::configure() in their > + * configure() implementation. They may then use calculateNewEv() in process(). > + * To update the algorithm limits for example, in response to a > * FrameDurationLimit control being passed in queueRequest()) then > * setExposureLimits() must be called with the new values. > */ > @@ -379,6 +380,8 @@ void AgcMeanLuminance::configure(const SensorConfiguration &config, > > helper->configure(sensorConfig, sensorHelper); > } > + > + resetFrameCount(); > } > > /** > @@ -692,16 +695,6 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > return exposureModeHelper->splitExposure(newExposureValue); > } > > -/** > - * \fn AgcMeanLuminance::resetFrameCount() > - * \brief Reset the frame counter > - * > - * This function resets the internal frame counter, which exists to help the > - * algorithm decide whether it should respond instantly or not. The expectation > - * is for derived classes to call this function before each camera start call in > - * their configure() function. > - */ > - > } /* namespace ipa */ > > } /* namespace libcamera */ > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > index 12316ca8bbd7d8b5783a948f5e01d5f0f56bfe3a..acbefc4e5765413bc803417eae1dbd0a943bc95e 100644 > --- a/src/ipa/libipa/agc_mean_luminance.h > +++ b/src/ipa/libipa/agc_mean_luminance.h > @@ -87,11 +87,6 @@ public: > > double effectiveYTarget() const; > > - void resetFrameCount() > - { > - frameCount_ = 0; > - } > - > private: > virtual double estimateLuminance(const double gain) const = 0; > > @@ -104,6 +99,10 @@ private: > const Histogram &hist, > double gain); > utils::Duration filterExposure(utils::Duration exposureValue); > + void resetFrameCount() > + { > + frameCount_ = 0; > + } Can't we just drop the whole function and move that line into configure()? It is only called from one place. Anyways: Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Best regards, Stefan > > double exposureCompensation_; > uint64_t frameCount_; > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > index 731b29ced1030ecb3f44b83ad28a0691dd5d2f0d..91b1438f7e5ca0498373c86fd75b91f9c5a81c3f 100644 > --- a/src/ipa/mali-c55/algorithms/agc.cpp > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > @@ -186,8 +186,6 @@ int Agc::configure(IPAContext &context, > > /* \todo Update AGC limits when FrameDurationLimits is passed in */ > > - resetFrameCount(); > - > return 0; > } > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index c8210e175186a282faf586378c5a0a761612047c..b9a94ba03c910f73420579dd6737d8d46b26e576 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -213,8 +213,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) > > context.activeState.agc.automatic.yTarget = effectiveYTarget(); > > - resetFrameCount(); > - > return 0; > } > > > -- > 2.51.1 >
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 002ee574c02b79c25834a9d87a5881a9de52e39e..c9d41f93cff5b81710b76592303f1e0d10697326 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -128,8 +128,6 @@ int Agc::configure(IPAContext &context, /* \todo Update AGC limits when FrameDurationLimits is passed in */ - resetFrameCount(); - return 0; } diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp index 9fc275ea9e5b81ce107eabe1982be3c44c01479c..725a23ef2f6f612c6d3408701246db7415fd8327 100644 --- a/src/ipa/libipa/agc_mean_luminance.cpp +++ b/src/ipa/libipa/agc_mean_luminance.cpp @@ -171,9 +171,10 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95; * * IPA modules that want to use this class to implement their AEGC algorithm * should derive it and provide an overriding estimateLuminance() function for - * this class to use. They must call parseTuningData() in init(), and must also - * call resetFrameCounter() in configure(). They may then use calculateNewEv() - * in process(). To update the algorithm limits for example, in response to a + * this class to use. They must call parseTuningData() in init() and the use the + * sensor configuration data to call AgcMeanLuminance::configure() in their + * configure() implementation. They may then use calculateNewEv() in process(). + * To update the algorithm limits for example, in response to a * FrameDurationLimit control being passed in queueRequest()) then * setExposureLimits() must be called with the new values. */ @@ -379,6 +380,8 @@ void AgcMeanLuminance::configure(const SensorConfiguration &config, helper->configure(sensorConfig, sensorHelper); } + + resetFrameCount(); } /** @@ -692,16 +695,6 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, return exposureModeHelper->splitExposure(newExposureValue); } -/** - * \fn AgcMeanLuminance::resetFrameCount() - * \brief Reset the frame counter - * - * This function resets the internal frame counter, which exists to help the - * algorithm decide whether it should respond instantly or not. The expectation - * is for derived classes to call this function before each camera start call in - * their configure() function. - */ - } /* namespace ipa */ } /* namespace libcamera */ diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h index 12316ca8bbd7d8b5783a948f5e01d5f0f56bfe3a..acbefc4e5765413bc803417eae1dbd0a943bc95e 100644 --- a/src/ipa/libipa/agc_mean_luminance.h +++ b/src/ipa/libipa/agc_mean_luminance.h @@ -87,11 +87,6 @@ public: double effectiveYTarget() const; - void resetFrameCount() - { - frameCount_ = 0; - } - private: virtual double estimateLuminance(const double gain) const = 0; @@ -104,6 +99,10 @@ private: const Histogram &hist, double gain); utils::Duration filterExposure(utils::Duration exposureValue); + void resetFrameCount() + { + frameCount_ = 0; + } double exposureCompensation_; uint64_t frameCount_; diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp index 731b29ced1030ecb3f44b83ad28a0691dd5d2f0d..91b1438f7e5ca0498373c86fd75b91f9c5a81c3f 100644 --- a/src/ipa/mali-c55/algorithms/agc.cpp +++ b/src/ipa/mali-c55/algorithms/agc.cpp @@ -186,8 +186,6 @@ int Agc::configure(IPAContext &context, /* \todo Update AGC limits when FrameDurationLimits is passed in */ - resetFrameCount(); - return 0; } diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index c8210e175186a282faf586378c5a0a761612047c..b9a94ba03c910f73420579dd6737d8d46b26e576 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -213,8 +213,6 @@ int Agc::configure(IPAContext &context, const IPACameraSensorInfo &configInfo) context.activeState.agc.automatic.yTarget = effectiveYTarget(); - resetFrameCount(); - return 0; }
The AgcMeanLuminance::resetFrameCount() function has to be called after a call to AgcMeanLuminance::configure(). As the two calls always happen one after another, do not require each IPA implementation to do that but fold instead the call to resetFrameCount() in AgcMeanLuminance::configure(). Update the AgcMeanLuminance class documentation accordingly. Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 2 -- src/ipa/libipa/agc_mean_luminance.cpp | 19 ++++++------------- src/ipa/libipa/agc_mean_luminance.h | 9 ++++----- src/ipa/mali-c55/algorithms/agc.cpp | 2 -- src/ipa/rkisp1/algorithms/agc.cpp | 2 -- 5 files changed, 10 insertions(+), 24 deletions(-)