| Message ID | 20240920133941.90629-8-stefan.klug@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Stefan, Thank you for the patch. On Fri, Sep 20, 2024 at 03:39:22PM +0200, Stefan Klug wrote: > Add a basic class to represent polynomials as specified in the DNG spec > for vignetting correction. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > Changes in v3: > - Fixed typos > - Fixed unintialized variables > - Collected tags > --- > src/ipa/libipa/lsc_polynomial.cpp | 81 +++++++++++++++++++++++ > src/ipa/libipa/lsc_polynomial.h | 105 ++++++++++++++++++++++++++++++ > src/ipa/libipa/meson.build | 2 + > 3 files changed, 188 insertions(+) > create mode 100644 src/ipa/libipa/lsc_polynomial.cpp > create mode 100644 src/ipa/libipa/lsc_polynomial.h > > diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp > new file mode 100644 > index 000000000000..f607d86c54c3 > --- /dev/null > +++ b/src/ipa/libipa/lsc_polynomial.cpp > @@ -0,0 +1,81 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Ideas On Board > + * > + * Polynomial class to represent lens shading correction > + */ > + > +#include "lsc_polynomial.h" > + > +#include <libcamera/base/log.h> > + > +/** > + * \file lsc_polynomial.h > + * \brief LscPolynomial class > + */ > + > +namespace libcamera { > + > +LOG_DEFINE_CATEGORY(LscPolynomial) > + > +namespace ipa { > + > +/** > + * \class LscPolynomial > + * \brief Class for handling even polynomials used in lens shading correction > + * > + * Shading artifacts of camera lenses can be modeled using even radial > + * polynomials. This class implements a polynomial with 5 coefficients which > + * follows the definition of the FixVignetteRadial opcode in the Adobe DNG > + * specification. Any chance you could add the formula here ? > + */ > + > +/** > + * \fn LscPolynomial::LscPolynomial(double cx = 0.0, double cy = 0.0, double k0 = 0.0, > + double k1 = 0.0, double k2 = 0.0, double k3 = 0.0, > + double k4 = 0.0) > + * \brief Construct a polynomial using the given coefficients > + * \param cx Center-x relative to the image in normalized coordinates (0..1) \param[in] Same everywhere. > + * \param cy Center-y relative to the image in normalized coordinates (0..1) > + * \param k0 Coefficient of the polynomial > + * \param k1 Coefficient of the polynomial > + * \param k2 Coefficient of the polynomial > + * \param k3 Coefficient of the polynomial > + * \param k4 Coefficient of the polynomial I wonder if the coefficients should be a Span<double, 5>. > + */ > + > +/** > + * \fn LscPolynomial::sampleAtNormalizedPixelPos(double x, double y) That's a very long name, I would have named is just sample() as there's no other sampling function. > + * \brief Sample the polynomial at the given normalized pixel position > + * > + * This functions samples the polynomial at the given pixel position divided by > + * the value returned by getM(). > + * > + * \param x x position in normalized coordinates > + * \param y y position in normalized coordinates \param goes just after \brief. Same below. > + * \return The sampled value > + */ > + > +/** > + * \fn LscPolynomial::getM() > + * \brief Get the value m as described in the dng specification s/dng/DNG/ Same below. > + * > + * Returns m according to dng spec. m represents the Euclidean distance s/spec/specification. > + * (in pixels) from the optical center to the farthest pixel in the > + * image. > + * > + * \return The sampled value > + */ > + > +/** > + * \fn LscPolynomial::setReferenceImageSize(const Size &size) > + * \brief Set the reference image size > + * > + * Set the reference image size that is used for subsequent calls to getM() and > + * sampleAtNormalizedPixelPos() > + * > + * \param size The size of the reference image > + */ > + > +} // namespace ipa > +} // namespace libcamera C++-style comments. > diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h > new file mode 100644 > index 000000000000..c898faeb13db > --- /dev/null > +++ b/src/ipa/libipa/lsc_polynomial.h > @@ -0,0 +1,105 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Ideas On Board > + * > + * Helper for radial polynomial used in lens shading correction. > + */ > +#pragma once > + > +#include <algorithm> > +#include <array> > +#include <assert.h> > +#include <cmath> > + > +#include <libcamera/base/log.h> > +#include <libcamera/base/span.h> > + > +#include "libcamera/internal/yaml_parser.h" > + > +namespace libcamera { > + > +LOG_DECLARE_CATEGORY(LscPolynomial) > + > +namespace ipa { > + > +class LscPolynomial > +{ > +public: > + LscPolynomial(double cx = 0.0, double cy = 0.0, double k0 = 0.0, > + double k1 = 0.0, double k2 = 0.0, double k3 = 0.0, > + double k4 = 0.0) It doesn't make much sense to construct a polynomial with part of the coefficients only, so I assume the defaults are meant to make some sort of default constructor ? You should have two separate constructors then. > + : cx_(cx), cy_(cy), cnx_(0), cny_(0), > + coefficients_({ k0, k1, k2, k3, k4 }) > + { > + } > + > + double sampleAtNormalizedPixelPos(double x, double y) const > + { > + double dx = x - cnx_; > + double dy = y - cny_; > + double r = sqrt(dx * dx + dy * dy); > + double res = 1.0; > + for (unsigned int i = 0; i < coefficients_.size(); i++) { > + res += coefficients_[i] * std::pow(r, (i + 1) * 2); Given that you'll need to call this function many times, this feels quite inefficient. Could it be expressed as double r2 = dx * dx + dy * dy; double res = 0.0; for (double coeff : utils::reverse(coefficients_)) { res += coeff; res *= r2; } res += 1.0; or would that be slower ? > + } Unneeded curly braces. > + return res; > + } > + > + double getM() const > + { > + double cpx = imageSize_.width * cx_; > + double cpy = imageSize_.height * cy_; > + double mx = std::max(cpx, std::fabs(imageSize_.width - cpx)); > + double my = std::max(cpy, std::fabs(imageSize_.height - cpy)); s/std::fabs/std::abs/ > + > + return sqrt(mx * mx + my * my); > + } > + > + void setReferenceImageSize(const Size &size) > + { > + assert(!size.isNull()); > + imageSize_ = size; > + > + /* Calculate normalized centers */ s/centers/centers./ > + double m = getM(); > + cnx_ = (size.width * cx_) / m; > + cny_ = (size.height * cy_) / m; > + } There's no need for all those functions to be inline, I think they should all move to the cpp file. > + > +private: > + double cx_; > + double cy_; > + double cnx_; > + double cny_; > + std::array<double, 5> coefficients_; > + > + Size imageSize_; > +}; > + > +} /* namespace ipa */ > + > +#ifndef __DOXYGEN__ > + > +template<> > +struct YamlObject::Getter<ipa::LscPolynomial> { > + std::optional<ipa::LscPolynomial> get(const YamlObject &obj) const > + { > + std::optional<double> cx = obj["cx"].get<double>(); > + std::optional<double> cy = obj["cy"].get<double>(); > + std::optional<double> k0 = obj["k0"].get<double>(); > + std::optional<double> k1 = obj["k1"].get<double>(); > + std::optional<double> k2 = obj["k2"].get<double>(); > + std::optional<double> k3 = obj["k3"].get<double>(); > + std::optional<double> k4 = obj["k4"].get<double>(); > + > + if (!(cx && cy && k0 && k1 && k2 && k3 && k4)) > + LOG(LscPolynomial, Error) > + << "Polynomial is missing a parameter"; Don't you need to return {} here ? > + > + return ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4); > + } > +}; No need for this to be inline either. > + > +#endif > + > +} /* namespace libcamera */ > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build > index 3740178b2909..e78cbcd63633 100644 > --- a/src/ipa/libipa/meson.build > +++ b/src/ipa/libipa/meson.build > @@ -8,6 +8,7 @@ libipa_headers = files([ > 'fc_queue.h', > 'histogram.h', > 'interpolator.h', > + 'lsc_polynomial.h', > 'matrix.h', > 'module.h', > 'pwl.h', > @@ -22,6 +23,7 @@ libipa_sources = files([ > 'fc_queue.cpp', > 'histogram.cpp', > 'interpolator.cpp', > + 'lsc_polynomial.cpp', > 'matrix.cpp', > 'module.cpp', > 'pwl.cpp',
diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp new file mode 100644 index 000000000000..f607d86c54c3 --- /dev/null +++ b/src/ipa/libipa/lsc_polynomial.cpp @@ -0,0 +1,81 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2024, Ideas On Board + * + * Polynomial class to represent lens shading correction + */ + +#include "lsc_polynomial.h" + +#include <libcamera/base/log.h> + +/** + * \file lsc_polynomial.h + * \brief LscPolynomial class + */ + +namespace libcamera { + +LOG_DEFINE_CATEGORY(LscPolynomial) + +namespace ipa { + +/** + * \class LscPolynomial + * \brief Class for handling even polynomials used in lens shading correction + * + * Shading artifacts of camera lenses can be modeled using even radial + * polynomials. This class implements a polynomial with 5 coefficients which + * follows the definition of the FixVignetteRadial opcode in the Adobe DNG + * specification. + */ + +/** + * \fn LscPolynomial::LscPolynomial(double cx = 0.0, double cy = 0.0, double k0 = 0.0, + double k1 = 0.0, double k2 = 0.0, double k3 = 0.0, + double k4 = 0.0) + * \brief Construct a polynomial using the given coefficients + * \param cx Center-x relative to the image in normalized coordinates (0..1) + * \param cy Center-y relative to the image in normalized coordinates (0..1) + * \param k0 Coefficient of the polynomial + * \param k1 Coefficient of the polynomial + * \param k2 Coefficient of the polynomial + * \param k3 Coefficient of the polynomial + * \param k4 Coefficient of the polynomial + */ + +/** + * \fn LscPolynomial::sampleAtNormalizedPixelPos(double x, double y) + * \brief Sample the polynomial at the given normalized pixel position + * + * This functions samples the polynomial at the given pixel position divided by + * the value returned by getM(). + * + * \param x x position in normalized coordinates + * \param y y position in normalized coordinates + * \return The sampled value + */ + +/** + * \fn LscPolynomial::getM() + * \brief Get the value m as described in the dng specification + * + * Returns m according to dng spec. m represents the Euclidean distance + * (in pixels) from the optical center to the farthest pixel in the + * image. + * + * \return The sampled value + */ + +/** + * \fn LscPolynomial::setReferenceImageSize(const Size &size) + * \brief Set the reference image size + * + * Set the reference image size that is used for subsequent calls to getM() and + * sampleAtNormalizedPixelPos() + * + * \param size The size of the reference image + */ + +} // namespace ipa +} // namespace libcamera diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h new file mode 100644 index 000000000000..c898faeb13db --- /dev/null +++ b/src/ipa/libipa/lsc_polynomial.h @@ -0,0 +1,105 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2024, Ideas On Board + * + * Helper for radial polynomial used in lens shading correction. + */ +#pragma once + +#include <algorithm> +#include <array> +#include <assert.h> +#include <cmath> + +#include <libcamera/base/log.h> +#include <libcamera/base/span.h> + +#include "libcamera/internal/yaml_parser.h" + +namespace libcamera { + +LOG_DECLARE_CATEGORY(LscPolynomial) + +namespace ipa { + +class LscPolynomial +{ +public: + LscPolynomial(double cx = 0.0, double cy = 0.0, double k0 = 0.0, + double k1 = 0.0, double k2 = 0.0, double k3 = 0.0, + double k4 = 0.0) + : cx_(cx), cy_(cy), cnx_(0), cny_(0), + coefficients_({ k0, k1, k2, k3, k4 }) + { + } + + double sampleAtNormalizedPixelPos(double x, double y) const + { + double dx = x - cnx_; + double dy = y - cny_; + double r = sqrt(dx * dx + dy * dy); + double res = 1.0; + for (unsigned int i = 0; i < coefficients_.size(); i++) { + res += coefficients_[i] * std::pow(r, (i + 1) * 2); + } + return res; + } + + double getM() const + { + double cpx = imageSize_.width * cx_; + double cpy = imageSize_.height * cy_; + double mx = std::max(cpx, std::fabs(imageSize_.width - cpx)); + double my = std::max(cpy, std::fabs(imageSize_.height - cpy)); + + return sqrt(mx * mx + my * my); + } + + void setReferenceImageSize(const Size &size) + { + assert(!size.isNull()); + imageSize_ = size; + + /* Calculate normalized centers */ + double m = getM(); + cnx_ = (size.width * cx_) / m; + cny_ = (size.height * cy_) / m; + } + +private: + double cx_; + double cy_; + double cnx_; + double cny_; + std::array<double, 5> coefficients_; + + Size imageSize_; +}; + +} /* namespace ipa */ + +#ifndef __DOXYGEN__ + +template<> +struct YamlObject::Getter<ipa::LscPolynomial> { + std::optional<ipa::LscPolynomial> get(const YamlObject &obj) const + { + std::optional<double> cx = obj["cx"].get<double>(); + std::optional<double> cy = obj["cy"].get<double>(); + std::optional<double> k0 = obj["k0"].get<double>(); + std::optional<double> k1 = obj["k1"].get<double>(); + std::optional<double> k2 = obj["k2"].get<double>(); + std::optional<double> k3 = obj["k3"].get<double>(); + std::optional<double> k4 = obj["k4"].get<double>(); + + if (!(cx && cy && k0 && k1 && k2 && k3 && k4)) + LOG(LscPolynomial, Error) + << "Polynomial is missing a parameter"; + + return ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4); + } +}; + +#endif + +} /* namespace libcamera */ diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build index 3740178b2909..e78cbcd63633 100644 --- a/src/ipa/libipa/meson.build +++ b/src/ipa/libipa/meson.build @@ -8,6 +8,7 @@ libipa_headers = files([ 'fc_queue.h', 'histogram.h', 'interpolator.h', + 'lsc_polynomial.h', 'matrix.h', 'module.h', 'pwl.h', @@ -22,6 +23,7 @@ libipa_sources = files([ 'fc_queue.cpp', 'histogram.cpp', 'interpolator.cpp', + 'lsc_polynomial.cpp', 'matrix.cpp', 'module.cpp', 'pwl.cpp',