| Message ID | 20250818082909.2001635-2-stefan.klug@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Quoting Stefan Klug (2025-08-18 09:28:39) > To use AgcMeanLuminance an algorithm derives from it and overrides the > estimateLuminance() function. This override is the only reason to > derive from this class. It has however two downsides: > - It is more difficult for the reader to know which functionality comes > from where. > - It is necessary to keep references to data needed in the > estimateLuminance() function inside the class, even though that date > is only accessed inside AgcMenaLuminance::calculateNewEv(). > > Remove the need to derive from AgcMeanLuminance by passing > estimateLuminance as function object into calculateNewEv(). > As someone who wants to integrate AgcMeanLuminance in another IPA, personally I think I like the idea of having the AgcMeanLuminance as a clean separate object which is 'easy' to integrate into IPA modules. - the interface might even closely resemble the current module > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 4 +- > src/ipa/ipu3/algorithms/agc.h | 2 +- > src/ipa/libipa/agc_mean_luminance.cpp | 57 ++++++++++++++------------- > src/ipa/libipa/agc_mean_luminance.h | 9 +++-- > src/ipa/mali-c55/algorithms/agc.cpp | 5 ++- > src/ipa/mali-c55/algorithms/agc.h | 2 +- > src/ipa/rkisp1/algorithms/agc.cpp | 23 ++++++----- > src/ipa/rkisp1/algorithms/agc.h | 5 +-- > 8 files changed, 60 insertions(+), 47 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 39d0aebb0838..98a034a47625 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -27,6 +27,7 @@ > namespace libcamera { > > using namespace std::literals::chrono_literals; > +using namespace std::placeholders; > > namespace ipa::ipu3::algorithms { > > @@ -224,7 +225,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > utils::Duration newExposureTime; > double aGain, dGain; > std::tie(newExposureTime, aGain, dGain) = > - calculateNewEv(context.activeState.agc.constraintMode, > + calculateNewEv(std::bind(&Agc::estimateLuminance, this, _1), > + context.activeState.agc.constraintMode, > context.activeState.agc.exposureMode, hist, > effectiveExposureValue); Shouldn't this be more like: agcMean.calculateNewEv(context.activeState.agc.constraintMode, context.activeState.agc.exposureMode, hist, effectiveExposureValue); Oh - it's about passing in the estimateLuminance function ... which has custom parameters. Ok - so that's the only part I'm less certain about so far.. > > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > index 890c271b4462..8e182cd7cff3 100644 > --- a/src/ipa/ipu3/algorithms/agc.h > +++ b/src/ipa/ipu3/algorithms/agc.h > @@ -38,7 +38,7 @@ public: > ControlList &metadata) override; > > private: > - double estimateLuminance(double gain) const override; > + double estimateLuminance(double gain) const; > Histogram parseStatistics(const ipu3_uapi_stats_3a *stats, > const ipu3_uapi_grid_config &grid); > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > index ff96a381ffce..df7efb284a89 100644 > --- a/src/ipa/libipa/agc_mean_luminance.cpp > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > @@ -18,7 +18,7 @@ using namespace libcamera::controls; > > /** > * \file agc_mean_luminance.h > - * \brief Base class implementing mean luminance AEGC > + * \brief Class implementing mean luminance AEGC > */ > > namespace libcamera { > @@ -53,6 +53,21 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16; > */ > static constexpr double kMaxRelativeLuminanceTarget = 0.95; > > +/** > + * \fn AgcMeanLuminance::EstimateLuminanceFn > + * \brief Function to estimate the mean luminance given a gain > + * \param[in] gain The gain with which to adjust the luminance estimate > + * > + * Callback functions of this type are used within \a calculateNewEv to estimate > + * the average relative luminance of the frame that would be output by the > + * sensor if an additional \a gain was applied. It is a is implemented as > + * callback function because estimation of luminance is a hardware-specific > + * operation, which depends wholly on the format of the stats that are delivered > + * to libcamera from the ISP. > + * > + * \return The normalised relative luminance of the image > + */ > + > /** > * \struct AgcMeanLuminance::AgcConstraint > * \brief The boundaries and target for an AeConstraintMode constraint > @@ -133,13 +148,11 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95; > * will determine the supportable precision of the constraints. > * > * 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 setLimits() and resetFrameCounter() in configure(). They may then use > - * calculateNewEv() in process(). If the limits passed to setLimits() change for > - * any reason (for example, in response to a FrameDurationLimit control being > - * passed in queueRequest()) then setLimits() must be called again with the new > - * values. > + * must call parseTuningData() in init(), and must also call setLimits() and > + * resetFrameCounter() in configure(). They may then use calculateNewEv() in > + * process(). If the limits passed to setLimits() change for any reason (for > + * example, in response to a FrameDurationLimit control being passed in > + * queueRequest()) then setLimits() must be called again with the new values. > */ > > AgcMeanLuminance::AgcMeanLuminance() > @@ -420,28 +433,12 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime, > * \brief Get the controls that have been generated after parsing tuning data > */ > > -/** > - * \fn AgcMeanLuminance::estimateLuminance(const double gain) > - * \brief Estimate the luminance of an image, adjusted by a given gain > - * \param[in] gain The gain with which to adjust the luminance estimate > - * > - * This function estimates the average relative luminance of the frame that > - * would be output by the sensor if an additional \a gain was applied. It is a > - * pure virtual function because estimation of luminance is a hardware-specific > - * operation, which depends wholly on the format of the stats that are delivered > - * to libcamera from the ISP. Derived classes must override this function with > - * one that calculates the normalised mean luminance value across the entire > - * image. > - * > - * \return The normalised relative luminance of the image > - */ > - > /** > * \brief Estimate the initial gain needed to achieve a relative luminance > * target > * \return The calculated initial gain > */ > -double AgcMeanLuminance::estimateInitialGain() const > +double AgcMeanLuminance::estimateInitialGain(EstimateLuminanceFn estimateLuminance) const > { > double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_, > kMaxRelativeLuminanceTarget); > @@ -542,6 +539,7 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue) > /** > * \brief Calculate the new exposure value and splut it between exposure time > * and gain > + * \param[in] estimateLuminance A function to get luminance estimates > * \param[in] constraintModeIndex The index of the current constraint mode > * \param[in] exposureModeIndex The index of the current exposure mode > * \param[in] yHist A Histogram from the ISP statistics to use in constraining > @@ -553,10 +551,15 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue) > * exposure value is filtered to prevent rapid changes from frame to frame, and > * divided into exposure time, analogue and digital gain. > * > + * The \a estimateLuminance shall estimate the average relative luminance of the > + * frame that would be output by the sensor if an additional \a gain was > + * applied. > + * > * \return Tuple of exposure time, analogue gain, and digital gain > */ > std::tuple<utils::Duration, double, double> > -AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > +AgcMeanLuminance::calculateNewEv(EstimateLuminanceFn estimateLuminance, > + uint32_t constraintModeIndex, > uint32_t exposureModeIndex, > const Histogram &yHist, > utils::Duration effectiveExposureValue) > @@ -580,7 +583,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > return exposureModeHelper->splitExposure(10ms); > } > > - double gain = estimateInitialGain(); > + double gain = estimateInitialGain(estimateLuminance); > gain = constraintClampGain(constraintModeIndex, yHist, gain); > > /* > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > index cad7ef845487..5e4c598c8ab9 100644 > --- a/src/ipa/libipa/agc_mean_luminance.h > +++ b/src/ipa/libipa/agc_mean_luminance.h > @@ -31,6 +31,8 @@ public: > AgcMeanLuminance(); > virtual ~AgcMeanLuminance(); > > + using EstimateLuminanceFn = std::function<double(double)>; > + > struct AgcConstraint { > enum class Bound { > Lower = 0, > @@ -68,7 +70,8 @@ public: > } > > std::tuple<utils::Duration, double, double> > - calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex, > + calculateNewEv(EstimateLuminanceFn estimateLuminance, > + uint32_t constraintModeIndex, uint32_t exposureModeIndex, > const Histogram &yHist, utils::Duration effectiveExposureValue); > > void resetFrameCount() > @@ -77,13 +80,11 @@ public: > } > > private: > - virtual double estimateLuminance(const double gain) const = 0; > - > void parseRelativeLuminanceTarget(const YamlObject &tuningData); > void parseConstraint(const YamlObject &modeDict, int32_t id); > int parseConstraintModes(const YamlObject &tuningData); > int parseExposureModes(const YamlObject &tuningData); > - double estimateInitialGain() const; > + double estimateInitialGain(EstimateLuminanceFn estimateLuminance) const; > double constraintClampGain(uint32_t constraintModeIndex, > const Histogram &hist, > double gain); > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > index 15963994b2d6..4bfb4aaeedf0 100644 > --- a/src/ipa/mali-c55/algorithms/agc.cpp > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > @@ -8,6 +8,7 @@ > #include "agc.h" > > #include <cmath> > +#include <functional> > > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > @@ -21,6 +22,7 @@ > namespace libcamera { > > using namespace std::literals::chrono_literals; > +using namespace std::placeholders; > > namespace ipa::mali_c55::algorithms { > > @@ -383,7 +385,8 @@ void Agc::process(IPAContext &context, > utils::Duration shutterTime; > double aGain, dGain; > std::tie(shutterTime, aGain, dGain) = > - calculateNewEv(activeState.agc.constraintMode, > + calculateNewEv(std::bind(&Agc::estimateLuminance, this, _1), > + activeState.agc.constraintMode, > activeState.agc.exposureMode, statistics_.yHist, > effectiveExposureValue); > > diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h > index 0b4bf7eda1c2..698de57e1ba8 100644 > --- a/src/ipa/mali-c55/algorithms/agc.h > +++ b/src/ipa/mali-c55/algorithms/agc.h > @@ -64,7 +64,7 @@ public: > ControlList &metadata) override; > > private: > - double estimateLuminance(const double gain) const override; > + double estimateLuminance(const double gain) const; > size_t fillGainParamBlock(IPAContext &context, > IPAFrameContext &frameContext, > mali_c55_params_block block); > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 35440b67e999..3a078f9753f6 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -440,16 +440,17 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext, > * > * \return The relative luminance > */ > -double Agc::estimateLuminance(double gain) const > +double Agc::estimateLuminance(Span<const uint8_t> expMeans, > + Span<const uint8_t> weights, double gain) > { > - ASSERT(expMeans_.size() == weights_.size()); > + ASSERT(expMeans.size() == weights.size()); > double ySum = 0.0; > double wSum = 0.0; > > /* Sum the averages, saturated to 255. */ > - for (unsigned i = 0; i < expMeans_.size(); i++) { > - double w = weights_[i]; > - ySum += std::min(expMeans_[i] * gain, 255.0) * w; > + for (unsigned i = 0; i < expMeans.size(); i++) { > + double w = weights[i]; > + ySum += std::min(expMeans[i] * gain, 255.0) * w; > wSum += w; > } > > @@ -522,9 +523,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > /* The lower 4 bits are fractional and meant to be discarded. */ > Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins }, > [](uint32_t x) { return x >> 4; }); > - expMeans_ = { params->ae.exp_mean, context.hw->numAeCells }; > std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode); > - weights_ = { modeWeights.data(), modeWeights.size() }; > > /* > * Set the AGC limits using the fixed exposure time and/or gain in > @@ -566,10 +565,17 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > setExposureCompensation(pow(2.0, frameContext.agc.exposureValue)); > > + AgcMeanLuminance::EstimateLuminanceFn estimateLuminanceFn = std::bind( > + &Agc::estimateLuminance, this, > + Span<const uint8_t>(params->ae.exp_mean, context.hw->numAeCells), > + Span<const uint8_t>(modeWeights.data(), modeWeights.size()), > + std::placeholders::_1); > + > utils::Duration newExposureTime; > double aGain, dGain; > std::tie(newExposureTime, aGain, dGain) = > - calculateNewEv(frameContext.agc.constraintMode, > + calculateNewEv(estimateLuminanceFn, > + frameContext.agc.constraintMode, > frameContext.agc.exposureMode, > hist, effectiveExposureValue); > > @@ -590,7 +596,6 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > std::max(frameContext.agc.minFrameDuration, newExposureTime)); > > fillMetadata(context, frameContext, metadata); > - expMeans_ = {}; > } > > REGISTER_IPA_ALGORITHM(Agc, "Agc") > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > index 7867eed9c4e3..742c8f7371f3 100644 > --- a/src/ipa/rkisp1/algorithms/agc.h > +++ b/src/ipa/rkisp1/algorithms/agc.h > @@ -43,19 +43,18 @@ public: > ControlList &metadata) override; > > private: > + double estimateLuminance(Span<const uint8_t> expMeans, > + Span<const uint8_t> weights, double gain); > int parseMeteringModes(IPAContext &context, const YamlObject &tuningData); > uint8_t computeHistogramPredivider(const Size &size, > enum rkisp1_cif_isp_histogram_mode mode); > > void fillMetadata(IPAContext &context, IPAFrameContext &frameContext, > ControlList &metadata); > - double estimateLuminance(double gain) const override; > void processFrameDuration(IPAContext &context, > IPAFrameContext &frameContext, > utils::Duration frameDuration); > > - Span<const uint8_t> expMeans_; > - Span<const uint8_t> weights_; > > std::map<int32_t, std::vector<uint8_t>> meteringModes_; > }; > -- > 2.48.1 >
Hi 2025. 08. 18. 10:28 keltezéssel, Stefan Klug írta: > To use AgcMeanLuminance an algorithm derives from it and overrides the > estimateLuminance() function. This override is the only reason to > derive from this class. It has however two downsides: > - It is more difficult for the reader to know which functionality comes > from where. > - It is necessary to keep references to data needed in the > estimateLuminance() function inside the class, even though that date > is only accessed inside AgcMenaLuminance::calculateNewEv(). > > Remove the need to derive from AgcMeanLuminance by passing > estimateLuminance as function object into calculateNewEv(). > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/ipu3/algorithms/agc.cpp | 4 +- > src/ipa/ipu3/algorithms/agc.h | 2 +- > src/ipa/libipa/agc_mean_luminance.cpp | 57 ++++++++++++++------------- > src/ipa/libipa/agc_mean_luminance.h | 9 +++-- > src/ipa/mali-c55/algorithms/agc.cpp | 5 ++- > src/ipa/mali-c55/algorithms/agc.h | 2 +- > src/ipa/rkisp1/algorithms/agc.cpp | 23 ++++++----- > src/ipa/rkisp1/algorithms/agc.h | 5 +-- > 8 files changed, 60 insertions(+), 47 deletions(-) > > diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp > index 39d0aebb0838..98a034a47625 100644 > --- a/src/ipa/ipu3/algorithms/agc.cpp > +++ b/src/ipa/ipu3/algorithms/agc.cpp > @@ -27,6 +27,7 @@ > namespace libcamera { > > using namespace std::literals::chrono_literals; > +using namespace std::placeholders; > > namespace ipa::ipu3::algorithms { > > @@ -224,7 +225,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > utils::Duration newExposureTime; > double aGain, dGain; > std::tie(newExposureTime, aGain, dGain) = > - calculateNewEv(context.activeState.agc.constraintMode, > + calculateNewEv(std::bind(&Agc::estimateLuminance, this, _1), > + context.activeState.agc.constraintMode, > context.activeState.agc.exposureMode, hist, > effectiveExposureValue); > > diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h > index 890c271b4462..8e182cd7cff3 100644 > --- a/src/ipa/ipu3/algorithms/agc.h > +++ b/src/ipa/ipu3/algorithms/agc.h > @@ -38,7 +38,7 @@ public: > ControlList &metadata) override; > > private: > - double estimateLuminance(double gain) const override; > + double estimateLuminance(double gain) const; > Histogram parseStatistics(const ipu3_uapi_stats_3a *stats, > const ipu3_uapi_grid_config &grid); > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp > index ff96a381ffce..df7efb284a89 100644 > --- a/src/ipa/libipa/agc_mean_luminance.cpp > +++ b/src/ipa/libipa/agc_mean_luminance.cpp > @@ -18,7 +18,7 @@ using namespace libcamera::controls; > > /** > * \file agc_mean_luminance.h > - * \brief Base class implementing mean luminance AEGC > + * \brief Class implementing mean luminance AEGC > */ > > namespace libcamera { > @@ -53,6 +53,21 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16; > */ > static constexpr double kMaxRelativeLuminanceTarget = 0.95; > > +/** > + * \fn AgcMeanLuminance::EstimateLuminanceFn > + * \brief Function to estimate the mean luminance given a gain > + * \param[in] gain The gain with which to adjust the luminance estimate > + * > + * Callback functions of this type are used within \a calculateNewEv to estimate > + * the average relative luminance of the frame that would be output by the > + * sensor if an additional \a gain was applied. It is a is implemented as > + * callback function because estimation of luminance is a hardware-specific > + * operation, which depends wholly on the format of the stats that are delivered > + * to libcamera from the ISP. > + * > + * \return The normalised relative luminance of the image > + */ > + > /** > * \struct AgcMeanLuminance::AgcConstraint > * \brief The boundaries and target for an AeConstraintMode constraint > @@ -133,13 +148,11 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95; > * will determine the supportable precision of the constraints. > * > * 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 setLimits() and resetFrameCounter() in configure(). They may then use > - * calculateNewEv() in process(). If the limits passed to setLimits() change for > - * any reason (for example, in response to a FrameDurationLimit control being > - * passed in queueRequest()) then setLimits() must be called again with the new > - * values. > + * must call parseTuningData() in init(), and must also call setLimits() and > + * resetFrameCounter() in configure(). They may then use calculateNewEv() in > + * process(). If the limits passed to setLimits() change for any reason (for > + * example, in response to a FrameDurationLimit control being passed in > + * queueRequest()) then setLimits() must be called again with the new values. > */ > > AgcMeanLuminance::AgcMeanLuminance() > @@ -420,28 +433,12 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime, > * \brief Get the controls that have been generated after parsing tuning data > */ > > -/** > - * \fn AgcMeanLuminance::estimateLuminance(const double gain) > - * \brief Estimate the luminance of an image, adjusted by a given gain > - * \param[in] gain The gain with which to adjust the luminance estimate > - * > - * This function estimates the average relative luminance of the frame that > - * would be output by the sensor if an additional \a gain was applied. It is a > - * pure virtual function because estimation of luminance is a hardware-specific > - * operation, which depends wholly on the format of the stats that are delivered > - * to libcamera from the ISP. Derived classes must override this function with > - * one that calculates the normalised mean luminance value across the entire > - * image. > - * > - * \return The normalised relative luminance of the image > - */ > - > /** > * \brief Estimate the initial gain needed to achieve a relative luminance > * target > * \return The calculated initial gain > */ > -double AgcMeanLuminance::estimateInitialGain() const > +double AgcMeanLuminance::estimateInitialGain(EstimateLuminanceFn estimateLuminance) const > { > double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_, > kMaxRelativeLuminanceTarget); > @@ -542,6 +539,7 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue) > /** > * \brief Calculate the new exposure value and splut it between exposure time > * and gain > + * \param[in] estimateLuminance A function to get luminance estimates > * \param[in] constraintModeIndex The index of the current constraint mode > * \param[in] exposureModeIndex The index of the current exposure mode > * \param[in] yHist A Histogram from the ISP statistics to use in constraining > @@ -553,10 +551,15 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue) > * exposure value is filtered to prevent rapid changes from frame to frame, and > * divided into exposure time, analogue and digital gain. > * > + * The \a estimateLuminance shall estimate the average relative luminance of the > + * frame that would be output by the sensor if an additional \a gain was > + * applied. > + * > * \return Tuple of exposure time, analogue gain, and digital gain > */ > std::tuple<utils::Duration, double, double> > -AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > +AgcMeanLuminance::calculateNewEv(EstimateLuminanceFn estimateLuminance, > + uint32_t constraintModeIndex, > uint32_t exposureModeIndex, > const Histogram &yHist, > utils::Duration effectiveExposureValue) > @@ -580,7 +583,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, > return exposureModeHelper->splitExposure(10ms); > } > > - double gain = estimateInitialGain(); > + double gain = estimateInitialGain(estimateLuminance); > gain = constraintClampGain(constraintModeIndex, yHist, gain); > > /* > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h > index cad7ef845487..5e4c598c8ab9 100644 > --- a/src/ipa/libipa/agc_mean_luminance.h > +++ b/src/ipa/libipa/agc_mean_luminance.h > @@ -31,6 +31,8 @@ public: > AgcMeanLuminance(); > virtual ~AgcMeanLuminance(); > > + using EstimateLuminanceFn = std::function<double(double)>; > + > struct AgcConstraint { > enum class Bound { > Lower = 0, > @@ -68,7 +70,8 @@ public: > } > > std::tuple<utils::Duration, double, double> > - calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex, > + calculateNewEv(EstimateLuminanceFn estimateLuminance, > + uint32_t constraintModeIndex, uint32_t exposureModeIndex, > const Histogram &yHist, utils::Duration effectiveExposureValue); > > void resetFrameCount() > @@ -77,13 +80,11 @@ public: > } > > private: > - virtual double estimateLuminance(const double gain) const = 0; > - > void parseRelativeLuminanceTarget(const YamlObject &tuningData); > void parseConstraint(const YamlObject &modeDict, int32_t id); > int parseConstraintModes(const YamlObject &tuningData); > int parseExposureModes(const YamlObject &tuningData); > - double estimateInitialGain() const; > + double estimateInitialGain(EstimateLuminanceFn estimateLuminance) const; > double constraintClampGain(uint32_t constraintModeIndex, > const Histogram &hist, > double gain); > diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp > index 15963994b2d6..4bfb4aaeedf0 100644 > --- a/src/ipa/mali-c55/algorithms/agc.cpp > +++ b/src/ipa/mali-c55/algorithms/agc.cpp > @@ -8,6 +8,7 @@ > #include "agc.h" > > #include <cmath> > +#include <functional> > > #include <libcamera/base/log.h> > #include <libcamera/base/utils.h> > @@ -21,6 +22,7 @@ > namespace libcamera { > > using namespace std::literals::chrono_literals; > +using namespace std::placeholders; > > namespace ipa::mali_c55::algorithms { > > @@ -383,7 +385,8 @@ void Agc::process(IPAContext &context, > utils::Duration shutterTime; > double aGain, dGain; > std::tie(shutterTime, aGain, dGain) = > - calculateNewEv(activeState.agc.constraintMode, > + calculateNewEv(std::bind(&Agc::estimateLuminance, this, _1), > + activeState.agc.constraintMode, > activeState.agc.exposureMode, statistics_.yHist, > effectiveExposureValue); > > diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h > index 0b4bf7eda1c2..698de57e1ba8 100644 > --- a/src/ipa/mali-c55/algorithms/agc.h > +++ b/src/ipa/mali-c55/algorithms/agc.h > @@ -64,7 +64,7 @@ public: > ControlList &metadata) override; > > private: > - double estimateLuminance(const double gain) const override; > + double estimateLuminance(const double gain) const; > size_t fillGainParamBlock(IPAContext &context, > IPAFrameContext &frameContext, > mali_c55_params_block block); > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp > index 35440b67e999..3a078f9753f6 100644 > --- a/src/ipa/rkisp1/algorithms/agc.cpp > +++ b/src/ipa/rkisp1/algorithms/agc.cpp > @@ -440,16 +440,17 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext, > * > * \return The relative luminance > */ > -double Agc::estimateLuminance(double gain) const > +double Agc::estimateLuminance(Span<const uint8_t> expMeans, > + Span<const uint8_t> weights, double gain) > { > - ASSERT(expMeans_.size() == weights_.size()); > + ASSERT(expMeans.size() == weights.size()); > double ySum = 0.0; > double wSum = 0.0; > > /* Sum the averages, saturated to 255. */ > - for (unsigned i = 0; i < expMeans_.size(); i++) { > - double w = weights_[i]; > - ySum += std::min(expMeans_[i] * gain, 255.0) * w; > + for (unsigned i = 0; i < expMeans.size(); i++) { > + double w = weights[i]; > + ySum += std::min(expMeans[i] * gain, 255.0) * w; > wSum += w; > } > > @@ -522,9 +523,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > /* The lower 4 bits are fractional and meant to be discarded. */ > Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins }, > [](uint32_t x) { return x >> 4; }); > - expMeans_ = { params->ae.exp_mean, context.hw->numAeCells }; > std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode); > - weights_ = { modeWeights.data(), modeWeights.size() }; > > /* > * Set the AGC limits using the fixed exposure time and/or gain in > @@ -566,10 +565,17 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > > setExposureCompensation(pow(2.0, frameContext.agc.exposureValue)); > > + AgcMeanLuminance::EstimateLuminanceFn estimateLuminanceFn = std::bind( > + &Agc::estimateLuminance, this, > + Span<const uint8_t>(params->ae.exp_mean, context.hw->numAeCells), > + Span<const uint8_t>(modeWeights.data(), modeWeights.size()), > + std::placeholders::_1); I think switching to `std::function` it is a bit unfortunate. But short of making the function inline, or manifesting `std::function_ref` from C++26 (which is more appropriate for these "direct callback" scenarious), it is indeed the simplest approach. What's especially unfortunate is that for example the above bind expression is almost guaranteed to always be heap allocated in the `std::function`. I don't think this is ideal in a "hot path" like `Agc::process()`. Now, one could use `std::ref()`: auto estimate = [&](...) { return estimateLuminance(...); }; ... = calculateNewEv(std::ref(estimate), ...); as `std::function` should guarantee SSO when a function ptr or reference wrapper is used. Of course this is not ideal, it is very easy to miss when writing. Regards, Barnabás Pőcze > + > utils::Duration newExposureTime; > double aGain, dGain; > std::tie(newExposureTime, aGain, dGain) = > - calculateNewEv(frameContext.agc.constraintMode, > + calculateNewEv(estimateLuminanceFn, > + frameContext.agc.constraintMode, > frameContext.agc.exposureMode, > hist, effectiveExposureValue); > > @@ -590,7 +596,6 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, > std::max(frameContext.agc.minFrameDuration, newExposureTime)); > > fillMetadata(context, frameContext, metadata); > - expMeans_ = {}; > } > > REGISTER_IPA_ALGORITHM(Agc, "Agc") > diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h > index 7867eed9c4e3..742c8f7371f3 100644 > --- a/src/ipa/rkisp1/algorithms/agc.h > +++ b/src/ipa/rkisp1/algorithms/agc.h > @@ -43,19 +43,18 @@ public: > ControlList &metadata) override; > > private: > + double estimateLuminance(Span<const uint8_t> expMeans, > + Span<const uint8_t> weights, double gain); > int parseMeteringModes(IPAContext &context, const YamlObject &tuningData); > uint8_t computeHistogramPredivider(const Size &size, > enum rkisp1_cif_isp_histogram_mode mode); > > void fillMetadata(IPAContext &context, IPAFrameContext &frameContext, > ControlList &metadata); > - double estimateLuminance(double gain) const override; > void processFrameDuration(IPAContext &context, > IPAFrameContext &frameContext, > utils::Duration frameDuration); > > - Span<const uint8_t> expMeans_; > - Span<const uint8_t> weights_; > > std::map<int32_t, std::vector<uint8_t>> meteringModes_; > };
diff --git a/src/ipa/ipu3/algorithms/agc.cpp b/src/ipa/ipu3/algorithms/agc.cpp index 39d0aebb0838..98a034a47625 100644 --- a/src/ipa/ipu3/algorithms/agc.cpp +++ b/src/ipa/ipu3/algorithms/agc.cpp @@ -27,6 +27,7 @@ namespace libcamera { using namespace std::literals::chrono_literals; +using namespace std::placeholders; namespace ipa::ipu3::algorithms { @@ -224,7 +225,8 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, utils::Duration newExposureTime; double aGain, dGain; std::tie(newExposureTime, aGain, dGain) = - calculateNewEv(context.activeState.agc.constraintMode, + calculateNewEv(std::bind(&Agc::estimateLuminance, this, _1), + context.activeState.agc.constraintMode, context.activeState.agc.exposureMode, hist, effectiveExposureValue); diff --git a/src/ipa/ipu3/algorithms/agc.h b/src/ipa/ipu3/algorithms/agc.h index 890c271b4462..8e182cd7cff3 100644 --- a/src/ipa/ipu3/algorithms/agc.h +++ b/src/ipa/ipu3/algorithms/agc.h @@ -38,7 +38,7 @@ public: ControlList &metadata) override; private: - double estimateLuminance(double gain) const override; + double estimateLuminance(double gain) const; Histogram parseStatistics(const ipu3_uapi_stats_3a *stats, const ipu3_uapi_grid_config &grid); diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp index ff96a381ffce..df7efb284a89 100644 --- a/src/ipa/libipa/agc_mean_luminance.cpp +++ b/src/ipa/libipa/agc_mean_luminance.cpp @@ -18,7 +18,7 @@ using namespace libcamera::controls; /** * \file agc_mean_luminance.h - * \brief Base class implementing mean luminance AEGC + * \brief Class implementing mean luminance AEGC */ namespace libcamera { @@ -53,6 +53,21 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16; */ static constexpr double kMaxRelativeLuminanceTarget = 0.95; +/** + * \fn AgcMeanLuminance::EstimateLuminanceFn + * \brief Function to estimate the mean luminance given a gain + * \param[in] gain The gain with which to adjust the luminance estimate + * + * Callback functions of this type are used within \a calculateNewEv to estimate + * the average relative luminance of the frame that would be output by the + * sensor if an additional \a gain was applied. It is a is implemented as + * callback function because estimation of luminance is a hardware-specific + * operation, which depends wholly on the format of the stats that are delivered + * to libcamera from the ISP. + * + * \return The normalised relative luminance of the image + */ + /** * \struct AgcMeanLuminance::AgcConstraint * \brief The boundaries and target for an AeConstraintMode constraint @@ -133,13 +148,11 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95; * will determine the supportable precision of the constraints. * * 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 setLimits() and resetFrameCounter() in configure(). They may then use - * calculateNewEv() in process(). If the limits passed to setLimits() change for - * any reason (for example, in response to a FrameDurationLimit control being - * passed in queueRequest()) then setLimits() must be called again with the new - * values. + * must call parseTuningData() in init(), and must also call setLimits() and + * resetFrameCounter() in configure(). They may then use calculateNewEv() in + * process(). If the limits passed to setLimits() change for any reason (for + * example, in response to a FrameDurationLimit control being passed in + * queueRequest()) then setLimits() must be called again with the new values. */ AgcMeanLuminance::AgcMeanLuminance() @@ -420,28 +433,12 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime, * \brief Get the controls that have been generated after parsing tuning data */ -/** - * \fn AgcMeanLuminance::estimateLuminance(const double gain) - * \brief Estimate the luminance of an image, adjusted by a given gain - * \param[in] gain The gain with which to adjust the luminance estimate - * - * This function estimates the average relative luminance of the frame that - * would be output by the sensor if an additional \a gain was applied. It is a - * pure virtual function because estimation of luminance is a hardware-specific - * operation, which depends wholly on the format of the stats that are delivered - * to libcamera from the ISP. Derived classes must override this function with - * one that calculates the normalised mean luminance value across the entire - * image. - * - * \return The normalised relative luminance of the image - */ - /** * \brief Estimate the initial gain needed to achieve a relative luminance * target * \return The calculated initial gain */ -double AgcMeanLuminance::estimateInitialGain() const +double AgcMeanLuminance::estimateInitialGain(EstimateLuminanceFn estimateLuminance) const { double yTarget = std::min(relativeLuminanceTarget_ * exposureCompensation_, kMaxRelativeLuminanceTarget); @@ -542,6 +539,7 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue) /** * \brief Calculate the new exposure value and splut it between exposure time * and gain + * \param[in] estimateLuminance A function to get luminance estimates * \param[in] constraintModeIndex The index of the current constraint mode * \param[in] exposureModeIndex The index of the current exposure mode * \param[in] yHist A Histogram from the ISP statistics to use in constraining @@ -553,10 +551,15 @@ utils::Duration AgcMeanLuminance::filterExposure(utils::Duration exposureValue) * exposure value is filtered to prevent rapid changes from frame to frame, and * divided into exposure time, analogue and digital gain. * + * The \a estimateLuminance shall estimate the average relative luminance of the + * frame that would be output by the sensor if an additional \a gain was + * applied. + * * \return Tuple of exposure time, analogue gain, and digital gain */ std::tuple<utils::Duration, double, double> -AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, +AgcMeanLuminance::calculateNewEv(EstimateLuminanceFn estimateLuminance, + uint32_t constraintModeIndex, uint32_t exposureModeIndex, const Histogram &yHist, utils::Duration effectiveExposureValue) @@ -580,7 +583,7 @@ AgcMeanLuminance::calculateNewEv(uint32_t constraintModeIndex, return exposureModeHelper->splitExposure(10ms); } - double gain = estimateInitialGain(); + double gain = estimateInitialGain(estimateLuminance); gain = constraintClampGain(constraintModeIndex, yHist, gain); /* diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h index cad7ef845487..5e4c598c8ab9 100644 --- a/src/ipa/libipa/agc_mean_luminance.h +++ b/src/ipa/libipa/agc_mean_luminance.h @@ -31,6 +31,8 @@ public: AgcMeanLuminance(); virtual ~AgcMeanLuminance(); + using EstimateLuminanceFn = std::function<double(double)>; + struct AgcConstraint { enum class Bound { Lower = 0, @@ -68,7 +70,8 @@ public: } std::tuple<utils::Duration, double, double> - calculateNewEv(uint32_t constraintModeIndex, uint32_t exposureModeIndex, + calculateNewEv(EstimateLuminanceFn estimateLuminance, + uint32_t constraintModeIndex, uint32_t exposureModeIndex, const Histogram &yHist, utils::Duration effectiveExposureValue); void resetFrameCount() @@ -77,13 +80,11 @@ public: } private: - virtual double estimateLuminance(const double gain) const = 0; - void parseRelativeLuminanceTarget(const YamlObject &tuningData); void parseConstraint(const YamlObject &modeDict, int32_t id); int parseConstraintModes(const YamlObject &tuningData); int parseExposureModes(const YamlObject &tuningData); - double estimateInitialGain() const; + double estimateInitialGain(EstimateLuminanceFn estimateLuminance) const; double constraintClampGain(uint32_t constraintModeIndex, const Histogram &hist, double gain); diff --git a/src/ipa/mali-c55/algorithms/agc.cpp b/src/ipa/mali-c55/algorithms/agc.cpp index 15963994b2d6..4bfb4aaeedf0 100644 --- a/src/ipa/mali-c55/algorithms/agc.cpp +++ b/src/ipa/mali-c55/algorithms/agc.cpp @@ -8,6 +8,7 @@ #include "agc.h" #include <cmath> +#include <functional> #include <libcamera/base/log.h> #include <libcamera/base/utils.h> @@ -21,6 +22,7 @@ namespace libcamera { using namespace std::literals::chrono_literals; +using namespace std::placeholders; namespace ipa::mali_c55::algorithms { @@ -383,7 +385,8 @@ void Agc::process(IPAContext &context, utils::Duration shutterTime; double aGain, dGain; std::tie(shutterTime, aGain, dGain) = - calculateNewEv(activeState.agc.constraintMode, + calculateNewEv(std::bind(&Agc::estimateLuminance, this, _1), + activeState.agc.constraintMode, activeState.agc.exposureMode, statistics_.yHist, effectiveExposureValue); diff --git a/src/ipa/mali-c55/algorithms/agc.h b/src/ipa/mali-c55/algorithms/agc.h index 0b4bf7eda1c2..698de57e1ba8 100644 --- a/src/ipa/mali-c55/algorithms/agc.h +++ b/src/ipa/mali-c55/algorithms/agc.h @@ -64,7 +64,7 @@ public: ControlList &metadata) override; private: - double estimateLuminance(const double gain) const override; + double estimateLuminance(const double gain) const; size_t fillGainParamBlock(IPAContext &context, IPAFrameContext &frameContext, mali_c55_params_block block); diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp index 35440b67e999..3a078f9753f6 100644 --- a/src/ipa/rkisp1/algorithms/agc.cpp +++ b/src/ipa/rkisp1/algorithms/agc.cpp @@ -440,16 +440,17 @@ void Agc::fillMetadata(IPAContext &context, IPAFrameContext &frameContext, * * \return The relative luminance */ -double Agc::estimateLuminance(double gain) const +double Agc::estimateLuminance(Span<const uint8_t> expMeans, + Span<const uint8_t> weights, double gain) { - ASSERT(expMeans_.size() == weights_.size()); + ASSERT(expMeans.size() == weights.size()); double ySum = 0.0; double wSum = 0.0; /* Sum the averages, saturated to 255. */ - for (unsigned i = 0; i < expMeans_.size(); i++) { - double w = weights_[i]; - ySum += std::min(expMeans_[i] * gain, 255.0) * w; + for (unsigned i = 0; i < expMeans.size(); i++) { + double w = weights[i]; + ySum += std::min(expMeans[i] * gain, 255.0) * w; wSum += w; } @@ -522,9 +523,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, /* The lower 4 bits are fractional and meant to be discarded. */ Histogram hist({ params->hist.hist_bins, context.hw->numHistogramBins }, [](uint32_t x) { return x >> 4; }); - expMeans_ = { params->ae.exp_mean, context.hw->numAeCells }; std::vector<uint8_t> &modeWeights = meteringModes_.at(frameContext.agc.meteringMode); - weights_ = { modeWeights.data(), modeWeights.size() }; /* * Set the AGC limits using the fixed exposure time and/or gain in @@ -566,10 +565,17 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, setExposureCompensation(pow(2.0, frameContext.agc.exposureValue)); + AgcMeanLuminance::EstimateLuminanceFn estimateLuminanceFn = std::bind( + &Agc::estimateLuminance, this, + Span<const uint8_t>(params->ae.exp_mean, context.hw->numAeCells), + Span<const uint8_t>(modeWeights.data(), modeWeights.size()), + std::placeholders::_1); + utils::Duration newExposureTime; double aGain, dGain; std::tie(newExposureTime, aGain, dGain) = - calculateNewEv(frameContext.agc.constraintMode, + calculateNewEv(estimateLuminanceFn, + frameContext.agc.constraintMode, frameContext.agc.exposureMode, hist, effectiveExposureValue); @@ -590,7 +596,6 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame, std::max(frameContext.agc.minFrameDuration, newExposureTime)); fillMetadata(context, frameContext, metadata); - expMeans_ = {}; } REGISTER_IPA_ALGORITHM(Agc, "Agc") diff --git a/src/ipa/rkisp1/algorithms/agc.h b/src/ipa/rkisp1/algorithms/agc.h index 7867eed9c4e3..742c8f7371f3 100644 --- a/src/ipa/rkisp1/algorithms/agc.h +++ b/src/ipa/rkisp1/algorithms/agc.h @@ -43,19 +43,18 @@ public: ControlList &metadata) override; private: + double estimateLuminance(Span<const uint8_t> expMeans, + Span<const uint8_t> weights, double gain); int parseMeteringModes(IPAContext &context, const YamlObject &tuningData); uint8_t computeHistogramPredivider(const Size &size, enum rkisp1_cif_isp_histogram_mode mode); void fillMetadata(IPAContext &context, IPAFrameContext &frameContext, ControlList &metadata); - double estimateLuminance(double gain) const override; void processFrameDuration(IPAContext &context, IPAFrameContext &frameContext, utils::Duration frameDuration); - Span<const uint8_t> expMeans_; - Span<const uint8_t> weights_; std::map<int32_t, std::vector<uint8_t>> meteringModes_; };
To use AgcMeanLuminance an algorithm derives from it and overrides the estimateLuminance() function. This override is the only reason to derive from this class. It has however two downsides: - It is more difficult for the reader to know which functionality comes from where. - It is necessary to keep references to data needed in the estimateLuminance() function inside the class, even though that date is only accessed inside AgcMenaLuminance::calculateNewEv(). Remove the need to derive from AgcMeanLuminance by passing estimateLuminance as function object into calculateNewEv(). Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/ipu3/algorithms/agc.cpp | 4 +- src/ipa/ipu3/algorithms/agc.h | 2 +- src/ipa/libipa/agc_mean_luminance.cpp | 57 ++++++++++++++------------- src/ipa/libipa/agc_mean_luminance.h | 9 +++-- src/ipa/mali-c55/algorithms/agc.cpp | 5 ++- src/ipa/mali-c55/algorithms/agc.h | 2 +- src/ipa/rkisp1/algorithms/agc.cpp | 23 ++++++----- src/ipa/rkisp1/algorithms/agc.h | 5 +-- 8 files changed, 60 insertions(+), 47 deletions(-)