[PATCH v2 6/9] ipa: rkisp1: Move loader functions into helper class

[Kieran Bingham]

Quoting Stefan Klug (2024-09-13 08:57:24)
> In preparation to the polynomial LSC data, move the current loader into
> it's own helper class.
> 
> Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> ---
>  src/ipa/rkisp1/algorithms/lsc.cpp | 125 ++++++++++++++++++------------
>  1 file changed, 76 insertions(+), 49 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp
> index 87a04ec048f8..12867b8f7d0f 100644
> --- a/src/ipa/rkisp1/algorithms/lsc.cpp
> +++ b/src/ipa/rkisp1/algorithms/lsc.cpp
> @@ -70,6 +70,70 @@ namespace ipa::rkisp1::algorithms {
>  
>  LOG_DEFINE_CATEGORY(RkISP1Lsc)
>  
> +class LscClassicLoader
> +{
> +public:
> +       int parseLscData(const YamlObject &yamlSets,
> +                        std::map<unsigned int, LensShadingCorrection::Components> &lscData)
> +       {
> +               const auto &sets = yamlSets.asList();
> +
> +               for (const auto &yamlSet : sets) {
> +                       uint32_t ct = yamlSet["ct"].get<uint32_t>(0);
> +
> +                       if (lscData.count(ct)) {
> +                               LOG(RkISP1Lsc, Error)
> +                                       << "Multiple sets found for color temperature "
> +                                       << ct;
> +                               return -EINVAL;
> +                       }
> +
> +                       LensShadingCorrection::Components &set = lscData[ct];
> +
> +                       set.ct = ct;
> +                       set.r = parseTable(yamlSet, "r");
> +                       set.gr = parseTable(yamlSet, "gr");
> +                       set.gb = parseTable(yamlSet, "gb");
> +                       set.b = parseTable(yamlSet, "b");
> +
> +                       if (set.r.empty() || set.gr.empty() ||
> +                           set.gb.empty() || set.b.empty()) {
> +                               LOG(RkISP1Lsc, Error)
> +                                       << "Set for color temperature " << ct
> +                                       << " is missing tables";
> +                               return -EINVAL;
> +                       }
> +               }
> +
> +               if (lscData.empty()) {
> +                       LOG(RkISP1Lsc, Error) << "Failed to load any sets";
> +                       return -EINVAL;
> +               }
> +
> +               return 0;
> +       }
> +
> +private:
> +       std::vector<uint16_t> parseTable(const YamlObject &tuningData,
> +                                        const char *prop)
> +       {
> +               static constexpr unsigned int kLscNumSamples =
> +                       RKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;
> +
> +               std::vector<uint16_t> table =
> +                       tuningData[prop].getList<uint16_t>().value_or(std::vector<uint16_t>{});
> +               if (table.size() != kLscNumSamples) {
> +                       LOG(RkISP1Lsc, Error)
> +                               << "Invalid '" << prop << "' values: expected "
> +                               << kLscNumSamples
> +                               << " elements, got " << table.size();
> +                       return {};

Aha - I think this is where the input validation happens?

This enforces all the tables are identically sized and we'd never hit
the assertion right?

I think that earns the tag for the previous patch ...



> +               }
> +
> +               return table;
> +       }
> +};
> +
>  static std::vector<double> parseSizes(const YamlObject &tuningData,
>                                       const char *prop)
>  {
> @@ -100,25 +164,6 @@ static std::vector<double> parseSizes(const YamlObject &tuningData,
>         return sizes;
>  }
>  
> -static std::vector<uint16_t> parseTable(const YamlObject &tuningData,
> -                                       const char *prop)
> -{
> -       static constexpr unsigned int kLscNumSamples =
> -               RKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX;
> -
> -       std::vector<uint16_t> table =
> -               tuningData[prop].getList<uint16_t>().value_or(std::vector<uint16_t>{});
> -       if (table.size() != kLscNumSamples) {
> -               LOG(RkISP1Lsc, Error)
> -                       << "Invalid '" << prop << "' values: expected "
> -                       << kLscNumSamples
> -                       << " elements, got " << table.size();
> -               return {};
> -       }
> -
> -       return table;
> -}
> -
>  LensShadingCorrection::LensShadingCorrection()
>         : lastAppliedCt_(0), lastAppliedQuantizedCt_(0)
>  {
> @@ -145,39 +190,21 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context,
>                 return -EINVAL;
>         }
>  
> -       const auto &sets = yamlSets.asList();
>         std::map<unsigned int, Components> lscData;
> -       for (const auto &yamlSet : sets) {
> -               uint32_t ct = yamlSet["ct"].get<uint32_t>(0);
> -
> -               if (lscData.count(ct)) {
> -                       LOG(RkISP1Lsc, Error)
> -                               << "Multiple sets found for color temperature "
> -                               << ct;
> -                       return -EINVAL;
> -               }
> -
> -               Components &set = lscData[ct];
> -
> -               set.ct = ct;
> -               set.r = parseTable(yamlSet, "r");
> -               set.gr = parseTable(yamlSet, "gr");
> -               set.gb = parseTable(yamlSet, "gb");
> -               set.b = parseTable(yamlSet, "b");
> -
> -               if (set.r.empty() || set.gr.empty() ||
> -                   set.gb.empty() || set.b.empty()) {
> -                       LOG(RkISP1Lsc, Error)
> -                               << "Set for color temperature " << ct
> -                               << " is missing tables";
> -                       return -EINVAL;
> -               }
> +       int res = 0;
> +       std::optional<std::string> type = tuningData["type"].get<std::string>();
> +       if (!type.has_value()) {
> +               LOG(RkISP1Lsc, Warning) << "LSC data is in classic format. "

I wonder if we'd call this the 'table' format rather than classic ? But
I don't think it matters.

I could imagine some crazy use case might still want the full control of
the table ... so it might not ... get deprecated.

That makes me ponder if we should just call them 'table' and
'polynomial'.

But either way, for me this loader is a good way to abstract and move
forwards.


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

> +                                       << "This will be deprecated soon.";
> +               auto loader = LscClassicLoader();
> +               res = loader.parseLscData(yamlSets, lscData);
> +       } else {
> +               LOG(RkISP1Lsc, Error) << "Unsupported LSC type '" << *type << "'";
> +               res = -EINVAL;
>         }
>  
> -       if (lscData.empty()) {
> -               LOG(RkISP1Lsc, Error) << "Failed to load any sets";
> -               return -EINVAL;
> -       }
> +       if (res)
> +               return res;
>  
>         sets_.setData(std::move(lscData));
>  
> -- 
> 2.43.0
>