[{"id":29733,"web_url":"https://patchwork.libcamera.org/comment/29733/","msgid":"<20240602000518.GH5213@pendragon.ideasonboard.com>","date":"2024-06-02T00:05:18","subject":"Re: [PATCH v5] ipa: rkisp1: Add a helper to convert floating-point\n\tto 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, and sorry for the late review.\n\nOn Fri, May 31, 2024 at 03:51:21PM +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> Reviewed-by: Stefan Klug \n> Reviewed-by: Kieran Bingham \n> \n> ---\n> Changes in v5:\n> - fix test comments\n> - add documentation for the actual parameters (only template parameters\n> were documented previously)\n> \n> Changes in v4:\n> - relax the test case floating point precision\n> - optimize the fixed -> floating point converter\n> \n> Changes in v3:\n> - (used to be \"libcamera: utils: Add a helper to convert floating-point to fixed-point\")\n> - move the everything from libcamera global utils to the rkisp1 ipa\n> \n> Changes in v2:\n> - added the reverse conversion function (fixed -> floating)\n> - added tests\n> - make the conversion code cleaner\n> ---\n> src/ipa/rkisp1/meson.build | 1 +\n> src/ipa/rkisp1/utils.cpp | 42 +++++++++++++++\n> src/ipa/rkisp1/utils.h | 66 ++++++++++++++++++++++++\n> test/ipa/meson.build | 2 +\n> test/ipa/rkisp1/meson.build | 15 ++++++\n> test/ipa/rkisp1/rkisp1-utils.cpp | 87 ++++++++++++++++++++++++++++++++\n> 6 files changed, 213 insertions(+)\n> create mode 100644 src/ipa/rkisp1/utils.cpp\n> create mode 100644 src/ipa/rkisp1/utils.h\n> create mode 100644 test/ipa/rkisp1/meson.build\n> create mode 100644 test/ipa/rkisp1/rkisp1-utils.cpp\n> \n> diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build\n> index e813da53a..cf05cdb27 100644\n> --- a/src/ipa/rkisp1/meson.build\n> +++ b/src/ipa/rkisp1/meson.build\n> @@ -8,6 +8,7 @@ ipa_name = 'ipa_rkisp1'\n> rkisp1_ipa_sources = files([\n> 'ipa_context.cpp',\n> 'rkisp1.cpp',\n> + 'utils.cpp',\n> ])\n> \n> rkisp1_ipa_sources += rkisp1_ipa_algorithms\n> diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp\n> new file mode 100644\n> index 000000000..960ec64e9\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/utils.cpp\n> @@ -0,0 +1,42 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Miscellaneous utility functions specific to rkisp1\n> + */\n> +\n> +#include \"utils.h\"\n> +\n> +/**\n> + * \\file utils.h\n\nMissing \\brief\n\n> + */\n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::utils {\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> + * \\param number The floating point number to convert to fixed point\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> + * \\param number The fixed point number to convert to floating point\n> + * \\return The converted value\n> + */\n> +\n> +} /* namespace ipa::rkisp1::utils */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/ipa/rkisp1/utils.h b/src/ipa/rkisp1/utils.h\n> new file mode 100644\n> index 000000000..450f22442\n> --- /dev/null\n> +++ b/src/ipa/rkisp1/utils.h\n> @@ -0,0 +1,66 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Miscellaneous utility functions specific to rkisp1\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace ipa::rkisp1::utils {\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(sizeof(int) >= sizeof(R));\n> +\tstatic_assert(I + F <= sizeof(R) * 8);\n> +\n> +\t/*\n> +\t * The intermediate 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 mask = (1 << (F + I)) - 1;\n> +\tR frac = static_cast(static_cast(std::round(number * (1 << F)))) & mask;\n> +\n> +\treturn 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(sizeof(int) >= sizeof(T));\n> +\tstatic_assert(I + F <= sizeof(T) * 8);\n> +\n> +\t/*\n> +\t * Recreate the upper bits in case of a negative number by shifting the sign\n> +\t * bit from the fixed point to the first bit of the unsigned and then right shifting\n> +\t * by the same amount which keeps the sign bit in place.\n> +\t * This can be optimized by the compiler quite well.\n> +\t */\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> +} /* namespace ipa::rkisp1::utils */\n> +\n> +} /* namespace libcamera */\n> diff --git a/test/ipa/meson.build b/test/ipa/meson.build\n> index 180b0da0a..dc956284c 100644\n> --- a/test/ipa/meson.build\n> +++ b/test/ipa/meson.build\n> @@ -1,5 +1,7 @@\n> # SPDX-License-Identifier: CC0-1.0\n> \n> +subdir('rkisp1')\n> +\n> ipa_test = [\n> {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']},\n> {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']},\n> diff --git a/test/ipa/rkisp1/meson.build b/test/ipa/rkisp1/meson.build\n> new file mode 100644\n> index 000000000..5ffc5dd60\n> --- /dev/null\n> +++ b/test/ipa/rkisp1/meson.build\n> @@ -0,0 +1,15 @@\n> +# SPDX-License-Identifier: CC0-1.0\n> +\n> +rkisp1_ipa_test = [\n> + {'name': 'rkisp1-utils', 'sources': ['rkisp1-utils.cpp']},\n> +]\n> +\n> +foreach test : rkisp1_ipa_test\n> + exe = executable(test['name'], test['sources'], libcamera_generated_ipa_headers,\n> + dependencies : libcamera_private,\n> + link_with : [libipa, test_libraries],\n> + include_directories : [libipa_includes, test_includes_internal,\n> + '../../../src/ipa/rkisp1/'])\n> +\n> + test(test['name'], exe, suite : 'ipa')\n> +endforeach\n> diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp\n> new file mode 100644\n> index 000000000..ed6f44e9b\n> --- /dev/null\n> +++ b/test/ipa/rkisp1/rkisp1-utils.cpp\n> @@ -0,0 +1,87 @@\n> +/* SPDX-License-Identifier: GPL-2.0-or-later */\n> +/*\n> + * Copyright (C) 2024, Paul Elder \n> + *\n> + * Miscellaneous utility tests\n> + */\n> +\n> +#include \n> +#include \n> +#include \n> +\n> +#include \"../src/ipa/rkisp1/utils.h\"\n> +\n> +#include \"test.h\"\n> +\n> +using namespace std;\n> +using namespace libcamera;\n> +using namespace ipa::rkisp1;\n> +\n> +class RkISP1UtilsTest : public Test\n> +{\n> +protected:\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\nShouldn't it be based on the fracPrec value instead ?\n\n> +\t\t */\n> +\t\tdouble f = utils::fixedToFloatingPoint(ret);\n> +\t\tif (std::abs(f - input) > 0.005) {\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/*\n> +\t\t * The second 7.992 test is to test that unused bits don't\n> +\t\t * affect the result.\n> +\t\t */\n> +\t\tstd::map testCases = {\n> +\t\t\t{ 7.992, 0x3FF },\n> +\t\t\t{ 7.992, 0xBFF },\n\nI had a bit of a WTF moment, not understanding how the same double could\nconvert to two different fixed point values. Then I realized you're\nusing a std::map here, which doesn't support multiple items with the\nsame key.\n\nWhat you need is a\n\n\t\tstd::vector> testCases = {\n\n(don't forget to include vector and utility instead of map). With that,\nthe test will fail.\n\nAs this patch has been merged already, could you send patches to fix\nthose issues ?\n\n> +\t\t\t{ 0.2, 0x01A },\n> +\t\t\t{ -0.2, 0x7E6 },\n> +\t\t\t{ -0.8, 0x79A },\n> +\t\t\t{ -0.4, 0x7CD },\n> +\t\t\t{ -1.4, 0x74D },\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/* fixed point conversion test */\n> +\t\tif (testFixedPoint() != TestPass)\n> +\t\t\treturn TestFail;\n> +\n> +\t\treturn TestPass;\n> +\t}\n> +};\n> +\n> +TEST_REGISTER(RkISP1UtilsTest)","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 59342BD87C\n\tfor ;\n\tSun, 2 Jun 2024 00:05:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 80A67634BA;\n\tSun, 2 Jun 2024 02:05:34 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id D652161A46\n\tfor ;\n\tSun, 2 Jun 2024 02:05:32 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9497B66B;\n\tSun, 2 Jun 2024 02:05:26 +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=\"LBPMmIV1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717286726;\n\tbh=rfKkSksmxpGS4KcXZe7XVfRVXpgHBHpdGb2dHMc0DL4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=LBPMmIV1R1RBwkMiouh50b/XuK/sHUxww4d6qvYgYxAABP2SwfrTs+Am/IPDUHV0J\n\tLyd9Ph4oRn/T1EeIaeXk7ksPYXRvU6hGHOR3u4JmB7YYgE3sCRcGBE+JDuQxyhDFVB\n\t7SAIyGGpZrz2xXZc/Oh1tpI6WQjafU7oAJOU3tww=","Date":"Sun, 2 Jun 2024 03:05:18 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tKieran Bingham ","Subject":"Re: [PATCH v5] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","Message-ID":"<20240602000518.GH5213@pendragon.ideasonboard.com>","References":"<20240531065121.2562756-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240531065121.2562756-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":29734,"web_url":"https://patchwork.libcamera.org/comment/29734/","msgid":"","date":"2024-06-02T00:10:17","subject":"Re: [PATCH v5] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","submitter":{"id":133,"url":"https://patchwork.libcamera.org/api/people/133/","name":"Pőcze Barnabás","email":"pobrn@protonmail.com"},"content":"Hi\n\n\n2024. június 2., vasárnap 2:05 keltezéssel, Laurent Pinchart írta:\n\n> Hi Paul,\n> \n> Thank you for the patch, and sorry for the late review.\n> \n> On Fri, May 31, 2024 at 03:51:21PM +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> > Reviewed-by: Stefan Klug \n> > Reviewed-by: Kieran Bingham \n> >\n> > ---\n> > Changes in v5:\n> > - fix test comments\n> > - add documentation for the actual parameters (only template parameters\n> > were documented previously)\n> >\n> > Changes in v4:\n> > - relax the test case floating point precision\n> > - optimize the fixed -> floating point converter\n> >\n> > Changes in v3:\n> > - (used to be \"libcamera: utils: Add a helper to convert floating-point to fixed-point\")\n> > - move the everything from libcamera global utils to the rkisp1 ipa\n> >\n> > Changes in v2:\n> > - added the reverse conversion function (fixed -> floating)\n> > - added tests\n> > - make the conversion code cleaner\n> > ---\n> [...]\n> > +\tint testFixedPoint()\n> > +\t{\n> > +\t\t/*\n> > +\t\t * The second 7.992 test is to test that unused bits don't\n> > +\t\t * affect the result.\n> > +\t\t */\n> > +\t\tstd::map testCases = {\n> > +\t\t\t{ 7.992, 0x3FF },\n> > +\t\t\t{ 7.992, 0xBFF },\n> \n> I had a bit of a WTF moment, not understanding how the same double could\n> convert to two different fixed point values. Then I realized you're\n> using a std::map here, which doesn't support multiple items with the\n> same key.\n> \n> What you need is a\n> \n> \t\tstd::vector> testCases = {\n> \n> (don't forget to include vector and utility instead of map). With that,\n> the test will fail.\n> \n> As this patch has been merged already, could you send patches to fix\n> those issues ?\n\nI think\n\n static const std::pair testCases[] = { ... };\n\nis more than sufficient here.\n\n\nRegards,\nBarnabás Pőcze\n\n\n> \n> > +\t\t\t{ 0.2, 0x01A },\n> > +\t\t\t{ -0.2, 0x7E6 },\n> > +\t\t\t{ -0.8, 0x79A },\n> > +\t\t\t{ -0.4, 0x7CD },\n> > +\t\t\t{ -1.4, 0x74D },\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/* fixed point conversion test */\n> > +\t\tif (testFixedPoint() != TestPass)\n> > +\t\t\treturn TestFail;\n> > +\n> > +\t\treturn TestPass;\n> > +\t}\n> > +};\n> > +\n> > +TEST_REGISTER(RkISP1UtilsTest)\n> \n> --\n> Regards,\n> \n> Laurent Pinchart\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 858BBBDE6B\n\tfor ;\n\tSun, 2 Jun 2024 00:10:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7DBA8634CB;\n\tSun, 2 Jun 2024 02:10:23 +0200 (CEST)","from mail-4316.protonmail.ch (mail-4316.protonmail.ch\n\t[185.70.43.16])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E90AA634BA\n\tfor ;\n\tSun, 2 Jun 2024 02:10:21 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=protonmail.com header.i=@protonmail.com\n\theader.b=\"xUXRlQDW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=protonmail.com;\n\ts=protonmail3; t=1717287021; x=1717546221;\n\tbh=Ydc9kqELutTu1ZzOjpfEcYqHBllXcn66qi2bdp6/Ims=;\n\th=Date:To:From:Cc:Subject:Message-ID:In-Reply-To:References:\n\tFeedback-ID:From:To:Cc:Date:Subject:Reply-To:Feedback-ID:\n\tMessage-ID:BIMI-Selector;\n\tb=xUXRlQDWxiSHUcfuvG6+QTHnBXudkijnlT4Ngz3eYUicTsYkq5f5pSZwSxju0cCjD\n\tsAYs8PVABbeFKJWS+enuqlYZ9OJO7CcriG600PsVgAVkeboXqw+9mr6KMIYAlaufV5\n\t7k6nFy6juqatcpdv1SuzC9KIYNf8ECoA0/kTrehTcf9QlCh4cPU0dAFsaol1Q2bWVh\n\tsrmnOcFODtZde8dS86p1sKOHO9WhMRlLif5tdHoL4GWCw4C2cCHlFqX3/JjQr1SnVK\n\tHO4+8zJp0RyE9YaCuoMRY0M1eYKzufz7HTRnn8tQjRqK4Xi78vAdOfaDanyPwg/DoA\n\tQATUfI8rxiCcA==","Date":"Sun, 02 Jun 2024 00:10:17 +0000","To":"Laurent Pinchart ","From":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tKieran Bingham ","Subject":"Re: [PATCH v5] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","Message-ID":"","In-Reply-To":"<20240602000518.GH5213@pendragon.ideasonboard.com>","References":"<20240531065121.2562756-1-paul.elder@ideasonboard.com>\n\t<20240602000518.GH5213@pendragon.ideasonboard.com>","Feedback-ID":"20568564:user:proton","X-Pm-Message-ID":"551a3acba9019fbe8992283ffccf82686c78853d","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Transfer-Encoding":"quoted-printable","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":29735,"web_url":"https://patchwork.libcamera.org/comment/29735/","msgid":"<20240602002301.GH6683@pendragon.ideasonboard.com>","date":"2024-06-02T00:23:01","subject":"Re: [PATCH v5] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Sun, Jun 02, 2024 at 12:10:17AM +0000, Barnabás Pőcze wrote:\n> 2024. június 2., vasárnap 2:05 keltezéssel, Laurent Pinchart írta:\n> \n> > Hi Paul,\n> > \n> > Thank you for the patch, and sorry for the late review.\n> > \n> > On Fri, May 31, 2024 at 03:51:21PM +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> > > Reviewed-by: Stefan Klug \n> > > Reviewed-by: Kieran Bingham \n> > >\n> > > ---\n> > > Changes in v5:\n> > > - fix test comments\n> > > - add documentation for the actual parameters (only template parameters\n> > > were documented previously)\n> > >\n> > > Changes in v4:\n> > > - relax the test case floating point precision\n> > > - optimize the fixed -> floating point converter\n> > >\n> > > Changes in v3:\n> > > - (used to be \"libcamera: utils: Add a helper to convert floating-point to fixed-point\")\n> > > - move the everything from libcamera global utils to the rkisp1 ipa\n> > >\n> > > Changes in v2:\n> > > - added the reverse conversion function (fixed -> floating)\n> > > - added tests\n> > > - make the conversion code cleaner\n> > > ---\n> > [...]\n> > > +\tint testFixedPoint()\n> > > +\t{\n> > > +\t\t/*\n> > > +\t\t * The second 7.992 test is to test that unused bits don't\n> > > +\t\t * affect the result.\n> > > +\t\t */\n> > > +\t\tstd::map testCases = {\n> > > +\t\t\t{ 7.992, 0x3FF },\n> > > +\t\t\t{ 7.992, 0xBFF },\n> > \n> > I had a bit of a WTF moment, not understanding how the same double could\n> > convert to two different fixed point values. Then I realized you're\n> > using a std::map here, which doesn't support multiple items with the\n> > same key.\n> > \n> > What you need is a\n> > \n> > \t\tstd::vector> testCases = {\n> > \n> > (don't forget to include vector and utility instead of map). With that,\n> > the test will fail.\n> > \n> > As this patch has been merged already, could you send patches to fix\n> > those issues ?\n> \n> I think\n> \n> static const std::pair testCases[] = { ... };\n> \n> is more than sufficient here.\n\nYes, or an std::array<>. As it's a test, I don't mind much either way.\n\n> > > +\t\t\t{ 0.2, 0x01A },\n> > > +\t\t\t{ -0.2, 0x7E6 },\n> > > +\t\t\t{ -0.8, 0x79A },\n> > > +\t\t\t{ -0.4, 0x7CD },\n> > > +\t\t\t{ -1.4, 0x74D },\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/* fixed point conversion test */\n> > > +\t\tif (testFixedPoint() != TestPass)\n> > > +\t\t\treturn TestFail;\n> > > +\n> > > +\t\treturn TestPass;\n> > > +\t}\n> > > +};\n> > > +\n> > > +TEST_REGISTER(RkISP1UtilsTest)","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 61A53BD87C\n\tfor ;\n\tSun, 2 Jun 2024 00:23:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 357FB634CA;\n\tSun, 2 Jun 2024 02:23:17 +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 407A761A46\n\tfor ;\n\tSun, 2 Jun 2024 02:23:16 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id E2D3B66B;\n\tSun, 2 Jun 2024 02:23:09 +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=\"tI4SsE2U\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717287790;\n\tbh=TmA8MHfrrrsKf3emNXNCUYDtPiE0Gh7ziO60+h5cp00=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tI4SsE2UpOjCfTv8DefmGJ8zRJwEqMrvmkPWdLLOBzqUJr39/Z1WrdVmo2zIxjVEd\n\t79+eI+7aaZ+Ikohbp2iGy3LuZxVgG+2x3j2mAJKHPPAB/ICb34ioMsTyG8bS0ioKYc\n\ta2VD/qXN+kLTxS89uz6lXCDLjiSNWYjYWPd1L970=","Date":"Sun, 2 Jun 2024 03:23:01 +0300","From":"Laurent Pinchart ","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"Paul Elder ,\n\tlibcamera-devel@lists.libcamera.org,\n\tStefan Klug ,\n\tKieran Bingham ","Subject":"Re: [PATCH v5] ipa: rkisp1: Add a helper to convert floating-point\n\tto fixed-point","Message-ID":"<20240602002301.GH6683@pendragon.ideasonboard.com>","References":"<20240531065121.2562756-1-paul.elder@ideasonboard.com>\n\t<20240602000518.GH5213@pendragon.ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","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\" "}}]