[v8,04/18] ipa: libipa: agc_mean_luminance: Split tuning file parsing

28822 diff mbox series

Create a separate type that contains every piece of information that
is found in tuning files, and add a separate `parse()` function to
populate an instance of that.

The main goal of this is for the tuning file data to be inspected
before the fallback defaults are added, so those are removed from
the parsing and moved into the constructor.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 src/ipa/libipa/agc.cpp                |   9 +-
 src/ipa/libipa/agc_mean_luminance.cpp | 205 +++++++++++++++-----------
 src/ipa/libipa/agc_mean_luminance.h   |  24 +--
 3 files changed, 140 insertions(+), 98 deletions(-)

Hi Barnabás,

Quoting Barnabás Pőcze (2026-11-09 17:12:31)
> Create a separate type that contains every piece of information that
> is found in tuning files, and add a separate `parse()` function to
> populate an instance of that.
> 
> The main goal of this is for the tuning file data to be inspected
> before the fallback defaults are added, so those are removed from
> the parsing and moved into the constructor.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

I like that version better.

Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>

Best regards,
Stefan

> ---
>  src/ipa/libipa/agc.cpp                |   9 +-
>  src/ipa/libipa/agc_mean_luminance.cpp | 205 +++++++++++++++-----------
>  src/ipa/libipa/agc_mean_luminance.h   |  24 +--
>  3 files changed, 140 insertions(+), 98 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 182dd7380e..77d101def8 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -310,10 +310,11 @@ int AgcAlgorithm::init(const ValueNode &tuningData, CameraSensorHelper *sensor,
>                        const ConfigurationParams &config)
>  {
>         if (sensor) {
> -               auto &impl = impl_.emplace<AgcMeanLuminance>();
> -               int ret = impl.parseTuningData(tuningData);
> -               if (ret)
> -                       return ret;
> +               auto agcConfig = AgcMeanLuminance::Configuration::parse(tuningData);
> +               if (!agcConfig)
> +                       return -EINVAL;
> +
> +               impl_.emplace<AgcMeanLuminance>(std::move(*agcConfig));
>         } else {
>                 impl_.emplace<AgcMSV>();
>         }
> diff --git a/src/ipa/libipa/agc_mean_luminance.cpp b/src/ipa/libipa/agc_mean_luminance.cpp
> index a763a3b94d..1ade717c17 100644
> --- a/src/ipa/libipa/agc_mean_luminance.cpp
> +++ b/src/ipa/libipa/agc_mean_luminance.cpp
> @@ -9,7 +9,7 @@
>  
>  #include <algorithm>
>  #include <cmath>
> -#include <span>
> +#include <optional>
>  
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/utils.h>
> @@ -106,6 +106,29 @@ static constexpr unsigned int kDefaultLuxLevel = 500;
>   * \brief The luminance target for the constraint
>   */
>  
> +/**
> + * \class AgcMeanLuminance::Configuration
> + * \brief A collection of parameters
> + *
> + * This type contains various configuration parameters for the algorithm
> + * that are generally parsed from a tuning file.
> + */
> +
> +/**
> + * \var AgcMeanLuminance::Configuration::relativeLuminanceTarget
> + * \brief The relative mean luminance target by lux value
> + */
> +
> +/**
> + * \var AgcMeanLuminance::Configuration::constraintModes
> + * \brief The available constraint modes
> + */
> +
> +/**
> + * \var AgcMeanLuminance::Configuration::exposureModeHelpers
> + * \brief The available exposure modes
> + */
> +
>  /**
>   * \class AgcMeanLuminance::Traits
>   * \brief A collection of callbacks
> @@ -170,27 +193,19 @@ static constexpr unsigned int kDefaultLuxLevel = 500;
>   *
>   * IPA modules that want to use this class to implement their AEGC algorithm
>   * should derive AgcMeanLuminance::Traits and override the necessary functions.
> - * The users must call parseTuningData() in init(), and must also call configure()
> - * and setLimits() in configure(). They may then use calculateNewEv() in process().
> - * If the limits passed to setLimits() change for any reason (for example, in
> - * response to a FrameDurationLimit control being passed in queueRequest()) then
> - * setLimits() must be called again with the new values.
> + * The users must call configure() and setLimits() in configure(). They may
> + * then use calculateNewEv() in process(). If the limits passed to setLimits()
> + * change for any reason (for example, in response to a FrameDurationLimit control
> + * being passed in queueRequest()) then setLimits() must be called again with the
> + * new values.
>   */
>  
> -AgcMeanLuminance::AgcMeanLuminance()
> -       : filteredExposure_(0s), luxWarningEnabled_(true), frameCount_(0)
> -{
> -}
> +namespace {
>  
> -AgcMeanLuminance::~AgcMeanLuminance() = default;
> -
> -int AgcMeanLuminance::parseRelativeLuminanceTarget(const ValueNode &tuningData)
> +int parseRelativeLuminanceTarget(AgcMeanLuminance::Configuration &config, const ValueNode &target)
>  {
> -       auto &target = tuningData["relativeLuminanceTarget"];
> -       if (!target) {
> -               relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };
> +       if (!target)
>                 return 0;
> -       }
>  
>         auto pwl = target.get<Pwl>();
>         if (!pwl) {
> @@ -199,11 +214,11 @@ int AgcMeanLuminance::parseRelativeLuminanceTarget(const ValueNode &tuningData)
>                 return -EINVAL;
>         }
>  
> -       relativeLuminanceTarget_ = std::move(*pwl);
> +       config.relativeLuminanceTarget = std::move(*pwl);
>         return 0;
>  }
>  
> -int AgcMeanLuminance::parseConstraint(const ValueNode &modeDict, int32_t id)
> +int parseConstraint(AgcMeanLuminance::Configuration &config, const ValueNode &modeDict, int32_t id)
>  {
>         for (const auto &[boundName, content] : modeDict.asDict()) {
>                 if (boundName != "upper" && boundName != "lower") {
> @@ -213,7 +228,7 @@ int AgcMeanLuminance::parseConstraint(const ValueNode &modeDict, int32_t id)
>                 }
>  
>                 unsigned int idx = static_cast<unsigned int>(boundName == "upper");
> -               AgcConstraint::Bound bound = static_cast<AgcConstraint::Bound>(idx);
> +               auto bound = static_cast<AgcMeanLuminance::AgcConstraint::Bound>(idx);
>                 double qLo = content["qLo"].get<double>().value_or(0.98);
>                 double qHi = content["qHi"].get<double>().value_or(1.0);
>                 auto yTarget = content["yTarget"].get<Pwl>();
> @@ -223,8 +238,13 @@ int AgcMeanLuminance::parseConstraint(const ValueNode &modeDict, int32_t id)
>                         return -EINVAL;
>                 }
>  
> -               AgcConstraint constraint = { bound, qLo, qHi, std::move(*yTarget) };
> -               auto &constraints = constraintModes_[id];
> +               AgcMeanLuminance::AgcConstraint constraint = {
> +                       bound,
> +                       qLo,
> +                       qHi,
> +                       std::move(*yTarget),
> +               };
> +               auto &constraints = config.constraintModes[id];
>  
>                 if (idx)
>                         constraints.push_back(std::move(constraint));
> @@ -235,7 +255,7 @@ int AgcMeanLuminance::parseConstraint(const ValueNode &modeDict, int32_t id)
>         return 0;
>  }
>  
> -int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
> +int parseConstraintModes(AgcMeanLuminance::Configuration &config, const ValueNode &constraintModes)
>  {
>         static constexpr std::tuple<std::string_view, std::string_view, controls::AeConstraintModeEnum> compat[] = {
>                 { "ConstraintNormal", "Normal", controls::AeConstraintModeNormal },
> @@ -244,7 +264,6 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
>                 { "ConstraintCustom", "Custom", controls::AeConstraintModeCustom },
>         };
>  
> -       const ValueNode &constraintModes = tuningData[controls::AeConstraintMode.name()];
>         if (constraintModes.isDictionary()) {
>                 for (const auto &[modeName, modeDict] : constraintModes.asDict()) {
>                         std::optional<controls::AeConstraintModeEnum> modeValue;
> @@ -278,33 +297,16 @@ int AgcMeanLuminance::parseConstraintModes(const ValueNode &tuningData)
>                                 return -EINVAL;
>                         }
>  
> -                       int ret = parseConstraint(modeDict, *modeValue);
> +                       int ret = parseConstraint(config, modeDict, *modeValue);
>                         if (ret)
>                                 return ret;
>                 }
>         }
>  
> -       /*
> -        * If the tuning data file contains no constraints then we use the
> -        * default constraint that the IPU3/RkISP1 Agc algorithms were adhering
> -        * to anyway before centralisation; this constraint forces the top 2% of
> -        * the histogram to be at least 0.5.
> -        */
> -       if (constraintModes_.empty()) {
> -               AgcConstraint constraint = {
> -                       AgcConstraint::Bound::Lower,
> -                       0.98,
> -                       1.0,
> -                       Pwl({ { { 0.0, 0.5 } } })
> -               };
> -
> -               constraintModes_[controls::AeConstraintModeNormal].push_back(std::move(constraint));
> -       }
> -
>         return 0;
>  }
>  
> -int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
> +int parseExposureModes(AgcMeanLuminance::Configuration &config, const ValueNode &exposureModes)
>  {
>         static constexpr std::tuple<std::string_view, std::string_view, controls::AeExposureModeEnum> compat[] = {
>                 { "ExposureNormal", "Normal", controls::AeExposureModeNormal },
> @@ -313,7 +315,6 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>                 { "ExposureCustom", "Custom", controls::AeExposureModeCustom },
>         };
>  
> -       const ValueNode &exposureModes = tuningData[controls::AeExposureMode.name()];
>         if (exposureModes.isDictionary()) {
>                 for (const auto &[modeName, modeValues] : exposureModes.asDict()) {
>                         std::optional<controls::AeExposureModeEnum> modeValue;
> @@ -372,44 +373,18 @@ int AgcMeanLuminance::parseExposureModes(const ValueNode &tuningData)
>                                 });
>                         }
>  
> -                       exposureModeHelpers_.try_emplace(*modeValue, std::move(stages));
> +                       config.exposureModeHelpers.try_emplace(
> +                               *modeValue, std::move(stages));
>                 }
>         }
>  
> -       /*
> -        * If we don't have any exposure modes in the tuning data we create an
> -        * ExposureModeHelper using an empty vector of stages. This will result
> -        * in the ExposureModeHelper simply driving the exposure time as high as
> -        * possible before touching gain.
> -        */
> -       if (exposureModeHelpers_.empty())
> -               exposureModeHelpers_.try_emplace(controls::AeExposureModeNormal,
> -                                                std::vector<ExposureModeHelper::Stage>{});
> -
>         return 0;
>  }
>  
> -/**
> - * \brief Configure the exposure mode helpers
> - * \param[in] lineDuration The sensor line length
> - * \param[in] sensorHelper The sensor helper
> - *
> - * This function configures the exposure mode helpers so they can correctly
> - * take quantization effects into account.
> - */
> -void AgcMeanLuminance::configure(utils::Duration lineDuration,
> -                                const CameraSensorHelper *sensorHelper)
> -{
> -       for (auto &[id, helper] : exposureModeHelpers_)
> -               helper.configure(lineDuration, sensorHelper);
> -
> -       luxWarningEnabled_ = true;
> -       filteredExposure_ = 0s;
> -       frameCount_ = 0;
> -}
> +} /* namespace */
>  
>  /**
> - * \brief Parse tuning data for AeConstraintMode and AeExposureMode controls
> + * \brief Parse tuning data into an a Configuration instance
>   * \param[in] tuningData the ValueNode representing the tuning data
>   *
>   * This function parses tuning data to build the list of allowed values for the
> @@ -463,19 +438,79 @@ void AgcMeanLuminance::configure(utils::Duration lineDuration,
>   *
>   * \return 0 on success or a negative error code
>   */
> -int AgcMeanLuminance::parseTuningData(const ValueNode &tuningData)
> +std::optional<AgcMeanLuminance::Configuration>
> +AgcMeanLuminance::Configuration::parse(const ValueNode &tuningData)
>  {
> -       int ret;
> +       AgcMeanLuminance::Configuration config;
> +
> +       if (parseRelativeLuminanceTarget(config, tuningData["relativeLuminanceTarget"]))
> +               return {};
> +
> +       if (parseConstraintModes(config, tuningData[controls::AeConstraintMode.name()]))
> +               return {};
> +
> +       if (parseExposureModes(config, tuningData[controls::AeExposureMode.name()]))
> +               return {};
> +
> +       return config;
> +}
> +
> +AgcMeanLuminance::AgcMeanLuminance(Configuration &&config)
> +       : filteredExposure_(0s), luxWarningEnabled_(true), frameCount_(0),
> +         relativeLuminanceTarget_(std::move(config.relativeLuminanceTarget)),
> +         constraintModes_(std::move(config.constraintModes)),
> +         exposureModeHelpers_(std::move(config.exposureModeHelpers))
> +{
> +       if (relativeLuminanceTarget_.empty())
> +               relativeLuminanceTarget_ = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };
> +
> +       /*
> +        * If the tuning data file contains no constraints then we use the
> +        * default constraint that the IPU3/RkISP1 Agc algorithms were adhering
> +        * to anyway before centralisation; this constraint forces the top 2% of
> +        * the histogram to be at least 0.5.
> +        */
> +       if (constraintModes_.empty()) {
> +               AgcConstraint constraint = {
> +                       AgcConstraint::Bound::Lower,
> +                       0.98,
> +                       1.0,
> +                       Pwl({ { { 0.0, 0.5 } } }),
> +               };
> +
> +               constraintModes_[controls::AeConstraintModeNormal].push_back(std::move(constraint));
> +       }
>  
> -       ret = parseRelativeLuminanceTarget(tuningData);
> -       if (ret)
> -               return ret;
> +       /*
> +        * If we don't have any exposure modes in the tuning data we create an
> +        * ExposureModeHelper using an empty vector of stages. This will result
> +        * in the ExposureModeHelper simply driving the exposure time as high as
> +        * possible before touching gain.
> +        */
> +       if (exposureModeHelpers_.empty())
> +               exposureModeHelpers_.try_emplace(controls::AeExposureModeNormal,
> +                                                std::vector<ExposureModeHelper::Stage>{});
> +}
>  
> -       ret = parseConstraintModes(tuningData);
> -       if (ret)
> -               return ret;
> +AgcMeanLuminance::~AgcMeanLuminance() = default;
>  
> -       return parseExposureModes(tuningData);
> +/**
> + * \brief Configure the exposure mode helpers
> + * \param[in] lineDuration The sensor line length
> + * \param[in] sensorHelper The sensor helper
> + *
> + * This function configures the exposure mode helpers so they can correctly
> + * take quantization effects into account.
> + */
> +void AgcMeanLuminance::configure(utils::Duration lineDuration,
> +                                const CameraSensorHelper *sensorHelper)
> +{
> +       for (auto &[id, helper] : exposureModeHelpers_)
> +               helper.configure(lineDuration, sensorHelper);
> +
> +       luxWarningEnabled_ = true;
> +       filteredExposure_ = 0s;
> +       frameCount_ = 0;
>  }
>  
>  /**
> @@ -502,12 +537,12 @@ void AgcMeanLuminance::setLimits(utils::Duration minExposureTime,
>  
>  /**
>   * \fn AgcMeanLuminance::constraintModes()
> - * \brief Get the constraint modes that have been parsed from tuning data
> + * \brief Get the available constraint modes
>   */
>  
>  /**
>   * \fn AgcMeanLuminance::exposureModeHelpers()
> - * \brief Get the ExposureModeHelpers that have been parsed from tuning data
> + * \brief Get the available ExposureModeHelpers
>   */
>  
>  /**
> diff --git a/src/ipa/libipa/agc_mean_luminance.h b/src/ipa/libipa/agc_mean_luminance.h
> index f799618ffc..8c32ec8ede 100644
> --- a/src/ipa/libipa/agc_mean_luminance.h
> +++ b/src/ipa/libipa/agc_mean_luminance.h
> @@ -8,7 +8,7 @@
>  #pragma once
>  
>  #include <map>
> -#include <memory>
> +#include <optional>
>  #include <vector>
>  
>  #include <libcamera/base/utils.h>
> @@ -26,9 +26,6 @@ namespace ipa {
>  class AgcMeanLuminance
>  {
>  public:
> -       AgcMeanLuminance();
> -       ~AgcMeanLuminance();
> -
>         struct AgcConstraint {
>                 enum class Bound {
>                         Lower = 0,
> @@ -45,6 +42,18 @@ public:
>                 virtual double estimateLuminance(double gain) const = 0;
>         };
>  
> +       struct Configuration {
> +               Pwl relativeLuminanceTarget;
> +               std::map<int32_t, std::vector<AgcConstraint>> constraintModes;
> +               std::map<int32_t, ExposureModeHelper> exposureModeHelpers;
> +
> +               [[nodiscard]]
> +               static std::optional<Configuration> parse(const ValueNode &tuningData);
> +       };
> +
> +       AgcMeanLuminance(Configuration &&config);
> +       ~AgcMeanLuminance();
> +
>         void configure(utils::Duration lineDuration, const CameraSensorHelper *sensorHelper);
>         int parseTuningData(const ValueNode &tuningData);
>  
> @@ -80,20 +89,17 @@ public:
>         double effectiveYTarget(double lux, double exposureCompensation) const;
>  
>  private:
> -       int parseRelativeLuminanceTarget(const ValueNode &tuningData);
> -       int parseConstraint(const ValueNode &modeDict, int32_t id);
> -       int parseConstraintModes(const ValueNode &tuningData);
> -       int parseExposureModes(const ValueNode &tuningData);
>         double estimateInitialGain(const Traits &traits, double yTarget) const;
>         double constraintClampGain(const Params &params, double gain) const;
>         utils::Duration filterExposure(utils::Duration exposureValue);
>  
>         utils::Duration filteredExposure_;
>         mutable bool luxWarningEnabled_;
> -       Pwl relativeLuminanceTarget_;
>         uint64_t frameCount_;
>  
>         std::vector<AgcConstraint> additionalConstraints_;
> +
> +       Pwl relativeLuminanceTarget_;
>         std::map<int32_t, std::vector<AgcConstraint>> constraintModes_;
>         std::map<int32_t, ExposureModeHelper> exposureModeHelpers_;
>  };
> -- 
> 2.56.0
>