[v2,3/3] ipa: libipa: agc_mean_luminance: Fix yTarget handling in constraints

24983 diff mbox series

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>
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

---

Changes in v2:
- Collected tag
- Drop special logic to handle single values as that was added
  generically to the yaml Pwl loader
---
 src/ipa/libipa/agc_mean_luminance.cpp | 41 +++++++++++++++++++--------
 src/ipa/libipa/agc_mean_luminance.h   |  5 ++--
 src/ipa/rkisp1/algorithms/wdr.cpp     |  3 +-
 3 files changed, 33 insertions(+), 16 deletions(-)

Quoting Stefan Klug (2025-11-06 16:42:27)
> 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>
> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Can't find anything to say here so :

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

> ---
> 
> Changes in v2:
> - Collected tag
> - Drop special logic to handle single values as that was added
>   generically to the yaml Pwl loader
> ---
>  src/ipa/libipa/agc_mean_luminance.cpp | 41 +++++++++++++++++++--------
>  src/ipa/libipa/agc_mean_luminance.h   |  5 ++--
>  src/ipa/rkisp1/algorithms/wdr.cpp     |  3 +-
>  3 files changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index b80af92a2c0f..7fac8c760396 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -173,7 +173,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") {
> @@ -186,10 +186,14 @@ 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 yTarget = content["yTarget"].get<Pwl>();
> +               if (!yTarget) {
> +                       LOG(AgcMeanLuminance, Error)
> +                               << "Failed to parse yTarget";
> +                       return -EINVAL;
> +               }
>  
> -               AgcConstraint constraint = { bound, qLo, qHi, yTarget };
> +               AgcConstraint constraint = { bound, qLo, qHi, std::move(*yTarget) };
>  
>                 if (!constraintModes_.count(id))
>                         constraintModes_[id] = {};
> @@ -199,6 +203,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)
> @@ -221,8 +227,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));
>                 }
> @@ -239,7 +248,7 @@ int AgcMeanLuminance::parseConstraintModes(const YamlObject &tuningData)
>                         AgcConstraint::Bound::Lower,
>                         0.98,
>                         1.0,
> -                       0.5
> +                       Pwl({ { { 0.0, 0.5 } } })
>                 };
>  
>                 constraintModes_[controls::ConstraintNormal].insert(
> @@ -352,8 +361,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:
> @@ -372,7 +382,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
>   *
> @@ -528,8 +538,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;
>  }
>  
> -- 
> 2.51.0
>