[v1,4/4] ipa: libipa: agc_mean_luminance: Fix yTarget handling in constraints
diff mbox series

Message ID 20251014142427.3107490-5-stefan.klug@ideasonboard.com
State New
Headers show
Series
  • libipa: agc: Fix constraints yTarget handling and add PWL support
Related show

Commit Message

Stefan Klug Oct. 14, 2025, 2:24 p.m. UTC 
The yTarget loading code is broken and works neither for plain values
nor for arrays of values to form a PWL. Fix this by supporting both
cases. If a list is provided in the tuning file construct a PWL,
otherwise construct a single point PWL with the given value.

Fixes: 24247a12c7d3 ("ipa: libipa: Add AgcMeanLuminance base class")
Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/libipa/agc_mean_luminance.cpp | 54 +++++++++++++++++++++------
 src/ipa/libipa/agc_mean_luminance.h   |  5 +--
 src/ipa/rkisp1/algorithms/wdr.cpp     |  3 +-
 3 files changed, 46 insertions(+), 16 deletions(-)

Comments

Dan Scally Oct. 31, 2025, 4:01 p.m. UTC | #1 
Hi Stefan

On 14/10/2025 15:24, Stefan Klug wrote:
> The yTarget loading code is broken and works neither for plain values
> nor for arrays of values to form a PWL. Fix this by supporting both
> cases. If a list is provided in the tuning file construct a PWL,
> otherwise construct a single point PWL with the given value.
> 
> Fixes: 24247a12c7d3 ("ipa: libipa: Add AgcMeanLuminance base class")

Thanks :D

> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   src/ipa/libipa/agc_mean_luminance.cpp | 54 +++++++++++++++++++++------
>   src/ipa/libipa/agc_mean_luminance.h   |  5 +--
>   src/ipa/rkisp1/algorithms/wdr.cpp     |  3 +-
>   3 files changed, 46 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index 62b1918a45a7..551f6515e849 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -181,7 +181,7 @@ int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
>   	return 0;
>   }
>   
> -void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> +int AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
>   {
>   	for (const auto &[boundName, content] : modeDict.asDict()) {
>   		if (boundName != "upper" && boundName != "lower") {
> @@ -194,10 +194,27 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
>   		AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx);
>   		double qLo = content["qLo"].get<double>().value_or(0.98);
>   		double qHi = content["qHi"].get<double>().value_or(1.0);
> -		double yTarget =
> -			content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0);
> +		auto &target = content["yTarget"];
> +		Pwl yTarget;
> +		if (target.isValue()) {
> +			auto v = target.get<double>();
> +			if (!v) {
> +				LOG(AgcMeanLuminance, Error)
> +					<< "Failed to parse single value yTarget";
> +				return -EINVAL;
> +			}
> +			yTarget.append(0, *v);
> +		} else {
> +			auto v = target.get<Pwl>();
> +			if (!v) {
> +				LOG(AgcMeanLuminance, Error)
> +					<< "Failed to parse PWL based yTarget";
> +				return -EINVAL;
> +			}
> +			yTarget.swap(*v);
> +		}

Given this has much the same structure as ::parseRelativeLuminanceTarget() now it might be nice if 
they could share an single function that does the parsing >
> -		AgcConstraint constraint = { bound, qLo, qHi, yTarget };
> +		AgcConstraint constraint = { bound, qLo, qHi, std::move(yTarget) };
>   
>   		if (!constraintModes_.count(id))
>   			constraintModes_[id] = {};
> @@ -207,6 +224,8 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
>   		else
>   			constraintModes_[id].insert(constraintModes_[id].begin(), constraint);
>   	}
> +
> +	return 0;
>   }
>   
>   int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
> @@ -229,8 +248,11 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
>   				return -EINVAL;
>   			}
>   
> -			parseConstraint(modeDict,
> -					AeConstraintModeNameValueMap.at(modeName));
> +			int ret = parseConstraint(modeDict,
> +						  AeConstraintModeNameValueMap.at(modeName));
> +			if (ret)
> +				return ret;
> +
>   			availableConstraintModes.push_back(
>   				AeConstraintModeNameValueMap.at(modeName));
>   		}
> @@ -247,7 +269,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
>   			AgcConstraint::Bound::Lower,
>   			0.98,
>   			1.0,
> -			0.5
> +			{ 0.0, 0.5 }
>   		};
>   
>   		constraintModes_[controls::ConstraintNormal].insert(
> @@ -360,8 +382,9 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
>    * the data in a specific format; the Agc algorithm's tuning data should contain
>    * a dictionary called AeConstraintMode containing per-mode setting dictionaries
>    * with the key being a value from \ref controls::AeConstraintModeNameValueMap.
> - * Each mode dict may contain either a "lower" or "upper" key or both, for
> - * example:
> + * The yTarget can either be provided as single value or as array in which case
> + * it is interpreted as a PWL mapping lux levels to yTarget values. Each mode
> + * dict may contain either a "lower" or "upper" key or both, for example:

I think maybe make it clearer here that a scalar value is wrong and only supported for legacy reasons.

Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

>    *
>    * \code{.unparsed}
>    * algorithms:
> @@ -380,7 +403,7 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
>    *           upper:
>    *             qLo: 0.98
>    *             qHi: 1.0
> - *             yTarget: 0.8
> + *             yTarget: [ 100, 0.8, 20000, 0.5 ]
>    *
>    * \endcode
>    *
> @@ -536,8 +559,15 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>   					     const Histogram &hist,
>   					     double gain)
>   {
> -	auto applyConstraint = [&gain, &hist](const AgcConstraint &constraint) {
> -		double newGain = constraint.yTarget * hist.bins() /
> +	auto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {
> +		double lux = lux_;
> +
> +		if (relativeLuminanceTarget_.size() > 1 && lux_ == 0)
> +			lux = kDefaultLuxLevel;
> +
> +		double target = constraint.yTarget.eval(
> +			constraint.yTarget.domain().clamp(lux));
> +		double newGain = target * hist.bins() /
>   				 hist.interQuantileMean(constraint.qLo, constraint.qHi);
>   
>   		if (constraint.bound == AgcConstraint::Bound::Lower &&
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index 1fbff304a72b..d0d1fc139509 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -40,7 +40,7 @@ public:
>   		Bound bound;
>   		double qLo;
>   		double qHi;
> -		double yTarget;
> +		Pwl yTarget;
>   	};
>   
>   	void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
> @@ -88,8 +88,7 @@ public:
>   private:
>   	virtual double estimateLuminance(const double gain) const = 0;
>   	int parseRelativeLuminanceTarget(const YamlObject &tuningData);
> -
> -	void parseConstraint(const YamlObject &modeDict, int32_t id);
> +	int parseConstraint(const YamlObject &modeDict, int32_t id);
>   	int parseConstraintModes(const YamlObject &tuningData);
>   	int parseExposureModes(const YamlObject &tuningData);
>   	double estimateInitialGain() const;
> diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp
> index 45144913dcd8..ed81628c032c 100644
> --- a/src/ipa/rkisp1/algorithms/wdr.cpp
> +++ b/src/ipa/rkisp1/algorithms/wdr.cpp
> @@ -175,7 +175,8 @@ int WideDynamicRange::configure(IPAContext &context,
>   	constraint.bound = AgcMeanLuminance::AgcConstraint::Bound::Upper;
>   	constraint.qHi = 1.0;
>   	constraint.qLo = 1.0 - exposureConstraintMaxBrightPixels_;
> -	constraint.yTarget = exposureConstraintY_;
> +	constraint.yTarget.clear();
> +	constraint.yTarget.append(0, exposureConstraintY_);
>   	return 0;
>   }
>
Stefan Klug Nov. 3, 2025, 1:22 p.m. UTC | #2 
Hi Dan,

Thank you for the review.

Quoting Dan Scally (2025-10-31 17:01:02)
> Hi Stefan
> 
> On 14/10/2025 15:24, Stefan Klug wrote:
> > The yTarget loading code is broken and works neither for plain values
> > nor for arrays of values to form a PWL. Fix this by supporting both
> > cases. If a list is provided in the tuning file construct a PWL,
> > otherwise construct a single point PWL with the given value.
> > 
> > Fixes: 24247a12c7d3 ("ipa: libipa: Add AgcMeanLuminance base class")
> 
> Thanks :D
> 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >   src/ipa/libipa/agc_mean_luminance.cpp | 54 +++++++++++++++++++++------
> >   src/ipa/libipa/agc_mean_luminance.h   |  5 +--
> >   src/ipa/rkisp1/algorithms/wdr.cpp     |  3 +-
> >   3 files changed, 46 insertions(+), 16 deletions(-)
> > 
> > diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> > index 62b1918a45a7..551f6515e849 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.cpp
> > +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> > @@ -181,7 +181,7 @@ int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
> >       return 0;
> >   }
> >   
> > -void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> > +int AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> >   {
> >       for (const auto &[boundName, content] : modeDict.asDict()) {
> >               if (boundName != "upper" && boundName != "lower") {
> > @@ -194,10 +194,27 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> >               AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx);
> >               double qLo = content["qLo"].get<double>().value_or(0.98);
> >               double qHi = content["qHi"].get<double>().value_or(1.0);
> > -             double yTarget =
> > -                     content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0);
> > +             auto &target = content["yTarget"];
> > +             Pwl yTarget;
> > +             if (target.isValue()) {
> > +                     auto v = target.get<double>();
> > +                     if (!v) {
> > +                             LOG(AgcMeanLuminance, Error)
> > +                                     << "Failed to parse single value yTarget";
> > +                             return -EINVAL;
> > +                     }
> > +                     yTarget.append(0, *v);
> > +             } else {
> > +                     auto v = target.get<Pwl>();
> > +                     if (!v) {
> > +                             LOG(AgcMeanLuminance, Error)
> > +                                     << "Failed to parse PWL based yTarget";
> > +                             return -EINVAL;
> > +                     }
> > +                     yTarget.swap(*v);
> > +             }
> 
> Given this has much the same structure as ::parseRelativeLuminanceTarget() now it might be nice if 
> they could share an single function that does the parsing >
> > -             AgcConstraint constraint = { bound, qLo, qHi, yTarget };
> > +             AgcConstraint constraint = { bound, qLo, qHi, std::move(yTarget) };
> >   
> >               if (!constraintModes_.count(id))
> >                       constraintModes_[id] = {};
> > @@ -207,6 +224,8 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
> >               else
> >                       constraintModes_[id].insert(constraintModes_[id].begin(), constraint);
> >       }
> > +
> > +     return 0;
> >   }
> >   
> >   int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
> > @@ -229,8 +248,11 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
> >                               return -EINVAL;
> >                       }
> >   
> > -                     parseConstraint(modeDict,
> > -                                     AeConstraintModeNameValueMap.at(modeName));
> > +                     int ret = parseConstraint(modeDict,
> > +                                               AeConstraintModeNameValueMap.at(modeName));
> > +                     if (ret)
> > +                             return ret;
> > +
> >                       availableConstraintModes.push_back(
> >                               AeConstraintModeNameValueMap.at(modeName));
> >               }
> > @@ -247,7 +269,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
> >                       AgcConstraint::Bound::Lower,
> >                       0.98,
> >                       1.0,
> > -                     0.5
> > +                     { 0.0, 0.5 }
> >               };
> >   
> >               constraintModes_[controls::ConstraintNormal].insert(
> > @@ -360,8 +382,9 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
> >    * the data in a specific format; the Agc algorithm's tuning data should contain
> >    * a dictionary called AeConstraintMode containing per-mode setting dictionaries
> >    * with the key being a value from \ref controls::AeConstraintModeNameValueMap.
> > - * Each mode dict may contain either a "lower" or "upper" key or both, for
> > - * example:
> > + * The yTarget can either be provided as single value or as array in which case
> > + * it is interpreted as a PWL mapping lux levels to yTarget values. Each mode
> > + * dict may contain either a "lower" or "upper" key or both, for example:
> 
> I think maybe make it clearer here that a scalar value is wrong and only supported for legacy reasons.

You mentioned something similar on one patch before. Actually I didn't
see the single value as a legacy feature. I like the idea that you can
specify either a value or a PWL. That makes the tuning files easier to
read and write. Thinking about that we could even go one step further
and allow the PWL parser to read a single value so we could ditch the
special cases in the loading code. Functionality and tuning file wise I
like the idea that we document a value to be a PWL and you can
transparently specify it either as constant a list of pairs.

What do you think?

Best regards,
Stefan

> 
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
> 
> >    *
> >    * \code{.unparsed}
> >    * algorithms:
> > @@ -380,7 +403,7 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
> >    *           upper:
> >    *             qLo: 0.98
> >    *             qHi: 1.0
> > - *             yTarget: 0.8
> > + *             yTarget: [ 100, 0.8, 20000, 0.5 ]
> >    *
> >    * \endcode
> >    *
> > @@ -536,8 +559,15 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
> >                                            const Histogram &hist,
> >                                            double gain)
> >   {
> > -     auto applyConstraint = [&gain, &hist](const AgcConstraint &constraint) {
> > -             double newGain = constraint.yTarget * hist.bins() /
> > +     auto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {
> > +             double lux = lux_;
> > +
> > +             if (relativeLuminanceTarget_.size() > 1 && lux_ == 0)
> > +                     lux = kDefaultLuxLevel;
> > +
> > +             double target = constraint.yTarget.eval(
> > +                     constraint.yTarget.domain().clamp(lux));
> > +             double newGain = target * hist.bins() /
> >                                hist.interQuantileMean(constraint.qLo, constraint.qHi);
> >   
> >               if (constraint.bound == AgcConstraint::Bound::Lower &&
> > diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> > index 1fbff304a72b..d0d1fc139509 100644
> > --- a/src/ipa/libipa/agc_mean_luminance.h
> > +++ b/src/ipa/libipa/agc_mean_luminance.h
> > @@ -40,7 +40,7 @@ public:
> >               Bound bound;
> >               double qLo;
> >               double qHi;
> > -             double yTarget;
> > +             Pwl yTarget;
> >       };
> >   
> >       void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
> > @@ -88,8 +88,7 @@ public:
> >   private:
> >       virtual double estimateLuminance(const double gain) const = 0;
> >       int parseRelativeLuminanceTarget(const YamlObject &tuningData);
> > -
> > -     void parseConstraint(const YamlObject &modeDict, int32_t id);
> > +     int parseConstraint(const YamlObject &modeDict, int32_t id);
> >       int parseConstraintModes(const YamlObject &tuningData);
> >       int parseExposureModes(const YamlObject &tuningData);
> >       double estimateInitialGain() const;
> > diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp
> > index 45144913dcd8..ed81628c032c 100644
> > --- a/src/ipa/rkisp1/algorithms/wdr.cpp
> > +++ b/src/ipa/rkisp1/algorithms/wdr.cpp
> > @@ -175,7 +175,8 @@ int WideDynamicRange::configure(IPAContext &context,
> >       constraint.bound = AgcMeanLuminance::AgcConstraint::Bound::Upper;
> >       constraint.qHi = 1.0;
> >       constraint.qLo = 1.0 - exposureConstraintMaxBrightPixels_;
> > -     constraint.yTarget = exposureConstraintY_;
> > +     constraint.yTarget.clear();
> > +     constraint.yTarget.append(0, exposureConstraintY_);
> >       return 0;
> >   }
> >   
>
Dan Scally Nov. 3, 2025, 3:22 p.m. UTC | #3 
Hi Stefan

On 03/11/2025 13:22, Stefan Klug wrote:
> Hi Dan,
> 
> Thank you for the review.
> 
> Quoting Dan Scally (2025-10-31 17:01:02)
>> Hi Stefan
>>
>> On 14/10/2025 15:24, Stefan Klug wrote:
>>> The yTarget loading code is broken and works neither for plain values
>>> nor for arrays of values to form a PWL. Fix this by supporting both
>>> cases. If a list is provided in the tuning file construct a PWL,
>>> otherwise construct a single point PWL with the given value.
>>>
>>> Fixes: 24247a12c7d3 ("ipa: libipa: Add AgcMeanLuminance base class")
>>
>> Thanks :D
>>
>>> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
>>> ---
>>>    src/ipa/libipa/agc_mean_luminance.cpp | 54 +++++++++++++++++++++------
>>>    src/ipa/libipa/agc_mean_luminance.h   |  5 +--
>>>    src/ipa/rkisp1/algorithms/wdr.cpp     |  3 +-
>>>    3 files changed, 46 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
>>> index 62b1918a45a7..551f6515e849 100644
>>> --- a/src/ipa/libipa/agc_mean_luminance.cpp
>>> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
>>> @@ -181,7 +181,7 @@ int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
>>>        return 0;
>>>    }
>>>    
>>> -void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
>>> +int AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
>>>    {
>>>        for (const auto &[boundName, content] : modeDict.asDict()) {
>>>                if (boundName != "upper" && boundName != "lower") {
>>> @@ -194,10 +194,27 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
>>>                AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx);
>>>                double qLo = content["qLo"].get<double>().value_or(0.98);
>>>                double qHi = content["qHi"].get<double>().value_or(1.0);
>>> -             double yTarget =
>>> -                     content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0);
>>> +             auto &target = content["yTarget"];
>>> +             Pwl yTarget;
>>> +             if (target.isValue()) {
>>> +                     auto v = target.get<double>();
>>> +                     if (!v) {
>>> +                             LOG(AgcMeanLuminance, Error)
>>> +                                     << "Failed to parse single value yTarget";
>>> +                             return -EINVAL;
>>> +                     }
>>> +                     yTarget.append(0, *v);
>>> +             } else {
>>> +                     auto v = target.get<Pwl>();
>>> +                     if (!v) {
>>> +                             LOG(AgcMeanLuminance, Error)
>>> +                                     << "Failed to parse PWL based yTarget";
>>> +                             return -EINVAL;
>>> +                     }
>>> +                     yTarget.swap(*v);
>>> +             }
>>
>> Given this has much the same structure as ::parseRelativeLuminanceTarget() now it might be nice if
>> they could share an single function that does the parsing >
>>> -             AgcConstraint constraint = { bound, qLo, qHi, yTarget };
>>> +             AgcConstraint constraint = { bound, qLo, qHi, std::move(yTarget) };
>>>    
>>>                if (!constraintModes_.count(id))
>>>                        constraintModes_[id] = {};
>>> @@ -207,6 +224,8 @@ void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
>>>                else
>>>                        constraintModes_[id].insert(constraintModes_[id].begin(), constraint);
>>>        }
>>> +
>>> +     return 0;
>>>    }
>>>    
>>>    int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
>>> @@ -229,8 +248,11 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
>>>                                return -EINVAL;
>>>                        }
>>>    
>>> -                     parseConstraint(modeDict,
>>> -                                     AeConstraintModeNameValueMap.at(modeName));
>>> +                     int ret = parseConstraint(modeDict,
>>> +                                               AeConstraintModeNameValueMap.at(modeName));
>>> +                     if (ret)
>>> +                             return ret;
>>> +
>>>                        availableConstraintModes.push_back(
>>>                                AeConstraintModeNameValueMap.at(modeName));
>>>                }
>>> @@ -247,7 +269,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
>>>                        AgcConstraint::Bound::Lower,
>>>                        0.98,
>>>                        1.0,
>>> -                     0.5
>>> +                     { 0.0, 0.5 }
>>>                };
>>>    
>>>                constraintModes_[controls::ConstraintNormal].insert(
>>> @@ -360,8 +382,9 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
>>>     * the data in a specific format; the Agc algorithm's tuning data should contain
>>>     * a dictionary called AeConstraintMode containing per-mode setting dictionaries
>>>     * with the key being a value from \ref controls::AeConstraintModeNameValueMap.
>>> - * Each mode dict may contain either a "lower" or "upper" key or both, for
>>> - * example:
>>> + * The yTarget can either be provided as single value or as array in which case
>>> + * it is interpreted as a PWL mapping lux levels to yTarget values. Each mode
>>> + * dict may contain either a "lower" or "upper" key or both, for example:
>>
>> I think maybe make it clearer here that a scalar value is wrong and only supported for legacy reasons.
> 
> You mentioned something similar on one patch before. Actually I didn't
> see the single value as a legacy feature. I like the idea that you can
> specify either a value or a PWL. That makes the tuning files easier to
> read and write. Thinking about that we could even go one step further
> and allow the PWL parser to read a single value so we could ditch the
> special cases in the loading code. Functionality and tuning file wise I
> like the idea that we document a value to be a PWL and you can
> transparently specify it either as constant a list of pairs.
> 
> What do you think?

Ah! Yeah sure; it certainly will make it easier to handle the tuning files if you only want it 
lux-agnostic anyway. I guess you mean a new constructor in libcamera::ipa::Pwl? If so then yes I 
agree that that would be a nice way of doing it.

Thanks
Dan

> 
> Best regards,
> Stefan
> 
>>
>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>>
>>>     *
>>>     * \code{.unparsed}
>>>     * algorithms:
>>> @@ -380,7 +403,7 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
>>>     *           upper:
>>>     *             qLo: 0.98
>>>     *             qHi: 1.0
>>> - *             yTarget: 0.8
>>> + *             yTarget: [ 100, 0.8, 20000, 0.5 ]
>>>     *
>>>     * \endcode
>>>     *
>>> @@ -536,8 +559,15 @@ double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
>>>                                             const Histogram &hist,
>>>                                             double gain)
>>>    {
>>> -     auto applyConstraint = [&gain, &hist](const AgcConstraint &constraint) {
>>> -             double newGain = constraint.yTarget * hist.bins() /
>>> +     auto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {
>>> +             double lux = lux_;
>>> +
>>> +             if (relativeLuminanceTarget_.size() > 1 && lux_ == 0)
>>> +                     lux = kDefaultLuxLevel;
>>> +
>>> +             double target = constraint.yTarget.eval(
>>> +                     constraint.yTarget.domain().clamp(lux));
>>> +             double newGain = target * hist.bins() /
>>>                                 hist.interQuantileMean(constraint.qLo, constraint.qHi);
>>>    
>>>                if (constraint.bound == AgcConstraint::Bound::Lower &&
>>> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
>>> index 1fbff304a72b..d0d1fc139509 100644
>>> --- a/src/ipa/libipa/agc_mean_luminance.h
>>> +++ b/src/ipa/libipa/agc_mean_luminance.h
>>> @@ -40,7 +40,7 @@ public:
>>>                Bound bound;
>>>                double qLo;
>>>                double qHi;
>>> -             double yTarget;
>>> +             Pwl yTarget;
>>>        };
>>>    
>>>        void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
>>> @@ -88,8 +88,7 @@ public:
>>>    private:
>>>        virtual double estimateLuminance(const double gain) const = 0;
>>>        int parseRelativeLuminanceTarget(const YamlObject &tuningData);
>>> -
>>> -     void parseConstraint(const YamlObject &modeDict, int32_t id);
>>> +     int parseConstraint(const YamlObject &modeDict, int32_t id);
>>>        int parseConstraintModes(const YamlObject &tuningData);
>>>        int parseExposureModes(const YamlObject &tuningData);
>>>        double estimateInitialGain() const;
>>> diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp
>>> index 45144913dcd8..ed81628c032c 100644
>>> --- a/src/ipa/rkisp1/algorithms/wdr.cpp
>>> +++ b/src/ipa/rkisp1/algorithms/wdr.cpp
>>> @@ -175,7 +175,8 @@ int WideDynamicRange::configure(IPAContext &context,
>>>        constraint.bound = AgcMeanLuminance::AgcConstraint::Bound::Upper;
>>>        constraint.qHi = 1.0;
>>>        constraint.qLo = 1.0 - exposureConstraintMaxBrightPixels_;
>>> -     constraint.yTarget = exposureConstraintY_;
>>> +     constraint.yTarget.clear();
>>> +     constraint.yTarget.append(0, exposureConstraintY_);
>>>        return 0;
>>>    }
>>>    
>>

Patch
diff mbox series 

diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
index 62b1918a45a7..551f6515e849 100644
--- a/src/ipa/libipa/agc_mean_luminance.cpp
+++ b/src/ipa/libipa/agc_mean_luminance.cpp
@@ -181,7 +181,7 @@  int AgcMeanLuminance::parseRelativeLuminanceTarget(const YamlObject &tuningData)
 	return 0;
 }
 
-void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
+int AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
 {
 	for (const auto &[boundName, content] : modeDict.asDict()) {
 		if (boundName != "upper" && boundName != "lower") {
@@ -194,10 +194,27 @@  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
 		AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx);
 		double qLo = content["qLo"].get<double>().value_or(0.98);
 		double qHi = content["qHi"].get<double>().value_or(1.0);
-		double yTarget =
-			content["yTarget"].getList<double>().value_or(std::vector<double>{ 0.5 }).at(0);
+		auto &target = content["yTarget"];
+		Pwl yTarget;
+		if (target.isValue()) {
+			auto v = target.get<double>();
+			if (!v) {
+				LOG(AgcMeanLuminance, Error)
+					<< "Failed to parse single value yTarget";
+				return -EINVAL;
+			}
+			yTarget.append(0, *v);
+		} else {
+			auto v = target.get<Pwl>();
+			if (!v) {
+				LOG(AgcMeanLuminance, Error)
+					<< "Failed to parse PWL based yTarget";
+				return -EINVAL;
+			}
+			yTarget.swap(*v);
+		}
 
-		AgcConstraint constraint = { bound, qLo, qHi, yTarget };
+		AgcConstraint constraint = { bound, qLo, qHi, std::move(yTarget) };
 
 		if (!constraintModes_.count(id))
 			constraintModes_[id] = {};
@@ -207,6 +224,8 @@  void AgcMeanLuminance::parseConstraint(const YamlObject &modeDict, int32_t id)
 		else
 			constraintModes_[id].insert(constraintModes_[id].begin(), constraint);
 	}
+
+	return 0;
 }
 
 int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
@@ -229,8 +248,11 @@  int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
 				return -EINVAL;
 			}
 
-			parseConstraint(modeDict,
-					AeConstraintModeNameValueMap.at(modeName));
+			int ret = parseConstraint(modeDict,
+						  AeConstraintModeNameValueMap.at(modeName));
+			if (ret)
+				return ret;
+
 			availableConstraintModes.push_back(
 				AeConstraintModeNameValueMap.at(modeName));
 		}
@@ -247,7 +269,7 @@  int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
 			AgcConstraint::Bound::Lower,
 			0.98,
 			1.0,
-			0.5
+			{ 0.0, 0.5 }
 		};
 
 		constraintModes_[controls::ConstraintNormal].insert(
@@ -360,8 +382,9 @@  void AgcMeanLuminance::configure(utils::Duration lineDuration,
  * the data in a specific format; the Agc algorithm's tuning data should contain
  * a dictionary called AeConstraintMode containing per-mode setting dictionaries
  * with the key being a value from \ref controls::AeConstraintModeNameValueMap.
- * Each mode dict may contain either a "lower" or "upper" key or both, for
- * example:
+ * The yTarget can either be provided as single value or as array in which case
+ * it is interpreted as a PWL mapping lux levels to yTarget values. Each mode
+ * dict may contain either a "lower" or "upper" key or both, for example:
  *
  * \code{.unparsed}
  * algorithms:
@@ -380,7 +403,7 @@  void AgcMeanLuminance::configure(utils::Duration lineDuration,
  *           upper:
  *             qLo: 0.98
  *             qHi: 1.0
- *             yTarget: 0.8
+ *             yTarget: [ 100, 0.8, 20000, 0.5 ]
  *
  * \endcode
  *
@@ -536,8 +559,15 @@  double AgcMeanLuminance::constraintClampGain(uint32_t constraintModeIndex,
 					     const Histogram &hist,
 					     double gain)
 {
-	auto applyConstraint = [&gain, &hist](const AgcConstraint &constraint) {
-		double newGain = constraint.yTarget * hist.bins() /
+	auto applyConstraint = [this, &gain, &hist](const AgcConstraint &constraint) {
+		double lux = lux_;
+
+		if (relativeLuminanceTarget_.size() > 1 && lux_ == 0)
+			lux = kDefaultLuxLevel;
+
+		double target = constraint.yTarget.eval(
+			constraint.yTarget.domain().clamp(lux));
+		double newGain = target * hist.bins() /
 				 hist.interQuantileMean(constraint.qLo, constraint.qHi);
 
 		if (constraint.bound == AgcConstraint::Bound::Lower &&
diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
index 1fbff304a72b..d0d1fc139509 100644
--- a/src/ipa/libipa/agc_mean_luminance.h
+++ b/src/ipa/libipa/agc_mean_luminance.h
@@ -40,7 +40,7 @@  public:
 		Bound bound;
 		double qLo;
 		double qHi;
-		double yTarget;
+		Pwl yTarget;
 	};
 
 	void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
@@ -88,8 +88,7 @@  public:
 private:
 	virtual double estimateLuminance(const double gain) const = 0;
 	int parseRelativeLuminanceTarget(const YamlObject &tuningData);
-
-	void parseConstraint(const YamlObject &modeDict, int32_t id);
+	int parseConstraint(const YamlObject &modeDict, int32_t id);
 	int parseConstraintModes(const YamlObject &tuningData);
 	int parseExposureModes(const YamlObject &tuningData);
 	double estimateInitialGain() const;
diff --git a/src/ipa/rkisp1/algorithms/wdr.cpp b/src/ipa/rkisp1/algorithms/wdr.cpp
index 45144913dcd8..ed81628c032c 100644
--- a/src/ipa/rkisp1/algorithms/wdr.cpp
+++ b/src/ipa/rkisp1/algorithms/wdr.cpp
@@ -175,7 +175,8 @@  int WideDynamicRange::configure(IPAContext &context,
 	constraint.bound = AgcMeanLuminance::AgcConstraint::Bound::Upper;
 	constraint.qHi = 1.0;
 	constraint.qLo = 1.0 - exposureConstraintMaxBrightPixels_;
-	constraint.yTarget = exposureConstraintY_;
+	constraint.yTarget.clear();
+	constraint.yTarget.append(0, exposureConstraintY_);
 	return 0;
 }