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

[Paul Elder]

On Fri, Sep 13, 2024 at 10:55:10AM +0100, Kieran Bingham wrote:
> 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'.

+1 if it doesn't get deprecated yeah.

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

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

> 
> 
> 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
> >