[{"id":31319,"web_url":"https://patchwork.libcamera.org/comment/31319/","msgid":"<20240923195907.GA8631@pendragon.ideasonboard.com>","date":"2024-09-23T19:59:07","subject":"Re: [PATCH v3 7/9] ipa: libipa: Add lsc polynomial class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch.\n\nOn Fri, Sep 20, 2024 at 03:39:22PM +0200, Stefan Klug wrote:\n> Add a basic class to represent polynomials as specified in the DNG spec\n> for vignetting correction.\n> \n> Signed-off-by: Stefan Klug \n> Reviewed-by: Kieran Bingham \n> \n> ---\n> \n> Changes in v3:\n> - Fixed typos\n> - Fixed unintialized variables\n> - Collected tags\n> ---\n> src/ipa/libipa/lsc_polynomial.cpp | 81 +++++++++++++++++++++++\n> src/ipa/libipa/lsc_polynomial.h | 105 ++++++++++++++++++++++++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 188 insertions(+)\n> create mode 100644 src/ipa/libipa/lsc_polynomial.cpp\n> create mode 100644 src/ipa/libipa/lsc_polynomial.h\n> \n> diff --git a/src/ipa/libipa/lsc_polynomial.cpp b/src/ipa/libipa/lsc_polynomial.cpp\n> new file mode 100644\n> index 000000000000..f607d86c54c3\n> --- /dev/null\n> +++ b/src/ipa/libipa/lsc_polynomial.cpp\n> @@ -0,0 +1,81 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * Polynomial class to represent lens shading correction\n> + */\n> +\n> +#include \"lsc_polynomial.h\"\n> +\n> +#include \n> +\n> +/**\n> + * \\file lsc_polynomial.h\n> + * \\brief LscPolynomial class\n> + */\n> +\n> +namespace libcamera {\n> +\n> +LOG_DEFINE_CATEGORY(LscPolynomial)\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\class LscPolynomial\n> + * \\brief Class for handling even polynomials used in lens shading correction\n> + *\n> + * Shading artifacts of camera lenses can be modeled using even radial\n> + * polynomials. This class implements a polynomial with 5 coefficients which\n> + * follows the definition of the FixVignetteRadial opcode in the Adobe DNG\n> + * specification.\n\nAny chance you could add the formula here ?\n\n> + */\n> +\n> +/**\n> + * \\fn LscPolynomial::LscPolynomial(double cx = 0.0, double cy = 0.0, double k0 = 0.0,\n> +\t\t double k1 = 0.0, double k2 = 0.0, double k3 = 0.0,\n> +\t\t double k4 = 0.0)\n> + * \\brief Construct a polynomial using the given coefficients\n> + * \\param cx Center-x relative to the image in normalized coordinates (0..1)\n\n\\param[in]\n\nSame everywhere.\n\n> + * \\param cy Center-y relative to the image in normalized coordinates (0..1)\n> + * \\param k0 Coefficient of the polynomial\n> + * \\param k1 Coefficient of the polynomial\n> + * \\param k2 Coefficient of the polynomial\n> + * \\param k3 Coefficient of the polynomial\n> + * \\param k4 Coefficient of the polynomial\n\nI wonder if the coefficients should be a Span.\n\n> + */\n> +\n> +/**\n> + * \\fn LscPolynomial::sampleAtNormalizedPixelPos(double x, double y)\n\nThat's a very long name, I would have named is just sample() as there's\nno other sampling function.\n\n> + * \\brief Sample the polynomial at the given normalized pixel position\n> + *\n> + * This functions samples the polynomial at the given pixel position divided by\n> + * the value returned by getM().\n> + *\n> + * \\param x x position in normalized coordinates\n> + * \\param y y position in normalized coordinates\n\n\\param goes just after \\brief. Same below.\n\n> + * \\return The sampled value\n> + */\n> +\n> +/**\n> + * \\fn LscPolynomial::getM()\n> + * \\brief Get the value m as described in the dng specification\n\ns/dng/DNG/\n\nSame below.\n\n> + *\n> + * Returns m according to dng spec. m represents the Euclidean distance\n\ns/spec/specification.\n\n> + * (in pixels) from the optical center to the farthest pixel in the\n> + * image.\n> + *\n> + * \\return The sampled value\n> + */\n> +\n> +/**\n> + * \\fn LscPolynomial::setReferenceImageSize(const Size &size)\n> + * \\brief Set the reference image size\n> + *\n> + * Set the reference image size that is used for subsequent calls to getM() and\n> + * sampleAtNormalizedPixelPos()\n> + *\n> + * \\param size The size of the reference image\n> + */\n> +\n> +} // namespace ipa\n> +} // namespace libcamera\n\nC++-style comments.\n\n> diff --git a/src/ipa/libipa/lsc_polynomial.h b/src/ipa/libipa/lsc_polynomial.h\n> new file mode 100644\n> index 000000000000..c898faeb13db\n> --- /dev/null\n> +++ b/src/ipa/libipa/lsc_polynomial.h\n> @@ -0,0 +1,105 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Ideas On Board\n> + *\n> + * Helper for radial polynomial used in lens shading correction.\n> + */\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +#include \n> +\n> +#include \n> +#include \n> +\n> +#include \"libcamera/internal/yaml_parser.h\"\n> +\n> +namespace libcamera {\n> +\n> +LOG_DECLARE_CATEGORY(LscPolynomial)\n> +\n> +namespace ipa {\n> +\n> +class LscPolynomial\n> +{\n> +public:\n> +\tLscPolynomial(double cx = 0.0, double cy = 0.0, double k0 = 0.0,\n> +\t\t double k1 = 0.0, double k2 = 0.0, double k3 = 0.0,\n> +\t\t double k4 = 0.0)\n\nIt doesn't make much sense to construct a polynomial with part of the\ncoefficients only, so I assume the defaults are meant to make some sort\nof default constructor ? You should have two separate constructors then.\n\n> +\t\t: cx_(cx), cy_(cy), cnx_(0), cny_(0),\n> +\t\t coefficients_({ k0, k1, k2, k3, k4 })\n> +\t{\n> +\t}\n> +\n> +\tdouble sampleAtNormalizedPixelPos(double x, double y) const\n> +\t{\n> +\t\tdouble dx = x - cnx_;\n> +\t\tdouble dy = y - cny_;\n> +\t\tdouble r = sqrt(dx * dx + dy * dy);\n> +\t\tdouble res = 1.0;\n> +\t\tfor (unsigned int i = 0; i < coefficients_.size(); i++) {\n> +\t\t\tres += coefficients_[i] * std::pow(r, (i + 1) * 2);\n\nGiven that you'll need to call this function many times, this feels\nquite inefficient. Could it be expressed as\n\n\tdouble r2 = dx * dx + dy * dy;\n\tdouble res = 0.0;\n\n\tfor (double coeff : utils::reverse(coefficients_)) {\n\t\tres += coeff;\n\t\tres *= r2;\n\t}\n\n\tres += 1.0;\n\nor would that be slower ?\n\n> +\t\t}\n\nUnneeded curly braces.\n\n> +\t\treturn res;\n> +\t}\n> +\n> +\tdouble getM() const\n> +\t{\n> +\t\tdouble cpx = imageSize_.width * cx_;\n> +\t\tdouble cpy = imageSize_.height * cy_;\n> +\t\tdouble mx = std::max(cpx, std::fabs(imageSize_.width - cpx));\n> +\t\tdouble my = std::max(cpy, std::fabs(imageSize_.height - cpy));\n\ns/std::fabs/std::abs/\n\n> +\n> +\t\treturn sqrt(mx * mx + my * my);\n> +\t}\n> +\n> +\tvoid setReferenceImageSize(const Size &size)\n> +\t{\n> +\t\tassert(!size.isNull());\n> +\t\timageSize_ = size;\n> +\n> +\t\t/* Calculate normalized centers */\n\ns/centers/centers./\n\n> +\t\tdouble m = getM();\n> +\t\tcnx_ = (size.width * cx_) / m;\n> +\t\tcny_ = (size.height * cy_) / m;\n> +\t}\n\nThere's no need for all those functions to be inline, I think they\nshould all move to the cpp file.\n\n> +\n> +private:\n> +\tdouble cx_;\n> +\tdouble cy_;\n> +\tdouble cnx_;\n> +\tdouble cny_;\n> +\tstd::array coefficients_;\n> +\n> +\tSize imageSize_;\n> +};\n> +\n> +} /* namespace ipa */\n> +\n> +#ifndef __DOXYGEN__\n> +\n> +template<>\n> +struct YamlObject::Getter {\n> +\tstd::optional get(const YamlObject &obj) const\n> +\t{\n> +\t\tstd::optional cx = obj[\"cx\"].get();\n> +\t\tstd::optional cy = obj[\"cy\"].get();\n> +\t\tstd::optional k0 = obj[\"k0\"].get();\n> +\t\tstd::optional k1 = obj[\"k1\"].get();\n> +\t\tstd::optional k2 = obj[\"k2\"].get();\n> +\t\tstd::optional k3 = obj[\"k3\"].get();\n> +\t\tstd::optional k4 = obj[\"k4\"].get();\n> +\n> +\t\tif (!(cx && cy && k0 && k1 && k2 && k3 && k4))\n> +\t\t\tLOG(LscPolynomial, Error)\n> +\t\t\t\t<< \"Polynomial is missing a parameter\";\n\nDon't you need to return {} here ?\n\n> +\n> +\t\treturn ipa::LscPolynomial(*cx, *cy, *k0, *k1, *k2, *k3, *k4);\n> +\t}\n> +};\n\nNo need for this to be inline either.\n\n> +\n> +#endif\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index 3740178b2909..e78cbcd63633 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -8,6 +8,7 @@ libipa_headers = files([\n> 'fc_queue.h',\n> 'histogram.h',\n> 'interpolator.h',\n> + 'lsc_polynomial.h',\n> 'matrix.h',\n> 'module.h',\n> 'pwl.h',\n> @@ -22,6 +23,7 @@ libipa_sources = files([\n> 'fc_queue.cpp',\n> 'histogram.cpp',\n> 'interpolator.cpp',\n> + 'lsc_polynomial.cpp',\n> 'matrix.cpp',\n> 'module.cpp',\n> 'pwl.cpp',","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 685EBC3257\n\tfor ;\n\tMon, 23 Sep 2024 19:59:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8398F6350B;\n\tMon, 23 Sep 2024 21:59:40 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E93676037E\n\tfor ;\n\tMon, 23 Sep 2024 21:59:38 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 4219263D;\n\tMon, 23 Sep 2024 21:58:12 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"gIUCaR3p\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1727121492;\n\tbh=90UIwTpd2XrHSQ0LCVfvUw+JlDf992Z4OMi8HR0mK9w=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=gIUCaR3puBMIhfDFFrr9293gt8vHxbuYWKP9VNEyCuA1HbBbN/caqnKqTudoUQFfu\n\tWwsigtK9GBUYlwH7NSQUJRhwl1NNV/0yTffSoIPJiXyyydlPZi3xtwhXJmsTk0OezS\n\tuPB/82vFBNj3OqvAp2Cqa2z7Uta5qrqOwCsrO3RE=","Date":"Mon, 23 Sep 2024 22:59:07 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v3 7/9] ipa: libipa: Add lsc polynomial class","Message-ID":"<20240923195907.GA8631@pendragon.ideasonboard.com>","References":"<20240920133941.90629-1-stefan.klug@ideasonboard.com>\n\t<20240920133941.90629-8-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240920133941.90629-8-stefan.klug@ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]