[{"id":29780,"web_url":"https://patchwork.libcamera.org/comment/29780/","msgid":"<3tignw3xbankkides7dg5dyclakpu2mcjpcagk4ngtkxxjqv3u@qx6u5pxeyqwk>","date":"2024-06-05T10:14:30","subject":"Re: [PATCH] test: ipa: rkisp1: utils: Fix floating and fixed point\n\tconversion test","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 Mon, Jun 03, 2024 at 09:53:24PM +0900, Paul Elder wrote:\n> There was an issue where using map to store the test cases meant that\n> the test for ignoring unused bits was skipped because of clashing keys.\n> Fix this by changing the test to an array of tuples.\n> \n> While at it, reorganize the tests so that it's possible to test only\n> reverse conversion, which was the case here to test that multiple fixed\n> point numbers (due to unused bits) would result in the same floating\n> point number.\n> \n> While at it, also change the arbitrary floating comparison precision to\n> be more precise.\n> \n> Also fix a missing documentation brief.\n> \n> Fixes: 9d152e9c6 ipa: rkisp1: Add a helper to convert floating-point to\n> fixed-point\n> Signed-off-by: Paul Elder \n> ---\n> src/ipa/rkisp1/utils.cpp | 1 +\n> test/ipa/rkisp1/rkisp1-utils.cpp | 97 +++++++++++++++++++++++---------\n> 2 files changed, 70 insertions(+), 28 deletions(-)\n> \n> diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp\n> index 960ec64e9..8edcf5bf7 100644\n> --- a/src/ipa/rkisp1/utils.cpp\n> +++ b/src/ipa/rkisp1/utils.cpp\n> @@ -9,6 +9,7 @@\n> \n> /**\n> * \\file utils.h\n> + * \\brief Utility functions for rkisp1\n> */\n> \n> namespace libcamera {\n> diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp\n> index e48f8d362..4757ca069 100644\n> --- a/test/ipa/rkisp1/rkisp1-utils.cpp\n> +++ b/test/ipa/rkisp1/rkisp1-utils.cpp\n> @@ -7,8 +7,8 @@\n> \n> #include \n> #include \n> -#include \n> #include \n> +#include \n> \n> #include \"../src/ipa/rkisp1/utils.h\"\n> \n> @@ -21,53 +21,94 @@ using namespace ipa::rkisp1;\n> class RkISP1UtilsTest : public Test\n> {\n> protected:\n> -\ttemplate\n> -\tint testSingleFixedPoint(double input, T expected)\n> +\t/* R for real, I for integer */\n> +\ttemplate\n> +\tint testFixedToFloat(I input, R expected, R *output = nullptr)\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\tR out = utils::fixedToFloatingPoint(input);\n> +\t\tif (output)\n> +\t\t\t*output = out;\n> +\t\tR prec = 1.0 / (1 << FracPrec);\n> +\t\tif (std::abs(out - expected) > prec) {\n> +\t\t\tcerr << \"Reverse conversion expected \" << input\n> +\t\t\t << \" to convert to \" << expected\n> +\t\t\t << \", got \" << out << 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.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\treturn TestPass;\n> +\t}\n> +\n> +\t/* R for real, I for integer */\n> +\ttemplate\n> +\tint testFloatToFixed(R input, I expected, I *output = nullptr)\n> +\t{\n> +\t\tI out = utils::floatingToFixedPoint(input);\n> +\t\tif (output)\n> +\t\t\t*output = out;\n> +\t\tif (out != expected) {\n> +\t\t\tcerr << \"Expected \" << input << \" to convert to \"\n> +\t\t\t << expected << \", got \" << out << std::endl;\n> \t\t\treturn TestFail;\n> \t\t}\n> \n> \t\treturn TestPass;\n> \t}\n> \n> +\t/* R for real, I for integer */\n> +\ttemplate\n> +\tint testFullConversion(R input, I expected)\n> +\t{\n> +\t\tI outInt;\n> +\t\tR outReal;\n> +\t\tint status;\n> +\n> +\t\tstatus = testFloatToFixed(input, expected, &outInt);\n> +\t\tif (status != TestPass)\n> +\t\t\treturn status;\n> +\n> +\t\tstatus = testFixedToFloat(outInt, input, &outReal);\n> +\t\tif (status != TestPass)\n> +\t\t\treturn status;\n> +\n> +\t\treturn TestPass;\n> +\t}\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\t * Third parameter is for testing forward, fourth parameter is\n> +\t\t * for testing reverse.\n> \t\t */\n> -\t\tstd::map testCases = {\n> -\t\t\t{ 7.992, 0x3ff },\n> -\t\t\t{ 7.992, 0xbff },\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\tstatic const std::tuple testCases[] = {\n> +\t\t\t{ 7.992, 0x3ff, true, true },\n> +\t\t\t{ 7.992, 0xbff, false, true },\n> +\t\t\t{ 0.2, 0x01a, true, true },\n> +\t\t\t{ -0.2, 0x7e6, true, true },\n> +\t\t\t{ -0.8, 0x79a, true, true },\n> +\t\t\t{ -0.4, 0x7cd, true, true },\n> +\t\t\t{ -1.4, 0x74d, true, true },\n> +\t\t\t{ -8, 0x400, true, true },\n> +\t\t\t{ 0, 0, true, true },\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\tdouble floating;\n> +\t\t\tuint16_t fixed;\n> +\t\t\tbool forward, backward;\n> +\t\t\tstd::tie(floating, fixed, forward, backward) = testCase;\n> +\t\t\tif (forward && backward)\n> +\t\t\t\tret = testFullConversion<4, 7, double, uint16_t>(floating, fixed);\n> +\t\t\telse if (forward)\n> +\t\t\t\tret = testFloatToFixed<4, 7, double, uint16_t>(floating, fixed);\n> +\t\t\telse if (backward)\n> +\t\t\t\tret = testFixedToFloat<4, 7, uint16_t, double>(fixed, floating);\n> +\n\nThis is quite involved for only one single exception. What about keeping\nthe old loop for all \"standard\" cases and adding a \n\n/* special case with a superfluous one in the unused bits */\nret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);\nif (ret != TestPass)\n\treturn ret;\n\nbelow the loop?\n\nDo as you like.\n\nReviewed-by: Stefan Klug \n\nCheers,\nStefan\n\n> \t\t\tif (ret != TestPass)\n> \t\t\t\treturn ret;\n> \t\t}\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 6AD7EBD87C\n\tfor ;\n\tWed, 5 Jun 2024 10:14:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEA5E6543C;\n\tWed, 5 Jun 2024 12:14:35 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1EC3061A33\n\tfor ;\n\tWed, 5 Jun 2024 12:14:34 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:dcc4:f2dc:93ce:ff8f])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6DDE016D4;\n\tWed, 5 Jun 2024 12:14:25 +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=\"T0TzR5rS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717582465;\n\tbh=0H/H2bognwdyKEg/Fw91YWGu9sJ8+rFfPuBlLJsoUTg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=T0TzR5rSSWKcfFC43gRaQSfhmGs6iLxJB1bUeKTFTHzhhsPQMqgpMJfUBNsGr1Sfo\n\tlS4bRG47fVKVXIsHnxzVfmmWC3iWQDLmUSoLxISwS9en0Ovl98YAUEd/iWvGx6Jt1C\n\tIQbQ0QTnh8Wra3BlIyQzRtM4cykFe4xBY48JfYsQ=","Date":"Wed, 5 Jun 2024 12:14:30 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"libcamera-devel@lists.libcamera.org, \n\tLaurent Pinchart ,\n\tpobrn@protonmail.com","Subject":"Re: [PATCH] test: ipa: rkisp1: utils: Fix floating and fixed point\n\tconversion test","Message-ID":"<3tignw3xbankkides7dg5dyclakpu2mcjpcagk4ngtkxxjqv3u@qx6u5pxeyqwk>","References":"<20240603125324.3472888-1-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240603125324.3472888-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":29786,"web_url":"https://patchwork.libcamera.org/comment/29786/","msgid":"<171762852489.2248009.8529103050679417970@ping.linuxembedded.co.uk>","date":"2024-06-05T23:02:04","subject":"Re: [PATCH] test: ipa: rkisp1: utils: Fix floating and fixed point\n\tconversion test","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-06-05 11:14:30)\n> Hi Paul,\n> \n> thanks for the patch.\n> \n> On Mon, Jun 03, 2024 at 09:53:24PM +0900, Paul Elder wrote:\n> > There was an issue where using map to store the test cases meant that\n> > the test for ignoring unused bits was skipped because of clashing keys.\n> > Fix this by changing the test to an array of tuples.\n> > \n> > While at it, reorganize the tests so that it's possible to test only\n> > reverse conversion, which was the case here to test that multiple fixed\n> > point numbers (due to unused bits) would result in the same floating\n> > point number.\n> > \n> > While at it, also change the arbitrary floating comparison precision to\n> > be more precise.\n> > \n> > Also fix a missing documentation brief.\n> > \n> > Fixes: 9d152e9c6 ipa: rkisp1: Add a helper to convert floating-point to\n> > fixed-point\n> > Signed-off-by: Paul Elder \n> > ---\n> > src/ipa/rkisp1/utils.cpp | 1 +\n> > test/ipa/rkisp1/rkisp1-utils.cpp | 97 +++++++++++++++++++++++---------\n> > 2 files changed, 70 insertions(+), 28 deletions(-)\n> > \n> > diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp\n> > index 960ec64e9..8edcf5bf7 100644\n> > --- a/src/ipa/rkisp1/utils.cpp\n> > +++ b/src/ipa/rkisp1/utils.cpp\n> > @@ -9,6 +9,7 @@\n> > \n> > /**\n> > * \\file utils.h\n> > + * \\brief Utility functions for rkisp1\n> > */\n> > \n> > namespace libcamera {\n> > diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp\n> > index e48f8d362..4757ca069 100644\n> > --- a/test/ipa/rkisp1/rkisp1-utils.cpp\n> > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp\n> > @@ -7,8 +7,8 @@\n> > \n> > #include \n> > #include \n> > -#include \n> > #include \n> > +#include \n> > \n> > #include \"../src/ipa/rkisp1/utils.h\"\n> > \n> > @@ -21,53 +21,94 @@ using namespace ipa::rkisp1;\n> > class RkISP1UtilsTest : public Test\n> > {\n> > protected:\n> > - template\n> > - int testSingleFixedPoint(double input, T expected)\n> > + /* R for real, I for integer */\n> > + template\n> > + int testFixedToFloat(I input, R expected, R *output = nullptr)\n> > {\n> > - T ret = utils::floatingToFixedPoint(input);\n> > - if (ret != expected) {\n> > - cerr << \"Expected \" << input << \" to convert to \"\n> > - << expected << \", got \" << ret << std::endl;\n> > + R out = utils::fixedToFloatingPoint(input);\n> > + if (output)\n> > + *output = out;\n> > + R prec = 1.0 / (1 << FracPrec);\n> > + if (std::abs(out - expected) > prec) {\n> > + cerr << \"Reverse conversion expected \" << input\n> > + << \" to convert to \" << expected\n> > + << \", got \" << out << std::endl;\n> > return TestFail;\n> > }\n> > \n> > - /*\n> > - * The precision check is fairly arbitrary but is based on what\n> > - * the rkisp1 is capable of in the crosstalk module.\n> > - */\n> > - double f = utils::fixedToFloatingPoint(ret);\n> > - if (std::abs(f - input) > 0.005) {\n> > - cerr << \"Reverse conversion expected \" << ret\n> > - << \" to convert to \" << input\n> > - << \", got \" << f << std::endl;\n> > + return TestPass;\n> > + }\n> > +\n> > + /* R for real, I for integer */\n> > + template\n> > + int testFloatToFixed(R input, I expected, I *output = nullptr)\n> > + {\n> > + I out = utils::floatingToFixedPoint(input);\n> > + if (output)\n> > + *output = out;\n> > + if (out != expected) {\n> > + cerr << \"Expected \" << input << \" to convert to \"\n> > + << expected << \", got \" << out << std::endl;\n> > return TestFail;\n> > }\n> > \n> > return TestPass;\n> > }\n> > \n> > + /* R for real, I for integer */\n> > + template\n> > + int testFullConversion(R input, I expected)\n> > + {\n> > + I outInt;\n> > + R outReal;\n> > + int status;\n> > +\n> > + status = testFloatToFixed(input, expected, &outInt);\n> > + if (status != TestPass)\n> > + return status;\n> > +\n> > + status = testFixedToFloat(outInt, input, &outReal);\n> > + if (status != TestPass)\n> > + return status;\n> > +\n> > + return TestPass;\n> > + }\n> > +\n> > +\n> > int testFixedPoint()\n> > {\n> > /*\n> > * The second 7.992 test is to test that unused bits don't\n> > * affect the result.\n> > + *\n> > + * Third parameter is for testing forward, fourth parameter is\n> > + * for testing reverse.\n> > */\n> > - std::map testCases = {\n> > - { 7.992, 0x3ff },\n> > - { 7.992, 0xbff },\n> > - { 0.2, 0x01a },\n> > - { -0.2, 0x7e6 },\n> > - { -0.8, 0x79a },\n> > - { -0.4, 0x7cd },\n> > - { -1.4, 0x74d },\n> > - { -8, 0x400 },\n> > - { 0, 0 },\n> > + static const std::tuple testCases[] = {\n> > + { 7.992, 0x3ff, true, true },\n> > + { 7.992, 0xbff, false, true },\n> > + { 0.2, 0x01a, true, true },\n> > + { -0.2, 0x7e6, true, true },\n> > + { -0.8, 0x79a, true, true },\n> > + { -0.4, 0x7cd, true, true },\n> > + { -1.4, 0x74d, true, true },\n> > + { -8, 0x400, true, true },\n> > + { 0, 0, true, true },\n> > };\n> > \n> > int ret;\n> > for (const auto &testCase : testCases) {\n> > - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,\n> > - testCase.second);\n> > + double floating;\n> > + uint16_t fixed;\n> > + bool forward, backward;\n> > + std::tie(floating, fixed, forward, backward) = testCase;\n> > + if (forward && backward)\n> > + ret = testFullConversion<4, 7, double, uint16_t>(floating, fixed);\n> > + else if (forward)\n> > + ret = testFloatToFixed<4, 7, double, uint16_t>(floating, fixed);\n> > + else if (backward)\n> > + ret = testFixedToFloat<4, 7, uint16_t, double>(fixed, floating);\n> > +\n> \n> This is quite involved for only one single exception. What about keeping\n> the old loop for all \"standard\" cases and adding a \n> \n> /* special case with a superfluous one in the unused bits */\n> ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);\n> if (ret != TestPass)\n> return ret;\n> \n> below the loop?\n> \n> Do as you like.\n\nGiven the complexity above, and 18 bools to determine one false\ncondition - I think I like this suggestion quite a bit!\n\n--\nKieran\n\n\n> \n> Reviewed-by: Stefan Klug \n> \n> Cheers,\n> Stefan\n> \n> > if (ret != TestPass)\n> > return ret;\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 6C333BDE6B\n\tfor ;\n\tWed, 5 Jun 2024 23:02:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5D9E36544E;\n\tThu, 6 Jun 2024 01:02:09 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57D7561A34\n\tfor ;\n\tThu, 6 Jun 2024 01:02:07 +0200 (CEST)","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 8560E908;\n\tThu, 6 Jun 2024 01:01:58 +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=\"MVZzq0uw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1717628518;\n\tbh=ze31eYfX0WjKetWRmPhnJpbpYufGL1GROOxfIlkJbzw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=MVZzq0uwtlmS/MqISdgxV6wjTcaI+o+/qpRjBaCd3rkuCml/aybsCck5avPHhaItO\n\t6A9XgUJm7XYHJiQdP2hwjvV4y61ZIWCy6x3n5Pl1eJWu/fbYLK+jb6Q2ah7vZ5kWwt\n\tGq866/z60mUC7TCq6BS8sUj0ls/KGRQJyf15lUcM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<3tignw3xbankkides7dg5dyclakpu2mcjpcagk4ngtkxxjqv3u@qx6u5pxeyqwk>","References":"<20240603125324.3472888-1-paul.elder@ideasonboard.com>\n\t<3tignw3xbankkides7dg5dyclakpu2mcjpcagk4ngtkxxjqv3u@qx6u5pxeyqwk>","Subject":"Re: [PATCH] test: ipa: rkisp1: utils: Fix floating and fixed point\n\tconversion test","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org,\n\tLaurent Pinchart ,\n\tpobrn@protonmail.com","To":"Paul Elder ,\n\tStefan Klug ","Date":"Thu, 06 Jun 2024 00:02:04 +0100","Message-ID":"<171762852489.2248009.8529103050679417970@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\" "}}]