| Message ID | 20260720-libipa-algorithms-v6-23-ececb73f97cb@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: > The current definition of lsc::Components assumes 4 gain vectors named > after the colour components supported by the RkISP1 ISP (r, gr, gb and > b). > > As different ISP support different colour channels for their Lsc tables > make lsc::Components a map that associates the name of the colour > channel to the vector of gains. > > The use of strings as index for the map comes from the requirement of > parsing the tuning file using string identifiers. Pass the strings to > use for tuning file parsing to the LscAlgorithm::init() function as part > of a newly introduced LscDescriptor type and use the same strings for > indexing the gain arrays. > > The newly introduced LscDescriptor type also provides to the > LscAlgorithm class the expected size of the Lsc grid, allowing to remove > RkISP1 specific values from the libipa generic implementation. > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > v5->v6: > - Rename LscDescriptor::num*Cells to num*Samples > --- > src/ipa/libipa/lsc.cpp | 10 +++--- > src/ipa/libipa/lsc.h | 4 +-- > src/ipa/libipa/lsc_base.cpp | 49 ++++++++++++++++++++-------- > src/ipa/libipa/lsc_base.h | 18 ++++++----- > src/ipa/libipa/lsc_polynomial.cpp | 65 +++++++++++++++++++------------------ > src/ipa/libipa/lsc_polynomial.h | 10 ++---- > src/ipa/libipa/lsc_table.cpp | 68 +++++++++++++++++++++++---------------- > src/ipa/libipa/lsc_table.h | 9 ++++-- > src/ipa/rkisp1/algorithms/lsc.cpp | 20 ++++++++---- > 9 files changed, 150 insertions(+), 103 deletions(-) > > [...] > diff --git a/src/ipa/libipa/lsc_base.cpp b/src/ipa/libipa/lsc_base.cpp > index b3671376a12a..01616d1c2e2a 100644 > --- a/src/ipa/libipa/lsc_base.cpp > +++ b/src/ipa/libipa/lsc_base.cpp > @@ -19,23 +19,20 @@ namespace ipa { > namespace lsc { > > /** > - * \struct Components > + * \typedef Components > * \brief Associate colour components with a list of gains > * > * LSC tables are defined as a list of gain values associated to a colour > * component. > * > - * \var Components::r > - * \brief The list of gains for the Red colour component > + * As different ISP support different colour components (usually 'r', 'gr', > + * 'gb', 'b' or just 'r', 'g', 'b') this class associates a string > + * identifier for the colour component to a list of gains. > * > - * \var Components::gr > - * \brief The list of gains for the Green/Red colour component > + * Each key name shall match an entry in the tuning file. > * > - * \var Components::gb > - * \brief The list of gains for the Green/Blue colour component > - * > - * \var Components::b > - * \brief The list of gains for the Blue colour component > + * The list of keys is provided to the LscAlgorithm class using \a > + * LscDescriptor::keys. > */ > > /** > @@ -57,13 +54,32 @@ void Interpolator<lsc::Components>:: > lsc::Components &dest, > double lambda) > { > - interpolateVector(a.r, b.r, dest.r, lambda); > - interpolateVector(a.gr, b.gr, dest.gr, lambda); > - interpolateVector(a.gb, b.gb, dest.gb, lambda); > - interpolateVector(a.b, b.b, dest.b, lambda); > + for (auto const &[k, v] : a) > + interpolateVector(v, b.at(k), dest[k], lambda); > } > #endif > > +/** > + * \struct LscDescriptor > + * \brief Describe the ISP LSC engine > + * > + * \var LscDescriptor::keys > + * \brief The list of colour components to which a list of gains is associated > + * with in the tuning file. Used for parsing the tuning file > + * > + * \var LscDescriptor::numHSamples > + * \brief Number of horizontal gain samples of the ISP LSC grid. Used for > + * validating the list of gains parsed from tuning file > + * > + * \var LscDescriptor::numVSamples > + * \brief Number of vertical gain samples of the ISP LSC grid. Used for > + * validating the list of gains parsed from tuning file > + * > + * \var LscDescriptor::sensorSize > + * \brief The physical sensor size. This is the largest frame size used to > + * generate the LSC table. Only used by the polynomial LSC algorithm > + */ > + > /** > * \class LscImplementation > * \brief Pure virtual base class for LSC algorithm implementations > @@ -80,6 +96,11 @@ void Interpolator<lsc::Components>:: > * \fn LscImplementation::parseLscData > * \brief Parse \a tuningData > * \param[in] tuningData The tuning data > + * \param[in] descriptor The LSC engine descriptor > + * > + * Parse the tuning file using the \a descriptor to identify the colour > + * components in the tuning data and validate the size of the loaded gains > + * tables. > * > * \return 0 on success, a negative error number otherwise > */ > diff --git a/src/ipa/libipa/lsc_base.h b/src/ipa/libipa/lsc_base.h > index 1bdb7d5c8ed9..b0b9b3b0712d 100644 > --- a/src/ipa/libipa/lsc_base.h > +++ b/src/ipa/libipa/lsc_base.h > @@ -26,13 +26,7 @@ namespace ipa { > > namespace lsc { > > -struct Components { > - std::vector<uint16_t> r; > - std::vector<uint16_t> gr; > - std::vector<uint16_t> gb; > - std::vector<uint16_t> b; > -}; > - > +using Components = std::map<std::string, std::vector<uint16_t>>; I suggest using Components = std::map<std::string, std::vector<uint16_t>, std::less<>> because that allows some map operations (e.g. find()) without having to construct a value of the key type (std::string). > using ComponentsMap = std::map<unsigned int, Components>; > > } /* namespace lsc */ > @@ -56,12 +50,20 @@ void Interpolator<lsc::Components>:: > double lambda); > #endif /* __DOXYGEN__ */ > > +struct LscDescriptor { > + std::vector<std::string> keys; > + unsigned int numHSamples; > + unsigned int numVSamples; > + Size sensorSize; > +}; > + > class LscImplementation > { > public: > virtual ~LscImplementation() {} > > - virtual int parseLscData(const ValueNode &tuningData) = 0; > + virtual int parseLscData(const ValueNode &tuningData, > + const LscDescriptor &descriptor) = 0; > > virtual lsc::ComponentsMap > sampleForCrop(const Rectangle &cropRectangle, > diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp > index 682e2f14fe09..e17ba85448be 100644 > --- a/src/ipa/libipa/lsc_polynomial.cpp > +++ b/src/ipa/libipa/lsc_polynomial.cpp > @@ -133,44 +133,43 @@ void Polynomial::setReferenceImageSize(const Size &size) > /** > * \brief Parse polynomial LSC data > * \param[in] sets The tuning file content > + * \param[in] descriptor The LSC engine descriptor > * > * Parse the LSC data in polyomial form from the \a sets tuning data. > * > * \return 0 on success or a negative error number otherwise > */ > -int LscPolynomial::parseLscData(const ValueNode &sets) > +int LscPolynomial::parseLscData(const ValueNode &sets, > + const LscDescriptor &descriptor) > { > for (const auto &set : sets.asList()) { > - std::optional<lsc::Polynomial> pr, pgr, pgb, pb; > uint32_t ct = set["ct"].get<uint32_t>(0); > > - if (lscData_.count(ct)) { > - LOG(LscPolynomial, Error) > - << "Multiple sets found for " > - << "color temperature " << ct; > - return -EINVAL; > + PolynomialComponents components; > + for (auto &k : descriptor.keys) { > + auto polynomial = set[k.c_str()].get<lsc::Polynomial>(); I would drop the `.c_str()`, it shouldn't be needed. > + if (!polynomial) { > + LOG(LscPolynomial, Error) > + << "Missing polynomial for component " > + << k; > + return -EINVAL; > + } > + > + auto [it, inserted] = > + components.emplace(std::piecewise_construct, > + std::forward_as_tuple(k.c_str()), > + std::forward_as_tuple(*polynomial)); I would suggest using `try_emplace` over `emplace()` in essentially all situations: auto [it, inserted] = components.try_emplace(k, std::move(*polynomial)); and even just an `ASSERT(inserted)` could be useful to notice if the same key has been specified multiple times by accident. > + > + it->second.setReferenceImageSize(descriptor.sensorSize); > } > > - pr = set["r"].get<lsc::Polynomial>(); > - pgr = set["gr"].get<lsc::Polynomial>(); > - pgb = set["gb"].get<lsc::Polynomial>(); > - pb = set["b"].get<lsc::Polynomial>(); > - > - if (!(pr || pgr || pgb || pb)) { > + auto [it, inserted] = lscData_.emplace(ct, components); Same here. And `std::move(components)`. > + if (!inserted) { > LOG(LscPolynomial, Error) > - << "Failed to parse polynomial for " > - << "colour temperature " << ct; > + << "Multiple sets found for " > + << "color temperature " << ct; > return -EINVAL; > } > - > - pr->setReferenceImageSize(sensorSize_); > - pgr->setReferenceImageSize(sensorSize_); > - pgb->setReferenceImageSize(sensorSize_); > - pb->setReferenceImageSize(sensorSize_); > - > - lscData_.emplace(std::piecewise_construct, > - std::forward_as_tuple(ct), > - std::forward_as_tuple(PolynomialComponents{ *pr, *pgr, *pgb, *pb })); > } > > if (lscData_.empty()) { > @@ -210,13 +209,17 @@ LscPolynomial::sampleForCrop(const Rectangle &cropRectangle, > > lsc::ComponentsMap components; > > - for (const auto &[k, p] : lscData_) { > - components[k] = { > - samplePolynomial(p.pr, xPos, yPos, cropRectangle), > - samplePolynomial(p.pgr, xPos, yPos, cropRectangle), > - samplePolynomial(p.pgb, xPos, yPos, cropRectangle), > - samplePolynomial(p.pb, xPos, yPos, cropRectangle) > - }; > + for (const auto &[t, c] : lscData_) { > + lsc::Components comp; > + > + for (const auto &[k, p] : c) { > + comp.emplace(std::piecewise_construct, > + std::forward_as_tuple(k), > + std::forward_as_tuple(samplePolynomial(p, xPos, yPos, > + cropRectangle))); Same here wrt. `try_emplace()`. > + } > + > + components[t] = comp; > } > > return components; > diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h > index d2276ecc6f6b..2a26db499d68 100644 > --- a/src/ipa/libipa/lsc_polynomial.h > +++ b/src/ipa/libipa/lsc_polynomial.h > @@ -53,12 +53,7 @@ private: > class LscPolynomial : public LscImplementation > { > private: > - struct PolynomialComponents { > - lsc::Polynomial pr; > - lsc::Polynomial pgr; > - lsc::Polynomial pgb; > - lsc::Polynomial pb; > - }; > + using PolynomialComponents = std::map<std::string, lsc::Polynomial>; Same here wrt. `std::less<>`. > using PolynomialComponentsMap = std::map<unsigned int, PolynomialComponents>; > > public: > @@ -67,7 +62,8 @@ public: > { > } > > - int parseLscData(const ValueNode &sets) override; > + int parseLscData(const ValueNode &sets, > + const LscDescriptor &descriptor) override; > > lsc::ComponentsMap > sampleForCrop(const Rectangle &cropRectangle, > diff --git a/src/ipa/libipa/lsc_table.cpp b/src/ipa/libipa/lsc_table.cpp > index 47dff156963a..92e24bc5160a 100644 > --- a/src/ipa/libipa/lsc_table.cpp > +++ b/src/ipa/libipa/lsc_table.cpp > @@ -7,9 +7,6 @@ > > #include "lsc_table.h" > > -/* \todo Remove RkISP1 from libipa. */ > -#include "linux/rkisp1-config.h" > - > namespace libcamera { > > LOG_DEFINE_CATEGORY(LscTable) > @@ -29,42 +26,56 @@ namespace ipa { > /** > * \brief Parse tabular LSC data > * \param[in] sets The tuning file content > + * \param[in] descriptor The LSC engine descriptor > * > * Parse the LSC data in tabular form from the \a sets tuning data. > * > * \return 0 on success or a negative error number otherwise > */ > -int LscTable::parseLscData(const ValueNode &sets) > +int LscTable::parseLscData(const ValueNode &sets, > + const LscDescriptor &descriptor) > { > for (const auto &set : sets.asList()) { > uint32_t ct = set["ct"].get<uint32_t>(0); > > - if (lscData_.count(ct)) { > - LOG(LscTable, Error) > - << "Multiple sets found for color temperature " > - << ct; > - return -EINVAL; > - } > + int ret = parseLscComponent(set, ct, descriptor); > + if (ret) > + return ret; > + } > > - lsc::Components components; > - components.r = parseTable(set, "r"); > - components.gr = parseTable(set, "gr"); > - components.gb = parseTable(set, "gb"); > - components.b = parseTable(set, "b"); > + if (lscData_.empty()) { > + LOG(LscTable, Error) << "Failed to load any sets"; > + return -EINVAL; > + } > > - if (components.r.empty() || components.gr.empty() || > - components.gb.empty() || components.b.empty()) { > + return 0; > +} > + > +int LscTable::parseLscComponent(const ValueNode &yamlSet, > + unsigned int ct, const LscDescriptor &descriptor) > +{ > + lsc::Components component; > + for (auto &k : descriptor.keys) { > + auto [it, inserted] = component.emplace( > + std::piecewise_construct, > + std::forward_as_tuple(k.c_str()), > + std::forward_as_tuple(parseTable(yamlSet, > + k.c_str(), > + descriptor.numHSamples, > + descriptor.numVSamples))); Same here wrt. `try_emplace()` and dropping `.c_str()`. > + if (!inserted || it->second.empty()) { > LOG(LscTable, Error) > - << "Set for color temperature " << ct > - << " is missing tables"; > + << "Set " << k << " for color temperature " > + << ct << " is missing"; `!inserted` means that `k` is already in `component`, meaning `descriptor.keys` contains duplicates. I'm not sure the error message is appropriate for that case. > return -EINVAL; > } > - > - lscData_.emplace(ct, std::move(components)); > } > > - if (lscData_.empty()) { > - LOG(LscTable, Error) << "Failed to load any sets"; > + auto [it, inserted] = lscData_.emplace(ct, component); std::move(component) > + if (!inserted) { > + LOG(LscTable, Error) > + << "Multiple sets found for color temperature " > + << ct; > return -EINVAL; > } > > @@ -72,17 +83,18 @@ int LscTable::parseLscData(const ValueNode &sets) > } > > std::vector<uint16_t> LscTable::parseTable(const ValueNode &tuningData, > - const char *prop) > + const char *prop, > + unsigned int numHSamples, > + unsigned int numVSamples) > { > - static constexpr unsigned int kLscNumSamples = > - RKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX; > + unsigned int lscNumSamples = numHSamples * numVSamples; > > std::vector<uint16_t> table = > tuningData[prop].get<std::vector<uint16_t>>().value_or(utils::defopt); > - if (table.size() != kLscNumSamples) { > + if (table.size() != lscNumSamples) { > LOG(LscTable, Error) > << "Invalid '" << prop << "' values: expected " > - << kLscNumSamples > + << lscNumSamples > << " elements, got " << table.size(); > return {}; > } > [...]
Hi Barnabás On Fri, Jul 24, 2026 at 10:43:15AM +0200, Barnabás Pőcze wrote: > 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: > > The current definition of lsc::Components assumes 4 gain vectors named > > after the colour components supported by the RkISP1 ISP (r, gr, gb and > > b). > > > > As different ISP support different colour channels for their Lsc tables > > make lsc::Components a map that associates the name of the colour > > channel to the vector of gains. > > > > The use of strings as index for the map comes from the requirement of > > parsing the tuning file using string identifiers. Pass the strings to > > use for tuning file parsing to the LscAlgorithm::init() function as part > > of a newly introduced LscDescriptor type and use the same strings for > > indexing the gain arrays. > > > > The newly introduced LscDescriptor type also provides to the > > LscAlgorithm class the expected size of the Lsc grid, allowing to remove > > RkISP1 specific values from the libipa generic implementation. > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > v5->v6: > > - Rename LscDescriptor::num*Cells to num*Samples > > --- > > src/ipa/libipa/lsc.cpp | 10 +++--- > > src/ipa/libipa/lsc.h | 4 +-- > > src/ipa/libipa/lsc_base.cpp | 49 ++++++++++++++++++++-------- > > src/ipa/libipa/lsc_base.h | 18 ++++++----- > > src/ipa/libipa/lsc_polynomial.cpp | 65 +++++++++++++++++++------------------ > > src/ipa/libipa/lsc_polynomial.h | 10 ++---- > > src/ipa/libipa/lsc_table.cpp | 68 +++++++++++++++++++++++---------------- > > src/ipa/libipa/lsc_table.h | 9 ++++-- > > src/ipa/rkisp1/algorithms/lsc.cpp | 20 ++++++++---- > > 9 files changed, 150 insertions(+), 103 deletions(-) > > > > [...] > > diff --git a/src/ipa/libipa/lsc_base.cpp b/src/ipa/libipa/lsc_base.cpp > > index b3671376a12a..01616d1c2e2a 100644 > > --- a/src/ipa/libipa/lsc_base.cpp > > +++ b/src/ipa/libipa/lsc_base.cpp > > @@ -19,23 +19,20 @@ namespace ipa { > > namespace lsc { > > /** > > - * \struct Components > > + * \typedef Components > > * \brief Associate colour components with a list of gains > > * > > * LSC tables are defined as a list of gain values associated to a colour > > * component. > > * > > - * \var Components::r > > - * \brief The list of gains for the Red colour component > > + * As different ISP support different colour components (usually 'r', 'gr', > > + * 'gb', 'b' or just 'r', 'g', 'b') this class associates a string > > + * identifier for the colour component to a list of gains. > > * > > - * \var Components::gr > > - * \brief The list of gains for the Green/Red colour component > > + * Each key name shall match an entry in the tuning file. > > * > > - * \var Components::gb > > - * \brief The list of gains for the Green/Blue colour component > > - * > > - * \var Components::b > > - * \brief The list of gains for the Blue colour component > > + * The list of keys is provided to the LscAlgorithm class using \a > > + * LscDescriptor::keys. > > */ > > /** > > @@ -57,13 +54,32 @@ void Interpolator<lsc::Components>:: > > lsc::Components &dest, > > double lambda) > > { > > - interpolateVector(a.r, b.r, dest.r, lambda); > > - interpolateVector(a.gr, b.gr, dest.gr, lambda); > > - interpolateVector(a.gb, b.gb, dest.gb, lambda); > > - interpolateVector(a.b, b.b, dest.b, lambda); > > + for (auto const &[k, v] : a) > > + interpolateVector(v, b.at(k), dest[k], lambda); > > } > > #endif > > +/** > > + * \struct LscDescriptor > > + * \brief Describe the ISP LSC engine > > + * > > + * \var LscDescriptor::keys > > + * \brief The list of colour components to which a list of gains is associated > > + * with in the tuning file. Used for parsing the tuning file > > + * > > + * \var LscDescriptor::numHSamples > > + * \brief Number of horizontal gain samples of the ISP LSC grid. Used for > > + * validating the list of gains parsed from tuning file > > + * > > + * \var LscDescriptor::numVSamples > > + * \brief Number of vertical gain samples of the ISP LSC grid. Used for > > + * validating the list of gains parsed from tuning file > > + * > > + * \var LscDescriptor::sensorSize > > + * \brief The physical sensor size. This is the largest frame size used to > > + * generate the LSC table. Only used by the polynomial LSC algorithm > > + */ > > + > > /** > > * \class LscImplementation > > * \brief Pure virtual base class for LSC algorithm implementations > > @@ -80,6 +96,11 @@ void Interpolator<lsc::Components>:: > > * \fn LscImplementation::parseLscData > > * \brief Parse \a tuningData > > * \param[in] tuningData The tuning data > > + * \param[in] descriptor The LSC engine descriptor > > + * > > + * Parse the tuning file using the \a descriptor to identify the colour > > + * components in the tuning data and validate the size of the loaded gains > > + * tables. > > * > > * \return 0 on success, a negative error number otherwise > > */ > > diff --git a/src/ipa/libipa/lsc_base.h b/src/ipa/libipa/lsc_base.h > > index 1bdb7d5c8ed9..b0b9b3b0712d 100644 > > --- a/src/ipa/libipa/lsc_base.h > > +++ b/src/ipa/libipa/lsc_base.h > > @@ -26,13 +26,7 @@ namespace ipa { > > namespace lsc { > > -struct Components { > > - std::vector<uint16_t> r; > > - std::vector<uint16_t> gr; > > - std::vector<uint16_t> gb; > > - std::vector<uint16_t> b; > > -}; > > - > > +using Components = std::map<std::string, std::vector<uint16_t>>; > > I suggest > > using Components = std::map<std::string, std::vector<uint16_t>, std::less<>> > > because that allows some map operations (e.g. find()) without having to > construct a value of the key type (std::string). Isn't std::less the default parameter argument for std::map already ? template< class Key, class T, class Compare = std::less<Key>, class Allocator = std::allocator<std::pair<const Key, T>> > class map; https://en.cppreference.com/cpp/container/map ? > > > > using ComponentsMap = std::map<unsigned int, Components>; > > } /* namespace lsc */ > > @@ -56,12 +50,20 @@ void Interpolator<lsc::Components>:: > > double lambda); > > #endif /* __DOXYGEN__ */ > > +struct LscDescriptor { > > + std::vector<std::string> keys; > > + unsigned int numHSamples; > > + unsigned int numVSamples; > > + Size sensorSize; > > +}; > > + > > class LscImplementation > > { > > public: > > virtual ~LscImplementation() {} > > - virtual int parseLscData(const ValueNode &tuningData) = 0; > > + virtual int parseLscData(const ValueNode &tuningData, > > + const LscDescriptor &descriptor) = 0; > > virtual lsc::ComponentsMap > > sampleForCrop(const Rectangle &cropRectangle, > > diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp > > index 682e2f14fe09..e17ba85448be 100644 > > --- a/src/ipa/libipa/lsc_polynomial.cpp > > +++ b/src/ipa/libipa/lsc_polynomial.cpp > > @@ -133,44 +133,43 @@ void Polynomial::setReferenceImageSize(const Size &size) > > /** > > * \brief Parse polynomial LSC data > > * \param[in] sets The tuning file content > > + * \param[in] descriptor The LSC engine descriptor > > * > > * Parse the LSC data in polyomial form from the \a sets tuning data. > > * > > * \return 0 on success or a negative error number otherwise > > */ > > -int LscPolynomial::parseLscData(const ValueNode &sets) > > +int LscPolynomial::parseLscData(const ValueNode &sets, > > + const LscDescriptor &descriptor) > > { > > for (const auto &set : sets.asList()) { > > - std::optional<lsc::Polynomial> pr, pgr, pgb, pb; > > uint32_t ct = set["ct"].get<uint32_t>(0); > > - if (lscData_.count(ct)) { > > - LOG(LscPolynomial, Error) > > - << "Multiple sets found for " > > - << "color temperature " << ct; > > - return -EINVAL; > > + PolynomialComponents components; > > + for (auto &k : descriptor.keys) { > > + auto polynomial = set[k.c_str()].get<lsc::Polynomial>(); > > I would drop the `.c_str()`, it shouldn't be needed. > > > > + if (!polynomial) { > > + LOG(LscPolynomial, Error) > > + << "Missing polynomial for component " > > + << k; > > + return -EINVAL; > > + } > > + > > + auto [it, inserted] = > > + components.emplace(std::piecewise_construct, > > + std::forward_as_tuple(k.c_str()), > > + std::forward_as_tuple(*polynomial)); > > I would suggest using `try_emplace` over `emplace()` in essentially all situations: > > auto [it, inserted] = components.try_emplace(k, std::move(*polynomial)); > > and even just an `ASSERT(inserted)` could be useful to notice if the same key > has been specified multiple times by accident. > > > + > > + it->second.setReferenceImageSize(descriptor.sensorSize); > > } > > - pr = set["r"].get<lsc::Polynomial>(); > > - pgr = set["gr"].get<lsc::Polynomial>(); > > - pgb = set["gb"].get<lsc::Polynomial>(); > > - pb = set["b"].get<lsc::Polynomial>(); > > - > > - if (!(pr || pgr || pgb || pb)) { > > + auto [it, inserted] = lscData_.emplace(ct, components); > > Same here. And `std::move(components)`. > > > > + if (!inserted) { > > LOG(LscPolynomial, Error) > > - << "Failed to parse polynomial for " > > - << "colour temperature " << ct; > > + << "Multiple sets found for " > > + << "color temperature " << ct; > > return -EINVAL; > > } > > - > > - pr->setReferenceImageSize(sensorSize_); > > - pgr->setReferenceImageSize(sensorSize_); > > - pgb->setReferenceImageSize(sensorSize_); > > - pb->setReferenceImageSize(sensorSize_); > > - > > - lscData_.emplace(std::piecewise_construct, > > - std::forward_as_tuple(ct), > > - std::forward_as_tuple(PolynomialComponents{ *pr, *pgr, *pgb, *pb })); > > } > > if (lscData_.empty()) { > > @@ -210,13 +209,17 @@ LscPolynomial::sampleForCrop(const Rectangle &cropRectangle, > > lsc::ComponentsMap components; > > - for (const auto &[k, p] : lscData_) { > > - components[k] = { > > - samplePolynomial(p.pr, xPos, yPos, cropRectangle), > > - samplePolynomial(p.pgr, xPos, yPos, cropRectangle), > > - samplePolynomial(p.pgb, xPos, yPos, cropRectangle), > > - samplePolynomial(p.pb, xPos, yPos, cropRectangle) > > - }; > > + for (const auto &[t, c] : lscData_) { > > + lsc::Components comp; > > + > > + for (const auto &[k, p] : c) { > > + comp.emplace(std::piecewise_construct, > > + std::forward_as_tuple(k), > > + std::forward_as_tuple(samplePolynomial(p, xPos, yPos, > > + cropRectangle))); > > Same here wrt. `try_emplace()`. > > > > + } > > + > > + components[t] = comp; > > } > > return components; > > diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h > > index d2276ecc6f6b..2a26db499d68 100644 > > --- a/src/ipa/libipa/lsc_polynomial.h > > +++ b/src/ipa/libipa/lsc_polynomial.h > > @@ -53,12 +53,7 @@ private: > > class LscPolynomial : public LscImplementation > > { > > private: > > - struct PolynomialComponents { > > - lsc::Polynomial pr; > > - lsc::Polynomial pgr; > > - lsc::Polynomial pgb; > > - lsc::Polynomial pb; > > - }; > > + using PolynomialComponents = std::map<std::string, lsc::Polynomial>; > > Same here wrt. `std::less<>`. > > > > using PolynomialComponentsMap = std::map<unsigned int, PolynomialComponents>; > > public: > > @@ -67,7 +62,8 @@ public: > > { > > } > > - int parseLscData(const ValueNode &sets) override; > > + int parseLscData(const ValueNode &sets, > > + const LscDescriptor &descriptor) override; > > lsc::ComponentsMap > > sampleForCrop(const Rectangle &cropRectangle, > > diff --git a/src/ipa/libipa/lsc_table.cpp b/src/ipa/libipa/lsc_table.cpp > > index 47dff156963a..92e24bc5160a 100644 > > --- a/src/ipa/libipa/lsc_table.cpp > > +++ b/src/ipa/libipa/lsc_table.cpp > > @@ -7,9 +7,6 @@ > > #include "lsc_table.h" > > -/* \todo Remove RkISP1 from libipa. */ > > -#include "linux/rkisp1-config.h" > > - > > namespace libcamera { > > LOG_DEFINE_CATEGORY(LscTable) > > @@ -29,42 +26,56 @@ namespace ipa { > > /** > > * \brief Parse tabular LSC data > > * \param[in] sets The tuning file content > > + * \param[in] descriptor The LSC engine descriptor > > * > > * Parse the LSC data in tabular form from the \a sets tuning data. > > * > > * \return 0 on success or a negative error number otherwise > > */ > > -int LscTable::parseLscData(const ValueNode &sets) > > +int LscTable::parseLscData(const ValueNode &sets, > > + const LscDescriptor &descriptor) > > { > > for (const auto &set : sets.asList()) { > > uint32_t ct = set["ct"].get<uint32_t>(0); > > - if (lscData_.count(ct)) { > > - LOG(LscTable, Error) > > - << "Multiple sets found for color temperature " > > - << ct; > > - return -EINVAL; > > - } > > + int ret = parseLscComponent(set, ct, descriptor); > > + if (ret) > > + return ret; > > + } > > - lsc::Components components; > > - components.r = parseTable(set, "r"); > > - components.gr = parseTable(set, "gr"); > > - components.gb = parseTable(set, "gb"); > > - components.b = parseTable(set, "b"); > > + if (lscData_.empty()) { > > + LOG(LscTable, Error) << "Failed to load any sets"; > > + return -EINVAL; > > + } > > - if (components.r.empty() || components.gr.empty() || > > - components.gb.empty() || components.b.empty()) { > > + return 0; > > +} > > + > > +int LscTable::parseLscComponent(const ValueNode &yamlSet, > > + unsigned int ct, const LscDescriptor &descriptor) > > +{ > > + lsc::Components component; > > + for (auto &k : descriptor.keys) { > > + auto [it, inserted] = component.emplace( > > + std::piecewise_construct, > > + std::forward_as_tuple(k.c_str()), > > + std::forward_as_tuple(parseTable(yamlSet, > > + k.c_str(), > > + descriptor.numHSamples, > > + descriptor.numVSamples))); > > Same here wrt. `try_emplace()` and dropping `.c_str()`. > > > > + if (!inserted || it->second.empty()) { > > LOG(LscTable, Error) > > - << "Set for color temperature " << ct > > - << " is missing tables"; > > + << "Set " << k << " for color temperature " > > + << ct << " is missing"; > > `!inserted` means that `k` is already in `component`, meaning `descriptor.keys` > contains duplicates. I'm not sure the error message is appropriate for that case. > > > > return -EINVAL; > > } > > - > > - lscData_.emplace(ct, std::move(components)); > > } > > - if (lscData_.empty()) { > > - LOG(LscTable, Error) << "Failed to load any sets"; > > + auto [it, inserted] = lscData_.emplace(ct, component); > > std::move(component) > > > > + if (!inserted) { > > + LOG(LscTable, Error) > > + << "Multiple sets found for color temperature " > > + << ct; > > return -EINVAL; > > } > > @@ -72,17 +83,18 @@ int LscTable::parseLscData(const ValueNode &sets) > > } > > std::vector<uint16_t> LscTable::parseTable(const ValueNode &tuningData, > > - const char *prop) > > + const char *prop, > > + unsigned int numHSamples, > > + unsigned int numVSamples) > > { > > - static constexpr unsigned int kLscNumSamples = > > - RKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX; > > + unsigned int lscNumSamples = numHSamples * numVSamples; > > std::vector<uint16_t> table = > > tuningData[prop].get<std::vector<uint16_t>>().value_or(utils::defopt); > > - if (table.size() != kLscNumSamples) { > > + if (table.size() != lscNumSamples) { > > LOG(LscTable, Error) > > << "Invalid '" << prop << "' values: expected " > > - << kLscNumSamples > > + << lscNumSamples > > << " elements, got " << table.size(); > > return {}; > > } > > [...]
2026. 08. 05. 11:08 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Fri, Jul 24, 2026 at 10:43:15AM +0200, Barnabás Pőcze wrote: >> 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: >>> The current definition of lsc::Components assumes 4 gain vectors named >>> after the colour components supported by the RkISP1 ISP (r, gr, gb and >>> b). >>> >>> As different ISP support different colour channels for their Lsc tables >>> make lsc::Components a map that associates the name of the colour >>> channel to the vector of gains. >>> >>> The use of strings as index for the map comes from the requirement of >>> parsing the tuning file using string identifiers. Pass the strings to >>> use for tuning file parsing to the LscAlgorithm::init() function as part >>> of a newly introduced LscDescriptor type and use the same strings for >>> indexing the gain arrays. >>> >>> The newly introduced LscDescriptor type also provides to the >>> LscAlgorithm class the expected size of the Lsc grid, allowing to remove >>> RkISP1 specific values from the libipa generic implementation. >>> >>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> >>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>> >>> --- >>> v5->v6: >>> - Rename LscDescriptor::num*Cells to num*Samples >>> --- >>> src/ipa/libipa/lsc.cpp | 10 +++--- >>> src/ipa/libipa/lsc.h | 4 +-- >>> src/ipa/libipa/lsc_base.cpp | 49 ++++++++++++++++++++-------- >>> src/ipa/libipa/lsc_base.h | 18 ++++++----- >>> src/ipa/libipa/lsc_polynomial.cpp | 65 +++++++++++++++++++------------------ >>> src/ipa/libipa/lsc_polynomial.h | 10 ++---- >>> src/ipa/libipa/lsc_table.cpp | 68 +++++++++++++++++++++++---------------- >>> src/ipa/libipa/lsc_table.h | 9 ++++-- >>> src/ipa/rkisp1/algorithms/lsc.cpp | 20 ++++++++---- >>> 9 files changed, 150 insertions(+), 103 deletions(-) >>> > [...] >>> diff --git a/src/ipa/libipa/lsc_base.h b/src/ipa/libipa/lsc_base.h >>> index 1bdb7d5c8ed9..b0b9b3b0712d 100644 >>> --- a/src/ipa/libipa/lsc_base.h >>> +++ b/src/ipa/libipa/lsc_base.h >>> @@ -26,13 +26,7 @@ namespace ipa { >>> namespace lsc { >>> -struct Components { >>> - std::vector<uint16_t> r; >>> - std::vector<uint16_t> gr; >>> - std::vector<uint16_t> gb; >>> - std::vector<uint16_t> b; >>> -}; >>> - >>> +using Components = std::map<std::string, std::vector<uint16_t>>; >> >> I suggest >> >> using Components = std::map<std::string, std::vector<uint16_t>, std::less<>> >> >> because that allows some map operations (e.g. find()) without having to >> construct a value of the key type (std::string). > > Isn't std::less the default parameter argument for std::map already ? > > template< > class Key, > class T, > class Compare = std::less<Key>, > class Allocator = std::allocator<std::pair<const Key, T>> > > class map; > > https://en.cppreference.com/cpp/container/map > > ? `std::less<>` is equivalent to `std::less<void>`, which is different from `std::less<Key>`, which is what `std::map` uses by default. Not the most intuitive design for sure: https://en.cppreference.com/cpp/utility/functional/less_void The idea is that `std::less<void>` can compare any two values, even if they have different types, as long as there is an appropriate `operator<` defined. But other `std::less<Key>` only accept `const Key&`. > >> >> >>> using ComponentsMap = std::map<unsigned int, Components>; >>> } /* namespace lsc */ > [...]
Hi Barnabás On Wed, Aug 05, 2026 at 11:27:18AM +0200, Barnabás Pőcze wrote: > 2026. 08. 05. 11:08 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Fri, Jul 24, 2026 at 10:43:15AM +0200, Barnabás Pőcze wrote: > > > 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: > > > > The current definition of lsc::Components assumes 4 gain vectors named > > > > after the colour components supported by the RkISP1 ISP (r, gr, gb and > > > > b). > > > > > > > > As different ISP support different colour channels for their Lsc tables > > > > make lsc::Components a map that associates the name of the colour > > > > channel to the vector of gains. > > > > > > > > The use of strings as index for the map comes from the requirement of > > > > parsing the tuning file using string identifiers. Pass the strings to > > > > use for tuning file parsing to the LscAlgorithm::init() function as part > > > > of a newly introduced LscDescriptor type and use the same strings for > > > > indexing the gain arrays. > > > > > > > > The newly introduced LscDescriptor type also provides to the > > > > LscAlgorithm class the expected size of the Lsc grid, allowing to remove > > > > RkISP1 specific values from the libipa generic implementation. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > --- > > > > v5->v6: > > > > - Rename LscDescriptor::num*Cells to num*Samples > > > > --- > > > > src/ipa/libipa/lsc.cpp | 10 +++--- > > > > src/ipa/libipa/lsc.h | 4 +-- > > > > src/ipa/libipa/lsc_base.cpp | 49 ++++++++++++++++++++-------- > > > > src/ipa/libipa/lsc_base.h | 18 ++++++----- > > > > src/ipa/libipa/lsc_polynomial.cpp | 65 +++++++++++++++++++------------------ > > > > src/ipa/libipa/lsc_polynomial.h | 10 ++---- > > > > src/ipa/libipa/lsc_table.cpp | 68 +++++++++++++++++++++++---------------- > > > > src/ipa/libipa/lsc_table.h | 9 ++++-- > > > > src/ipa/rkisp1/algorithms/lsc.cpp | 20 ++++++++---- > > > > 9 files changed, 150 insertions(+), 103 deletions(-) > > > > > > [...] > > > > diff --git a/src/ipa/libipa/lsc_base.h b/src/ipa/libipa/lsc_base.h > > > > index 1bdb7d5c8ed9..b0b9b3b0712d 100644 > > > > --- a/src/ipa/libipa/lsc_base.h > > > > +++ b/src/ipa/libipa/lsc_base.h > > > > @@ -26,13 +26,7 @@ namespace ipa { > > > > namespace lsc { > > > > -struct Components { > > > > - std::vector<uint16_t> r; > > > > - std::vector<uint16_t> gr; > > > > - std::vector<uint16_t> gb; > > > > - std::vector<uint16_t> b; > > > > -}; > > > > - > > > > +using Components = std::map<std::string, std::vector<uint16_t>>; > > > > > > I suggest > > > > > > using Components = std::map<std::string, std::vector<uint16_t>, std::less<>> > > > > > > because that allows some map operations (e.g. find()) without having to > > > construct a value of the key type (std::string). > > > > Isn't std::less the default parameter argument for std::map already ? > > > > template< > > class Key, > > class T, > > class Compare = std::less<Key>, > > class Allocator = std::allocator<std::pair<const Key, T>> > > > class map; > > > > https://en.cppreference.com/cpp/container/map > > > > ? > > `std::less<>` is equivalent to `std::less<void>`, which is different from > `std::less<Key>`, which is what `std::map` uses by default. Not the most > intuitive design for sure: https://en.cppreference.com/cpp/utility/functional/less_void > > The idea is that `std::less<void>` can compare any two values, even if they > have different types, as long as there is an appropriate `operator<` defined. > But other `std::less<Key>` only accept `const Key&`. > Ok, so this means for a map<> indexed by strings, when a raw c string is passed in and compared, there is no need to implicitly construct a string object, but the string is compared to the raw array of chars, right ? I'll use std::less<> then, thanks > > > > > > > > > > > > > using ComponentsMap = std::map<unsigned int, Components>; > > > > } /* namespace lsc */ > > [...]
2026. 08. 05. 11:34 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Wed, Aug 05, 2026 at 11:27:18AM +0200, Barnabás Pőcze wrote: >> 2026. 08. 05. 11:08 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Fri, Jul 24, 2026 at 10:43:15AM +0200, Barnabás Pőcze wrote: >>>> 2026. 07. 20. 16:59 keltezéssel, Jacopo Mondi írta: >>>>> The current definition of lsc::Components assumes 4 gain vectors named >>>>> after the colour components supported by the RkISP1 ISP (r, gr, gb and >>>>> b). >>>>> >>>>> As different ISP support different colour channels for their Lsc tables >>>>> make lsc::Components a map that associates the name of the colour >>>>> channel to the vector of gains. >>>>> >>>>> The use of strings as index for the map comes from the requirement of >>>>> parsing the tuning file using string identifiers. Pass the strings to >>>>> use for tuning file parsing to the LscAlgorithm::init() function as part >>>>> of a newly introduced LscDescriptor type and use the same strings for >>>>> indexing the gain arrays. >>>>> >>>>> The newly introduced LscDescriptor type also provides to the >>>>> LscAlgorithm class the expected size of the Lsc grid, allowing to remove >>>>> RkISP1 specific values from the libipa generic implementation. >>>>> >>>>> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>>>> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> >>>>> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >>>>> >>>>> --- >>>>> v5->v6: >>>>> - Rename LscDescriptor::num*Cells to num*Samples >>>>> --- >>>>> src/ipa/libipa/lsc.cpp | 10 +++--- >>>>> src/ipa/libipa/lsc.h | 4 +-- >>>>> src/ipa/libipa/lsc_base.cpp | 49 ++++++++++++++++++++-------- >>>>> src/ipa/libipa/lsc_base.h | 18 ++++++----- >>>>> src/ipa/libipa/lsc_polynomial.cpp | 65 +++++++++++++++++++------------------ >>>>> src/ipa/libipa/lsc_polynomial.h | 10 ++---- >>>>> src/ipa/libipa/lsc_table.cpp | 68 +++++++++++++++++++++++---------------- >>>>> src/ipa/libipa/lsc_table.h | 9 ++++-- >>>>> src/ipa/rkisp1/algorithms/lsc.cpp | 20 ++++++++---- >>>>> 9 files changed, 150 insertions(+), 103 deletions(-) >>>>> >>> [...] >>>>> diff --git a/src/ipa/libipa/lsc_base.h b/src/ipa/libipa/lsc_base.h >>>>> index 1bdb7d5c8ed9..b0b9b3b0712d 100644 >>>>> --- a/src/ipa/libipa/lsc_base.h >>>>> +++ b/src/ipa/libipa/lsc_base.h >>>>> @@ -26,13 +26,7 @@ namespace ipa { >>>>> namespace lsc { >>>>> -struct Components { >>>>> - std::vector<uint16_t> r; >>>>> - std::vector<uint16_t> gr; >>>>> - std::vector<uint16_t> gb; >>>>> - std::vector<uint16_t> b; >>>>> -}; >>>>> - >>>>> +using Components = std::map<std::string, std::vector<uint16_t>>; >>>> >>>> I suggest >>>> >>>> using Components = std::map<std::string, std::vector<uint16_t>, std::less<>> >>>> >>>> because that allows some map operations (e.g. find()) without having to >>>> construct a value of the key type (std::string). >>> >>> Isn't std::less the default parameter argument for std::map already ? >>> >>> template< >>> class Key, >>> class T, >>> class Compare = std::less<Key>, >>> class Allocator = std::allocator<std::pair<const Key, T>> >>> > class map; >>> >>> https://en.cppreference.com/cpp/container/map >>> >>> ? >> >> `std::less<>` is equivalent to `std::less<void>`, which is different from >> `std::less<Key>`, which is what `std::map` uses by default. Not the most >> intuitive design for sure: https://en.cppreference.com/cpp/utility/functional/less_void >> >> The idea is that `std::less<void>` can compare any two values, even if they >> have different types, as long as there is an appropriate `operator<` defined. >> But other `std::less<Key>` only accept `const Key&`. >> > > Ok, so this means for a map<> indexed by strings, when a raw c string > is passed in and compared, there is no need to implicitly construct a > string object, but the string is compared to the raw array of chars, > right ? Yes, it should work like that. > > I'll use std::less<> then, thanks > >> >>> >>>> >>>> >>>>> using ComponentsMap = std::map<unsigned int, Components>; >>>>> } /* namespace lsc */ >>> [...]
diff --git a/src/ipa/libipa/lsc.cpp b/src/ipa/libipa/lsc.cpp index dd69c7a80b17..831f6b003df1 100644 --- a/src/ipa/libipa/lsc.cpp +++ b/src/ipa/libipa/lsc.cpp @@ -86,16 +86,16 @@ namespace lsc { /** * \param[in] tuningData The tuning data - * \param[in] sensorSize The physical sensor size * \param[in] controls The IPA list of supported controls + * \param[in] descriptor The LSC engine descriptor * * Parse \a tuningData according to the settings specified in \a descriptor to * populate the LSC data and registers LSC controls in \a controls. * * \return 0 on success, a negative error code otherwise */ -int LscAlgorithm::init(const ValueNode &tuningData, const Size &sensorSize, - ControlInfoMap::Map &controls) +int LscAlgorithm::init(const ValueNode &tuningData, ControlInfoMap::Map &controls, + const LscDescriptor &descriptor) { polynomial_ = false; @@ -108,7 +108,7 @@ int LscAlgorithm::init(const ValueNode &tuningData, const Size &sensorSize, * \todo Most likely the reference frame should be native_size. * Let's wait how the internal discussions progress. */ - impl_ = std::make_unique<LscPolynomial>(sensorSize); + impl_ = std::make_unique<LscPolynomial>(descriptor.sensorSize); polynomial_ = true; LOG(Lsc, Debug) << "Using polynomial Lsc"; } else { @@ -123,7 +123,7 @@ int LscAlgorithm::init(const ValueNode &tuningData, const Size &sensorSize, return -EINVAL; } - int ret = impl_->parseLscData(yamlSets); + int ret = impl_->parseLscData(yamlSets, descriptor); if (ret) return ret; diff --git a/src/ipa/libipa/lsc.h b/src/ipa/libipa/lsc.h index 9a51d32c3464..9fe8ad67ea33 100644 --- a/src/ipa/libipa/lsc.h +++ b/src/ipa/libipa/lsc.h @@ -38,8 +38,8 @@ struct FrameContext { class LscAlgorithm { public: - int init(const ValueNode &tuningData, const Size &sensorSize, - ControlInfoMap::Map &controls); + int init(const ValueNode &tuningData, ControlInfoMap::Map &controls, + const LscDescriptor &descriptor); int configure(lsc::ActiveState &state, const Rectangle &analogCrop, const std::vector<double> &xPos, diff --git a/src/ipa/libipa/lsc_base.cpp b/src/ipa/libipa/lsc_base.cpp index b3671376a12a..01616d1c2e2a 100644 --- a/src/ipa/libipa/lsc_base.cpp +++ b/src/ipa/libipa/lsc_base.cpp @@ -19,23 +19,20 @@ namespace ipa { namespace lsc { /** - * \struct Components + * \typedef Components * \brief Associate colour components with a list of gains * * LSC tables are defined as a list of gain values associated to a colour * component. * - * \var Components::r - * \brief The list of gains for the Red colour component + * As different ISP support different colour components (usually 'r', 'gr', + * 'gb', 'b' or just 'r', 'g', 'b') this class associates a string + * identifier for the colour component to a list of gains. * - * \var Components::gr - * \brief The list of gains for the Green/Red colour component + * Each key name shall match an entry in the tuning file. * - * \var Components::gb - * \brief The list of gains for the Green/Blue colour component - * - * \var Components::b - * \brief The list of gains for the Blue colour component + * The list of keys is provided to the LscAlgorithm class using \a + * LscDescriptor::keys. */ /** @@ -57,13 +54,32 @@ void Interpolator<lsc::Components>:: lsc::Components &dest, double lambda) { - interpolateVector(a.r, b.r, dest.r, lambda); - interpolateVector(a.gr, b.gr, dest.gr, lambda); - interpolateVector(a.gb, b.gb, dest.gb, lambda); - interpolateVector(a.b, b.b, dest.b, lambda); + for (auto const &[k, v] : a) + interpolateVector(v, b.at(k), dest[k], lambda); } #endif +/** + * \struct LscDescriptor + * \brief Describe the ISP LSC engine + * + * \var LscDescriptor::keys + * \brief The list of colour components to which a list of gains is associated + * with in the tuning file. Used for parsing the tuning file + * + * \var LscDescriptor::numHSamples + * \brief Number of horizontal gain samples of the ISP LSC grid. Used for + * validating the list of gains parsed from tuning file + * + * \var LscDescriptor::numVSamples + * \brief Number of vertical gain samples of the ISP LSC grid. Used for + * validating the list of gains parsed from tuning file + * + * \var LscDescriptor::sensorSize + * \brief The physical sensor size. This is the largest frame size used to + * generate the LSC table. Only used by the polynomial LSC algorithm + */ + /** * \class LscImplementation * \brief Pure virtual base class for LSC algorithm implementations @@ -80,6 +96,11 @@ void Interpolator<lsc::Components>:: * \fn LscImplementation::parseLscData * \brief Parse \a tuningData * \param[in] tuningData The tuning data + * \param[in] descriptor The LSC engine descriptor + * + * Parse the tuning file using the \a descriptor to identify the colour + * components in the tuning data and validate the size of the loaded gains + * tables. * * \return 0 on success, a negative error number otherwise */ diff --git a/src/ipa/libipa/lsc_base.h b/src/ipa/libipa/lsc_base.h index 1bdb7d5c8ed9..b0b9b3b0712d 100644 --- a/src/ipa/libipa/lsc_base.h +++ b/src/ipa/libipa/lsc_base.h @@ -26,13 +26,7 @@ namespace ipa { namespace lsc { -struct Components { - std::vector<uint16_t> r; - std::vector<uint16_t> gr; - std::vector<uint16_t> gb; - std::vector<uint16_t> b; -}; - +using Components = std::map<std::string, std::vector<uint16_t>>; using ComponentsMap = std::map<unsigned int, Components>; } /* namespace lsc */ @@ -56,12 +50,20 @@ void Interpolator<lsc::Components>:: double lambda); #endif /* __DOXYGEN__ */ +struct LscDescriptor { + std::vector<std::string> keys; + unsigned int numHSamples; + unsigned int numVSamples; + Size sensorSize; +}; + class LscImplementation { public: virtual ~LscImplementation() {} - virtual int parseLscData(const ValueNode &tuningData) = 0; + virtual int parseLscData(const ValueNode &tuningData, + const LscDescriptor &descriptor) = 0; virtual lsc::ComponentsMap sampleForCrop(const Rectangle &cropRectangle, diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp index 682e2f14fe09..e17ba85448be 100644 --- a/src/ipa/libipa/lsc_polynomial.cpp +++ b/src/ipa/libipa/lsc_polynomial.cpp @@ -133,44 +133,43 @@ void Polynomial::setReferenceImageSize(const Size &size) /** * \brief Parse polynomial LSC data * \param[in] sets The tuning file content + * \param[in] descriptor The LSC engine descriptor * * Parse the LSC data in polyomial form from the \a sets tuning data. * * \return 0 on success or a negative error number otherwise */ -int LscPolynomial::parseLscData(const ValueNode &sets) +int LscPolynomial::parseLscData(const ValueNode &sets, + const LscDescriptor &descriptor) { for (const auto &set : sets.asList()) { - std::optional<lsc::Polynomial> pr, pgr, pgb, pb; uint32_t ct = set["ct"].get<uint32_t>(0); - if (lscData_.count(ct)) { - LOG(LscPolynomial, Error) - << "Multiple sets found for " - << "color temperature " << ct; - return -EINVAL; + PolynomialComponents components; + for (auto &k : descriptor.keys) { + auto polynomial = set[k.c_str()].get<lsc::Polynomial>(); + if (!polynomial) { + LOG(LscPolynomial, Error) + << "Missing polynomial for component " + << k; + return -EINVAL; + } + + auto [it, inserted] = + components.emplace(std::piecewise_construct, + std::forward_as_tuple(k.c_str()), + std::forward_as_tuple(*polynomial)); + + it->second.setReferenceImageSize(descriptor.sensorSize); } - pr = set["r"].get<lsc::Polynomial>(); - pgr = set["gr"].get<lsc::Polynomial>(); - pgb = set["gb"].get<lsc::Polynomial>(); - pb = set["b"].get<lsc::Polynomial>(); - - if (!(pr || pgr || pgb || pb)) { + auto [it, inserted] = lscData_.emplace(ct, components); + if (!inserted) { LOG(LscPolynomial, Error) - << "Failed to parse polynomial for " - << "colour temperature " << ct; + << "Multiple sets found for " + << "color temperature " << ct; return -EINVAL; } - - pr->setReferenceImageSize(sensorSize_); - pgr->setReferenceImageSize(sensorSize_); - pgb->setReferenceImageSize(sensorSize_); - pb->setReferenceImageSize(sensorSize_); - - lscData_.emplace(std::piecewise_construct, - std::forward_as_tuple(ct), - std::forward_as_tuple(PolynomialComponents{ *pr, *pgr, *pgb, *pb })); } if (lscData_.empty()) { @@ -210,13 +209,17 @@ LscPolynomial::sampleForCrop(const Rectangle &cropRectangle, lsc::ComponentsMap components; - for (const auto &[k, p] : lscData_) { - components[k] = { - samplePolynomial(p.pr, xPos, yPos, cropRectangle), - samplePolynomial(p.pgr, xPos, yPos, cropRectangle), - samplePolynomial(p.pgb, xPos, yPos, cropRectangle), - samplePolynomial(p.pb, xPos, yPos, cropRectangle) - }; + for (const auto &[t, c] : lscData_) { + lsc::Components comp; + + for (const auto &[k, p] : c) { + comp.emplace(std::piecewise_construct, + std::forward_as_tuple(k), + std::forward_as_tuple(samplePolynomial(p, xPos, yPos, + cropRectangle))); + } + + components[t] = comp; } return components; diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h index d2276ecc6f6b..2a26db499d68 100644 --- a/src/ipa/libipa/lsc_polynomial.h +++ b/src/ipa/libipa/lsc_polynomial.h @@ -53,12 +53,7 @@ private: class LscPolynomial : public LscImplementation { private: - struct PolynomialComponents { - lsc::Polynomial pr; - lsc::Polynomial pgr; - lsc::Polynomial pgb; - lsc::Polynomial pb; - }; + using PolynomialComponents = std::map<std::string, lsc::Polynomial>; using PolynomialComponentsMap = std::map<unsigned int, PolynomialComponents>; public: @@ -67,7 +62,8 @@ public: { } - int parseLscData(const ValueNode &sets) override; + int parseLscData(const ValueNode &sets, + const LscDescriptor &descriptor) override; lsc::ComponentsMap sampleForCrop(const Rectangle &cropRectangle, diff --git a/src/ipa/libipa/lsc_table.cpp b/src/ipa/libipa/lsc_table.cpp index 47dff156963a..92e24bc5160a 100644 --- a/src/ipa/libipa/lsc_table.cpp +++ b/src/ipa/libipa/lsc_table.cpp @@ -7,9 +7,6 @@ #include "lsc_table.h" -/* \todo Remove RkISP1 from libipa. */ -#include "linux/rkisp1-config.h" - namespace libcamera { LOG_DEFINE_CATEGORY(LscTable) @@ -29,42 +26,56 @@ namespace ipa { /** * \brief Parse tabular LSC data * \param[in] sets The tuning file content + * \param[in] descriptor The LSC engine descriptor * * Parse the LSC data in tabular form from the \a sets tuning data. * * \return 0 on success or a negative error number otherwise */ -int LscTable::parseLscData(const ValueNode &sets) +int LscTable::parseLscData(const ValueNode &sets, + const LscDescriptor &descriptor) { for (const auto &set : sets.asList()) { uint32_t ct = set["ct"].get<uint32_t>(0); - if (lscData_.count(ct)) { - LOG(LscTable, Error) - << "Multiple sets found for color temperature " - << ct; - return -EINVAL; - } + int ret = parseLscComponent(set, ct, descriptor); + if (ret) + return ret; + } - lsc::Components components; - components.r = parseTable(set, "r"); - components.gr = parseTable(set, "gr"); - components.gb = parseTable(set, "gb"); - components.b = parseTable(set, "b"); + if (lscData_.empty()) { + LOG(LscTable, Error) << "Failed to load any sets"; + return -EINVAL; + } - if (components.r.empty() || components.gr.empty() || - components.gb.empty() || components.b.empty()) { + return 0; +} + +int LscTable::parseLscComponent(const ValueNode &yamlSet, + unsigned int ct, const LscDescriptor &descriptor) +{ + lsc::Components component; + for (auto &k : descriptor.keys) { + auto [it, inserted] = component.emplace( + std::piecewise_construct, + std::forward_as_tuple(k.c_str()), + std::forward_as_tuple(parseTable(yamlSet, + k.c_str(), + descriptor.numHSamples, + descriptor.numVSamples))); + if (!inserted || it->second.empty()) { LOG(LscTable, Error) - << "Set for color temperature " << ct - << " is missing tables"; + << "Set " << k << " for color temperature " + << ct << " is missing"; return -EINVAL; } - - lscData_.emplace(ct, std::move(components)); } - if (lscData_.empty()) { - LOG(LscTable, Error) << "Failed to load any sets"; + auto [it, inserted] = lscData_.emplace(ct, component); + if (!inserted) { + LOG(LscTable, Error) + << "Multiple sets found for color temperature " + << ct; return -EINVAL; } @@ -72,17 +83,18 @@ int LscTable::parseLscData(const ValueNode &sets) } std::vector<uint16_t> LscTable::parseTable(const ValueNode &tuningData, - const char *prop) + const char *prop, + unsigned int numHSamples, + unsigned int numVSamples) { - static constexpr unsigned int kLscNumSamples = - RKISP1_CIF_ISP_LSC_SAMPLES_MAX * RKISP1_CIF_ISP_LSC_SAMPLES_MAX; + unsigned int lscNumSamples = numHSamples * numVSamples; std::vector<uint16_t> table = tuningData[prop].get<std::vector<uint16_t>>().value_or(utils::defopt); - if (table.size() != kLscNumSamples) { + if (table.size() != lscNumSamples) { LOG(LscTable, Error) << "Invalid '" << prop << "' values: expected " - << kLscNumSamples + << lscNumSamples << " elements, got " << table.size(); return {}; } diff --git a/src/ipa/libipa/lsc_table.h b/src/ipa/libipa/lsc_table.h index 67c7464ea5b6..a33208761c15 100644 --- a/src/ipa/libipa/lsc_table.h +++ b/src/ipa/libipa/lsc_table.h @@ -26,7 +26,8 @@ namespace ipa { class LscTable : public LscImplementation { public: - int parseLscData(const ValueNode &sets) override; + int parseLscData(const ValueNode &sets, + const LscDescriptor &descriptor) override; lsc::ComponentsMap sampleForCrop([[maybe_unused]] const Rectangle &cropRectangle, @@ -39,8 +40,12 @@ public: } private: + int parseLscComponent(const ValueNode &yamlSet, + unsigned int ct, const LscDescriptor &descriptor); std::vector<uint16_t> parseTable(const ValueNode &tuningData, - const char *prop); + const char *prop, + unsigned int numHSamples, + unsigned int numVSamples); lsc::ComponentsMap lscData_; }; diff --git a/src/ipa/rkisp1/algorithms/lsc.cpp b/src/ipa/rkisp1/algorithms/lsc.cpp index b0d6649be4fd..7a95bfcfb93a 100644 --- a/src/ipa/rkisp1/algorithms/lsc.cpp +++ b/src/ipa/rkisp1/algorithms/lsc.cpp @@ -123,8 +123,12 @@ int LensShadingCorrection::init([[maybe_unused]] IPAContext &context, xPos_ = sizesListToPositions(xSize_); yPos_ = sizesListToPositions(ySize_); - return lscAlgo_.init(tuningData, context.sensorInfo.activeAreaSize, - context.ctrlMap); + return lscAlgo_.init(tuningData, context.ctrlMap, { + .keys = { "r", "gr", "gb", "b" }, + .numHSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX, + .numVSamples = RKISP1_CIF_ISP_LSC_SAMPLES_MAX, + .sensorSize = context.sensorInfo.activeAreaSize + }); } /** @@ -173,10 +177,14 @@ void LensShadingCorrection::setParameters(rkisp1_cif_isp_lsc_config &config) void LensShadingCorrection::copyTable(rkisp1_cif_isp_lsc_config &config, const lsc::Components &set) { - std::copy(set.r.begin(), set.r.end(), &config.r_data_tbl[0][0]); - std::copy(set.gr.begin(), set.gr.end(), &config.gr_data_tbl[0][0]); - std::copy(set.gb.begin(), set.gb.end(), &config.gb_data_tbl[0][0]); - std::copy(set.b.begin(), set.b.end(), &config.b_data_tbl[0][0]); + const auto &r = set.at("r"); + std::copy(r.begin(), r.end(), &config.r_data_tbl[0][0]); + const auto &gr = set.at("gr"); + std::copy(gr.begin(), gr.end(), &config.gr_data_tbl[0][0]); + const auto &gb = set.at("gb"); + std::copy(gb.begin(), gb.end(), &config.gb_data_tbl[0][0]); + const auto &b = set.at("b"); + std::copy(b.begin(), b.end(), &config.b_data_tbl[0][0]); } /**