[{"id":29326,"web_url":"https://patchwork.libcamera.org/comment/29326/","msgid":"<20240424075108.GF2134@pendragon.ideasonboard.com>","date":"2024-04-24T07:51:08","subject":"Re: [PATCH v2] libcamera: utils: Add a helper to convert\n\tfloating-point to fixed-point","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Wed, Apr 24, 2024 at 04:44:09PM +0900, Paul Elder wrote:\n> Add helper functions for converting between floating point and fixed\n> point numbers. Also add tests for them.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v2:\n> - added the reverse conversion function (fixed -> floating)\n> - added tests\n> - make the conversion code cleaner\n> ---\n> include/libcamera/base/utils.h | 37 +++++++++++++++++++++++++\n> src/libcamera/base/utils.cpp | 20 ++++++++++++++\n> test/utils.cpp | 49 ++++++++++++++++++++++++++++++++++\n\nI envision more math-related helpers in the future. Could we gather\nthose in a new include/libcamera/base/math.h header ?\n\n> 3 files changed, 106 insertions(+)\n> \n> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> index 37d9af60..da0767e3 100644\n> --- a/include/libcamera/base/utils.h\n> +++ b/include/libcamera/base/utils.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -369,6 +370,42 @@ decltype(auto) abs_diff(const T &a, const T &b)\n> \n> double strtod(const char *__restrict nptr, char **__restrict endptr);\n> \n> +#ifndef __DOXYGEN__\n> +template +\t std::enable_if_t &&\n> +\t\t\t std::is_floating_point_v> * = nullptr>\n> +#else\n> +template\n> +#endif\n> +constexpr R floatingToFixedPoint(T number)\n> +{\n> +\tstatic_assert(I + F <= sizeof(R) * 8);\n> +\n> +\tR maskI = (1 << I) - 1;\n> +\tR whole = (static_cast(number) & maskI) << F;\n> +\n> +\tR maskF = (1 << F) - 1;\n> +\tR frac = static_cast(std::round(number * (1 << F))) & maskF;\n> +\n> +\treturn whole | frac;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template +\t std::enable_if_t &&\n> +\t\t\t std::is_integral_v> * = nullptr>\n> +#else\n> +template\n> +#endif\n> +constexpr R fixedToFloatingPoint(T number)\n> +{\n> +\tstatic_assert(I + F <= sizeof(T) * 8);\n> +\n> +\tint coeff = number >> (I + F - 1) ? -1 : 1;\n> +\n> +\treturn coeff * static_cast(number) / static_cast(1 << F);\n> +}\n> +\n> } /* namespace utils */\n> \n> #ifndef __DOXYGEN__\n> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> index 3b73b442..ba36026d 100644\n> --- a/src/libcamera/base/utils.cpp\n> +++ b/src/libcamera/base/utils.cpp\n> @@ -521,6 +521,26 @@ double strtod(const char *__restrict nptr, char **__restrict endptr)\n> #endif\n> }\n> \n> +/**\n> + * \\fn R floatingToFixedPoint(T number)\n> + * \\brief Convert a floating point number to a fixed-point representation\n> + * \\tparam I Bit width of the integer part of the fixed-point\n> + * \\tparam F Bit width of the fractional part of the fixed-point\n> + * \\tparam R Return type of the fixed-point representation\n> + * \\tparam T Input type of the floating point representation\n> + * \\return The converted value\n> + */\n> +\n> +/**\n> + * \\fn R fixedToFloatingPoint(T number)\n> + * \\brief Convert a fixed-point number to a floating point representation\n> + * \\tparam I Bit width of the integer part of the fixed-point\n> + * \\tparam F Bit width of the fractional part of the fixed-point\n> + * \\tparam R Return type of the floating point representation\n> + * \\tparam T Input type of the fixed-point representation\n> + * \\return The converted value\n> + */\n> +\n> } /* namespace utils */\n> \n> #ifndef __DOXYGEN__\n> diff --git a/test/utils.cpp b/test/utils.cpp\n> index fc56e14e..f1805207 100644\n> --- a/test/utils.cpp\n> +++ b/test/utils.cpp\n> @@ -170,6 +170,51 @@ protected:\n> \t\treturn TestPass;\n> \t}\n> \n> +\ttemplate\n> +\tint testSingleFixedPoint(double input, T expected)\n> +\t{\n> +\t\tT ret = utils::floatingToFixedPoint(input);\n> +\t\tif (ret != expected) {\n> +\t\t\tcerr << \"Expected \" << input << \" to convert to \"\n> +\t\t\t << expected << \", got \" << ret << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * The precision check is fairly arbitrary but is based on what\n> +\t\t * the rkisp1 is capable of in the crosstalk module.\n> +\t\t */\n> +\t\tdouble f = utils::fixedToFloatingPoint(ret);\n> +\t\tif (std::abs(f - input) > 0.001) {\n> +\t\t\tcerr << \"Reverse conversion expected \" << ret\n> +\t\t\t << \" to convert to \" << input\n> +\t\t\t << \", got \" << f << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint testFixedPoint()\n> +\t{\n> +\t\t/* These are the only cases that we know for certain */\n> +\t\tstd::map testCases = {\n> +\t\t\t{ 7.992, 0x3FF },\n> +\t\t\t{ -8, 0x400 },\n> +\t\t\t{ 0, 0 },\n> +\t\t};\n> +\n> +\t\tint ret;\n> +\t\tfor (const auto &testCase : testCases) {\n> +\t\t\tret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> +\t\t\t\t\t\t\t\t testCase.second);\n> +\t\t\tif (ret != TestPass)\n> +\t\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> \tint run()\n> \t{\n> \t\t/* utils::hex() test. */\n> @@ -290,6 +335,10 @@ protected:\n> \t\tif (testDuration() != TestPass)\n> \t\t\treturn TestFail;\n> \n> +\t\t/* fixed point conversion test */\n> +\t\tif (testFixedPoint() != TestPass)\n> +\t\t\treturn TestFail;\n> +\n> \t\treturn TestPass;\n> \t}\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 D1D79BE08B\n\tfor ;\n\tWed, 24 Apr 2024 07:51:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EFB9C61B14;\n\tWed, 24 Apr 2024 09:51:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DEF9861B14\n\tfor ;\n\tWed, 24 Apr 2024 09:51:14 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D8573B1;\n\tWed, 24 Apr 2024 09:50:22 +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=\"Ksqw/gqi\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713945023;\n\tbh=pM9ETAstofqQGFeAuX7FIpXyl6kgy53dyNk02EV3AUM=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ksqw/gqiQVJGHWOELHAiXCWDX7wYIWtbJ8cRSt+NVe1e/Hq7Z1xFAEKr6n3IqxH3p\n\tbyWx+jo6C0vDsSsobX/KhEPAeAhna6pz0d5qbeCtAbAuhhr1wLF8JoQJbXlIsxRjT5\n\tgjAXgn637YewQpIZgnbIjdN7b1eWsD43JKarP+mo=","Date":"Wed, 24 Apr 2024 10:51:08 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] libcamera: utils: Add a helper to convert\n\tfloating-point to fixed-point","Message-ID":"<20240424075108.GF2134@pendragon.ideasonboard.com>","References":"<20240424074409.1275425-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240424074409.1275425-1-paul.elder@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":29338,"web_url":"https://patchwork.libcamera.org/comment/29338/","msgid":"<20240424181730.ytdtky6d7a4g77bw@jasper>","date":"2024-04-24T18:17:30","subject":"Re: [PATCH v2] libcamera: utils: Add a helper to convert\n\tfloating-point to fixed-point","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nthanks for the patch.\n\nOn Wed, Apr 24, 2024 at 04:44:09PM +0900, Paul Elder wrote:\n> Add helper functions for converting between floating point and fixed\n> point numbers. Also add tests for them.\n> \n> Signed-off-by: Paul Elder \n> ---\n> Changes in v2:\n> - added the reverse conversion function (fixed -> floating)\n> - added tests\n> - make the conversion code cleaner\n> ---\n> include/libcamera/base/utils.h | 37 +++++++++++++++++++++++++\n> src/libcamera/base/utils.cpp | 20 ++++++++++++++\n> test/utils.cpp | 49 ++++++++++++++++++++++++++++++++++\n> 3 files changed, 106 insertions(+)\n> \n> diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> index 37d9af60..da0767e3 100644\n> --- a/include/libcamera/base/utils.h\n> +++ b/include/libcamera/base/utils.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -369,6 +370,42 @@ decltype(auto) abs_diff(const T &a, const T &b)\n> \n> double strtod(const char *__restrict nptr, char **__restrict endptr);\n> \n> +#ifndef __DOXYGEN__\n> +template +\t std::enable_if_t &&\n> +\t\t\t std::is_floating_point_v> * = nullptr>\n> +#else\n> +template\n> +#endif\n> +constexpr R floatingToFixedPoint(T number)\n> +{\n> +\tstatic_assert(I + F <= sizeof(R) * 8);\n> +\n> +\tR maskI = (1 << I) - 1;\n> +\tR whole = (static_cast(number) & maskI) << F;\n> +\n> +\tR maskF = (1 << F) - 1;\n> +\tR frac = static_cast(std::round(number * (1 << F))) & maskF;\n> +\n> +\treturn whole | frac;\n> +}\n> +\n> +#ifndef __DOXYGEN__\n> +template +\t std::enable_if_t &&\n> +\t\t\t std::is_integral_v> * = nullptr>\n> +#else\n> +template\n> +#endif\n> +constexpr R fixedToFloatingPoint(T number)\n> +{\n> +\tstatic_assert(I + F <= sizeof(T) * 8);\n> +\n> +\tint coeff = number >> (I + F - 1) ? -1 : 1;\n> +\n> +\treturn coeff * static_cast(number) / static_cast(1 << F);\n> +}\n> +\n> } /* namespace utils */\n> \n> #ifndef __DOXYGEN__\n> diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> index 3b73b442..ba36026d 100644\n> --- a/src/libcamera/base/utils.cpp\n> +++ b/src/libcamera/base/utils.cpp\n> @@ -521,6 +521,26 @@ double strtod(const char *__restrict nptr, char **__restrict endptr)\n> #endif\n> }\n> \n> +/**\n> + * \\fn R floatingToFixedPoint(T number)\n> + * \\brief Convert a floating point number to a fixed-point representation\n> + * \\tparam I Bit width of the integer part of the fixed-point\n> + * \\tparam F Bit width of the fractional part of the fixed-point\n> + * \\tparam R Return type of the fixed-point representation\n> + * \\tparam T Input type of the floating point representation\n> + * \\return The converted value\n> + */\n> +\n> +/**\n> + * \\fn R fixedToFloatingPoint(T number)\n> + * \\brief Convert a fixed-point number to a floating point representation\n> + * \\tparam I Bit width of the integer part of the fixed-point\n> + * \\tparam F Bit width of the fractional part of the fixed-point\n> + * \\tparam R Return type of the floating point representation\n> + * \\tparam T Input type of the fixed-point representation\n> + * \\return The converted value\n> + */\n> +\n> } /* namespace utils */\n> \n> #ifndef __DOXYGEN__\n> diff --git a/test/utils.cpp b/test/utils.cpp\n> index fc56e14e..f1805207 100644\n> --- a/test/utils.cpp\n> +++ b/test/utils.cpp\n> @@ -170,6 +170,51 @@ protected:\n> \t\treturn TestPass;\n> \t}\n> \n> +\ttemplate\n> +\tint testSingleFixedPoint(double input, T expected)\n> +\t{\n> +\t\tT ret = utils::floatingToFixedPoint(input);\n> +\t\tif (ret != expected) {\n> +\t\t\tcerr << \"Expected \" << input << \" to convert to \"\n> +\t\t\t << expected << \", got \" << ret << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\t/*\n> +\t\t * The precision check is fairly arbitrary but is based on what\n> +\t\t * the rkisp1 is capable of in the crosstalk module.\n> +\t\t */\n> +\t\tdouble f = utils::fixedToFloatingPoint(ret);\n> +\t\tif (std::abs(f - input) > 0.001) {\n> +\t\t\tcerr << \"Reverse conversion expected \" << ret\n> +\t\t\t << \" to convert to \" << input\n> +\t\t\t << \", got \" << f << std::endl;\n> +\t\t\treturn TestFail;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> +\tint testFixedPoint()\n> +\t{\n> +\t\t/* These are the only cases that we know for certain */\n> +\t\tstd::map testCases = {\n> +\t\t\t{ 7.992, 0x3FF },\n> +\t\t\t{ -8, 0x400 },\n> +\t\t\t{ 0, 0 },\n> +\t\t};\n\nGreat to have a testcase :-) Unfortunately I still believe there is\nsomething not exactly right here. As the implementation differed from\nwhat I had in mind regarding fixed points I played a bit with it... I\nguess these testvalues where taken from the rkisp header. They all work\nwell with this implementation, but they miss the interesting point of\nnegative values (-0.4, -1.4) where two's complement gets interesting. In\nthat case the current implementation fails in the conversion back to\nfloat. I'm not sure if vsi implemented a special fixed point format but\nI actually don't expect that.\n\nI did a testprogram that shows the differences:\n\n---------- main.cpp\n#include \n#include \n#include \n#include \n#include \n#include \n#include \n#include \n#include \n#include \n\ntemplate &&\n\t\t\t std::is_floating_point_v> * = nullptr>\nconstexpr R floatingToFixedPoint(T number)\n{\n\tstatic_assert(I + F <= sizeof(R) * 8);\n\n\tR maskI = (1 << I) - 1;\n\tR whole = (static_cast(number) & maskI) << F;\n\n\tR maskF = (1 << F) - 1;\n\tR frac = static_cast(std::round(number * (1 << F))) & maskF;\n\n\treturn whole | frac;\n}\n\ntemplate &&\n\t\t\t std::is_floating_point_v> * = nullptr>\nconstexpr R floatingToFixedPoint2(T number)\n{\n\tstatic_assert(I + F <= sizeof(R) * 8);\n\n\tR mask = (1 << (F + I)) - 1;\n\tR frac = static_cast(std::round(number * (1 << (F)))) & mask;\n\n\treturn frac;\n}\n\n\ntemplate &&\n\t\t\t std::is_integral_v> * = nullptr>\nconstexpr R fixedToFloatingPoint(T number)\n{\n\tstatic_assert(I + F <= sizeof(T) * 8);\n\n\tint coeff = number >> (I + F - 1) ? -1 : 1;\n\n\treturn coeff * static_cast(number) / static_cast(1 << F);\n}\n\n\ntemplate &&\n\t\t\t std::is_integral_v> * = nullptr>\nconstexpr R fixedToFloatingPoint2(T number)\n{\n static_assert(sizeof(int) >= sizeof(T));\n\tstatic_assert(I + F <= sizeof(T) * 8);\n\n /* create a number with all upper bits set including the sign bit\n * of the fixed point number\n */\t\n\tint upper_ones = -1 << (I + F -1);\n\tif(number & upper_ones) {\n\t return static_cast( upper_ones | number ) / static_cast(1 << F);\n\t} else\n\t return static_cast( number) / static_cast(1 << F);\n}\n\nvoid doTest(double x) {\n uint16_t f = floatingToFixedPoint<4,7,uint16_t>(x);\n uint16_t f2 = floatingToFixedPoint2<4,7,uint16_t>(x);\n double b = fixedToFloatingPoint<4,7,double>(f);\n double b2 = fixedToFloatingPoint2<4,7,double>(f2);\n std::cout << std::hex << \"===== test \" << x << std::endl;\n std::cout << \"v1: \" << f << \"(\" << std::bitset<16>(f) << \") reverse: \" << b << std::endl;\n std::cout << \"v2: \" << f2 << \"(\" << std::bitset<16>(f2) << \") reverse: \" << b2 << std::endl;\n}\n\nint main()\n{\n doTest(-8.0);\n doTest(7.992);\n doTest(1.0);\n doTest(0.4);\n doTest(1.4);\n doTest(-0.4);\n doTest(-1.4);\n} \n\n--- end of main.cpp\n\n\nI couldn't come up with a simpler solution to handle the upper ones in\nthe fixed2FloatingPoint conversion. If we would have 16 or 32 bit wide\nfixed points that wouldn't be an issue. Maybe someone else has an idea\nhere.\n\nBest regards,\nStefan\n\n> +\n> +\t\tint ret;\n> +\t\tfor (const auto &testCase : testCases) {\n> +\t\t\tret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> +\t\t\t\t\t\t\t\t testCase.second);\n> +\t\t\tif (ret != TestPass)\n> +\t\t\t\treturn ret;\n> +\t\t}\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +\n> \tint run()\n> \t{\n> \t\t/* utils::hex() test. */\n> @@ -290,6 +335,10 @@ protected:\n> \t\tif (testDuration() != TestPass)\n> \t\t\treturn TestFail;\n> \n> +\t\t/* fixed point conversion test */\n> +\t\tif (testFixedPoint() != TestPass)\n> +\t\t\treturn TestFail;\n> +\n> \t\treturn TestPass;\n> \t}\n> };\n> -- \n> 2.39.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 3A503C3200\n\tfor ;\n\tWed, 24 Apr 2024 18:17:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2B677633F9;\n\tWed, 24 Apr 2024 20:17:35 +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 630A0633EB\n\tfor ;\n\tWed, 24 Apr 2024 20:17:33 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:7f38:2ed5:2bb1:eb70])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 34A2AB1;\n\tWed, 24 Apr 2024 20:16:41 +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=\"desf2IfD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1713982601;\n\tbh=NbPfvgFBgtNm2sSTR37FRL/wT3jiRQEbv4lDAf4CiuI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=desf2IfDyy9OnYh7hu4mVzi2w7nXda+NUfCkuxFgYe1URSKVKjCVqXKmW+WiwrX1T\n\tMM0JMv2ZU8NMrpnk3BhU4ydmaovXdbMivgvGignsRacq2VkXXPntDkERocKw1sA99c\n\twotS7j/rJzKt7V2zGwUqBpfJK+ziXyV60Zon/D9g=","Date":"Wed, 24 Apr 2024 20:17:30 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] libcamera: utils: Add a helper to convert\n\tfloating-point to fixed-point","Message-ID":"<20240424181730.ytdtky6d7a4g77bw@jasper>","References":"<20240424074409.1275425-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240424074409.1275425-1-paul.elder@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":29340,"web_url":"https://patchwork.libcamera.org/comment/29340/","msgid":"<20240425090910.lzgoh6hpovm4scgs@jasper>","date":"2024-04-25T09:09:10","subject":"Re: [PATCH v2] libcamera: utils: Add a helper to convert\n\tfloating-point to fixed-point","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Wed, Apr 24, 2024 at 08:17:30PM +0200, Stefan Klug wrote:\n> Hi Paul,\n> \n> thanks for the patch.\n> \n> On Wed, Apr 24, 2024 at 04:44:09PM +0900, Paul Elder wrote:\n> > Add helper functions for converting between floating point and fixed\n> > point numbers. Also add tests for them.\n> > \n> > Signed-off-by: Paul Elder \n> > ---\n> > Changes in v2:\n> > - added the reverse conversion function (fixed -> floating)\n> > - added tests\n> > - make the conversion code cleaner\n> > ---\n> > include/libcamera/base/utils.h | 37 +++++++++++++++++++++++++\n> > src/libcamera/base/utils.cpp | 20 ++++++++++++++\n> > test/utils.cpp | 49 ++++++++++++++++++++++++++++++++++\n> > 3 files changed, 106 insertions(+)\n> > \n> > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > index 37d9af60..da0767e3 100644\n> > --- a/include/libcamera/base/utils.h\n> > +++ b/include/libcamera/base/utils.h\n> > @@ -9,6 +9,7 @@\n> > \n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -369,6 +370,42 @@ decltype(auto) abs_diff(const T &a, const T &b)\n> > \n> > double strtod(const char *__restrict nptr, char **__restrict endptr);\n> > \n> > +#ifndef __DOXYGEN__\n> > +template > +\t std::enable_if_t &&\n> > +\t\t\t std::is_floating_point_v> * = nullptr>\n> > +#else\n> > +template\n> > +#endif\n> > +constexpr R floatingToFixedPoint(T number)\n> > +{\n> > +\tstatic_assert(I + F <= sizeof(R) * 8);\n> > +\n> > +\tR maskI = (1 << I) - 1;\n> > +\tR whole = (static_cast(number) & maskI) << F;\n> > +\n> > +\tR maskF = (1 << F) - 1;\n> > +\tR frac = static_cast(std::round(number * (1 << F))) & maskF;\n> > +\n> > +\treturn whole | frac;\n> > +}\n> > +\n> > +#ifndef __DOXYGEN__\n> > +template > +\t std::enable_if_t &&\n> > +\t\t\t std::is_integral_v> * = nullptr>\n> > +#else\n> > +template\n> > +#endif\n> > +constexpr R fixedToFloatingPoint(T number)\n> > +{\n> > +\tstatic_assert(I + F <= sizeof(T) * 8);\n> > +\n> > +\tint coeff = number >> (I + F - 1) ? -1 : 1;\n> > +\n> > +\treturn coeff * static_cast(number) / static_cast(1 << F);\n> > +}\n> > +\n> > } /* namespace utils */\n> > \n> > #ifndef __DOXYGEN__\n> > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > index 3b73b442..ba36026d 100644\n> > --- a/src/libcamera/base/utils.cpp\n> > +++ b/src/libcamera/base/utils.cpp\n> > @@ -521,6 +521,26 @@ double strtod(const char *__restrict nptr, char **__restrict endptr)\n> > #endif\n> > }\n> > \n> > +/**\n> > + * \\fn R floatingToFixedPoint(T number)\n> > + * \\brief Convert a floating point number to a fixed-point representation\n> > + * \\tparam I Bit width of the integer part of the fixed-point\n> > + * \\tparam F Bit width of the fractional part of the fixed-point\n> > + * \\tparam R Return type of the fixed-point representation\n> > + * \\tparam T Input type of the floating point representation\n> > + * \\return The converted value\n> > + */\n> > +\n> > +/**\n> > + * \\fn R fixedToFloatingPoint(T number)\n> > + * \\brief Convert a fixed-point number to a floating point representation\n> > + * \\tparam I Bit width of the integer part of the fixed-point\n> > + * \\tparam F Bit width of the fractional part of the fixed-point\n> > + * \\tparam R Return type of the floating point representation\n> > + * \\tparam T Input type of the fixed-point representation\n> > + * \\return The converted value\n> > + */\n> > +\n> > } /* namespace utils */\n> > \n> > #ifndef __DOXYGEN__\n> > diff --git a/test/utils.cpp b/test/utils.cpp\n> > index fc56e14e..f1805207 100644\n> > --- a/test/utils.cpp\n> > +++ b/test/utils.cpp\n> > @@ -170,6 +170,51 @@ protected:\n> > \t\treturn TestPass;\n> > \t}\n> > \n> > +\ttemplate\n> > +\tint testSingleFixedPoint(double input, T expected)\n> > +\t{\n> > +\t\tT ret = utils::floatingToFixedPoint(input);\n> > +\t\tif (ret != expected) {\n> > +\t\t\tcerr << \"Expected \" << input << \" to convert to \"\n> > +\t\t\t << expected << \", got \" << ret << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\t/*\n> > +\t\t * The precision check is fairly arbitrary but is based on what\n> > +\t\t * the rkisp1 is capable of in the crosstalk module.\n> > +\t\t */\n> > +\t\tdouble f = utils::fixedToFloatingPoint(ret);\n> > +\t\tif (std::abs(f - input) > 0.001) {\n> > +\t\t\tcerr << \"Reverse conversion expected \" << ret\n> > +\t\t\t << \" to convert to \" << input\n> > +\t\t\t << \", got \" << f << std::endl;\n> > +\t\t\treturn TestFail;\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > +\tint testFixedPoint()\n> > +\t{\n> > +\t\t/* These are the only cases that we know for certain */\n> > +\t\tstd::map testCases = {\n> > +\t\t\t{ 7.992, 0x3FF },\n> > +\t\t\t{ -8, 0x400 },\n> > +\t\t\t{ 0, 0 },\n> > +\t\t};\n> \n> Great to have a testcase :-) Unfortunately I still believe there is\n> something not exactly right here. As the implementation differed from\n> what I had in mind regarding fixed points I played a bit with it... I\n> guess these testvalues where taken from the rkisp header. They all work\n> well with this implementation, but they miss the interesting point of\n> negative values (-0.4, -1.4) where two's complement gets interesting. In\n> that case the current implementation fails in the conversion back to\n> float. I'm not sure if vsi implemented a special fixed point format but\n> I actually don't expect that.\n> \n> I did a testprogram that shows the differences:\n> \n> ---------- main.cpp\n> #include \n> #include \n> #include \n> #include \n> #include \n> #include \n> #include \n> #include \n> #include \n> #include \n> \n> template \t std::enable_if_t &&\n> \t\t\t std::is_floating_point_v> * = nullptr>\n> constexpr R floatingToFixedPoint(T number)\n> {\n> \tstatic_assert(I + F <= sizeof(R) * 8);\n> \n> \tR maskI = (1 << I) - 1;\n> \tR whole = (static_cast(number) & maskI) << F;\n> \n> \tR maskF = (1 << F) - 1;\n> \tR frac = static_cast(std::round(number * (1 << F))) & maskF;\n> \n> \treturn whole | frac;\n> }\n> \n> template \t std::enable_if_t &&\n> \t\t\t std::is_floating_point_v> * = nullptr>\n> constexpr R floatingToFixedPoint2(T number)\n> {\n> \tstatic_assert(I + F <= sizeof(R) * 8);\n> \n> \tR mask = (1 << (F + I)) - 1;\n> \tR frac = static_cast(std::round(number * (1 << (F)))) & mask;\n> \n> \treturn frac;\n> }\n> \n> \n> template \t std::enable_if_t &&\n> \t\t\t std::is_integral_v> * = nullptr>\n> constexpr R fixedToFloatingPoint(T number)\n> {\n> \tstatic_assert(I + F <= sizeof(T) * 8);\n> \n> \tint coeff = number >> (I + F - 1) ? -1 : 1;\n> \n> \treturn coeff * static_cast(number) / static_cast(1 << F);\n> }\n> \n> \n> template \t std::enable_if_t &&\n> \t\t\t std::is_integral_v> * = nullptr>\n> constexpr R fixedToFloatingPoint2(T number)\n> {\n> static_assert(sizeof(int) >= sizeof(T));\n> \tstatic_assert(I + F <= sizeof(T) * 8);\n> \n> /* create a number with all upper bits set including the sign bit\n> * of the fixed point number\n> */\t\n> \tint upper_ones = -1 << (I + F -1);\n> \tif(number & upper_ones) {\n> \t return static_cast( upper_ones | number ) / static_cast(1 << F);\n> \t} else\n> \t return static_cast( number) / static_cast(1 << F);\n> }\n\nActually this fails if number contains a one in the superfluous bits but\nis actually positive. I played a bit more (maybe we should have just\ncopied an existing implementation :-) ):\n\ntemplate &&\n\t\t\t std::is_integral_v> * = nullptr>\nconstexpr R fixedToFloatingPoint2(T number)\n{\n static_assert(sizeof(int) >= sizeof(T));\n\tstatic_assert(I + F <= sizeof(T) * 8);\n\t\n\t/* recreate the upper bits in case of a negative number by shifting the sign \n\tbit from the fixed point to the first bit of the unsigned and then right shifting \n\tby the same amount which keeps the sign bit in place. \n\tThis can be optimized by the compiler quite well */\n\tint remaining_bits = sizeof(int)*8 - (I+F);\n\tint t = static_cast(static_cast(number) << remaining_bits) >> remaining_bits;\n\treturn static_cast(t) / static_cast(1 << F);\n}\n\n\nThe question if this is actally the fixed format used by the rkisp\nstill needs to be proven though.\n\n> \n> void doTest(double x) {\n> uint16_t f = floatingToFixedPoint<4,7,uint16_t>(x);\n> uint16_t f2 = floatingToFixedPoint2<4,7,uint16_t>(x);\n> double b = fixedToFloatingPoint<4,7,double>(f);\n> double b2 = fixedToFloatingPoint2<4,7,double>(f2);\n> std::cout << std::hex << \"===== test \" << x << std::endl;\n> std::cout << \"v1: \" << f << \"(\" << std::bitset<16>(f) << \") reverse: \" << b << std::endl;\n> std::cout << \"v2: \" << f2 << \"(\" << std::bitset<16>(f2) << \") reverse: \" << b2 << std::endl;\n> }\n> \n> int main()\n> {\n> doTest(-8.0);\n> doTest(7.992);\n> doTest(1.0);\n> doTest(0.4);\n> doTest(1.4);\n> doTest(-0.4);\n> doTest(-1.4);\n> } \n> \n> --- end of main.cpp\n> \n> \n> I couldn't come up with a simpler solution to handle the upper ones in\n> the fixed2FloatingPoint conversion. If we would have 16 or 32 bit wide\n> fixed points that wouldn't be an issue. Maybe someone else has an idea\n> here.\n> \n> Best regards,\n> Stefan\n> \n> > +\n> > +\t\tint ret;\n> > +\t\tfor (const auto &testCase : testCases) {\n> > +\t\t\tret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> > +\t\t\t\t\t\t\t\t testCase.second);\n> > +\t\t\tif (ret != TestPass)\n> > +\t\t\t\treturn ret;\n> > +\t\t}\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +\n> > \tint run()\n> > \t{\n> > \t\t/* utils::hex() test. */\n> > @@ -290,6 +335,10 @@ protected:\n> > \t\tif (testDuration() != TestPass)\n> > \t\t\treturn TestFail;\n> > \n> > +\t\t/* fixed point conversion test */\n> > +\t\tif (testFixedPoint() != TestPass)\n> > +\t\t\treturn TestFail;\n> > +\n> > \t\treturn TestPass;\n> > \t}\n> > };\n> > -- \n> > 2.39.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 AB7C1C3220\n\tfor ;\n\tThu, 25 Apr 2024 09:09:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A93F7633F3;\n\tThu, 25 Apr 2024 11:09:16 +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 95D5C61ADE\n\tfor ;\n\tThu, 25 Apr 2024 11:09:14 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:6736:b333:3187:60e9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 646AFB1;\n\tThu, 25 Apr 2024 11:08:21 +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=\"m4abJMfL\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714036101;\n\tbh=lBI0K/dYfRbgS3fETIWyDCtIECrmEFOJSntFEKtR18U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=m4abJMfLfrCoYv64HnY6iUD6p2cR2KFGLMMyypJs2mBmiB6TbSyqzNS4+zRb5roaB\n\tQkI5zEKGqFzI2Jkj17I8lzo2B1iZNYxNtCZZKopQsOw8N3/YK41VJ/RxKWUw81YvG/\n\tfwBgIWne1ASUYptjFeOl1ADNUzSW6Y2igXz0J3GU=","Date":"Thu, 25 Apr 2024 11:09:10 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] libcamera: utils: Add a helper to convert\n\tfloating-point to fixed-point","Message-ID":"<20240425090910.lzgoh6hpovm4scgs@jasper>","References":"<20240424074409.1275425-1-paul.elder@ideasonboard.com>\n\t<20240424181730.ytdtky6d7a4g77bw@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240424181730.ytdtky6d7a4g77bw@jasper>","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":29343,"web_url":"https://patchwork.libcamera.org/comment/29343/","msgid":"<20240425153843.wyddjctl56qvtdks@jasper>","date":"2024-04-25T15:38:43","subject":"Re: [PATCH v2] libcamera: utils: Add a helper to convert\n\tfloating-point to fixed-point","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Paul,\n\nwhat else could go wrong...\n\nOn Thu, Apr 25, 2024 at 11:09:10AM +0200, Stefan Klug wrote:\n> On Wed, Apr 24, 2024 at 08:17:30PM +0200, Stefan Klug wrote:\n> > Hi Paul,\n> > \n> > thanks for the patch.\n> > \n> > On Wed, Apr 24, 2024 at 04:44:09PM +0900, Paul Elder wrote:\n> > > Add helper functions for converting between floating point and fixed\n> > > point numbers. Also add tests for them.\n> > > \n> > > Signed-off-by: Paul Elder \n> > > ---\n> > > Changes in v2:\n> > > - added the reverse conversion function (fixed -> floating)\n> > > - added tests\n> > > - make the conversion code cleaner\n> > > ---\n> > > include/libcamera/base/utils.h | 37 +++++++++++++++++++++++++\n> > > src/libcamera/base/utils.cpp | 20 ++++++++++++++\n> > > test/utils.cpp | 49 ++++++++++++++++++++++++++++++++++\n> > > 3 files changed, 106 insertions(+)\n> > > \n> > > diff --git a/include/libcamera/base/utils.h b/include/libcamera/base/utils.h\n> > > index 37d9af60..da0767e3 100644\n> > > --- a/include/libcamera/base/utils.h\n> > > +++ b/include/libcamera/base/utils.h\n> > > @@ -9,6 +9,7 @@\n> > > \n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > > #include \n> > > @@ -369,6 +370,42 @@ decltype(auto) abs_diff(const T &a, const T &b)\n> > > \n> > > double strtod(const char *__restrict nptr, char **__restrict endptr);\n> > > \n> > > +#ifndef __DOXYGEN__\n> > > +template > > +\t std::enable_if_t &&\n> > > +\t\t\t std::is_floating_point_v> * = nullptr>\n> > > +#else\n> > > +template\n> > > +#endif\n> > > +constexpr R floatingToFixedPoint(T number)\n> > > +{\n> > > +\tstatic_assert(I + F <= sizeof(R) * 8);\n> > > +\n> > > +\tR maskI = (1 << I) - 1;\n> > > +\tR whole = (static_cast(number) & maskI) << F;\n> > > +\n> > > +\tR maskF = (1 << F) - 1;\n> > > +\tR frac = static_cast(std::round(number * (1 << F))) & maskF;\n> > > +\n> > > +\treturn whole | frac;\n> > > +}\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +template > > +\t std::enable_if_t &&\n> > > +\t\t\t std::is_integral_v> * = nullptr>\n> > > +#else\n> > > +template\n> > > +#endif\n> > > +constexpr R fixedToFloatingPoint(T number)\n> > > +{\n> > > +\tstatic_assert(I + F <= sizeof(T) * 8);\n> > > +\n> > > +\tint coeff = number >> (I + F - 1) ? -1 : 1;\n> > > +\n> > > +\treturn coeff * static_cast(number) / static_cast(1 << F);\n> > > +}\n> > > +\n> > > } /* namespace utils */\n> > > \n> > > #ifndef __DOXYGEN__\n> > > diff --git a/src/libcamera/base/utils.cpp b/src/libcamera/base/utils.cpp\n> > > index 3b73b442..ba36026d 100644\n> > > --- a/src/libcamera/base/utils.cpp\n> > > +++ b/src/libcamera/base/utils.cpp\n> > > @@ -521,6 +521,26 @@ double strtod(const char *__restrict nptr, char **__restrict endptr)\n> > > #endif\n> > > }\n> > > \n> > > +/**\n> > > + * \\fn R floatingToFixedPoint(T number)\n> > > + * \\brief Convert a floating point number to a fixed-point representation\n> > > + * \\tparam I Bit width of the integer part of the fixed-point\n> > > + * \\tparam F Bit width of the fractional part of the fixed-point\n> > > + * \\tparam R Return type of the fixed-point representation\n> > > + * \\tparam T Input type of the floating point representation\n> > > + * \\return The converted value\n> > > + */\n> > > +\n> > > +/**\n> > > + * \\fn R fixedToFloatingPoint(T number)\n> > > + * \\brief Convert a fixed-point number to a floating point representation\n> > > + * \\tparam I Bit width of the integer part of the fixed-point\n> > > + * \\tparam F Bit width of the fractional part of the fixed-point\n> > > + * \\tparam R Return type of the floating point representation\n> > > + * \\tparam T Input type of the fixed-point representation\n> > > + * \\return The converted value\n> > > + */\n> > > +\n> > > } /* namespace utils */\n> > > \n> > > #ifndef __DOXYGEN__\n> > > diff --git a/test/utils.cpp b/test/utils.cpp\n> > > index fc56e14e..f1805207 100644\n> > > --- a/test/utils.cpp\n> > > +++ b/test/utils.cpp\n> > > @@ -170,6 +170,51 @@ protected:\n> > > \t\treturn TestPass;\n> > > \t}\n> > > \n> > > +\ttemplate\n> > > +\tint testSingleFixedPoint(double input, T expected)\n> > > +\t{\n> > > +\t\tT ret = utils::floatingToFixedPoint(input);\n> > > +\t\tif (ret != expected) {\n> > > +\t\t\tcerr << \"Expected \" << input << \" to convert to \"\n> > > +\t\t\t << expected << \", got \" << ret << std::endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\t/*\n> > > +\t\t * The precision check is fairly arbitrary but is based on what\n> > > +\t\t * the rkisp1 is capable of in the crosstalk module.\n> > > +\t\t */\n> > > +\t\tdouble f = utils::fixedToFloatingPoint(ret);\n> > > +\t\tif (std::abs(f - input) > 0.001) {\n> > > +\t\t\tcerr << \"Reverse conversion expected \" << ret\n> > > +\t\t\t << \" to convert to \" << input\n> > > +\t\t\t << \", got \" << f << std::endl;\n> > > +\t\t\treturn TestFail;\n> > > +\t\t}\n> > > +\n> > > +\t\treturn TestPass;\n> > > +\t}\n> > > +\n> > > +\tint testFixedPoint()\n> > > +\t{\n> > > +\t\t/* These are the only cases that we know for certain */\n> > > +\t\tstd::map testCases = {\n> > > +\t\t\t{ 7.992, 0x3FF },\n> > > +\t\t\t{ -8, 0x400 },\n> > > +\t\t\t{ 0, 0 },\n> > > +\t\t};\n> > \n> > Great to have a testcase :-) Unfortunately I still believe there is\n> > something not exactly right here. As the implementation differed from\n> > what I had in mind regarding fixed points I played a bit with it... I\n> > guess these testvalues where taken from the rkisp header. They all work\n> > well with this implementation, but they miss the interesting point of\n> > negative values (-0.4, -1.4) where two's complement gets interesting. In\n> > that case the current implementation fails in the conversion back to\n> > float. I'm not sure if vsi implemented a special fixed point format but\n> > I actually don't expect that.\n> > \n> > I did a testprogram that shows the differences:\n> > \n> > ---------- main.cpp\n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > #include \n> > \n> > template > \t std::enable_if_t &&\n> > \t\t\t std::is_floating_point_v> * = nullptr>\n> > constexpr R floatingToFixedPoint(T number)\n> > {\n> > \tstatic_assert(I + F <= sizeof(R) * 8);\n> > \n> > \tR maskI = (1 << I) - 1;\n> > \tR whole = (static_cast(number) & maskI) << F;\n> > \n> > \tR maskF = (1 << F) - 1;\n> > \tR frac = static_cast(std::round(number * (1 << F))) & maskF;\n> > \n> > \treturn whole | frac;\n> > }\n> > \n> > template > \t std::enable_if_t &&\n> > \t\t\t std::is_floating_point_v> * = nullptr>\n> > constexpr R floatingToFixedPoint2(T number)\n> > {\n> > \tstatic_assert(I + F <= sizeof(R) * 8);\n> > \n> > \tR mask = (1 << (F + I)) - 1;\n> > \tR frac = static_cast(std::round(number * (1 << (F)))) & mask;\n\n... turns out, this doesn't work on arm ... \nSee https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n\nThis version works for me:\n\nSee template &&\n\t\t\t std::is_floating_point_v> * = nullptr>\nconstexpr R floatingToFixedPoint2(T number)\n{\n\tstatic_assert(sizeof(int) >= sizeof(R));\n\tstatic_assert(I + F <= sizeof(R) * 8);\n\n\tR mask = (1 << (F + I)) - 1;\n\t/* \n\t* The inbetween cast to int is needed on arm platforms to properly\n\t* cast negative values. See\n\t* https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/\n\t*/\n\tR frac = static_cast(static_cast(std::round(number * (1 << F)))) & mask;\n\t\n\treturn frac;\n}\n\n\nCheers\nStefan\n\n> > \n> > \treturn frac;\n> > }\n> > \n> > \n> > template > \t std::enable_if_t &&\n> > \t\t\t std::is_integral_v> * = nullptr>\n> > constexpr R fixedToFloatingPoint(T number)\n> > {\n> > \tstatic_assert(I + F <= sizeof(T) * 8);\n> > \n> > \tint coeff = number >> (I + F - 1) ? -1 : 1;\n> > \n> > \treturn coeff * static_cast(number) / static_cast(1 << F);\n> > }\n> > \n> > \n> > template > \t std::enable_if_t &&\n> > \t\t\t std::is_integral_v> * = nullptr>\n> > constexpr R fixedToFloatingPoint2(T number)\n> > {\n> > static_assert(sizeof(int) >= sizeof(T));\n> > \tstatic_assert(I + F <= sizeof(T) * 8);\n> > \n> > /* create a number with all upper bits set including the sign bit\n> > * of the fixed point number\n> > */\t\n> > \tint upper_ones = -1 << (I + F -1);\n> > \tif(number & upper_ones) {\n> > \t return static_cast( upper_ones | number ) / static_cast(1 << F);\n> > \t} else\n> > \t return static_cast( number) / static_cast(1 << F);\n> > }\n> \n> Actually this fails if number contains a one in the superfluous bits but\n> is actually positive. I played a bit more (maybe we should have just\n> copied an existing implementation :-) ):\n> \n> template \t std::enable_if_t &&\n> \t\t\t std::is_integral_v> * = nullptr>\n> constexpr R fixedToFloatingPoint2(T number)\n> {\n> static_assert(sizeof(int) >= sizeof(T));\n> \tstatic_assert(I + F <= sizeof(T) * 8);\n> \t\n> \t/* recreate the upper bits in case of a negative number by shifting the sign \n> \tbit from the fixed point to the first bit of the unsigned and then right shifting \n> \tby the same amount which keeps the sign bit in place. \n> \tThis can be optimized by the compiler quite well */\n> \tint remaining_bits = sizeof(int)*8 - (I+F);\n> \tint t = static_cast(static_cast(number) << remaining_bits) >> remaining_bits;\n> \treturn static_cast(t) / static_cast(1 << F);\n> }\n> \n> \n> The question if this is actally the fixed format used by the rkisp\n> still needs to be proven though.\n> \n> > \n> > void doTest(double x) {\n> > uint16_t f = floatingToFixedPoint<4,7,uint16_t>(x);\n> > uint16_t f2 = floatingToFixedPoint2<4,7,uint16_t>(x);\n> > double b = fixedToFloatingPoint<4,7,double>(f);\n> > double b2 = fixedToFloatingPoint2<4,7,double>(f2);\n> > std::cout << std::hex << \"===== test \" << x << std::endl;\n> > std::cout << \"v1: \" << f << \"(\" << std::bitset<16>(f) << \") reverse: \" << b << std::endl;\n> > std::cout << \"v2: \" << f2 << \"(\" << std::bitset<16>(f2) << \") reverse: \" << b2 << std::endl;\n> > }\n> > \n> > int main()\n> > {\n> > doTest(-8.0);\n> > doTest(7.992);\n> > doTest(1.0);\n> > doTest(0.4);\n> > doTest(1.4);\n> > doTest(-0.4);\n> > doTest(-1.4);\n> > } \n> > \n> > --- end of main.cpp\n> > \n> > \n> > I couldn't come up with a simpler solution to handle the upper ones in\n> > the fixed2FloatingPoint conversion. If we would have 16 or 32 bit wide\n> > fixed points that wouldn't be an issue. Maybe someone else has an idea\n> > here.\n> > \n> > Best regards,\n> > Stefan\n> > \n> > > +\n> > > +\t\tint ret;\n> > > +\t\tfor (const auto &testCase : testCases) {\n> > > +\t\t\tret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> > > +\t\t\t\t\t\t\t\t testCase.second);\n> > > +\t\t\tif (ret != TestPass)\n> > > +\t\t\t\treturn ret;\n> > > +\t\t}\n> > > +\n> > > +\t\treturn TestPass;\n> > > +\t}\n> > > +\n> > > \tint run()\n> > > \t{\n> > > \t\t/* utils::hex() test. */\n> > > @@ -290,6 +335,10 @@ protected:\n> > > \t\tif (testDuration() != TestPass)\n> > > \t\t\treturn TestFail;\n> > > \n> > > +\t\t/* fixed point conversion test */\n> > > +\t\tif (testFixedPoint() != TestPass)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > \t\treturn TestPass;\n> > > \t}\n> > > };\n> > > -- \n> > > 2.39.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 DEA95BE08B\n\tfor ;\n\tThu, 25 Apr 2024 15:38:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E6F91633F8;\n\tThu, 25 Apr 2024 17:38:47 +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 5098E61A9B\n\tfor ;\n\tThu, 25 Apr 2024 17:38:46 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:6736:b333:3187:60e9])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 72C1C674;\n\tThu, 25 Apr 2024 17:37:53 +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=\"ugVyhMHp\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1714059473;\n\tbh=YVHTFK8bDnYqt3OKaoRVVmQExXmBYikmcQ31AeTHqcE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ugVyhMHpBdUwl+HfM39ZXJrz68+XlHrAiH2xSKJdajtAXrrPGzSUEfll9ulnrO5sa\n\tBcPVdrxi5e0bsqr+njIbBk6l600AVQeGYMKMM4wuM/kM7e1hDxz+6kbDghVUjgRf7s\n\tPm6TD9+YWqguLbiBf20L2AmGEXSzKYT06y4HcRXQ=","Date":"Thu, 25 Apr 2024 17:38:43 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v2] libcamera: utils: Add a helper to convert\n\tfloating-point to fixed-point","Message-ID":"<20240425153843.wyddjctl56qvtdks@jasper>","References":"<20240424074409.1275425-1-paul.elder@ideasonboard.com>\n\t<20240424181730.ytdtky6d7a4g77bw@jasper>\n\t<20240425090910.lzgoh6hpovm4scgs@jasper>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240425090910.lzgoh6hpovm4scgs@jasper>","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\" "}}]