[PATCH v3 7/9] ipa: libipa: Add lsc polynomial class

[Laurent Pinchart]

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 at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at 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',

-- 
Regards,

Laurent Pinchart