[{"id":31998,"web_url":"https://patchwork.libcamera.org/comment/31998/","msgid":"","date":"2024-11-04T11:07:36","subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n\nOn Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:\n> We start to have functions that are effectively identical crop up\n> across the IPA modules. Add a file allowing those to be centralised\n> within libipa so that a single implementation can be used in all of\n> the IPAs.\n>\n> Signed-off-by: Daniel Scally \n> ---\n> src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++\n> src/ipa/libipa/helpers.h | 23 +++++++++\n> src/ipa/libipa/meson.build | 2 +\n> 3 files changed, 128 insertions(+)\n> create mode 100644 src/ipa/libipa/helpers.cpp\n> create mode 100644 src/ipa/libipa/helpers.h\n>\n> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp\n> new file mode 100644\n> index 00000000..cc0cd586\n> --- /dev/null\n> +++ b/src/ipa/libipa/helpers.cpp\n\nNot a huge fan of a collection of C functions, but I guess\nalternatives could be considered over-design\n\n> @@ -0,0 +1,103 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\n> + *\n> + * libipa miscellaneous helpers\n> + */\n> +\n> +#include \"helpers.h\"\n> +\n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +/**\n> + * \\file helpers.h\n> + * \\brief Functions to reduce code duplication between IPA modules\n> + */\n> +\n> +/**\n> + * \\brief Estimate luminance from RGB values following ITU-R BT.601\n> + * \\param[in] r The red value\n> + * \\param[in] g The green value\n> + * \\param[in] b The blue valye\n> + * \\return The estimated luminance value\n\n\\return statements usually go after the longer function documentation\n\n\n> + *\n> + * This function estimates a luminance value from a triplet of Red, Green and\n> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7\n> + * which can be found at https://www.itu.int/rec/R-REC-BT.601\n> + */\n> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)\n\nIn example, in the RPi IPA, the return value is assigned to a double,\nand the calculation suggests it might be worth not casing the result to\nunsigned int.\n\n> +{\n> +\treturn (r * .299) + (g * .587) + (b * .114);\n> +}\n> +\n> +/**\n> + * \\brief Estimate correlated colour temperature from RGB color space input\n> + * \\param[in] red The input red value\n> + * \\param[in] green The input green value\n> + * \\param[in] blue The input blue value\n> + * \\return The estimated color temperature\n\nsame here\n\n> + *\n> + * This function estimates the correlated color temperature RGB color space\n> + * input. In physics and color science, the Planckian locus or black body locus\n> + * is the path or locus that the color of an incandescent black body would take\n> + * in a particular chromaticity space as the blackbody temperature changes.\n> + *\n> + * If a narrow range of color temperatures is considered (those encapsulating\n> + * daylight being the most practical case) one can approximate the Planckian\n> + * locus in order to calculate the CCT in terms of chromaticity coordinates.\n> + *\n> + * More detailed information can be found in:\n> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation\n> + */\n> +uint32_t estimateCCT(double red, double green, double blue)\n> +{\n> +\t/* Convert the RGB values to CIE tristimulus values (XYZ) */\n> +\tdouble X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);\n> +\tdouble Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);\n> +\tdouble Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);\n> +\n> +\t/* Calculate the normalized chromaticity values */\n> +\tdouble x = X / (X + Y + Z);\n> +\tdouble y = Y / (X + Y + Z);\n> +\n> +\t/* Calculate CCT */\n> +\tdouble n = (x - 0.3320) / (0.1858 - y);\n> +\treturn 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;\n> +}\n\nThis seems to come straight from the IPU3 IPA, so ack to the text and\nimplementation\n\n> +\n> +/**\n> + * \\brief Convert double to Q(m.n) format\n\nNote these two helpers seems to be un-used.\n\nWhat Q formats are we referring to ? Wikipedia lists two (TI version\nand ARM version).\nhttps://en.wikipedia.org/wiki/Q_(number_format)\n\n> + * \\param[in] value The value to convert\n> + * \\param[in] m The number of bits used to represent the integer part\n> + * \\param[in] n The number of bits used to represent the fraction part\n> + *\n> + * This function converts a double into an unsigned Q format, clamping at the\n> + * largest possible value given m and n.\n\n\\return ?\n\n> + */\n> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)\n> +{\n> +\tdouble maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);\n> +\n> +\treturn std::clamp(value, 0.0, maxVal) * std::pow(2, n);\n> +}\n> +\n> +/**\n> + * \\brief Convert Q(m.n) formatted number to double\n> + * \\param[in] value The value to convert\n> + * \\param[in] n The number of bits used to represent the fraction part\n> + *\n> + * This function converts an unsigned Q formatted value into a double.\n> + */\n> +double fromQFormat(unsigned int value, unsigned int n)\n> +{\n> +\treturn value / std::pow(2, n);\n> +}\n\nLet's discuss these once the above is clarified\n\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h\n> new file mode 100644\n> index 00000000..361b63bd\n> --- /dev/null\n> +++ b/src/ipa/libipa/helpers.h\n> @@ -0,0 +1,23 @@\n> +/* SPDX-License-Identifier: BSD-2-Clause */\n> +/*\n> + * Copyright (C) 2024, Ideas on Board Oy\n> + *\n> + * libipa miscellaneous helpers\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa {\n> +\n> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);\n> +uint32_t estimateCCT(double red, double green, double blue);\n> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);\n> +double fromQFormat(unsigned int value, unsigned int n);\n> +\n> +} /* namespace ipa */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> index e78cbcd6..ca4f3e28 100644\n> --- a/src/ipa/libipa/meson.build\n> +++ b/src/ipa/libipa/meson.build\n> @@ -6,6 +6,7 @@ libipa_headers = files([\n> 'camera_sensor_helper.h',\n> 'exposure_mode_helper.h',\n> 'fc_queue.h',\n> + 'helpers.h',\n> 'histogram.h',\n> 'interpolator.h',\n> 'lsc_polynomial.h',\n> @@ -21,6 +22,7 @@ libipa_sources = files([\n> 'camera_sensor_helper.cpp',\n> 'exposure_mode_helper.cpp',\n> 'fc_queue.cpp',\n> + 'helpers.cpp',\n> 'histogram.cpp',\n> 'interpolator.cpp',\n> 'lsc_polynomial.cpp',\n> --\n> 2.30.2\n>","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 88849BDB1C\n\tfor ;\n\tMon, 4 Nov 2024 11:07:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97356653C7;\n\tMon, 4 Nov 2024 12:07:42 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BA8E065399\n\tfor ;\n\tMon, 4 Nov 2024 12:07:40 +0100 (CET)","from ideasonboard.com (mob-5-90-48-188.net.vodafone.it\n\t[5.90.48.188])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id C743C22E;\n\tMon, 4 Nov 2024 12:07:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"SdzLQKNc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730718454;\n\tbh=ZJY5hVZ/4IUuOeBiC1sh/k5SFjCergL2mGRkZBOpfXc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=SdzLQKNcUAMnDi2NSXNG1hL95GhxKkd6d0gJ8VPjbueyDMAl4bo5DQYhMaqkWP1v1\n\tREn76ohkdvTogbCrEuRAa7Lkpku9H5RKBuY7NzyyGi0eNhKZbsPEJB7ICwE7xtLKfk\n\tlOHQQeP61Pkc97Z13EzU9KfWGl4J6TDqolfmTaSg=","Date":"Mon, 4 Nov 2024 12:07:36 +0100","From":"Jacopo Mondi ","To":"Daniel Scally ","Cc":"libcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","Subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","Message-ID":"","References":"<20241031160741.253855-1-dan.scally@ideasonboard.com>\n\t<20241031160741.253855-2-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241031160741.253855-2-dan.scally@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\" "}},{"id":32036,"web_url":"https://patchwork.libcamera.org/comment/32036/","msgid":"","date":"2024-11-06T09:39:09","subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Jacopo - thanks for the review\n\nOn 04/11/2024 11:07, Jacopo Mondi wrote:\n> Hi Dan\n>\n> On Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:\n>> We start to have functions that are effectively identical crop up\n>> across the IPA modules. Add a file allowing those to be centralised\n>> within libipa so that a single implementation can be used in all of\n>> the IPAs.\n>>\n>> Signed-off-by: Daniel Scally \n>> ---\n>> src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++\n>> src/ipa/libipa/helpers.h | 23 +++++++++\n>> src/ipa/libipa/meson.build | 2 +\n>> 3 files changed, 128 insertions(+)\n>> create mode 100644 src/ipa/libipa/helpers.cpp\n>> create mode 100644 src/ipa/libipa/helpers.h\n>>\n>> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp\n>> new file mode 100644\n>> index 00000000..cc0cd586\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/helpers.cpp\n> Not a huge fan of a collection of C functions, but I guess\n> alternatives could be considered over-design\nThat was my thinking basically.\n>\n>> @@ -0,0 +1,103 @@\n>> +/* SPDX-License-Identifier: BSD-2-Clause */\n>> +/*\n>> + * Copyright (C) 2024, Ideas on Board Oy\n>> + *\n>> + * libipa miscellaneous helpers\n>> + */\n>> +\n>> +#include \"helpers.h\"\n>> +\n>> +#include \n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +/**\n>> + * \\file helpers.h\n>> + * \\brief Functions to reduce code duplication between IPA modules\n>> + */\n>> +\n>> +/**\n>> + * \\brief Estimate luminance from RGB values following ITU-R BT.601\n>> + * \\param[in] r The red value\n>> + * \\param[in] g The green value\n>> + * \\param[in] b The blue valye\n>> + * \\return The estimated luminance value\n> \\return statements usually go after the longer function documentation\n\n\nAck\n\n>\n>\n>> + *\n>> + * This function estimates a luminance value from a triplet of Red, Green and\n>> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7\n>> + * which can be found at https://www.itu.int/rec/R-REC-BT.601\n>> + */\n>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)\n> In example, in the RPi IPA, the return value is assigned to a double,\n> and the calculation suggests it might be worth not casing the result to\n> unsigned int.\nYou're right, that's wrong. Thanks.\n>\n>> +{\n>> +\treturn (r * .299) + (g * .587) + (b * .114);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Estimate correlated colour temperature from RGB color space input\n>> + * \\param[in] red The input red value\n>> + * \\param[in] green The input green value\n>> + * \\param[in] blue The input blue value\n>> + * \\return The estimated color temperature\n> same here\n>\n>> + *\n>> + * This function estimates the correlated color temperature RGB color space\n>> + * input. In physics and color science, the Planckian locus or black body locus\n>> + * is the path or locus that the color of an incandescent black body would take\n>> + * in a particular chromaticity space as the blackbody temperature changes.\n>> + *\n>> + * If a narrow range of color temperatures is considered (those encapsulating\n>> + * daylight being the most practical case) one can approximate the Planckian\n>> + * locus in order to calculate the CCT in terms of chromaticity coordinates.\n>> + *\n>> + * More detailed information can be found in:\n>> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation\n>> + */\n>> +uint32_t estimateCCT(double red, double green, double blue)\n>> +{\n>> +\t/* Convert the RGB values to CIE tristimulus values (XYZ) */\n>> +\tdouble X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);\n>> +\tdouble Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);\n>> +\tdouble Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);\n>> +\n>> +\t/* Calculate the normalized chromaticity values */\n>> +\tdouble x = X / (X + Y + Z);\n>> +\tdouble y = Y / (X + Y + Z);\n>> +\n>> +\t/* Calculate CCT */\n>> +\tdouble n = (x - 0.3320) / (0.1858 - y);\n>> +\treturn 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;\n>> +}\n> This seems to come straight from the IPU3 IPA, so ack to the text and\n> implementation\n>\n>> +\n>> +/**\n>> + * \\brief Convert double to Q(m.n) format\n> Note these two helpers seems to be un-used.\nAh yes, they're used in the C55 series which will be based on top of this...I can introduce them \nthere if that's better?\n> What Q formats are we referring to ? Wikipedia lists two (TI version\n> and ARM version).\n> https://en.wikipedia.org/wiki/Q_(number_format)\n\n\nSo far I'm only using numbers specified in documentation as being unsigned, and so it's the TI \nversion, but the \"UQ\" format rather than just \"Q\"...possibly renaming the function \n\"toUnsignedQFormat()\" would be clearer?\n\n>\n>> + * \\param[in] value The value to convert\n>> + * \\param[in] m The number of bits used to represent the integer part\n>> + * \\param[in] n The number of bits used to represent the fraction part\n>> + *\n>> + * This function converts a double into an unsigned Q format, clamping at the\n>> + * largest possible value given m and n.\n> \\return ?\n>\n>> + */\n>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)\n>> +{\n>> +\tdouble maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);\n>> +\n>> +\treturn std::clamp(value, 0.0, maxVal) * std::pow(2, n);\n>> +}\n>> +\n>> +/**\n>> + * \\brief Convert Q(m.n) formatted number to double\n>> + * \\param[in] value The value to convert\n>> + * \\param[in] n The number of bits used to represent the fraction part\n>> + *\n>> + * This function converts an unsigned Q formatted value into a double.\n>> + */\n>> +double fromQFormat(unsigned int value, unsigned int n)\n>> +{\n>> +\treturn value / std::pow(2, n);\n>> +}\n> Let's discuss these once the above is clarified\n>\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h\n>> new file mode 100644\n>> index 00000000..361b63bd\n>> --- /dev/null\n>> +++ b/src/ipa/libipa/helpers.h\n>> @@ -0,0 +1,23 @@\n>> +/* SPDX-License-Identifier: BSD-2-Clause */\n>> +/*\n>> + * Copyright (C) 2024, Ideas on Board Oy\n>> + *\n>> + * libipa miscellaneous helpers\n>> + */\n>> +\n>> +#pragma once\n>> +\n>> +#include \n>> +\n>> +namespace libcamera {\n>> +\n>> +namespace ipa {\n>> +\n>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);\n>> +uint32_t estimateCCT(double red, double green, double blue);\n>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);\n>> +double fromQFormat(unsigned int value, unsigned int n);\n>> +\n>> +} /* namespace ipa */\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>> index e78cbcd6..ca4f3e28 100644\n>> --- a/src/ipa/libipa/meson.build\n>> +++ b/src/ipa/libipa/meson.build\n>> @@ -6,6 +6,7 @@ libipa_headers = files([\n>> 'camera_sensor_helper.h',\n>> 'exposure_mode_helper.h',\n>> 'fc_queue.h',\n>> + 'helpers.h',\n>> 'histogram.h',\n>> 'interpolator.h',\n>> 'lsc_polynomial.h',\n>> @@ -21,6 +22,7 @@ libipa_sources = files([\n>> 'camera_sensor_helper.cpp',\n>> 'exposure_mode_helper.cpp',\n>> 'fc_queue.cpp',\n>> + 'helpers.cpp',\n>> 'histogram.cpp',\n>> 'interpolator.cpp',\n>> 'lsc_polynomial.cpp',\n>> --\n>> 2.30.2\n>>","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 20C7BBE173\n\tfor ;\n\tWed, 6 Nov 2024 09:39:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 29B2965417;\n\tWed, 6 Nov 2024 10:39:16 +0100 (CET)","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 2E92860393\n\tfor ;\n\tWed, 6 Nov 2024 10:39:14 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 76C4C475;\n\tWed, 6 Nov 2024 10:39:05 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"og+j4PI3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730885945;\n\tbh=FObAbqpLjzOqIzvZFkGtw0/6vU6yb6DPC3GzKfy3d2s=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=og+j4PI3YBujSduibbDLl6FVU94s82raa4Y/VQQyrq5mW6oBSG0BWjNvgthUCC7pI\n\t2qH7YDfyMQu2QMR5qA4fnm7LwCttP+DBboLK6sjEDGCjeJQ16rrwutCoqAXLKh55pu\n\tPbGqyXLKkRvIN+yPk/exrXxxUuTEpIeb81iajvkQ=","Message-ID":"","Date":"Wed, 6 Nov 2024 09:39:09 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","References":"<20241031160741.253855-1-dan.scally@ideasonboard.com>\n\t<20241031160741.253855-2-dan.scally@ideasonboard.com>\n\t","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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\" "}},{"id":32053,"web_url":"https://patchwork.libcamera.org/comment/32053/","msgid":"<173090140770.3147895.6548924488673408003@ping.linuxembedded.co.uk>","date":"2024-11-06T13:56:47","subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Dan Scally (2024-11-06 09:39:09)\n> Hi Jacopo - thanks for the review\n> \n> On 04/11/2024 11:07, Jacopo Mondi wrote:\n> > Hi Dan\n> >\n> > On Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:\n> >> We start to have functions that are effectively identical crop up\n> >> across the IPA modules. Add a file allowing those to be centralised\n> >> within libipa so that a single implementation can be used in all of\n> >> the IPAs.\n> >>\n> >> Signed-off-by: Daniel Scally \n> >> ---\n> >> src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++\n> >> src/ipa/libipa/helpers.h | 23 +++++++++\n> >> src/ipa/libipa/meson.build | 2 +\n> >> 3 files changed, 128 insertions(+)\n> >> create mode 100644 src/ipa/libipa/helpers.cpp\n> >> create mode 100644 src/ipa/libipa/helpers.h\n> >>\n> >> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp\n> >> new file mode 100644\n> >> index 00000000..cc0cd586\n> >> --- /dev/null\n> >> +++ b/src/ipa/libipa/helpers.cpp\n> > Not a huge fan of a collection of C functions, but I guess\n> > alternatives could be considered over-design\n> That was my thinking basically.\n\nI agree here. I'm fine moving these to a common location to get started.\n\nIf things grow - or become more specific then they could be separated to\na defined structure, but that could also be done on top ... I have one\nexample below for that...\n\n\n> >\n> >> @@ -0,0 +1,103 @@\n> >> +/* SPDX-License-Identifier: BSD-2-Clause */\n> >> +/*\n> >> + * Copyright (C) 2024, Ideas on Board Oy\n> >> + *\n> >> + * libipa miscellaneous helpers\n> >> + */\n> >> +\n> >> +#include \"helpers.h\"\n> >> +\n> >> +#include \n> >> +#include \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa {\n> >> +\n> >> +/**\n> >> + * \\file helpers.h\n> >> + * \\brief Functions to reduce code duplication between IPA modules\n> >> + */\n> >> +\n> >> +/**\n> >> + * \\brief Estimate luminance from RGB values following ITU-R BT.601\n> >> + * \\param[in] r The red value\n> >> + * \\param[in] g The green value\n> >> + * \\param[in] b The blue valye\n\ns/valye/value/\n\n> >> + * \\return The estimated luminance value\n> > \\return statements usually go after the longer function documentation\n> \n> \n> Ack\n> \n> >\n> >\n> >> + *\n> >> + * This function estimates a luminance value from a triplet of Red, Green and\n> >> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7\n> >> + * which can be found at https://www.itu.int/rec/R-REC-BT.601\n> >> + */\n> >> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)\n> > In example, in the RPi IPA, the return value is assigned to a double,\n> > and the calculation suggests it might be worth not casing the result to\n> > unsigned int.\n> You're right, that's wrong. Thanks.\n> >\n> >> +{\n> >> + return (r * .299) + (g * .587) + (b * .114);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Estimate correlated colour temperature from RGB color space input\n> >> + * \\param[in] red The input red value\n> >> + * \\param[in] green The input green value\n> >> + * \\param[in] blue The input blue value\n> >> + * \\return The estimated color temperature\n> > same here\n> >\n> >> + *\n> >> + * This function estimates the correlated color temperature RGB color space\n> >> + * input. In physics and color science, the Planckian locus or black body locus\n> >> + * is the path or locus that the color of an incandescent black body would take\n> >> + * in a particular chromaticity space as the blackbody temperature changes.\n> >> + *\n> >> + * If a narrow range of color temperatures is considered (those encapsulating\n> >> + * daylight being the most practical case) one can approximate the Planckian\n> >> + * locus in order to calculate the CCT in terms of chromaticity coordinates.\n> >> + *\n> >> + * More detailed information can be found in:\n> >> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation\n> >> + */\n> >> +uint32_t estimateCCT(double red, double green, double blue)\n> >> +{\n> >> + /* Convert the RGB values to CIE tristimulus values (XYZ) */\n> >> + double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);\n> >> + double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);\n> >> + double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);\n> >> +\n> >> + /* Calculate the normalized chromaticity values */\n> >> + double x = X / (X + Y + Z);\n> >> + double y = Y / (X + Y + Z);\n> >> +\n> >> + /* Calculate CCT */\n> >> + double n = (x - 0.3320) / (0.1858 - y);\n> >> + return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;\n> >> +}\n> > This seems to come straight from the IPU3 IPA, so ack to the text and\n> > implementation\n\nPerhaps those are both helpers for 'colorspace' so maybe this is a\ncolorspace.cpp ... but lets see...\n\n\n> >\n> >> +\n> >> +/**\n> >> + * \\brief Convert double to Q(m.n) format\n> > Note these two helpers seems to be un-used.\n> Ah yes, they're used in the C55 series which will be based on top of this...I can introduce them \n> there if that's better?\n> > What Q formats are we referring to ? Wikipedia lists two (TI version\n> > and ARM version).\n> > https://en.wikipedia.org/wiki/Q_(number_format)\n> \n> \n> So far I'm only using numbers specified in documentation as being unsigned, and so it's the TI \n> version, but the \"UQ\" format rather than just \"Q\"...possibly renaming the function \n> \"toUnsignedQFormat()\" would be clearer?\n> \n> >\n> >> + * \\param[in] value The value to convert\n> >> + * \\param[in] m The number of bits used to represent the integer part\n> >> + * \\param[in] n The number of bits used to represent the fraction part\n> >> + *\n> >> + * This function converts a double into an unsigned Q format, clamping at the\n> >> + * largest possible value given m and n.\n> > \\return ?\n> >\n> >> + */\n> >> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)\n> >> +{\n> >> + double maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);\n> >> +\n> >> + return std::clamp(value, 0.0, maxVal) * std::pow(2, n);\n> >> +}\n> >> +\n> >> +/**\n> >> + * \\brief Convert Q(m.n) formatted number to double\n> >> + * \\param[in] value The value to convert\n> >> + * \\param[in] n The number of bits used to represent the fraction part\n> >> + *\n> >> + * This function converts an unsigned Q formatted value into a double.\n> >> + */\n> >> +double fromQFormat(unsigned int value, unsigned int n)\n> >> +{\n> >> + return value / std::pow(2, n);\n> >> +}\n> > Let's discuss these once the above is clarified\n\nQ(a.b) formats come up a lot. I'd always thought we need a FixedPoint\n(templated?) class to help with this so we can consider it as a real\ntype.\n\nIf so - it would be it's own class, and warrant a set of unit tests...\n\nSo if you did want to split this helpers.cpp,h up - I'd move the color\nspace functionality to colorspace.cpp and Q helpers to fixedpoint.cpp\n...\n\nUltimately it depends how far you want to take the cleanup...\n\n\n\n> >\n> >> +\n> >> +} /* namespace ipa */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h\n> >> new file mode 100644\n> >> index 00000000..361b63bd\n> >> --- /dev/null\n> >> +++ b/src/ipa/libipa/helpers.h\n> >> @@ -0,0 +1,23 @@\n> >> +/* SPDX-License-Identifier: BSD-2-Clause */\n> >> +/*\n> >> + * Copyright (C) 2024, Ideas on Board Oy\n> >> + *\n> >> + * libipa miscellaneous helpers\n> >> + */\n> >> +\n> >> +#pragma once\n> >> +\n> >> +#include \n> >> +\n> >> +namespace libcamera {\n> >> +\n> >> +namespace ipa {\n> >> +\n> >> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);\n> >> +uint32_t estimateCCT(double red, double green, double blue);\n> >> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);\n> >> +double fromQFormat(unsigned int value, unsigned int n);\n> >> +\n> >> +} /* namespace ipa */\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> >> index e78cbcd6..ca4f3e28 100644\n> >> --- a/src/ipa/libipa/meson.build\n> >> +++ b/src/ipa/libipa/meson.build\n> >> @@ -6,6 +6,7 @@ libipa_headers = files([\n> >> 'camera_sensor_helper.h',\n> >> 'exposure_mode_helper.h',\n> >> 'fc_queue.h',\n> >> + 'helpers.h',\n> >> 'histogram.h',\n> >> 'interpolator.h',\n> >> 'lsc_polynomial.h',\n> >> @@ -21,6 +22,7 @@ libipa_sources = files([\n> >> 'camera_sensor_helper.cpp',\n> >> 'exposure_mode_helper.cpp',\n> >> 'fc_queue.cpp',\n> >> + 'helpers.cpp',\n> >> 'histogram.cpp',\n> >> 'interpolator.cpp',\n> >> 'lsc_polynomial.cpp',\n> >> --\n> >> 2.30.2\n> >>","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 CC072BDB13\n\tfor ;\n\tWed, 6 Nov 2024 13:56:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 230F86542B;\n\tWed, 6 Nov 2024 14:56:52 +0100 (CET)","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 6E733653C5\n\tfor ;\n\tWed, 6 Nov 2024 14:56:50 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0958A475;\n\tWed, 6 Nov 2024 14:56:42 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tizuEd2W\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730901402;\n\tbh=x42jOrckWXRfqx70tyfaRsgzuJVM2wqtfULyQ/R+9Oo=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=tizuEd2Wqfl+r8//JJ31U3B2Fpj7OKDVG4cFqXKlIvtvQ6amv1i5ardXPi7qSzL7z\n\tVxb4TBxeDGboqrmLNyHkHHma+rmFyYMQa2eMXCmKL7tW1sPXkfQIhJwThlHAvQhJ5x\n\tmRPAaMbEYtHtq9k2TUGslTaTcxqEeyodQfRfj9KQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20241031160741.253855-1-dan.scally@ideasonboard.com>\n\t<20241031160741.253855-2-dan.scally@ideasonboard.com>\n\t\n\t","Subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","To":"Dan Scally ,\n\tJacopo Mondi ","Date":"Wed, 06 Nov 2024 13:56:47 +0000","Message-ID":"<173090140770.3147895.6548924488673408003@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}},{"id":32054,"web_url":"https://patchwork.libcamera.org/comment/32054/","msgid":"<73f81282-4e62-4075-8581-760298897c91@ideasonboard.com>","date":"2024-11-06T14:01:25","subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Kiean\n\nOn 06/11/2024 13:56, Kieran Bingham wrote:\n> Quoting Dan Scally (2024-11-06 09:39:09)\n>> Hi Jacopo - thanks for the review\n>>\n>> On 04/11/2024 11:07, Jacopo Mondi wrote:\n>>> Hi Dan\n>>>\n>>> On Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:\n>>>> We start to have functions that are effectively identical crop up\n>>>> across the IPA modules. Add a file allowing those to be centralised\n>>>> within libipa so that a single implementation can be used in all of\n>>>> the IPAs.\n>>>>\n>>>> Signed-off-by: Daniel Scally \n>>>> ---\n>>>> src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++\n>>>> src/ipa/libipa/helpers.h | 23 +++++++++\n>>>> src/ipa/libipa/meson.build | 2 +\n>>>> 3 files changed, 128 insertions(+)\n>>>> create mode 100644 src/ipa/libipa/helpers.cpp\n>>>> create mode 100644 src/ipa/libipa/helpers.h\n>>>>\n>>>> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp\n>>>> new file mode 100644\n>>>> index 00000000..cc0cd586\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/libipa/helpers.cpp\n>>> Not a huge fan of a collection of C functions, but I guess\n>>> alternatives could be considered over-design\n>> That was my thinking basically.\n> I agree here. I'm fine moving these to a common location to get started.\n>\n> If things grow - or become more specific then they could be separated to\n> a defined structure, but that could also be done on top ... I have one\n> example below for that...\n>\n>\n>>>> @@ -0,0 +1,103 @@\n>>>> +/* SPDX-License-Identifier: BSD-2-Clause */\n>>>> +/*\n>>>> + * Copyright (C) 2024, Ideas on Board Oy\n>>>> + *\n>>>> + * libipa miscellaneous helpers\n>>>> + */\n>>>> +\n>>>> +#include \"helpers.h\"\n>>>> +\n>>>> +#include \n>>>> +#include \n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +namespace ipa {\n>>>> +\n>>>> +/**\n>>>> + * \\file helpers.h\n>>>> + * \\brief Functions to reduce code duplication between IPA modules\n>>>> + */\n>>>> +\n>>>> +/**\n>>>> + * \\brief Estimate luminance from RGB values following ITU-R BT.601\n>>>> + * \\param[in] r The red value\n>>>> + * \\param[in] g The green value\n>>>> + * \\param[in] b The blue valye\n> s/valye/value/\n>\n>>>> + * \\return The estimated luminance value\n>>> \\return statements usually go after the longer function documentation\n>>\n>> Ack\n>>\n>>>\n>>>> + *\n>>>> + * This function estimates a luminance value from a triplet of Red, Green and\n>>>> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7\n>>>> + * which can be found at https://www.itu.int/rec/R-REC-BT.601\n>>>> + */\n>>>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)\n>>> In example, in the RPi IPA, the return value is assigned to a double,\n>>> and the calculation suggests it might be worth not casing the result to\n>>> unsigned int.\n>> You're right, that's wrong. Thanks.\n>>>> +{\n>>>> + return (r * .299) + (g * .587) + (b * .114);\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Estimate correlated colour temperature from RGB color space input\n>>>> + * \\param[in] red The input red value\n>>>> + * \\param[in] green The input green value\n>>>> + * \\param[in] blue The input blue value\n>>>> + * \\return The estimated color temperature\n>>> same here\n>>>\n>>>> + *\n>>>> + * This function estimates the correlated color temperature RGB color space\n>>>> + * input. In physics and color science, the Planckian locus or black body locus\n>>>> + * is the path or locus that the color of an incandescent black body would take\n>>>> + * in a particular chromaticity space as the blackbody temperature changes.\n>>>> + *\n>>>> + * If a narrow range of color temperatures is considered (those encapsulating\n>>>> + * daylight being the most practical case) one can approximate the Planckian\n>>>> + * locus in order to calculate the CCT in terms of chromaticity coordinates.\n>>>> + *\n>>>> + * More detailed information can be found in:\n>>>> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation\n>>>> + */\n>>>> +uint32_t estimateCCT(double red, double green, double blue)\n>>>> +{\n>>>> + /* Convert the RGB values to CIE tristimulus values (XYZ) */\n>>>> + double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);\n>>>> + double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);\n>>>> + double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);\n>>>> +\n>>>> + /* Calculate the normalized chromaticity values */\n>>>> + double x = X / (X + Y + Z);\n>>>> + double y = Y / (X + Y + Z);\n>>>> +\n>>>> + /* Calculate CCT */\n>>>> + double n = (x - 0.3320) / (0.1858 - y);\n>>>> + return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;\n>>>> +}\n>>> This seems to come straight from the IPU3 IPA, so ack to the text and\n>>> implementation\n> Perhaps those are both helpers for 'colorspace' so maybe this is a\n> colorspace.cpp ... but lets see...\nYeah but then we get lots of little bitty files  - that seems worse to me.\n>\n>>>> +\n>>>> +/**\n>>>> + * \\brief Convert double to Q(m.n) format\n>>> Note these two helpers seems to be un-used.\n>> Ah yes, they're used in the C55 series which will be based on top of this...I can introduce them\n>> there if that's better?\n>>> What Q formats are we referring to ? Wikipedia lists two (TI version\n>>> and ARM version).\n>>> https://en.wikipedia.org/wiki/Q_(number_format)\n>>\n>> So far I'm only using numbers specified in documentation as being unsigned, and so it's the TI\n>> version, but the \"UQ\" format rather than just \"Q\"...possibly renaming the function\n>> \"toUnsignedQFormat()\" would be clearer?\n>>\n>>>> + * \\param[in] value The value to convert\n>>>> + * \\param[in] m The number of bits used to represent the integer part\n>>>> + * \\param[in] n The number of bits used to represent the fraction part\n>>>> + *\n>>>> + * This function converts a double into an unsigned Q format, clamping at the\n>>>> + * largest possible value given m and n.\n>>> \\return ?\n>>>\n>>>> + */\n>>>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)\n>>>> +{\n>>>> + double maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);\n>>>> +\n>>>> + return std::clamp(value, 0.0, maxVal) * std::pow(2, n);\n>>>> +}\n>>>> +\n>>>> +/**\n>>>> + * \\brief Convert Q(m.n) formatted number to double\n>>>> + * \\param[in] value The value to convert\n>>>> + * \\param[in] n The number of bits used to represent the fraction part\n>>>> + *\n>>>> + * This function converts an unsigned Q formatted value into a double.\n>>>> + */\n>>>> +double fromQFormat(unsigned int value, unsigned int n)\n>>>> +{\n>>>> + return value / std::pow(2, n);\n>>>> +}\n>>> Let's discuss these once the above is clarified\n> Q(a.b) formats come up a lot. I'd always thought we need a FixedPoint\n> (templated?) class to help with this so we can consider it as a real\n> type.\n>\n> If so - it would be it's own class, and warrant a set of unit tests...\n>\n> So if you did want to split this helpers.cpp,h up - I'd move the color\n> space functionality to colorspace.cpp and Q helpers to fixedpoint.cpp\n> ...\n>\n> Ultimately it depends how far you want to take the cleanup...\n\n\nWell, where else have they come up that we have the conversion open-coded at the moment? It seems a \nbit heavy handed for just the above, but if there're more places that could be tidied up perhaps \nit'd be worthwhile\n\n>\n>\n>\n>>>> +\n>>>> +} /* namespace ipa */\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h\n>>>> new file mode 100644\n>>>> index 00000000..361b63bd\n>>>> --- /dev/null\n>>>> +++ b/src/ipa/libipa/helpers.h\n>>>> @@ -0,0 +1,23 @@\n>>>> +/* SPDX-License-Identifier: BSD-2-Clause */\n>>>> +/*\n>>>> + * Copyright (C) 2024, Ideas on Board Oy\n>>>> + *\n>>>> + * libipa miscellaneous helpers\n>>>> + */\n>>>> +\n>>>> +#pragma once\n>>>> +\n>>>> +#include \n>>>> +\n>>>> +namespace libcamera {\n>>>> +\n>>>> +namespace ipa {\n>>>> +\n>>>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);\n>>>> +uint32_t estimateCCT(double red, double green, double blue);\n>>>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);\n>>>> +double fromQFormat(unsigned int value, unsigned int n);\n>>>> +\n>>>> +} /* namespace ipa */\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n>>>> index e78cbcd6..ca4f3e28 100644\n>>>> --- a/src/ipa/libipa/meson.build\n>>>> +++ b/src/ipa/libipa/meson.build\n>>>> @@ -6,6 +6,7 @@ libipa_headers = files([\n>>>> 'camera_sensor_helper.h',\n>>>> 'exposure_mode_helper.h',\n>>>> 'fc_queue.h',\n>>>> + 'helpers.h',\n>>>> 'histogram.h',\n>>>> 'interpolator.h',\n>>>> 'lsc_polynomial.h',\n>>>> @@ -21,6 +22,7 @@ libipa_sources = files([\n>>>> 'camera_sensor_helper.cpp',\n>>>> 'exposure_mode_helper.cpp',\n>>>> 'fc_queue.cpp',\n>>>> + 'helpers.cpp',\n>>>> 'histogram.cpp',\n>>>> 'interpolator.cpp',\n>>>> 'lsc_polynomial.cpp',\n>>>> --\n>>>> 2.30.2\n>>>>","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 18941BDB13\n\tfor ;\n\tWed, 6 Nov 2024 14:01:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0ABEF65430;\n\tWed, 6 Nov 2024 15:01:30 +0100 (CET)","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 31B5E65429\n\tfor ;\n\tWed, 6 Nov 2024 15:01:28 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7E137A44;\n\tWed, 6 Nov 2024 15:01:19 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"DM82HdJQ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730901679;\n\tbh=yaa+u24zm4EKf3At/EJJLq8APpe/MotuD+wxG+gBdWc=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=DM82HdJQMYcldz8sHw7QljN47Dhf9DsXDx0v2rcPOt5GUgGUmGZ/ucI966DXzC/Qc\n\tC11QSuGGL2mYksUVRsz4KAnNyCEtbLY7hGC5lHCUaHIfwplUg4k2ep0f7cvk2xRj2i\n\tIDZUANp30/ZfHIHWT7X5FWBhix5u+h/NPvgBCIPY=","Message-ID":"<73f81282-4e62-4075-8581-760298897c91@ideasonboard.com>","Date":"Wed, 6 Nov 2024 14:01:25 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","To":"Kieran Bingham ,\n\tJacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","References":"<20241031160741.253855-1-dan.scally@ideasonboard.com>\n\t<20241031160741.253855-2-dan.scally@ideasonboard.com>\n\t\n\t\n\t<173090140770.3147895.6548924488673408003@ping.linuxembedded.co.uk>","Content-Language":"en-US","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"<173090140770.3147895.6548924488673408003@ping.linuxembedded.co.uk>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"8bit","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\" "}},{"id":32056,"web_url":"https://patchwork.libcamera.org/comment/32056/","msgid":"<173090218913.3147895.7075722405527565505@ping.linuxembedded.co.uk>","date":"2024-11-06T14:09:49","subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Dan Scally (2024-11-06 14:01:25)\n> Hi Kiean\n> \n> On 06/11/2024 13:56, Kieran Bingham wrote:\n> > Quoting Dan Scally (2024-11-06 09:39:09)\n> >> Hi Jacopo - thanks for the review\n> >>\n> >> On 04/11/2024 11:07, Jacopo Mondi wrote:\n> >>> Hi Dan\n> >>>\n> >>> On Thu, Oct 31, 2024 at 04:07:36PM +0000, Daniel Scally wrote:\n> >>>> We start to have functions that are effectively identical crop up\n> >>>> across the IPA modules. Add a file allowing those to be centralised\n> >>>> within libipa so that a single implementation can be used in all of\n> >>>> the IPAs.\n> >>>>\n> >>>> Signed-off-by: Daniel Scally \n> >>>> ---\n> >>>> src/ipa/libipa/helpers.cpp | 103 +++++++++++++++++++++++++++++++++++++\n> >>>> src/ipa/libipa/helpers.h | 23 +++++++++\n> >>>> src/ipa/libipa/meson.build | 2 +\n> >>>> 3 files changed, 128 insertions(+)\n> >>>> create mode 100644 src/ipa/libipa/helpers.cpp\n> >>>> create mode 100644 src/ipa/libipa/helpers.h\n> >>>>\n> >>>> diff --git a/src/ipa/libipa/helpers.cpp b/src/ipa/libipa/helpers.cpp\n> >>>> new file mode 100644\n> >>>> index 00000000..cc0cd586\n> >>>> --- /dev/null\n> >>>> +++ b/src/ipa/libipa/helpers.cpp\n> >>> Not a huge fan of a collection of C functions, but I guess\n> >>> alternatives could be considered over-design\n> >> That was my thinking basically.\n> > I agree here. I'm fine moving these to a common location to get started.\n> >\n> > If things grow - or become more specific then they could be separated to\n> > a defined structure, but that could also be done on top ... I have one\n> > example below for that...\n> >\n> >\n> >>>> @@ -0,0 +1,103 @@\n> >>>> +/* SPDX-License-Identifier: BSD-2-Clause */\n> >>>> +/*\n> >>>> + * Copyright (C) 2024, Ideas on Board Oy\n> >>>> + *\n> >>>> + * libipa miscellaneous helpers\n> >>>> + */\n> >>>> +\n> >>>> +#include \"helpers.h\"\n> >>>> +\n> >>>> +#include \n> >>>> +#include \n> >>>> +\n> >>>> +namespace libcamera {\n> >>>> +\n> >>>> +namespace ipa {\n> >>>> +\n> >>>> +/**\n> >>>> + * \\file helpers.h\n> >>>> + * \\brief Functions to reduce code duplication between IPA modules\n> >>>> + */\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Estimate luminance from RGB values following ITU-R BT.601\n> >>>> + * \\param[in] r The red value\n> >>>> + * \\param[in] g The green value\n> >>>> + * \\param[in] b The blue valye\n> > s/valye/value/\n> >\n> >>>> + * \\return The estimated luminance value\n> >>> \\return statements usually go after the longer function documentation\n> >>\n> >> Ack\n> >>\n> >>>\n> >>>> + *\n> >>>> + * This function estimates a luminance value from a triplet of Red, Green and\n> >>>> + * Blue values, following the formula defined by ITU-R Recommendation BT.601-7\n> >>>> + * which can be found at https://www.itu.int/rec/R-REC-BT.601\n> >>>> + */\n> >>>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b)\n> >>> In example, in the RPi IPA, the return value is assigned to a double,\n> >>> and the calculation suggests it might be worth not casing the result to\n> >>> unsigned int.\n> >> You're right, that's wrong. Thanks.\n> >>>> +{\n> >>>> + return (r * .299) + (g * .587) + (b * .114);\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Estimate correlated colour temperature from RGB color space input\n> >>>> + * \\param[in] red The input red value\n> >>>> + * \\param[in] green The input green value\n> >>>> + * \\param[in] blue The input blue value\n> >>>> + * \\return The estimated color temperature\n> >>> same here\n> >>>\n> >>>> + *\n> >>>> + * This function estimates the correlated color temperature RGB color space\n> >>>> + * input. In physics and color science, the Planckian locus or black body locus\n> >>>> + * is the path or locus that the color of an incandescent black body would take\n> >>>> + * in a particular chromaticity space as the blackbody temperature changes.\n> >>>> + *\n> >>>> + * If a narrow range of color temperatures is considered (those encapsulating\n> >>>> + * daylight being the most practical case) one can approximate the Planckian\n> >>>> + * locus in order to calculate the CCT in terms of chromaticity coordinates.\n> >>>> + *\n> >>>> + * More detailed information can be found in:\n> >>>> + * https://en.wikipedia.org/wiki/Color_temperature#Approximation\n> >>>> + */\n> >>>> +uint32_t estimateCCT(double red, double green, double blue)\n> >>>> +{\n> >>>> + /* Convert the RGB values to CIE tristimulus values (XYZ) */\n> >>>> + double X = (-0.14282) * (red) + (1.54924) * (green) + (-0.95641) * (blue);\n> >>>> + double Y = (-0.32466) * (red) + (1.57837) * (green) + (-0.73191) * (blue);\n> >>>> + double Z = (-0.68202) * (red) + (0.77073) * (green) + (0.56332) * (blue);\n> >>>> +\n> >>>> + /* Calculate the normalized chromaticity values */\n> >>>> + double x = X / (X + Y + Z);\n> >>>> + double y = Y / (X + Y + Z);\n> >>>> +\n> >>>> + /* Calculate CCT */\n> >>>> + double n = (x - 0.3320) / (0.1858 - y);\n> >>>> + return 449 * n * n * n + 3525 * n * n + 6823.3 * n + 5520.33;\n> >>>> +}\n> >>> This seems to come straight from the IPU3 IPA, so ack to the text and\n> >>> implementation\n> > Perhaps those are both helpers for 'colorspace' so maybe this is a\n> > colorspace.cpp ... but lets see...\n> Yeah but then we get lots of little bitty files  - that seems worse to me.\n> >\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Convert double to Q(m.n) format\n> >>> Note these two helpers seems to be un-used.\n> >> Ah yes, they're used in the C55 series which will be based on top of this...I can introduce them\n> >> there if that's better?\n> >>> What Q formats are we referring to ? Wikipedia lists two (TI version\n> >>> and ARM version).\n> >>> https://en.wikipedia.org/wiki/Q_(number_format)\n> >>\n> >> So far I'm only using numbers specified in documentation as being unsigned, and so it's the TI\n> >> version, but the \"UQ\" format rather than just \"Q\"...possibly renaming the function\n> >> \"toUnsignedQFormat()\" would be clearer?\n> >>\n> >>>> + * \\param[in] value The value to convert\n> >>>> + * \\param[in] m The number of bits used to represent the integer part\n> >>>> + * \\param[in] n The number of bits used to represent the fraction part\n> >>>> + *\n> >>>> + * This function converts a double into an unsigned Q format, clamping at the\n> >>>> + * largest possible value given m and n.\n> >>> \\return ?\n> >>>\n> >>>> + */\n> >>>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n)\n> >>>> +{\n> >>>> + double maxVal = (std::pow(2, m + n) - 1) / std::pow(2, n);\n> >>>> +\n> >>>> + return std::clamp(value, 0.0, maxVal) * std::pow(2, n);\n> >>>> +}\n> >>>> +\n> >>>> +/**\n> >>>> + * \\brief Convert Q(m.n) formatted number to double\n> >>>> + * \\param[in] value The value to convert\n> >>>> + * \\param[in] n The number of bits used to represent the fraction part\n> >>>> + *\n> >>>> + * This function converts an unsigned Q formatted value into a double.\n> >>>> + */\n> >>>> +double fromQFormat(unsigned int value, unsigned int n)\n> >>>> +{\n> >>>> + return value / std::pow(2, n);\n> >>>> +}\n> >>> Let's discuss these once the above is clarified\n> > Q(a.b) formats come up a lot. I'd always thought we need a FixedPoint\n> > (templated?) class to help with this so we can consider it as a real\n> > type.\n> >\n> > If so - it would be it's own class, and warrant a set of unit tests...\n> >\n> > So if you did want to split this helpers.cpp,h up - I'd move the color\n> > space functionality to colorspace.cpp and Q helpers to fixedpoint.cpp\n> > ...\n> >\n> > Ultimately it depends how far you want to take the cleanup...\n> \n> \n> Well, where else have they come up that we have the conversion open-coded at the moment? It seems a \n> bit heavy handed for just the above, but if there're more places that could be tidied up perhaps \n> it'd be worthwhile\n\nGood question, I don't know off the top of my head. My first though\nwould have been - C55 ... But in fact, I think they're used in dewarper\non i.MX8MP too.\n\nIt seems like the sort of thing we'd need a lot - but I'm absolutely\nfine keeping this as is for now until there are more clear users.\n\n--\nKieran\n\n\n> \n> >\n> >\n> >\n> >>>> +\n> >>>> +} /* namespace ipa */\n> >>>> +\n> >>>> +} /* namespace libcamera */\n> >>>> diff --git a/src/ipa/libipa/helpers.h b/src/ipa/libipa/helpers.h\n> >>>> new file mode 100644\n> >>>> index 00000000..361b63bd\n> >>>> --- /dev/null\n> >>>> +++ b/src/ipa/libipa/helpers.h\n> >>>> @@ -0,0 +1,23 @@\n> >>>> +/* SPDX-License-Identifier: BSD-2-Clause */\n> >>>> +/*\n> >>>> + * Copyright (C) 2024, Ideas on Board Oy\n> >>>> + *\n> >>>> + * libipa miscellaneous helpers\n> >>>> + */\n> >>>> +\n> >>>> +#pragma once\n> >>>> +\n> >>>> +#include \n> >>>> +\n> >>>> +namespace libcamera {\n> >>>> +\n> >>>> +namespace ipa {\n> >>>> +\n> >>>> +unsigned int rec601LuminanceFromRGB(unsigned int r, unsigned int g, unsigned int b);\n> >>>> +uint32_t estimateCCT(double red, double green, double blue);\n> >>>> +unsigned int toQFormat(double value, unsigned int m, unsigned int n);\n> >>>> +double fromQFormat(unsigned int value, unsigned int n);\n> >>>> +\n> >>>> +} /* namespace ipa */\n> >>>> +\n> >>>> +} /* namespace libcamera */\n> >>>> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build\n> >>>> index e78cbcd6..ca4f3e28 100644\n> >>>> --- a/src/ipa/libipa/meson.build\n> >>>> +++ b/src/ipa/libipa/meson.build\n> >>>> @@ -6,6 +6,7 @@ libipa_headers = files([\n> >>>> 'camera_sensor_helper.h',\n> >>>> 'exposure_mode_helper.h',\n> >>>> 'fc_queue.h',\n> >>>> + 'helpers.h',\n> >>>> 'histogram.h',\n> >>>> 'interpolator.h',\n> >>>> 'lsc_polynomial.h',\n> >>>> @@ -21,6 +22,7 @@ libipa_sources = files([\n> >>>> 'camera_sensor_helper.cpp',\n> >>>> 'exposure_mode_helper.cpp',\n> >>>> 'fc_queue.cpp',\n> >>>> + 'helpers.cpp',\n> >>>> 'histogram.cpp',\n> >>>> 'interpolator.cpp',\n> >>>> 'lsc_polynomial.cpp',\n> >>>> --\n> >>>> 2.30.2\n> >>>>","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 7D61CBDB13\n\tfor ;\n\tWed, 6 Nov 2024 14:09:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 86C2E65430;\n\tWed, 6 Nov 2024 15:09:54 +0100 (CET)","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 213DA65429\n\tfor ;\n\tWed, 6 Nov 2024 15:09:52 +0100 (CET)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 97B5359D;\n\tWed, 6 Nov 2024 15:09:43 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"ZYOhPp95\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730902183;\n\tbh=PpN/xRDMxDiCEIJuFV30pa0mFTkEYqNJPFgtMnG7JwE=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ZYOhPp95cm3LUN3FmsX2VD2YYph7C2uWZRO4VKo867H/yoMpKpGGgCQvullWuXttt\n\t8eF2k8GdP3xz7mg9WiyjZZ2tG7fGPbyl5v03V082gayCQMSHQevvfjhuj67yjCoQWH\n\tzulXdn9e44WgYq0qiY7nGXO5LO3GI9lY6J7/e9DM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<73f81282-4e62-4075-8581-760298897c91@ideasonboard.com>","References":"<20241031160741.253855-1-dan.scally@ideasonboard.com>\n\t<20241031160741.253855-2-dan.scally@ideasonboard.com>\n\t\n\t\n\t<173090140770.3147895.6548924488673408003@ping.linuxembedded.co.uk>\n\t<73f81282-4e62-4075-8581-760298897c91@ideasonboard.com>","Subject":"Re: [PATCH 1/6] ipa: libipa: Add miscellaneous helpers","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org, mike.rudenko@gmail.com","To":"Dan Scally ,\n\tJacopo Mondi ","Date":"Wed, 06 Nov 2024 14:09:49 +0000","Message-ID":"<173090218913.3147895.7075722405527565505@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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\" "}}]