[v2,2/3] ipa: libipa: agc_mean_luminance: Change luminance target to piecewise linear function

24982 diff mbox series

In some situations it is necessary to specify the target brightness
value depending on the overall lux level. This is a rework [1] to fit
current master. As the PWL loading code loads a plain value as single
point PWL, backwards compatibility to existing tuning files is ensured.

[1] https://patchwork.libcamera.org/patch/20231/

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

---

Changes in v2:
- Collected tags
- Dropped special handling of plain values, as this is now part of the
  yaml PWL loader.
---
 src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----
 src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--
 src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
 3 files changed, 55 insertions(+), 8 deletions(-)

Quoting Stefan Klug (2025-11-06 16:42:26)
> In some situations it is necessary to specify the target brightness
> value depending on the overall lux level. This is a rework [1] to fit
> current master. As the PWL loading code loads a plain value as single
> point PWL, backwards compatibility to existing tuning files is ensured.
> 
> [1] https://patchwork.libcamera.org/patch/20231/

reworking [1] to master should not be in the commit message. When this
is merged, old versions are irrelevant.

> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> ---
> 
> Changes in v2:
> - Collected tags
> - Dropped special handling of plain values, as this is now part of the
>   yaml PWL loader.
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----
>  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--
>  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
>  3 files changed, 55 insertions(+), 8 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 64f36bd75dd2..b80af92a2c0f 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>   */
>  static constexpr double kMaxRelativeLuminanceTarget = 0.95;
>  
> +/*
> + * Default lux level
> + *
> + * If no lux level or a zero lux level is specified, but PWLs are used to
> + * specify luminance targets, this default level is used.
> + */
> +static constexpr unsigned int kDefaultLuxLevel = 500;
> +
>  /**
>   * \struct AgcMeanLuminance::AgcConstraint
>   * \brief The boundaries and target for an AeConstraintMode constraint
> @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
>  
>  AgcMeanLuminance::AgcMeanLuminance()
>         : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> -         relativeLuminanceTarget_(0)
> +         lux_(0)
>  {
>  }
>  
>  AgcMeanLuminance::~AgcMeanLuminance() = default;
>  
> -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
>  {
> -       relativeLuminanceTarget_ =
> -               tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
> +       auto &target = tuningData["relativeLuminanceTarget"];
> +       auto pwl = target.get<Pwl>();
> +       if (!pwl) {
> +               LOG(AgcMeanLuminance, Error)
> +                       << "Failed to load relative luminance target.";
> +               return -EINVAL;
> +       }
> +
> +       relativeLuminanceTarget_ = std::move(*pwl);
> +       return 0;
>  }
>  
>  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
>  {
>         int ret;
>  
> -       parseRelativeLuminanceTarget(tuningData);
> +       ret = parseRelativeLuminanceTarget(tuningData);
> +       if (ret)
> +               return ret;
>  
>         ret = parseConstraintModes(tuningData);
>         if (ret)
> @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
>   * AGC calculations. It is expressed as gain instead of EV.
>   */
>  
> +/**
> + * \fn AgcMeanLuminance::setLux(int lux)
> + * \brief Set the lux level
> + * \param[in] lux The lux level
> + *
> + * This function sets the lux level to be used in the AGC calculations. A value
> + * of 0 means no measurement and a default value of \a kDefaultLuxLevel is used
> + * if necessary.
> + */
> +
>  /**
>   * \brief Set the ExposureModeHelper limits for this class
>   * \param[in] minExposureTime Minimum exposure time to allow
> @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>   */
>  double AgcMeanLuminance::effectiveYTarget() const
>  {
> -       return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> +       double lux = lux_;
> +       if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> +               LOG(AgcMeanLuminance, Debug)
> +                       << "Missing lux value for luminance target calculation, default to "
> +                       << kDefaultLuxLevel;
> +               lux = kDefaultLuxLevel;
> +       }
> +
> +       double luminanceTarget = relativeLuminanceTarget_.eval(
> +               relativeLuminanceTarget_.domain().clamp(lux));
> +
> +       return std::min(luminanceTarget * exposureCompensation_,
>                         kMaxRelativeLuminanceTarget);
>  }
>  
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index d7ec548e3e58..1fbff304a72b 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -20,6 +20,7 @@
>  
>  #include "exposure_mode_helper.h"
>  #include "histogram.h"
> +#include "pwl.h"
>  
>  namespace libcamera {
>  
> @@ -50,6 +51,11 @@ public:
>                 exposureCompensation_ = gain;
>         }
>  
> +       void setLux(unsigned int lux)
> +       {
> +               lux_ = lux;
> +       }
> +
>         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>                        double minGain, double maxGain, std::vector<AgcConstraint> constraints);
>  
> @@ -81,8 +87,8 @@ public:
>  
>  private:
>         virtual double estimateLuminance(const double gain) const = 0;
> +       int parseRelativeLuminanceTarget(const YamlObject &tuningData);
>  
> -       void parseRelativeLuminanceTarget(const YamlObject &tuningData);
>         void parseConstraint(const YamlObject &modeDict, int32_t id);
>         int parseConstraintModes(const YamlObject &tuningData);
>         int parseExposureModes(const YamlObject &tuningData);
> @@ -95,7 +101,8 @@ private:
>         double exposureCompensation_;
>         uint64_t frameCount_;
>         utils::Duration filteredExposure_;
> -       double relativeLuminanceTarget_;
> +       unsigned int lux_;
> +       Pwl relativeLuminanceTarget_;
>  
>         std::vector<AgcConstraint> additionalConstraints_;
>         std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index f5a3c917cb69..1ecaff680978 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>                 effectiveExposureValue *= frameContext.agc.quantizationGain;
>  
>         setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> +       setLux(frameContext.lux.lux);

Should this be from the active state (the most current/latest known lux
level) rather than the frame context? Or do we know that the lux is run
before this and is correct for this frame ?

(Especially as Lux comes after Agc alphabetically)

Do we need to do anything to enforce that lux is run /before/ AGC
otherwise?

Otherwise I expect we'd see it default to zero and therefore default to
the kDefaultLux level 'always'?

--
Kieran


>  
>         utils::Duration newExposureTime;
>         double aGain, qGain, dGain;
> -- 
> 2.51.0
>

Hi Stefan

On Thu, Nov 06, 2025 at 05:42:26PM +0100, Stefan Klug wrote:
> In some situations it is necessary to specify the target brightness
> value depending on the overall lux level. This is a rework [1] to fit
> current master. As the PWL loading code loads a plain value as single
> point PWL, backwards compatibility to existing tuning files is ensured.
>
> [1] https://patchwork.libcamera.org/patch/20231/
>
As Kieran said

> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>
> ---
>
> Changes in v2:
> - Collected tags
> - Dropped special handling of plain values, as this is now part of the
>   yaml PWL loader.
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----
>  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--
>  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
>  3 files changed, 55 insertions(+), 8 deletions(-)
>
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 64f36bd75dd2..b80af92a2c0f 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>   */
>  static constexpr double kMaxRelativeLuminanceTarget = 0.95;
>
> +/*
> + * Default lux level
> + *
> + * If no lux level or a zero lux level is specified, but PWLs are used to
> + * specify luminance targets, this default level is used.
> + */
> +static constexpr unsigned int kDefaultLuxLevel = 500;
> +
>  /**
>   * \struct AgcMeanLuminance::AgcConstraint
>   * \brief The boundaries and target for an AeConstraintMode constraint
> @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
>
>  AgcMeanLuminance::AgcMeanLuminance()
>  	: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> -	  relativeLuminanceTarget_(0)
> +	  lux_(0)
>  {
>  }
>
>  AgcMeanLuminance::~AgcMeanLuminance() = default;
>
> -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
>  {
> -	relativeLuminanceTarget_ =
> -		tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
> +	auto &target = tuningData["relativeLuminanceTarget"];
> +	auto pwl = target.get<Pwl>();
> +	if (!pwl) {
> +		LOG(AgcMeanLuminance, Error)
> +			<< "Failed to load relative luminance target.";
> +		return -EINVAL;
> +	}
> +
> +	relativeLuminanceTarget_ = std::move(*pwl);
> +	return 0;
>  }
>
>  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
>  {
>  	int ret;
>
> -	parseRelativeLuminanceTarget(tuningData);
> +	ret = parseRelativeLuminanceTarget(tuningData);
> +	if (ret)
> +		return ret;
>
>  	ret = parseConstraintModes(tuningData);
>  	if (ret)
> @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
>   * AGC calculations. It is expressed as gain instead of EV.
>   */
>
> +/**
> + * \fn AgcMeanLuminance::setLux(int lux)
> + * \brief Set the lux level
> + * \param[in] lux The lux level
> + *
> + * This function sets the lux level to be used in the AGC calculations. A value
> + * of 0 means no measurement and a default value of \a kDefaultLuxLevel is used
> + * if necessary.
> + */
> +
>  /**
>   * \brief Set the ExposureModeHelper limits for this class
>   * \param[in] minExposureTime Minimum exposure time to allow
> @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>   */
>  double AgcMeanLuminance::effectiveYTarget() const
>  {
> -	return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> +	double lux = lux_;
> +	if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {

I might have missed why lux is mandatory only if
relativeLuminanceTarget_.size() > 1
but that's just me not knowing well how this work.


> +		LOG(AgcMeanLuminance, Debug)
> +			<< "Missing lux value for luminance target calculation, default to "
> +			<< kDefaultLuxLevel;

But I wonder if this message will appear for every frame ?

> +		lux = kDefaultLuxLevel;
> +	}
> +
> +	double luminanceTarget = relativeLuminanceTarget_.eval(
> +		relativeLuminanceTarget_.domain().clamp(lux));
> +
> +	return std::min(luminanceTarget * exposureCompensation_,
>  			kMaxRelativeLuminanceTarget);
>  }
>
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index d7ec548e3e58..1fbff304a72b 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -20,6 +20,7 @@
>
>  #include "exposure_mode_helper.h"
>  #include "histogram.h"
> +#include "pwl.h"
>
>  namespace libcamera {
>
> @@ -50,6 +51,11 @@ public:
>  		exposureCompensation_ = gain;
>  	}
>
> +	void setLux(unsigned int lux)
> +	{
> +		lux_ = lux;
> +	}
> +
>  	void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
>  		       double minGain, double maxGain, std::vector<AgcConstraint> constraints);
>
> @@ -81,8 +87,8 @@ public:
>
>  private:
>  	virtual double estimateLuminance(const double gain) const = 0;
> +	int parseRelativeLuminanceTarget(const YamlObject &tuningData);
>
> -	void parseRelativeLuminanceTarget(const YamlObject &tuningData);
>  	void parseConstraint(const YamlObject &modeDict, int32_t id);
>  	int parseConstraintModes(const YamlObject &tuningData);
>  	int parseExposureModes(const YamlObject &tuningData);
> @@ -95,7 +101,8 @@ private:
>  	double exposureCompensation_;
>  	uint64_t frameCount_;
>  	utils::Duration filteredExposure_;
> -	double relativeLuminanceTarget_;
> +	unsigned int lux_;
> +	Pwl relativeLuminanceTarget_;

very minor nit: it's nicer if you invert the two declarations :)

questions apart
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

>
>  	std::vector<AgcConstraint> additionalConstraints_;
>  	std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
> diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> index f5a3c917cb69..1ecaff680978 100644
> --- a/src/ipa/rkisp1/algorithms/agc.cpp
> +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
>  		effectiveExposureValue *= frameContext.agc.quantizationGain;
>
>  	setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> +	setLux(frameContext.lux.lux);
>
>  	utils::Duration newExposureTime;
>  	double aGain, qGain, dGain;
> --
> 2.51.0
>

Hi Kieran,

Thank you for the review.

Quoting Kieran Bingham (2025-11-06 18:47:11)
> Quoting Stefan Klug (2025-11-06 16:42:26)
> > In some situations it is necessary to specify the target brightness
> > value depending on the overall lux level. This is a rework [1] to fit
> > current master. As the PWL loading code loads a plain value as single
> > point PWL, backwards compatibility to existing tuning files is ensured.
> > 
> > [1] https://patchwork.libcamera.org/patch/20231/
> 
> reworking [1] to master should not be in the commit message. When this
> is merged, old versions are irrelevant.

Right, I'll drop that or move it to the changelog.

> 
> > 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > 
> > ---
> > 
> > Changes in v2:
> > - Collected tags
> > - Dropped special handling of plain values, as this is now part of the
> >   yaml PWL loader.
> > ---
> >  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----
> >  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--
> >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
> >  3 files changed, 55 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 64f36bd75dd2..b80af92a2c0f 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> >   */
> >  static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> >  
> > +/*
> > + * Default lux level
> > + *
> > + * If no lux level or a zero lux level is specified, but PWLs are used to
> > + * specify luminance targets, this default level is used.
> > + */
> > +static constexpr unsigned int kDefaultLuxLevel = 500;
> > +
> >  /**
> >   * \struct AgcMeanLuminance::AgcConstraint
> >   * \brief The boundaries and target for an AeConstraintMode constraint
> > @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> >  
> >  AgcMeanLuminance::AgcMeanLuminance()
> >         : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> > -         relativeLuminanceTarget_(0)
> > +         lux_(0)
> >  {
> >  }
> >  
> >  AgcMeanLuminance::~AgcMeanLuminance() = default;
> >  
> > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> >  {
> > -       relativeLuminanceTarget_ =
> > -               tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
> > +       auto &target = tuningData["relativeLuminanceTarget"];
> > +       auto pwl = target.get<Pwl>();
> > +       if (!pwl) {
> > +               LOG(AgcMeanLuminance, Error)
> > +                       << "Failed to load relative luminance target.";
> > +               return -EINVAL;
> > +       }
> > +
> > +       relativeLuminanceTarget_ = std::move(*pwl);
> > +       return 0;
> >  }
> >  
> >  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> > @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> >  {
> >         int ret;
> >  
> > -       parseRelativeLuminanceTarget(tuningData);
> > +       ret = parseRelativeLuminanceTarget(tuningData);
> > +       if (ret)
> > +               return ret;
> >  
> >         ret = parseConstraintModes(tuningData);
> >         if (ret)
> > @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> >   * AGC calculations. It is expressed as gain instead of EV.
> >   */
> >  
> > +/**
> > + * \fn AgcMeanLuminance::setLux(int lux)
> > + * \brief Set the lux level
> > + * \param[in] lux The lux level
> > + *
> > + * This function sets the lux level to be used in the AGC calculations. A value
> > + * of 0 means no measurement and a default value of \a kDefaultLuxLevel is used
> > + * if necessary.
> > + */
> > +
> >  /**
> >   * \brief Set the ExposureModeHelper limits for this class
> >   * \param[in] minExposureTime Minimum exposure time to allow
> > @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> >   */
> >  double AgcMeanLuminance::effectiveYTarget() const
> >  {
> > -       return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> > +       double lux = lux_;
> > +       if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> > +               LOG(AgcMeanLuminance, Debug)
> > +                       << "Missing lux value for luminance target calculation, default to "
> > +                       << kDefaultLuxLevel;
> > +               lux = kDefaultLuxLevel;
> > +       }
> > +
> > +       double luminanceTarget = relativeLuminanceTarget_.eval(
> > +               relativeLuminanceTarget_.domain().clamp(lux));
> > +
> > +       return std::min(luminanceTarget * exposureCompensation_,
> >                         kMaxRelativeLuminanceTarget);
> >  }
> >  
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index d7ec548e3e58..1fbff304a72b 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -20,6 +20,7 @@
> >  
> >  #include "exposure_mode_helper.h"
> >  #include "histogram.h"
> > +#include "pwl.h"
> >  
> >  namespace libcamera {
> >  
> > @@ -50,6 +51,11 @@ public:
> >                 exposureCompensation_ = gain;
> >         }
> >  
> > +       void setLux(unsigned int lux)
> > +       {
> > +               lux_ = lux;
> > +       }
> > +
> >         void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> >                        double minGain, double maxGain, std::vector<AgcConstraint> constraints);
> >  
> > @@ -81,8 +87,8 @@ public:
> >  
> >  private:
> >         virtual double estimateLuminance(const double gain) const = 0;
> > +       int parseRelativeLuminanceTarget(const YamlObject &tuningData);
> >  
> > -       void parseRelativeLuminanceTarget(const YamlObject &tuningData);
> >         void parseConstraint(const YamlObject &modeDict, int32_t id);
> >         int parseConstraintModes(const YamlObject &tuningData);
> >         int parseExposureModes(const YamlObject &tuningData);
> > @@ -95,7 +101,8 @@ private:
> >         double exposureCompensation_;
> >         uint64_t frameCount_;
> >         utils::Duration filteredExposure_;
> > -       double relativeLuminanceTarget_;
> > +       unsigned int lux_;
> > +       Pwl relativeLuminanceTarget_;
> >  
> >         std::vector<AgcConstraint> additionalConstraints_;
> >         std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index f5a3c917cb69..1ecaff680978 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >                 effectiveExposureValue *= frameContext.agc.quantizationGain;
> >  
> >         setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> > +       setLux(frameContext.lux.lux);
> 
> Should this be from the active state (the most current/latest known lux
> level) rather than the frame context? Or do we know that the lux is run
> before this and is correct for this frame ?
> 
> (Especially as Lux comes after Agc alphabetically)
> 
> Do we need to do anything to enforce that lux is run /before/ AGC
> otherwise?
> 
> Otherwise I expect we'd see it default to zero and therefore default to
> the kDefaultLux level 'always'?

Dang, I didn't look to closely at the logic here, as it needs rework in
the context of the regulation series anyways. But that fix wasn't ready
yet (but in use on all of my testpaths). But you're right the current
logic is too flaky. So I'll look again at the missing patch and will
include it in this series even before the regulation rework.

Best regards,
Stefan

> 
> --
> Kieran
> 
> 
> >  
> >         utils::Duration newExposureTime;
> >         double aGain, qGain, dGain;
> > -- 
> > 2.51.0
> >

Hi Jacopo,

Quoting Jacopo Mondi (2025-11-14 17:51:19)
> Hi Stefan
> 
> On Thu, Nov 06, 2025 at 05:42:26PM +0100, Stefan Klug wrote:
> > In some situations it is necessary to specify the target brightness
> > value depending on the overall lux level. This is a rework [1] to fit
> > current master. As the PWL loading code loads a plain value as single
> > point PWL, backwards compatibility to existing tuning files is ensured.
> >
> > [1] https://patchwork.libcamera.org/patch/20231/
> >
> As Kieran said
> 
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> >
> > ---
> >
> > Changes in v2:
> > - Collected tags
> > - Dropped special handling of plain values, as this is now part of the
> >   yaml PWL loader.
> > ---
> >  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----
> >  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--
> >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
> >  3 files changed, 55 insertions(+), 8 deletions(-)
> >
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 64f36bd75dd2..b80af92a2c0f 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> >   */
> >  static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> >
> > +/*
> > + * Default lux level
> > + *
> > + * If no lux level or a zero lux level is specified, but PWLs are used to
> > + * specify luminance targets, this default level is used.
> > + */
> > +static constexpr unsigned int kDefaultLuxLevel = 500;
> > +
> >  /**
> >   * \struct AgcMeanLuminance::AgcConstraint
> >   * \brief The boundaries and target for an AeConstraintMode constraint
> > @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> >
> >  AgcMeanLuminance::AgcMeanLuminance()
> >       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> > -       relativeLuminanceTarget_(0)
> > +       lux_(0)
> >  {
> >  }
> >
> >  AgcMeanLuminance::~AgcMeanLuminance() = default;
> >
> > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> >  {
> > -     relativeLuminanceTarget_ =
> > -             tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
> > +     auto &target = tuningData["relativeLuminanceTarget"];
> > +     auto pwl = target.get<Pwl>();
> > +     if (!pwl) {
> > +             LOG(AgcMeanLuminance, Error)
> > +                     << "Failed to load relative luminance target.";
> > +             return -EINVAL;
> > +     }
> > +
> > +     relativeLuminanceTarget_ = std::move(*pwl);
> > +     return 0;
> >  }
> >
> >  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> > @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> >  {
> >       int ret;
> >
> > -     parseRelativeLuminanceTarget(tuningData);
> > +     ret = parseRelativeLuminanceTarget(tuningData);
> > +     if (ret)
> > +             return ret;
> >
> >       ret = parseConstraintModes(tuningData);
> >       if (ret)
> > @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> >   * AGC calculations. It is expressed as gain instead of EV.
> >   */
> >
> > +/**
> > + * \fn AgcMeanLuminance::setLux(int lux)
> > + * \brief Set the lux level
> > + * \param[in] lux The lux level
> > + *
> > + * This function sets the lux level to be used in the AGC calculations. A value
> > + * of 0 means no measurement and a default value of \a kDefaultLuxLevel is used
> > + * if necessary.
> > + */
> > +
> >  /**
> >   * \brief Set the ExposureModeHelper limits for this class
> >   * \param[in] minExposureTime Minimum exposure time to allow
> > @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> >   */
> >  double AgcMeanLuminance::effectiveYTarget() const
> >  {
> > -     return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> > +     double lux = lux_;
> > +     if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> 
> I might have missed why lux is mandatory only if
> relativeLuminanceTarget_.size() > 1
> but that's just me not knowing well how this work.

If in the tuning file a single value is provided instead of an array, the
PWL is reduced to a single-point PWL returning always the same value. So
the relativeLuminanceTarget is no longer lux dependent.

> 
> 
> > +             LOG(AgcMeanLuminance, Debug)
> > +                     << "Missing lux value for luminance target calculation, default to "
> > +                     << kDefaultLuxLevel;
> 
> But I wonder if this message will appear for every frame ?

Yes, it would appear on every frame. It is debug only so it usually
doesn't show up. I was thinking about making it info level, as it
basically shows a error in the tuning file. Why would one create a
tuning file with a lux dependant luminance target but not include the
lux algorithm?

Ideally I'd like to have a LOG_ONCE and then make this a warning. But
that would be another rabbit hole :-)

> 
> > +             lux = kDefaultLuxLevel;
> > +     }
> > +
> > +     double luminanceTarget = relativeLuminanceTarget_.eval(
> > +             relativeLuminanceTarget_.domain().clamp(lux));
> > +
> > +     return std::min(luminanceTarget * exposureCompensation_,
> >                       kMaxRelativeLuminanceTarget);
> >  }
> >
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index d7ec548e3e58..1fbff304a72b 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -20,6 +20,7 @@
> >
> >  #include "exposure_mode_helper.h"
> >  #include "histogram.h"
> > +#include "pwl.h"
> >
> >  namespace libcamera {
> >
> > @@ -50,6 +51,11 @@ public:
> >               exposureCompensation_ = gain;
> >       }
> >
> > +     void setLux(unsigned int lux)
> > +     {
> > +             lux_ = lux;
> > +     }
> > +
> >       void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> >                      double minGain, double maxGain, std::vector<AgcConstraint> constraints);
> >
> > @@ -81,8 +87,8 @@ public:
> >
> >  private:
> >       virtual double estimateLuminance(const double gain) const = 0;
> > +     int parseRelativeLuminanceTarget(const YamlObject &tuningData);
> >
> > -     void parseRelativeLuminanceTarget(const YamlObject &tuningData);
> >       void parseConstraint(const YamlObject &modeDict, int32_t id);
> >       int parseConstraintModes(const YamlObject &tuningData);
> >       int parseExposureModes(const YamlObject &tuningData);
> > @@ -95,7 +101,8 @@ private:
> >       double exposureCompensation_;
> >       uint64_t frameCount_;
> >       utils::Duration filteredExposure_;
> > -     double relativeLuminanceTarget_;
> > +     unsigned int lux_;
> > +     Pwl relativeLuminanceTarget_;
> 
> very minor nit: it's nicer if you invert the two declarations :)

I often struggle with the order of members. When to add a blank line,
when to keep them in block and whether to order by logic or
alphabetically. Alphabetically is the hint I heard most often so I went
for that in this case. What is your hint here?

> 
> questions apart
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks,
Stefan

> 
> Thanks
>   j
> 
> >
> >       std::vector<AgcConstraint> additionalConstraints_;
> >       std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
> > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > index f5a3c917cb69..1ecaff680978 100644
> > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> >               effectiveExposureValue *= frameContext.agc.quantizationGain;
> >
> >       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> > +     setLux(frameContext.lux.lux);
> >
> >       utils::Duration newExposureTime;
> >       double aGain, qGain, dGain;
> > --
> > 2.51.0
> >

Hi Stefan

On Fri, Nov 14, 2025 at 06:22:49PM +0100, Stefan Klug wrote:
> Hi Jacopo,
>
> Quoting Jacopo Mondi (2025-11-14 17:51:19)
> > Hi Stefan
> >
> > On Thu, Nov 06, 2025 at 05:42:26PM +0100, Stefan Klug wrote:
> > > In some situations it is necessary to specify the target brightness
> > > value depending on the overall lux level. This is a rework [1] to fit
> > > current master. As the PWL loading code loads a plain value as single
> > > point PWL, backwards compatibility to existing tuning files is ensured.
> > >
> > > [1] https://patchwork.libcamera.org/patch/20231/
> > >
> > As Kieran said
> >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > >
> > > ---
> > >
> > > Changes in v2:
> > > - Collected tags
> > > - Dropped special handling of plain values, as this is now part of the
> > >   yaml PWL loader.
> > > ---
> > >  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----
> > >  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--
> > >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
> > >  3 files changed, 55 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > > index 64f36bd75dd2..b80af92a2c0f 100644
> > > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> > >   */
> > >  static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> > >
> > > +/*
> > > + * Default lux level
> > > + *
> > > + * If no lux level or a zero lux level is specified, but PWLs are used to
> > > + * specify luminance targets, this default level is used.
> > > + */
> > > +static constexpr unsigned int kDefaultLuxLevel = 500;
> > > +
> > >  /**
> > >   * \struct AgcMeanLuminance::AgcConstraint
> > >   * \brief The boundaries and target for an AeConstraintMode constraint
> > > @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> > >
> > >  AgcMeanLuminance::AgcMeanLuminance()
> > >       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> > > -       relativeLuminanceTarget_(0)
> > > +       lux_(0)
> > >  {
> > >  }
> > >
> > >  AgcMeanLuminance::~AgcMeanLuminance() = default;
> > >
> > > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> > > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> > >  {
> > > -     relativeLuminanceTarget_ =
> > > -             tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
> > > +     auto &target = tuningData["relativeLuminanceTarget"];
> > > +     auto pwl = target.get<Pwl>();
> > > +     if (!pwl) {
> > > +             LOG(AgcMeanLuminance, Error)
> > > +                     << "Failed to load relative luminance target.";
> > > +             return -EINVAL;
> > > +     }
> > > +
> > > +     relativeLuminanceTarget_ = std::move(*pwl);
> > > +     return 0;
> > >  }
> > >
> > >  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> > > @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> > >  {
> > >       int ret;
> > >
> > > -     parseRelativeLuminanceTarget(tuningData);
> > > +     ret = parseRelativeLuminanceTarget(tuningData);
> > > +     if (ret)
> > > +             return ret;
> > >
> > >       ret = parseConstraintModes(tuningData);
> > >       if (ret)
> > > @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> > >   * AGC calculations. It is expressed as gain instead of EV.
> > >   */
> > >
> > > +/**
> > > + * \fn AgcMeanLuminance::setLux(int lux)
> > > + * \brief Set the lux level
> > > + * \param[in] lux The lux level
> > > + *
> > > + * This function sets the lux level to be used in the AGC calculations. A value
> > > + * of 0 means no measurement and a default value of \a kDefaultLuxLevel is used
> > > + * if necessary.
> > > + */
> > > +
> > >  /**
> > >   * \brief Set the ExposureModeHelper limits for this class
> > >   * \param[in] minExposureTime Minimum exposure time to allow
> > > @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> > >   */
> > >  double AgcMeanLuminance::effectiveYTarget() const
> > >  {
> > > -     return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> > > +     double lux = lux_;
> > > +     if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> >
> > I might have missed why lux is mandatory only if
> > relativeLuminanceTarget_.size() > 1
> > but that's just me not knowing well how this work.
>
> If in the tuning file a single value is provided instead of an array, the
> PWL is reduced to a single-point PWL returning always the same value. So
> the relativeLuminanceTarget is no longer lux dependent.
>
> >
> >
> > > +             LOG(AgcMeanLuminance, Debug)
> > > +                     << "Missing lux value for luminance target calculation, default to "
> > > +                     << kDefaultLuxLevel;
> >
> > But I wonder if this message will appear for every frame ?
>
> Yes, it would appear on every frame. It is debug only so it usually
> doesn't show up. I was thinking about making it info level, as it
> basically shows a error in the tuning file. Why would one create a
> tuning file with a lux dependant luminance target but not include the
> lux algorithm?
>
> Ideally I'd like to have a LOG_ONCE and then make this a warning. But
> that would be another rabbit hole :-)

mmm, I'll let you judge if every frame is too much or not (I think it
is :) as this a config file error and could be reported once only.

You can use a static variable or some other trick to rate-limit the
message up to you.

>
> >
> > > +             lux = kDefaultLuxLevel;
> > > +     }
> > > +
> > > +     double luminanceTarget = relativeLuminanceTarget_.eval(
> > > +             relativeLuminanceTarget_.domain().clamp(lux));
> > > +
> > > +     return std::min(luminanceTarget * exposureCompensation_,
> > >                       kMaxRelativeLuminanceTarget);
> > >  }
> > >
> > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > > index d7ec548e3e58..1fbff304a72b 100644
> > > --- a/src/ipa/libipa/agc_mean_luminance.h
> > > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > > @@ -20,6 +20,7 @@
> > >
> > >  #include "exposure_mode_helper.h"
> > >  #include "histogram.h"
> > > +#include "pwl.h"
> > >
> > >  namespace libcamera {
> > >
> > > @@ -50,6 +51,11 @@ public:
> > >               exposureCompensation_ = gain;
> > >       }
> > >
> > > +     void setLux(unsigned int lux)
> > > +     {
> > > +             lux_ = lux;
> > > +     }
> > > +
> > >       void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> > >                      double minGain, double maxGain, std::vector<AgcConstraint> constraints);
> > >
> > > @@ -81,8 +87,8 @@ public:
> > >
> > >  private:
> > >       virtual double estimateLuminance(const double gain) const = 0;
> > > +     int parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > >
> > > -     void parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > >       void parseConstraint(const YamlObject &modeDict, int32_t id);
> > >       int parseConstraintModes(const YamlObject &tuningData);
> > >       int parseExposureModes(const YamlObject &tuningData);
> > > @@ -95,7 +101,8 @@ private:
> > >       double exposureCompensation_;
> > >       uint64_t frameCount_;
> > >       utils::Duration filteredExposure_;
> > > -     double relativeLuminanceTarget_;
> > > +     unsigned int lux_;
> > > +     Pwl relativeLuminanceTarget_;
> >
> > very minor nit: it's nicer if you invert the two declarations :)
>
> I often struggle with the order of members. When to add a blank line,
> when to keep them in block and whether to order by logic or
> alphabetically. Alphabetically is the hint I heard most often so I went
> for that in this case. What is your hint here?

Since you asked...

Reverse xmas tree!

isn't it much nicer
to have lines
sorted as a
reverse
x-mas
tree

?

This is just OCD though. I used to suggest it for Linux drivers, but
people not always apreciate it, so I only do that when the ordering
is already somewhat like this and someones adds a new variable

and messes up
my
once perfect
x-xmas
tree

>
> >
> > questions apart
> > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
>
> Thanks,
> Stefan
>
> >
> > Thanks
> >   j
> >
> > >
> > >       std::vector<AgcConstraint> additionalConstraints_;
> > >       std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
> > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > index f5a3c917cb69..1ecaff680978 100644
> > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > >               effectiveExposureValue *= frameContext.agc.quantizationGain;
> > >
> > >       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> > > +     setLux(frameContext.lux.lux);
> > >
> > >       utils::Duration newExposureTime;
> > >       double aGain, qGain, dGain;
> > > --
> > > 2.51.0
> > >

Hi,

On 14-Nov-25 8:30 PM, Jacopo Mondi wrote:
> Hi Stefan
> 
> On Fri, Nov 14, 2025 at 06:22:49PM +0100, Stefan Klug wrote:

...

>>>> @@ -95,7 +101,8 @@ private:
>>>>       double exposureCompensation_;
>>>>       uint64_t frameCount_;
>>>>       utils::Duration filteredExposure_;
>>>> -     double relativeLuminanceTarget_;
>>>> +     unsigned int lux_;
>>>> +     Pwl relativeLuminanceTarget_;
>>>
>>> very minor nit: it's nicer if you invert the two declarations :)
>>
>> I often struggle with the order of members. When to add a blank line,
>> when to keep them in block and whether to order by logic or
>> alphabetically. Alphabetically is the hint I heard most often so I went
>> for that in this case. What is your hint here?
> 
> Since you asked...
> 
> Reverse xmas tree!
> 
> isn't it much nicer
> to have lines
> sorted as a
> reverse
> x-mas
> tree
> 
> ?
> 
> This is just OCD though. I used to suggest it for Linux drivers, but
> people not always apreciate it, so I only do that when the ordering
> is already somewhat like this and someones adds a new variable

Using reverse xmas tree is actually somewhat of an unwritten rule for
variable declarations in the kernel which gets requested by lots of
kernel reviewers.

Since AFAICT libcamera tends to mostly follow the kernel coding style
Jacopo's suggestion to swap these 2 gets a +1 from me.

Regards,

Hans

On Fri, Nov 14, 2025 at 08:30:43PM +0100, Jacopo Mondi wrote:
> On Fri, Nov 14, 2025 at 06:22:49PM +0100, Stefan Klug wrote:
> > Quoting Jacopo Mondi (2025-11-14 17:51:19)
> > > On Thu, Nov 06, 2025 at 05:42:26PM +0100, Stefan Klug wrote:
> > > > In some situations it is necessary to specify the target brightness
> > > > value depending on the overall lux level. This is a rework [1] to fit
> > > > current master. As the PWL loading code loads a plain value as single
> > > > point PWL, backwards compatibility to existing tuning files is ensured.
> > > >
> > > > [1] https://patchwork.libcamera.org/patch/20231/
> > > >
> > > As Kieran said
> > >
> > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> > > >
> > > > ---
> > > >
> > > > Changes in v2:
> > > > - Collected tags
> > > > - Dropped special handling of plain values, as this is now part of the
> > > >   yaml PWL loader.
> > > > ---
> > > >  src/ipa/libipa/agc_mean_luminance.cpp | 51 +++++++++++++++++++++++----
> > > >  src/ipa/libipa/agc_mean_luminance.h   | 11 ++++--
> > > >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
> > > >  3 files changed, 55 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > > > index 64f36bd75dd2..b80af92a2c0f 100644
> > > > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > > > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > > > @@ -54,6 +54,14 @@ static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
> > > >   */
> > > >  static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> > > >
> > > > +/*
> > > > + * Default lux level
> > > > + *
> > > > + * If no lux level or a zero lux level is specified, but PWLs are used to
> > > > + * specify luminance targets, this default level is used.
> > > > + */
> > > > +static constexpr unsigned int kDefaultLuxLevel = 500;
> > > > +
> > > >  /**
> > > >   * \struct AgcMeanLuminance::AgcConstraint
> > > >   * \brief The boundaries and target for an AeConstraintMode constraint
> > > > @@ -145,16 +153,24 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> > > >
> > > >  AgcMeanLuminance::AgcMeanLuminance()
> > > >       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> > > > -       relativeLuminanceTarget_(0)
> > > > +       lux_(0)
> > > >  {
> > > >  }
> > > >
> > > >  AgcMeanLuminance::~AgcMeanLuminance() = default;
> > > >
> > > > -void AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> > > > +int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> > > >  {
> > > > -     relativeLuminanceTarget_ =
> > > > -             tuningData["relativeLuminanceTarget"].get<double>(kDefaultRelativeLuminanceTarget);
> > > > +     auto &target = tuningData["relativeLuminanceTarget"];
> > > > +     auto pwl = target.get<Pwl>();
> > > > +     if (!pwl) {
> > > > +             LOG(AgcMeanLuminance, Error)
> > > > +                     << "Failed to load relative luminance target.";
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     relativeLuminanceTarget_ = std::move(*pwl);
> > > > +     return 0;
> > > >  }
> > > >
> > > >  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> > > > @@ -385,7 +401,9 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> > > >  {
> > > >       int ret;
> > > >
> > > > -     parseRelativeLuminanceTarget(tuningData);
> > > > +     ret = parseRelativeLuminanceTarget(tuningData);
> > > > +     if (ret)
> > > > +             return ret;
> > > >
> > > >       ret = parseConstraintModes(tuningData);
> > > >       if (ret)
> > > > @@ -403,6 +421,16 @@ int AgcMeanLuminance::parseTuningData(const YamlObject &tuningData)
> > > >   * AGC calculations. It is expressed as gain instead of EV.
> > > >   */
> > > >
> > > > +/**
> > > > + * \fn AgcMeanLuminance::setLux(int lux)
> > > > + * \brief Set the lux level
> > > > + * \param[in] lux The lux level
> > > > + *
> > > > + * This function sets the lux level to be used in the AGC calculations. A value
> > > > + * of 0 means no measurement and a default value of \a kDefaultLuxLevel is used
> > > > + * if necessary.
> > > > + */
> > > > +
> > > >  /**
> > > >   * \brief Set the ExposureModeHelper limits for this class
> > > >   * \param[in] minExposureTime Minimum exposure time to allow
> > > > @@ -538,7 +566,18 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> > > >   */
> > > >  double AgcMeanLuminance::effectiveYTarget() const
> > > >  {
> > > > -     return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> > > > +     double lux = lux_;
> > > > +     if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> > >
> > > I might have missed why lux is mandatory only if
> > > relativeLuminanceTarget_.size() > 1
> > > but that's just me not knowing well how this work.
> >
> > If in the tuning file a single value is provided instead of an array, the
> > PWL is reduced to a single-point PWL returning always the same value. So
> > the relativeLuminanceTarget is no longer lux dependent.
> >
> > > > +             LOG(AgcMeanLuminance, Debug)
> > > > +                     << "Missing lux value for luminance target calculation, default to "
> > > > +                     << kDefaultLuxLevel;
> > >
> > > But I wonder if this message will appear for every frame ?
> >
> > Yes, it would appear on every frame. It is debug only so it usually
> > doesn't show up. I was thinking about making it info level, as it
> > basically shows a error in the tuning file. Why would one create a
> > tuning file with a lux dependant luminance target but not include the
> > lux algorithm?
> >
> > Ideally I'd like to have a LOG_ONCE and then make this a warning. But
> > that would be another rabbit hole :-)
> 
> mmm, I'll let you judge if every frame is too much or not (I think it
> is :) as this a config file error and could be reported once only.
> 
> You can use a static variable or some other trick to rate-limit the
> message up to you.
> 
> > > > +             lux = kDefaultLuxLevel;
> > > > +     }
> > > > +
> > > > +     double luminanceTarget = relativeLuminanceTarget_.eval(
> > > > +             relativeLuminanceTarget_.domain().clamp(lux));
> > > > +
> > > > +     return std::min(luminanceTarget * exposureCompensation_,
> > > >                       kMaxRelativeLuminanceTarget);
> > > >  }
> > > >
> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > > > index d7ec548e3e58..1fbff304a72b 100644
> > > > --- a/src/ipa/libipa/agc_mean_luminance.h
> > > > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > > > @@ -20,6 +20,7 @@
> > > >
> > > >  #include "exposure_mode_helper.h"
> > > >  #include "histogram.h"
> > > > +#include "pwl.h"
> > > >
> > > >  namespace libcamera {
> > > >
> > > > @@ -50,6 +51,11 @@ public:
> > > >               exposureCompensation_ = gain;
> > > >       }
> > > >
> > > > +     void setLux(unsigned int lux)
> > > > +     {
> > > > +             lux_ = lux;
> > > > +     }
> > > > +
> > > >       void setLimits(utils::Duration minExposureTime, utils::Duration maxExposureTime,
> > > >                      double minGain, double maxGain, std::vector<AgcConstraint> constraints);
> > > >
> > > > @@ -81,8 +87,8 @@ public:
> > > >
> > > >  private:
> > > >       virtual double estimateLuminance(const double gain) const = 0;
> > > > +     int parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > > >
> > > > -     void parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > > >       void parseConstraint(const YamlObject &modeDict, int32_t id);
> > > >       int parseConstraintModes(const YamlObject &tuningData);
> > > >       int parseExposureModes(const YamlObject &tuningData);
> > > > @@ -95,7 +101,8 @@ private:
> > > >       double exposureCompensation_;
> > > >       uint64_t frameCount_;
> > > >       utils::Duration filteredExposure_;
> > > > -     double relativeLuminanceTarget_;
> > > > +     unsigned int lux_;
> > > > +     Pwl relativeLuminanceTarget_;
> > >
> > > very minor nit: it's nicer if you invert the two declarations :)
> >
> > I often struggle with the order of members. When to add a blank line,
> > when to keep them in block and whether to order by logic or
> > alphabetically. Alphabetically is the hint I heard most often so I went
> > for that in this case. What is your hint here?
> 
> Since you asked...
> 
> Reverse xmas tree!
> 
> isn't it much nicer
> to have lines
> sorted as a
> reverse
> x-mas
> tree
> 
> ?

That rule comes from the Linux kernel and meant for local variables. For
C++ member variables, which are similar to C struct members, the kernel
usually groups them by purpose, and we tend to separate groups by blank
lines. There is no clear documented rule.

Within a group of related member variables, if there are no specific
logical order that makes sense, the reverse christmas tree order is a
good default.

All those rules are meant to improve readability, and that's the goal we
should target here. In addition, attention can also be given to
performance constraints (avoiding holes in structures, packing related
data in a single cache line, ...). That's mostly applicable to either
structures that are instantiated a very large number of times (to save
memory) or that are used in hot paths (to improve performance).

> This is just OCD though. I used to suggest it for Linux drivers, but
> people not always apreciate it, so I only do that when the ordering
> is already somewhat like this and someones adds a new variable
> 
> and messes up
> my
> once perfect
> x-xmas
> tree
> 
> > > questions apart
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> >
> > > >
> > > >       std::vector<AgcConstraint> additionalConstraints_;
> > > >       std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
> > > > diff --git a/src/ipa/rkisp1/algorithms/agc.cpp b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > index f5a3c917cb69..1ecaff680978 100644
> > > > --- a/src/ipa/rkisp1/algorithms/agc.cpp
> > > > +++ b/src/ipa/rkisp1/algorithms/agc.cpp
> > > > @@ -618,6 +618,7 @@ void Agc::process(IPAContext &context, [[maybe_unused]] const uint32_t frame,
> > > >               effectiveExposureValue *= frameContext.agc.quantizationGain;
> > > >
> > > >       setExposureCompensation(pow(2.0, frameContext.agc.exposureValue));
> > > > +     setLux(frameContext.lux.lux);
> > > >
> > > >       utils::Duration newExposureTime;
> > > >       double aGain, qGain, dGain;