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

25083 diff mbox series

In some situations it is necessary to specify the target brightness
value depending on the overall lux level. Replace the float
relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain
value as single point PWL, backwards compatibility to existing tuning
files is ensured.

While at it, order the class members in reverse xmas tree notation.

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 v3:
- Dropped reference to original patch from commit message. For the
  record, the old patch was https://patchwork.libcamera.org/patch/20231/
- Ordered member variables in reverse xmas tree order
- Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget
  is missing in the tuning file
- Warn once if lux level is not available after a few frames

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 | 67 ++++++++++++++++++++++++---
 src/ipa/libipa/agc_mean_luminance.h   | 14 ++++--
 src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
 3 files changed, 72 insertions(+), 10 deletions(-)

Quoting Stefan Klug (2025-11-19 13:22:12)
> In some situations it is necessary to specify the target brightness
> value depending on the overall lux level. Replace the float
> relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain
> value as single point PWL, backwards compatibility to existing tuning
> files is ensured.
> 
> While at it, order the class members in reverse xmas tree notation.
> 
> 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>

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 
> ---
> 
> Changes in v3:
> - Dropped reference to original patch from commit message. For the
>   record, the old patch was https://patchwork.libcamera.org/patch/20231/
> - Ordered member variables in reverse xmas tree order
> - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget
>   is missing in the tuning file
> - Warn once if lux level is not available after a few frames
> 
> 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 | 67 ++++++++++++++++++++++++---
>  src/ipa/libipa/agc_mean_luminance.h   | 14 ++++--
>  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
>  3 files changed, 72 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 64f36bd75dd2..4f2b5fad2082 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
> @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
>   */
>  
>  AgcMeanLuminance::AgcMeanLuminance()
> -       : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> -         relativeLuminanceTarget_(0)
> +       : filteredExposure_(0s), luxWarningEnabled_(true),
> +         exposureCompensation_(1.0), frameCount_(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"];
> +       if (!target) {
> +               relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };
> +               return 0;
> +       }
> +
> +       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)
> @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
>  {
>         for (auto &[id, helper] : exposureModeHelpers_)
>                 helper->configure(lineDuration, sensorHelper);
> +
> +       luxWarningEnabled_ = true;
>  }
>  
>  /**
> @@ -385,7 +408,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 +428,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 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>   */
>  double AgcMeanLuminance::effectiveYTarget() const
>  {
> -       return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> +       double lux = lux_;
> +       if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> +               if (frameCount_ > 10 && luxWarningEnabled_) {
> +                       luxWarningEnabled_ = false;
> +                       LOG(AgcMeanLuminance, Warning)
> +                               << "Missing lux value for luminance target "
> +                                  "calculation, default to "
> +                               << kDefaultLuxLevel
> +                               << ". Note that the Lux algorithm must be "
> +                                  "included before the Agc algorithm.";
> +               }
> +
> +               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 e5f164c3186b..746b97b16ffe 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);
>  
> @@ -82,7 +88,7 @@ public:
>  private:
>         virtual double estimateLuminance(const double gain) const = 0;
>  
> -       void parseRelativeLuminanceTarget(const YamlObject &tuningData);
> +       int parseRelativeLuminanceTarget(const YamlObject &tuningData);
>         void parseConstraint(const YamlObject &modeDict, int32_t id);
>         int parseConstraintModes(const YamlObject &tuningData);
>         int parseExposureModes(const YamlObject &tuningData);
> @@ -92,10 +98,12 @@ private:
>                                    double gain);
>         utils::Duration filterExposure(utils::Duration exposureValue);
>  
> +       utils::Duration filteredExposure_;
> +       mutable bool luxWarningEnabled_;
>         double exposureCompensation_;
> +       Pwl relativeLuminanceTarget_;
>         uint64_t frameCount_;
> -       utils::Duration filteredExposure_;
> -       double relativeLuminanceTarget_;
> +       unsigned int lux_;
>  
>         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 Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:
> In some situations it is necessary to specify the target brightness
> value depending on the overall lux level. Replace the float
> relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain
> value as single point PWL, backwards compatibility to existing tuning
> files is ensured.
>
> While at it, order the class members in reverse xmas tree notation.
>
> 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 v3:
> - Dropped reference to original patch from commit message. For the
>   record, the old patch was https://patchwork.libcamera.org/patch/20231/
> - Ordered member variables in reverse xmas tree order
> - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget
>   is missing in the tuning file
> - Warn once if lux level is not available after a few frames
>
> 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 | 67 ++++++++++++++++++++++++---
>  src/ipa/libipa/agc_mean_luminance.h   | 14 ++++--
>  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
>  3 files changed, 72 insertions(+), 10 deletions(-)
>
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 64f36bd75dd2..4f2b5fad2082 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
> @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
>   */
>
>  AgcMeanLuminance::AgcMeanLuminance()
> -	: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> -	  relativeLuminanceTarget_(0)
> +	: filteredExposure_(0s), luxWarningEnabled_(true),
> +	  exposureCompensation_(1.0), frameCount_(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"];
> +	if (!target) {
> +		relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };
> +		return 0;
> +	}
> +
> +	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)
> @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
>  {
>  	for (auto &[id, helper] : exposureModeHelpers_)
>  		helper->configure(lineDuration, sensorHelper);
> +
> +	luxWarningEnabled_ = true;
>  }
>
>  /**
> @@ -385,7 +408,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 +428,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 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>   */
>  double AgcMeanLuminance::effectiveYTarget() const
>  {
> -	return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> +	double lux = lux_;
> +	if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> +		if (frameCount_ > 10 && luxWarningEnabled_) {

My intention here was to push for all the messages related to parsing
errors to be emitted once at startup, but I understand this is not
possible here as the error condition depends on two things:

1) relativeLuminanceTarget_ is specified as a PWL which associates a
lux level to a desired Y target
2) the IPA doesn't call setLux()

Now, while 1) is relatively easy to spot at parsing time 2) only
depend on the IPA implementation.

AgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:

1) configure() where I guess it's ok not to have a lux level set yet ?
2) process() where it is expected to be always called after setLux() ?

if the above are true then I understand we should not Warn on
configure but we should Warn on the very first frame goes through
process for which effectiveYTarget() is called without a setLux() before?

I'm not sure I get why 10 is used, if that's the case.

Thanks
  j

> +			luxWarningEnabled_ = false;
> +			LOG(AgcMeanLuminance, Warning)
> +				<< "Missing lux value for luminance target "
> +				   "calculation, default to "
> +				<< kDefaultLuxLevel
> +				<< ". Note that the Lux algorithm must be "
> +				   "included before the Agc algorithm.";
> +		}
> +
> +		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 e5f164c3186b..746b97b16ffe 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);
>
> @@ -82,7 +88,7 @@ public:
>  private:
>  	virtual double estimateLuminance(const double gain) const = 0;
>
> -	void parseRelativeLuminanceTarget(const YamlObject &tuningData);
> +	int parseRelativeLuminanceTarget(const YamlObject &tuningData);
>  	void parseConstraint(const YamlObject &modeDict, int32_t id);
>  	int parseConstraintModes(const YamlObject &tuningData);
>  	int parseExposureModes(const YamlObject &tuningData);
> @@ -92,10 +98,12 @@ private:
>  				   double gain);
>  	utils::Duration filterExposure(utils::Duration exposureValue);
>
> +	utils::Duration filteredExposure_;
> +	mutable bool luxWarningEnabled_;
>  	double exposureCompensation_;
> +	Pwl relativeLuminanceTarget_;
>  	uint64_t frameCount_;
> -	utils::Duration filteredExposure_;
> -	double relativeLuminanceTarget_;
> +	unsigned int lux_;
>
>  	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
>

2025. 11. 20. 18:05 keltezéssel, Jacopo Mondi írta:
> Hi Stefan
> 
> On Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:
>> In some situations it is necessary to specify the target brightness
>> value depending on the overall lux level. Replace the float
>> relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain
>> value as single point PWL, backwards compatibility to existing tuning
>> files is ensured.
>>
>> While at it, order the class members in reverse xmas tree notation.
>>
>> 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 v3:
>> - Dropped reference to original patch from commit message. For the
>>    record, the old patch was https://patchwork.libcamera.org/patch/20231/
>> - Ordered member variables in reverse xmas tree order
>> - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget
>>    is missing in the tuning file
>> - Warn once if lux level is not available after a few frames
>>
>> 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 | 67 ++++++++++++++++++++++++---
>>   src/ipa/libipa/agc_mean_luminance.h   | 14 ++++--
>>   src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
>>   3 files changed, 72 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
>> index 64f36bd75dd2..4f2b5fad2082 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
>> @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
>>    */
>>
>>   AgcMeanLuminance::AgcMeanLuminance()
>> -	: exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
>> -	  relativeLuminanceTarget_(0)
>> +	: filteredExposure_(0s), luxWarningEnabled_(true),
>> +	  exposureCompensation_(1.0), frameCount_(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"];
>> +	if (!target) {
>> +		relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };
>> +		return 0;
>> +	}
>> +
>> +	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)
>> @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
>>   {
>>   	for (auto &[id, helper] : exposureModeHelpers_)
>>   		helper->configure(lineDuration, sensorHelper);
>> +
>> +	luxWarningEnabled_ = true;
>>   }
>>
>>   /**
>> @@ -385,7 +408,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 +428,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 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>>    */
>>   double AgcMeanLuminance::effectiveYTarget() const
>>   {
>> -	return std::min(relativeLuminanceTarget_ * exposureCompensation_,
>> +	double lux = lux_;
>> +	if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
>> +		if (frameCount_ > 10 && luxWarningEnabled_) {
> 
> My intention here was to push for all the messages related to parsing
> errors to be emitted once at startup, but I understand this is not
> possible here as the error condition depends on two things:
> 
> 1) relativeLuminanceTarget_ is specified as a PWL which associates a
> lux level to a desired Y target
> 2) the IPA doesn't call setLux()

Somewhat related, wouldn't it be better to have `lux` as a parameter of
`effectiveYTarget()`? (And hence `calculateNewEv()`, etc?)

It seems that both `lux` and `exposureCompensation` only need to be parameters
and need not be stored persistently in the class.


> 
> Now, while 1) is relatively easy to spot at parsing time 2) only
> depend on the IPA implementation.
> 
> AgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:
> 
> 1) configure() where I guess it's ok not to have a lux level set yet ?
> 2) process() where it is expected to be always called after setLux() ?
> 
> if the above are true then I understand we should not Warn on
> configure but we should Warn on the very first frame goes through
> process for which effectiveYTarget() is called without a setLux() before?
> 
> I'm not sure I get why 10 is used, if that's the case.
> 
> Thanks
>    j
> 
>> +			luxWarningEnabled_ = false;
>> +			LOG(AgcMeanLuminance, Warning)
>> +				<< "Missing lux value for luminance target "
>> +				   "calculation, default to "
>> +				<< kDefaultLuxLevel
>> +				<< ". Note that the Lux algorithm must be "
>> +				   "included before the Agc algorithm.";
>> +		}
>> +
>> +		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 e5f164c3186b..746b97b16ffe 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);
>>
>> @@ -82,7 +88,7 @@ public:
>>   private:
>>   	virtual double estimateLuminance(const double gain) const = 0;
>>
>> -	void parseRelativeLuminanceTarget(const YamlObject &tuningData);
>> +	int parseRelativeLuminanceTarget(const YamlObject &tuningData);
>>   	void parseConstraint(const YamlObject &modeDict, int32_t id);
>>   	int parseConstraintModes(const YamlObject &tuningData);
>>   	int parseExposureModes(const YamlObject &tuningData);
>> @@ -92,10 +98,12 @@ private:
>>   				   double gain);
>>   	utils::Duration filterExposure(utils::Duration exposureValue);
>>
>> +	utils::Duration filteredExposure_;
>> +	mutable bool luxWarningEnabled_;
>>   	double exposureCompensation_;
>> +	Pwl relativeLuminanceTarget_;
>>   	uint64_t frameCount_;
>> -	utils::Duration filteredExposure_;
>> -	double relativeLuminanceTarget_;
>> +	unsigned int lux_;
>>
>>   	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 Jacopo,

Quoting Jacopo Mondi (2025-11-20 18:05:41)
> Hi Stefan
> 
> On Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:
> > In some situations it is necessary to specify the target brightness
> > value depending on the overall lux level. Replace the float
> > relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain
> > value as single point PWL, backwards compatibility to existing tuning
> > files is ensured.
> >
> > While at it, order the class members in reverse xmas tree notation.
> >
> > 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 v3:
> > - Dropped reference to original patch from commit message. For the
> >   record, the old patch was https://patchwork.libcamera.org/patch/20231/
> > - Ordered member variables in reverse xmas tree order
> > - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget
> >   is missing in the tuning file
> > - Warn once if lux level is not available after a few frames
> >
> > 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 | 67 ++++++++++++++++++++++++---
> >  src/ipa/libipa/agc_mean_luminance.h   | 14 ++++--
> >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
> >  3 files changed, 72 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 64f36bd75dd2..4f2b5fad2082 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
> > @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> >   */
> >
> >  AgcMeanLuminance::AgcMeanLuminance()
> > -     : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> > -       relativeLuminanceTarget_(0)
> > +     : filteredExposure_(0s), luxWarningEnabled_(true),
> > +       exposureCompensation_(1.0), frameCount_(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"];
> > +     if (!target) {
> > +             relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };
> > +             return 0;
> > +     }
> > +
> > +     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)
> > @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
> >  {
> >       for (auto &[id, helper] : exposureModeHelpers_)
> >               helper->configure(lineDuration, sensorHelper);
> > +
> > +     luxWarningEnabled_ = true;
> >  }
> >
> >  /**
> > @@ -385,7 +408,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 +428,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 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> >   */
> >  double AgcMeanLuminance::effectiveYTarget() const
> >  {
> > -     return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> > +     double lux = lux_;
> > +     if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> > +             if (frameCount_ > 10 && luxWarningEnabled_) {
> 
> My intention here was to push for all the messages related to parsing
> errors to be emitted once at startup, but I understand this is not
> possible here as the error condition depends on two things:
> 
> 1) relativeLuminanceTarget_ is specified as a PWL which associates a
> lux level to a desired Y target
> 2) the IPA doesn't call setLux()
> 
> Now, while 1) is relatively easy to spot at parsing time 2) only
> depend on the IPA implementation.
> 
> AgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:
> 
> 1) configure() where I guess it's ok not to have a lux level set yet ?
> 2) process() where it is expected to be always called after setLux() ?
> 
> if the above are true then I understand we should not Warn on
> configure but we should Warn on the very first frame goes through
> process for which effectiveYTarget() is called without a setLux() before?
> 
> I'm not sure I get why 10 is used, if that's the case.

10 was arbitrarily chosen, so that we get at least something. 
Reading your message I realize, that I was a few steps further and
already planning for the regulation rework which makes this hard to
follow.

You are right, currently it is called within process() where the agc
calculation happens. The issue here is that this is plain wrong but
fixing needs to wait until after the regulation series.

The agc calculation and also setLux() should happen in prepare(). Reason
is that for example frameContext.agc.exposureValue feeds into the agc
calculations. So we get stats for frame n, calculate new exposure
settings based on fc[n].agc.exposureValue and these will then be used in
prepare(n+lookahead). That is wrong.

The correct thing to do would be to cache the stats but do the agc
calculations in prepare() so that fc[n+lookahed].agc.exposureValue feeds
into the calculation.

When the above is fixed, there will be no lux value available for the
first few frames as prepare() is run for these frames before the stats
for frame 0 have arrived which is the time when the first lux value can
be calculated.

10 is bigger than the number of frames that get prepared in advance and
is small enough to show up early on.

Does that make sense?

Best regards,
Stefan

> 
> Thanks
>   j
> 
> > +                     luxWarningEnabled_ = false;
> > +                     LOG(AgcMeanLuminance, Warning)
> > +                             << "Missing lux value for luminance target "
> > +                                "calculation, default to "
> > +                             << kDefaultLuxLevel
> > +                             << ". Note that the Lux algorithm must be "
> > +                                "included before the Agc algorithm.";
> > +             }
> > +
> > +             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 e5f164c3186b..746b97b16ffe 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);
> >
> > @@ -82,7 +88,7 @@ public:
> >  private:
> >       virtual double estimateLuminance(const double gain) const = 0;
> >
> > -     void parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > +     int parseRelativeLuminanceTarget(const YamlObject &tuningData);
> >       void parseConstraint(const YamlObject &modeDict, int32_t id);
> >       int parseConstraintModes(const YamlObject &tuningData);
> >       int parseExposureModes(const YamlObject &tuningData);
> > @@ -92,10 +98,12 @@ private:
> >                                  double gain);
> >       utils::Duration filterExposure(utils::Duration exposureValue);
> >
> > +     utils::Duration filteredExposure_;
> > +     mutable bool luxWarningEnabled_;
> >       double exposureCompensation_;
> > +     Pwl relativeLuminanceTarget_;
> >       uint64_t frameCount_;
> > -     utils::Duration filteredExposure_;
> > -     double relativeLuminanceTarget_;
> > +     unsigned int lux_;
> >
> >       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 Thu, Nov 20, 2025 at 06:54:28PM +0100, Stefan Klug wrote:
> Hi Jacopo,
>
> Quoting Jacopo Mondi (2025-11-20 18:05:41)
> > Hi Stefan
> >
> > On Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:
> > > In some situations it is necessary to specify the target brightness
> > > value depending on the overall lux level. Replace the float
> > > relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain
> > > value as single point PWL, backwards compatibility to existing tuning
> > > files is ensured.
> > >
> > > While at it, order the class members in reverse xmas tree notation.
> > >
> > > 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 v3:
> > > - Dropped reference to original patch from commit message. For the
> > >   record, the old patch was https://patchwork.libcamera.org/patch/20231/
> > > - Ordered member variables in reverse xmas tree order
> > > - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget
> > >   is missing in the tuning file
> > > - Warn once if lux level is not available after a few frames
> > >
> > > 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 | 67 ++++++++++++++++++++++++---
> > >  src/ipa/libipa/agc_mean_luminance.h   | 14 ++++--
> > >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
> > >  3 files changed, 72 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > > index 64f36bd75dd2..4f2b5fad2082 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
> > > @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> > >   */
> > >
> > >  AgcMeanLuminance::AgcMeanLuminance()
> > > -     : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> > > -       relativeLuminanceTarget_(0)
> > > +     : filteredExposure_(0s), luxWarningEnabled_(true),
> > > +       exposureCompensation_(1.0), frameCount_(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"];
> > > +     if (!target) {
> > > +             relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };
> > > +             return 0;
> > > +     }
> > > +
> > > +     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)
> > > @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
> > >  {
> > >       for (auto &[id, helper] : exposureModeHelpers_)
> > >               helper->configure(lineDuration, sensorHelper);
> > > +
> > > +     luxWarningEnabled_ = true;
> > >  }
> > >
> > >  /**
> > > @@ -385,7 +408,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 +428,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 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> > >   */
> > >  double AgcMeanLuminance::effectiveYTarget() const
> > >  {
> > > -     return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> > > +     double lux = lux_;
> > > +     if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> > > +             if (frameCount_ > 10 && luxWarningEnabled_) {
> >
> > My intention here was to push for all the messages related to parsing
> > errors to be emitted once at startup, but I understand this is not
> > possible here as the error condition depends on two things:
> >
> > 1) relativeLuminanceTarget_ is specified as a PWL which associates a
> > lux level to a desired Y target
> > 2) the IPA doesn't call setLux()
> >
> > Now, while 1) is relatively easy to spot at parsing time 2) only
> > depend on the IPA implementation.
> >
> > AgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:
> >
> > 1) configure() where I guess it's ok not to have a lux level set yet ?
> > 2) process() where it is expected to be always called after setLux() ?
> >
> > if the above are true then I understand we should not Warn on
> > configure but we should Warn on the very first frame goes through
> > process for which effectiveYTarget() is called without a setLux() before?
> >
> > I'm not sure I get why 10 is used, if that's the case.
>
> 10 was arbitrarily chosen, so that we get at least something.
> Reading your message I realize, that I was a few steps further and
> already planning for the regulation rework which makes this hard to
> follow.
>
> You are right, currently it is called within process() where the agc
> calculation happens. The issue here is that this is plain wrong but
> fixing needs to wait until after the regulation series.
>
> The agc calculation and also setLux() should happen in prepare(). Reason
> is that for example frameContext.agc.exposureValue feeds into the agc
> calculations. So we get stats for frame n, calculate new exposure
> settings based on fc[n].agc.exposureValue and these will then be used in
> prepare(n+lookahead). That is wrong.
>
> The correct thing to do would be to cache the stats but do the agc
> calculations in prepare() so that fc[n+lookahed].agc.exposureValue feeds
> into the calculation.
>
> When the above is fixed, there will be no lux value available for the
> first few frames as prepare() is run for these frames before the stats
> for frame 0 have arrived which is the time when the first lux value can
> be calculated.

Couldn't we then avoid all error condition by:

- coalesce setLux() in effectiveYTarget() as Barnabas suggested by
  making the Lux value an (optional<>?) parameter
- use a default Lux value in the caller until we don't get a real lux
  value OR pass nullopt

This will make mandatory for the IPA to pass a lux in. Is it something
that you would expect ?

>
> 10 is bigger than the number of frames that get prepared in advance and
> is small enough to show up early on.
>
> Does that make sense?
>
> Best regards,
> Stefan
>
> >
> > Thanks
> >   j
> >
> > > +                     luxWarningEnabled_ = false;
> > > +                     LOG(AgcMeanLuminance, Warning)
> > > +                             << "Missing lux value for luminance target "
> > > +                                "calculation, default to "
> > > +                             << kDefaultLuxLevel
> > > +                             << ". Note that the Lux algorithm must be "
> > > +                                "included before the Agc algorithm.";
> > > +             }
> > > +
> > > +             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 e5f164c3186b..746b97b16ffe 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);
> > >
> > > @@ -82,7 +88,7 @@ public:
> > >  private:
> > >       virtual double estimateLuminance(const double gain) const = 0;
> > >
> > > -     void parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > > +     int parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > >       void parseConstraint(const YamlObject &modeDict, int32_t id);
> > >       int parseConstraintModes(const YamlObject &tuningData);
> > >       int parseExposureModes(const YamlObject &tuningData);
> > > @@ -92,10 +98,12 @@ private:
> > >                                  double gain);
> > >       utils::Duration filterExposure(utils::Duration exposureValue);
> > >
> > > +     utils::Duration filteredExposure_;
> > > +     mutable bool luxWarningEnabled_;
> > >       double exposureCompensation_;
> > > +     Pwl relativeLuminanceTarget_;
> > >       uint64_t frameCount_;
> > > -     utils::Duration filteredExposure_;
> > > -     double relativeLuminanceTarget_;
> > > +     unsigned int lux_;
> > >
> > >       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 Jacopo,

Quoting Jacopo Mondi (2025-11-21 10:04:58)
> Hi Stefan
> 
> On Thu, Nov 20, 2025 at 06:54:28PM +0100, Stefan Klug wrote:
> > Hi Jacopo,
> >
> > Quoting Jacopo Mondi (2025-11-20 18:05:41)
> > > Hi Stefan
> > >
> > > On Wed, Nov 19, 2025 at 02:22:12PM +0100, Stefan Klug wrote:
> > > > In some situations it is necessary to specify the target brightness
> > > > value depending on the overall lux level. Replace the float
> > > > relativeLuminanceTraget by a PWL. As the PWL loading code loads a plain
> > > > value as single point PWL, backwards compatibility to existing tuning
> > > > files is ensured.
> > > >
> > > > While at it, order the class members in reverse xmas tree notation.
> > > >
> > > > 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 v3:
> > > > - Dropped reference to original patch from commit message. For the
> > > >   record, the old patch was https://patchwork.libcamera.org/patch/20231/
> > > > - Ordered member variables in reverse xmas tree order
> > > > - Fallback to kDefaultRelativeLuminanceTarget if relativeLuminanceTraget
> > > >   is missing in the tuning file
> > > > - Warn once if lux level is not available after a few frames
> > > >
> > > > 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 | 67 ++++++++++++++++++++++++---
> > > >  src/ipa/libipa/agc_mean_luminance.h   | 14 ++++--
> > > >  src/ipa/rkisp1/algorithms/agc.cpp     |  1 +
> > > >  3 files changed, 72 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > > > index 64f36bd75dd2..4f2b5fad2082 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
> > > > @@ -144,17 +152,30 @@ static constexpr double kMaxRelativeLuminanceTarget = 0.95;
> > > >   */
> > > >
> > > >  AgcMeanLuminance::AgcMeanLuminance()
> > > > -     : exposureCompensation_(1.0), frameCount_(0), filteredExposure_(0s),
> > > > -       relativeLuminanceTarget_(0)
> > > > +     : filteredExposure_(0s), luxWarningEnabled_(true),
> > > > +       exposureCompensation_(1.0), frameCount_(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"];
> > > > +     if (!target) {
> > > > +             relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };
> > > > +             return 0;
> > > > +     }
> > > > +
> > > > +     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)
> > > > @@ -325,6 +346,8 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
> > > >  {
> > > >       for (auto &[id, helper] : exposureModeHelpers_)
> > > >               helper->configure(lineDuration, sensorHelper);
> > > > +
> > > > +     luxWarningEnabled_ = true;
> > > >  }
> > > >
> > > >  /**
> > > > @@ -385,7 +408,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 +428,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 +573,25 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> > > >   */
> > > >  double AgcMeanLuminance::effectiveYTarget() const
> > > >  {
> > > > -     return std::min(relativeLuminanceTarget_ * exposureCompensation_,
> > > > +     double lux = lux_;
> > > > +     if (relativeLuminanceTarget_.size() > 1 && lux_ == 0) {
> > > > +             if (frameCount_ > 10 && luxWarningEnabled_) {
> > >
> > > My intention here was to push for all the messages related to parsing
> > > errors to be emitted once at startup, but I understand this is not
> > > possible here as the error condition depends on two things:
> > >
> > > 1) relativeLuminanceTarget_ is specified as a PWL which associates a
> > > lux level to a desired Y target
> > > 2) the IPA doesn't call setLux()
> > >
> > > Now, while 1) is relatively easy to spot at parsing time 2) only
> > > depend on the IPA implementation.
> > >
> > > AgcMeanLuminance::effectiveYTarget() is called in 2 places by IPAs:
> > >
> > > 1) configure() where I guess it's ok not to have a lux level set yet ?
> > > 2) process() where it is expected to be always called after setLux() ?
> > >
> > > if the above are true then I understand we should not Warn on
> > > configure but we should Warn on the very first frame goes through
> > > process for which effectiveYTarget() is called without a setLux() before?
> > >
> > > I'm not sure I get why 10 is used, if that's the case.
> >
> > 10 was arbitrarily chosen, so that we get at least something.
> > Reading your message I realize, that I was a few steps further and
> > already planning for the regulation rework which makes this hard to
> > follow.
> >
> > You are right, currently it is called within process() where the agc
> > calculation happens. The issue here is that this is plain wrong but
> > fixing needs to wait until after the regulation series.
> >
> > The agc calculation and also setLux() should happen in prepare(). Reason
> > is that for example frameContext.agc.exposureValue feeds into the agc
> > calculations. So we get stats for frame n, calculate new exposure
> > settings based on fc[n].agc.exposureValue and these will then be used in
> > prepare(n+lookahead). That is wrong.
> >
> > The correct thing to do would be to cache the stats but do the agc
> > calculations in prepare() so that fc[n+lookahed].agc.exposureValue feeds
> > into the calculation.
> >
> > When the above is fixed, there will be no lux value available for the
> > first few frames as prepare() is run for these frames before the stats
> > for frame 0 have arrived which is the time when the first lux value can
> > be calculated.
> 
> Couldn't we then avoid all error condition by:
> 
> - coalesce setLux() in effectiveYTarget() as Barnabas suggested by
>   making the Lux value an (optional<>?) parameter
> - use a default Lux value in the caller until we don't get a real lux
>   value OR pass nullopt

We can do that but then the (as we see not trivial) error logging would
also be on the caller side. The warning was intended for cases when:
- The user intended to use a lux dependent yTarget by providing the PWL
  in the tuning file
- And the user did not put the lux algorithm before the Agc, so that lux
  won't be set.

In this case the user would most likely not notice that anything is
wrong as these detailed regulations are difficult to test. So a warning
might help.

> 
> This will make mandatory for the IPA to pass a lux in. Is it something
> that you would expect ?

The reason for not making them parameters but passing them in via a side
channel was to not force us to implement lux on all platforms at the
same time (or splatter todos all over the place). Same for exposure
value.

Another option might be to pass in a configuration struct instead of
single parameters, so that we don't have to touch every IPA, when we
add additional features.

I don't think we should mandate a lux value. I'd be fine with a
std::optional but then imho in a config struct. But maybe on a separate
series?

Best regards,
Stefan

> 
> >
> > 10 is bigger than the number of frames that get prepared in advance and
> > is small enough to show up early on.
> >
> > Does that make sense?
> >
> > Best regards,
> > Stefan
> >
> > >
> > > Thanks
> > >   j
> > >
> > > > +                     luxWarningEnabled_ = false;
> > > > +                     LOG(AgcMeanLuminance, Warning)
> > > > +                             << "Missing lux value for luminance target "
> > > > +                                "calculation, default to "
> > > > +                             << kDefaultLuxLevel
> > > > +                             << ". Note that the Lux algorithm must be "
> > > > +                                "included before the Agc algorithm.";
> > > > +             }
> > > > +
> > > > +             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 e5f164c3186b..746b97b16ffe 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);
> > > >
> > > > @@ -82,7 +88,7 @@ public:
> > > >  private:
> > > >       virtual double estimateLuminance(const double gain) const = 0;
> > > >
> > > > -     void parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > > > +     int parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > > >       void parseConstraint(const YamlObject &modeDict, int32_t id);
> > > >       int parseConstraintModes(const YamlObject &tuningData);
> > > >       int parseExposureModes(const YamlObject &tuningData);
> > > > @@ -92,10 +98,12 @@ private:
> > > >                                  double gain);
> > > >       utils::Duration filterExposure(utils::Duration exposureValue);
> > > >
> > > > +     utils::Duration filteredExposure_;
> > > > +     mutable bool luxWarningEnabled_;
> > > >       double exposureCompensation_;
> > > > +     Pwl relativeLuminanceTarget_;
> > > >       uint64_t frameCount_;
> > > > -     utils::Duration filteredExposure_;
> > > > -     double relativeLuminanceTarget_;
> > > > +     unsigned int lux_;
> > > >
> > > >       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
> > > >