[v7,05/19] ipa: libipa: agc: Split tuning data fallback handling

28803 diff mbox series

Add a separate function whose job is to add the fallback values
for the various configuration parameters that were not specified
in the tuning file.

This allows some simplification of the code by making use of early
returns, so do that as well. And additionally, pass only the
relevant object to the "sub" parsers instead of the whole thing.

This is needed in order to be able to determine what exactly the tuning
file contains, before adding the fallback values.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/ipa/libipa/agc.cpp | 245 +++++++++++++++++++++--------------------
 1 file changed, 125 insertions(+), 120 deletions(-)

Hi Barnabás,

Quoting Barnabás Pőcze (2026-11-09 10:49:40)
> Add a separate function whose job is to add the fallback values
> for the various configuration parameters that were not specified
> in the tuning file.
> 
> This allows some simplification of the code by making use of early
> returns, so do that as well. And additionally, pass only the
> relevant object to the "sub" parsers instead of the whole thing.
> 
> This is needed in order to be able to determine what exactly the tuning
> file contains, before adding the fallback values.
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

I accidentally replied to the old patch. So here again.

Looks good to me.
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> 

Best regards,
Stefan

> ---
>  src/ipa/libipa/agc.cpp | 245 +++++++++++++++++++++--------------------
>  1 file changed, 125 insertions(+), 120 deletions(-)
> 
> diff --git a/src/ipa/libipa/agc.cpp b/src/ipa/libipa/agc.cpp
> index 38bf90879d..6127859d0f 100644
> --- a/src/ipa/libipa/agc.cpp
> +++ b/src/ipa/libipa/agc.cpp
> @@ -237,13 +237,10 @@ namespace {
>   */
>  static constexpr double kDefaultRelativeLuminanceTarget = 0.16;
>  
> -int parseRelativeLuminanceTarget(AgcMeanLuminance::Configuration &config, const ValueNode &tuningData)
> +int parseRelativeLuminanceTarget(AgcMeanLuminance::Configuration &config, const ValueNode &target)
>  {
> -       auto &target = tuningData["relativeLuminanceTarget"];
> -       if (!target) {
> -               config.relativeLuminanceTarget = { { { { 0.0, kDefaultRelativeLuminanceTarget } } } };
> +       if (!target)
>                 return 0;
> -       }
>  
>         auto pwl = target.get<Pwl>();
>         if (!pwl) {
> @@ -293,7 +290,7 @@ int parseConstraint(AgcMeanLuminance::Configuration &config, const ValueNode &mo
>         return 0;
>  }
>  
> -int parseConstraintModes(AgcMeanLuminance::Configuration &config, 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 },
> @@ -302,67 +299,50 @@ int parseConstraintModes(AgcMeanLuminance::Configuration &config, const ValueNod
>                 { "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;
> -
> -                       auto it = controls::AeConstraintModeNameValueMap.find(modeName);
> -                       if (it == controls::AeConstraintModeNameValueMap.end()) {
> -                               for (const auto &[oldName, newName, v] : compat) {
> -                                       if (modeName != oldName)
> -                                               continue;
> -
> -                                       modeValue = v;
> -
> -                                       LOG(Agc, Warning)
> -                                               << "Constraint mode '" << oldName << "' has been renamed to '"
> -                                               << newName << "', please fix the tuning file";
> -                                       break;
> -                               }
> -                       } else {
> -                               modeValue = static_cast<controls::AeConstraintModeEnum>(it->second);
> -                       }
> +       if (!constraintModes.isDictionary())
> +               return 0;
>  
> -                       if (!modeValue) {
> -                               LOG(Agc, Warning)
> -                                       << "Skipping unknown constraint mode '" << modeName << "'";
> -                               continue;
> -                       }
> +       for (const auto &[modeName, modeDict] : constraintModes.asDict()) {
> +               std::optional<controls::AeConstraintModeEnum> modeValue;
> +
> +               auto it = controls::AeConstraintModeNameValueMap.find(modeName);
> +               if (it == controls::AeConstraintModeNameValueMap.end()) {
> +                       for (const auto &[oldName, newName, v] : compat) {
> +                               if (modeName != oldName)
> +                                       continue;
>  
> -                       if (!modeDict.isDictionary()) {
> -                               LOG(Agc, Error)
> -                                       << "Invalid constraint mode '" << modeName << "'";
> -                               return -EINVAL;
> +                               modeValue = v;
> +
> +                               LOG(Agc, Warning)
> +                                       << "Constraint mode '" << oldName << "' has been renamed to '"
> +                                       << newName << "', please fix the tuning file";
> +                               break;
>                         }
> +               } else {
> +                       modeValue = static_cast<controls::AeConstraintModeEnum>(it->second);
> +               }
>  
> -                       int ret = parseConstraint(config, modeDict, *modeValue);
> -                       if (ret)
> -                               return ret;
> +               if (!modeValue) {
> +                       LOG(Agc, Warning)
> +                               << "Skipping unknown constraint mode '" << modeName << "'";
> +                       continue;
>                 }
> -       }
>  
> -       /*
> -        * 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 (config.constraintModes.empty()) {
> -               AgcMeanLuminance::AgcConstraint constraint = {
> -                       AgcMeanLuminance::AgcConstraint::Bound::Lower,
> -                       0.98,
> -                       1.0,
> -                       Pwl({ { { 0.0, 0.5 } } }),
> -               };
> +               if (!modeDict.isDictionary()) {
> +                       LOG(Agc, Error)
> +                               << "Invalid constraint mode '" << modeName << "'";
> +                       return -EINVAL;
> +               }
>  
> -               config.constraintModes[controls::AeConstraintModeNormal].push_back(std::move(constraint));
> +               int ret = parseConstraint(config, modeDict, *modeValue);
> +               if (ret)
> +                       return ret;
>         }
>  
>         return 0;
>  }
>  
> -int parseExposureModes(AgcMeanLuminance::Configuration &config, 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 },
> @@ -371,79 +351,68 @@ int parseExposureModes(AgcMeanLuminance::Configuration &config, const ValueNode
>                 { "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;
> -
> -                       auto it = controls::AeExposureModeNameValueMap.find(modeName);
> -                       if (it == controls::AeExposureModeNameValueMap.end()) {
> -                               for (const auto &[oldName, newName, v] : compat) {
> -                                       if (modeName != oldName)
> -                                               continue;
> -
> -                                       modeValue = v;
> -
> -                                       LOG(Agc, Warning)
> -                                               << "Exposure mode '" << oldName << "' has been renamed to '"
> -                                               << newName << "', please fix the tuning file";
> -                                       break;
> -                               }
> -                       } else {
> -                               modeValue = static_cast<controls::AeExposureModeEnum>(it->second);
> -                       }
> +       if (!exposureModes.isDictionary())
> +               return 0;
> +
> +       for (const auto &[modeName, modeValues] : exposureModes.asDict()) {
> +               std::optional<controls::AeExposureModeEnum> modeValue;
> +
> +               auto it = controls::AeExposureModeNameValueMap.find(modeName);
> +               if (it == controls::AeExposureModeNameValueMap.end()) {
> +                       for (const auto &[oldName, newName, v] : compat) {
> +                               if (modeName != oldName)
> +                                       continue;
> +
> +                               modeValue = v;
>  
> -                       if (!modeValue) {
>                                 LOG(Agc, Warning)
> -                                       << "Skipping unknown exposure mode '" << modeName << "'";
> -                               continue;
> +                                       << "Exposure mode '" << oldName << "' has been renamed to '"
> +                                       << newName << "', please fix the tuning file";
> +                               break;
>                         }
> +               } else {
> +                       modeValue = static_cast<controls::AeExposureModeEnum>(it->second);
> +               }
>  
> -                       if (!modeValues.isDictionary()) {
> -                               LOG(Agc, Error)
> -                                       << "Invalid exposure mode '" << modeName << "'";
> -                               return -EINVAL;
> -                       }
> +               if (!modeValue) {
> +                       LOG(Agc, Warning)
> +                               << "Skipping unknown exposure mode '" << modeName << "'";
> +                       continue;
> +               }
>  
> -                       std::vector<uint32_t> exposureTimes =
> -                               modeValues["exposureTime"].get<std::vector<uint32_t>>().value_or(utils::defopt);
> -                       std::vector<double> gains =
> -                               modeValues["gain"].get<std::vector<double>>().value_or(utils::defopt);
> +               if (!modeValues.isDictionary()) {
> +                       LOG(Agc, Error)
> +                               << "Invalid exposure mode '" << modeName << "'";
> +                       return -EINVAL;
> +               }
>  
> -                       if (exposureTimes.size() != gains.size()) {
> -                               LOG(Agc, Error)
> -                                       << "Exposure time and gain array sizes unequal";
> -                               return -EINVAL;
> -                       }
> +               std::vector<uint32_t> exposureTimes =
> +                       modeValues["exposureTime"].get<std::vector<uint32_t>>().value_or(utils::defopt);
> +               std::vector<double> gains =
> +                       modeValues["gain"].get<std::vector<double>>().value_or(utils::defopt);
>  
> -                       if (exposureTimes.empty()) {
> -                               LOG(Agc, Error)
> -                                       << "Exposure time and gain arrays are empty";
> -                               return -EINVAL;
> -                       }
> +               if (exposureTimes.size() != gains.size()) {
> +                       LOG(Agc, Error)
> +                               << "Exposure time and gain array sizes unequal";
> +                       return -EINVAL;
> +               }
>  
> -                       std::vector<ExposureModeHelper::Stage> stages;
> -                       for (size_t i = 0; i < exposureTimes.size(); i++) {
> -                               stages.push_back({
> -                                       .exposureTime = std::chrono::microseconds(exposureTimes[i]),
> -                                       .gain = gains[i],
> -                               });
> -                       }
> +               if (exposureTimes.empty()) {
> +                       LOG(Agc, Error)
> +                               << "Exposure time and gain arrays are empty";
> +                       return -EINVAL;
> +               }
>  
> -                       config.exposureModeHelpers.try_emplace(*modeValue, std::move(stages));
> +               std::vector<ExposureModeHelper::Stage> stages;
> +               for (size_t i = 0; i < exposureTimes.size(); i++) {
> +                       stages.push_back({
> +                               .exposureTime = std::chrono::microseconds(exposureTimes[i]),
> +                               .gain = gains[i],
> +                       });
>                 }
> -       }
>  
> -       /*
> -        * 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 (config.exposureModeHelpers.empty())
> -               config.exposureModeHelpers.try_emplace(
> -                       controls::AeExposureModeNormal,
> -                       std::vector<ExposureModeHelper::Stage>{});
> +               config.exposureModeHelpers.try_emplace(*modeValue, std::move(stages));
> +       }
>  
>         return 0;
>  }
> @@ -452,15 +421,49 @@ int parseTuningData(AgcMeanLuminance::Configuration &config, const ValueNode &tu
>  {
>         int ret;
>  
> -       ret = parseRelativeLuminanceTarget(config, tuningData);
> +       ret = parseRelativeLuminanceTarget(config, tuningData["relativeLuminanceTarget"]);
>         if (ret)
>                 return ret;
>  
> -       ret = parseConstraintModes(config, tuningData);
> +       ret = parseConstraintModes(config, tuningData[controls::AeConstraintMode.name()]);
>         if (ret)
>                 return ret;
>  
> -       return parseExposureModes(config, tuningData);
> +       return parseExposureModes(config, tuningData[controls::AeExposureMode.name()]);
> +}
> +
> +void addFallbacks(AgcMeanLuminance::Configuration &config)
> +{
> +       if (config.relativeLuminanceTarget.empty())
> +               config.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 (config.constraintModes.empty()) {
> +               AgcMeanLuminance::AgcConstraint constraint = {
> +                       AgcMeanLuminance::AgcConstraint::Bound::Lower,
> +                       0.98,
> +                       1.0,
> +                       Pwl({ { { 0.0, 0.5 } } }),
> +               };
> +
> +               config.constraintModes[controls::AeConstraintModeNormal].push_back(std::move(constraint));
> +       }
> +
> +       /*
> +        * 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 (config.exposureModeHelpers.empty())
> +               config.exposureModeHelpers.try_emplace(
> +                       controls::AeExposureModeNormal,
> +                       std::vector<ExposureModeHelper::Stage>{});
>  }
>  
>  } /* namespace */
> @@ -608,6 +611,8 @@ int AgcAlgorithm::init(const ValueNode &tuningData, CameraSensorHelper *sensor,
>                 if (ret)
>                         return ret;
>  
> +               addFallbacks(agcConfig);
> +
>                 impl_.emplace<AgcMeanLuminance>(std::move(agcConfig));
>         } else {
>                 impl_.emplace<AgcMSV>();
> -- 
> 2.56.0
>