| Message ID | 20240603125324.3472888-1-paul.elder@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, thanks for the patch. On Mon, Jun 03, 2024 at 09:53:24PM +0900, Paul Elder wrote: > There was an issue where using map to store the test cases meant that > the test for ignoring unused bits was skipped because of clashing keys. > Fix this by changing the test to an array of tuples. > > While at it, reorganize the tests so that it's possible to test only > reverse conversion, which was the case here to test that multiple fixed > point numbers (due to unused bits) would result in the same floating > point number. > > While at it, also change the arbitrary floating comparison precision to > be more precise. > > Also fix a missing documentation brief. > > Fixes: 9d152e9c6 ipa: rkisp1: Add a helper to convert floating-point to > fixed-point > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/utils.cpp | 1 + > test/ipa/rkisp1/rkisp1-utils.cpp | 97 +++++++++++++++++++++++--------- > 2 files changed, 70 insertions(+), 28 deletions(-) > > diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp > index 960ec64e9..8edcf5bf7 100644 > --- a/src/ipa/rkisp1/utils.cpp > +++ b/src/ipa/rkisp1/utils.cpp > @@ -9,6 +9,7 @@ > > /** > * \file utils.h > + * \brief Utility functions for rkisp1 > */ > > namespace libcamera { > diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp > index e48f8d362..4757ca069 100644 > --- a/test/ipa/rkisp1/rkisp1-utils.cpp > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp > @@ -7,8 +7,8 @@ > > #include <cmath> > #include <iostream> > -#include <map> > #include <stdint.h> > +#include <tuple> > > #include "../src/ipa/rkisp1/utils.h" > > @@ -21,53 +21,94 @@ using namespace ipa::rkisp1; > class RkISP1UtilsTest : public Test > { > protected: > - template<unsigned int IntPrec, unsigned FracPrec, typename T> > - int testSingleFixedPoint(double input, T expected) > + /* R for real, I for integer */ > + template<unsigned int IntPrec, unsigned FracPrec, typename I, typename R> > + int testFixedToFloat(I input, R expected, R *output = nullptr) > { > - T ret = utils::floatingToFixedPoint<IntPrec, FracPrec, T>(input); > - if (ret != expected) { > - cerr << "Expected " << input << " to convert to " > - << expected << ", got " << ret << std::endl; > + R out = utils::fixedToFloatingPoint<IntPrec, FracPrec, R>(input); > + if (output) > + *output = out; > + R prec = 1.0 / (1 << FracPrec); > + if (std::abs(out - expected) > prec) { > + cerr << "Reverse conversion expected " << input > + << " to convert to " << expected > + << ", got " << out << std::endl; > return TestFail; > } > > - /* > - * The precision check is fairly arbitrary but is based on what > - * the rkisp1 is capable of in the crosstalk module. > - */ > - double f = utils::fixedToFloatingPoint<IntPrec, FracPrec, double>(ret); > - if (std::abs(f - input) > 0.005) { > - cerr << "Reverse conversion expected " << ret > - << " to convert to " << input > - << ", got " << f << std::endl; > + return TestPass; > + } > + > + /* R for real, I for integer */ > + template<unsigned int IntPrec, unsigned FracPrec, typename R, typename I> > + int testFloatToFixed(R input, I expected, I *output = nullptr) > + { > + I out = utils::floatingToFixedPoint<IntPrec, FracPrec, I>(input); > + if (output) > + *output = out; > + if (out != expected) { > + cerr << "Expected " << input << " to convert to " > + << expected << ", got " << out << std::endl; > return TestFail; > } > > return TestPass; > } > > + /* R for real, I for integer */ > + template<unsigned int IntPrec, unsigned FracPrec, typename R, typename I> > + int testFullConversion(R input, I expected) > + { > + I outInt; > + R outReal; > + int status; > + > + status = testFloatToFixed<IntPrec, FracPrec, R, I>(input, expected, &outInt); > + if (status != TestPass) > + return status; > + > + status = testFixedToFloat<IntPrec, FracPrec, I, R>(outInt, input, &outReal); > + if (status != TestPass) > + return status; > + > + return TestPass; > + } > + > + > int testFixedPoint() > { > /* > * The second 7.992 test is to test that unused bits don't > * affect the result. > + * > + * Third parameter is for testing forward, fourth parameter is > + * for testing reverse. > */ > - std::map<double, uint16_t> testCases = { > - { 7.992, 0x3ff }, > - { 7.992, 0xbff }, > - { 0.2, 0x01a }, > - { -0.2, 0x7e6 }, > - { -0.8, 0x79a }, > - { -0.4, 0x7cd }, > - { -1.4, 0x74d }, > - { -8, 0x400 }, > - { 0, 0 }, > + static const std::tuple<double, uint16_t, bool, bool> testCases[] = { > + { 7.992, 0x3ff, true, true }, > + { 7.992, 0xbff, false, true }, > + { 0.2, 0x01a, true, true }, > + { -0.2, 0x7e6, true, true }, > + { -0.8, 0x79a, true, true }, > + { -0.4, 0x7cd, true, true }, > + { -1.4, 0x74d, true, true }, > + { -8, 0x400, true, true }, > + { 0, 0, true, true }, > }; > > int ret; > for (const auto &testCase : testCases) { > - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, > - testCase.second); > + double floating; > + uint16_t fixed; > + bool forward, backward; > + std::tie(floating, fixed, forward, backward) = testCase; > + if (forward && backward) > + ret = testFullConversion<4, 7, double, uint16_t>(floating, fixed); > + else if (forward) > + ret = testFloatToFixed<4, 7, double, uint16_t>(floating, fixed); > + else if (backward) > + ret = testFixedToFloat<4, 7, uint16_t, double>(fixed, floating); > + This is quite involved for only one single exception. What about keeping the old loop for all "standard" cases and adding a /* special case with a superfluous one in the unused bits */ ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992); if (ret != TestPass) return ret; below the loop? Do as you like. Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > if (ret != TestPass) > return ret; > } > -- > 2.39.2 >
Quoting Stefan Klug (2024-06-05 11:14:30) > Hi Paul, > > thanks for the patch. > > On Mon, Jun 03, 2024 at 09:53:24PM +0900, Paul Elder wrote: > > There was an issue where using map to store the test cases meant that > > the test for ignoring unused bits was skipped because of clashing keys. > > Fix this by changing the test to an array of tuples. > > > > While at it, reorganize the tests so that it's possible to test only > > reverse conversion, which was the case here to test that multiple fixed > > point numbers (due to unused bits) would result in the same floating > > point number. > > > > While at it, also change the arbitrary floating comparison precision to > > be more precise. > > > > Also fix a missing documentation brief. > > > > Fixes: 9d152e9c6 ipa: rkisp1: Add a helper to convert floating-point to > > fixed-point > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > src/ipa/rkisp1/utils.cpp | 1 + > > test/ipa/rkisp1/rkisp1-utils.cpp | 97 +++++++++++++++++++++++--------- > > 2 files changed, 70 insertions(+), 28 deletions(-) > > > > diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp > > index 960ec64e9..8edcf5bf7 100644 > > --- a/src/ipa/rkisp1/utils.cpp > > +++ b/src/ipa/rkisp1/utils.cpp > > @@ -9,6 +9,7 @@ > > > > /** > > * \file utils.h > > + * \brief Utility functions for rkisp1 > > */ > > > > namespace libcamera { > > diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp > > index e48f8d362..4757ca069 100644 > > --- a/test/ipa/rkisp1/rkisp1-utils.cpp > > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp > > @@ -7,8 +7,8 @@ > > > > #include <cmath> > > #include <iostream> > > -#include <map> > > #include <stdint.h> > > +#include <tuple> > > > > #include "../src/ipa/rkisp1/utils.h" > > > > @@ -21,53 +21,94 @@ using namespace ipa::rkisp1; > > class RkISP1UtilsTest : public Test > > { > > protected: > > - template<unsigned int IntPrec, unsigned FracPrec, typename T> > > - int testSingleFixedPoint(double input, T expected) > > + /* R for real, I for integer */ > > + template<unsigned int IntPrec, unsigned FracPrec, typename I, typename R> > > + int testFixedToFloat(I input, R expected, R *output = nullptr) > > { > > - T ret = utils::floatingToFixedPoint<IntPrec, FracPrec, T>(input); > > - if (ret != expected) { > > - cerr << "Expected " << input << " to convert to " > > - << expected << ", got " << ret << std::endl; > > + R out = utils::fixedToFloatingPoint<IntPrec, FracPrec, R>(input); > > + if (output) > > + *output = out; > > + R prec = 1.0 / (1 << FracPrec); > > + if (std::abs(out - expected) > prec) { > > + cerr << "Reverse conversion expected " << input > > + << " to convert to " << expected > > + << ", got " << out << std::endl; > > return TestFail; > > } > > > > - /* > > - * The precision check is fairly arbitrary but is based on what > > - * the rkisp1 is capable of in the crosstalk module. > > - */ > > - double f = utils::fixedToFloatingPoint<IntPrec, FracPrec, double>(ret); > > - if (std::abs(f - input) > 0.005) { > > - cerr << "Reverse conversion expected " << ret > > - << " to convert to " << input > > - << ", got " << f << std::endl; > > + return TestPass; > > + } > > + > > + /* R for real, I for integer */ > > + template<unsigned int IntPrec, unsigned FracPrec, typename R, typename I> > > + int testFloatToFixed(R input, I expected, I *output = nullptr) > > + { > > + I out = utils::floatingToFixedPoint<IntPrec, FracPrec, I>(input); > > + if (output) > > + *output = out; > > + if (out != expected) { > > + cerr << "Expected " << input << " to convert to " > > + << expected << ", got " << out << std::endl; > > return TestFail; > > } > > > > return TestPass; > > } > > > > + /* R for real, I for integer */ > > + template<unsigned int IntPrec, unsigned FracPrec, typename R, typename I> > > + int testFullConversion(R input, I expected) > > + { > > + I outInt; > > + R outReal; > > + int status; > > + > > + status = testFloatToFixed<IntPrec, FracPrec, R, I>(input, expected, &outInt); > > + if (status != TestPass) > > + return status; > > + > > + status = testFixedToFloat<IntPrec, FracPrec, I, R>(outInt, input, &outReal); > > + if (status != TestPass) > > + return status; > > + > > + return TestPass; > > + } > > + > > + > > int testFixedPoint() > > { > > /* > > * The second 7.992 test is to test that unused bits don't > > * affect the result. > > + * > > + * Third parameter is for testing forward, fourth parameter is > > + * for testing reverse. > > */ > > - std::map<double, uint16_t> testCases = { > > - { 7.992, 0x3ff }, > > - { 7.992, 0xbff }, > > - { 0.2, 0x01a }, > > - { -0.2, 0x7e6 }, > > - { -0.8, 0x79a }, > > - { -0.4, 0x7cd }, > > - { -1.4, 0x74d }, > > - { -8, 0x400 }, > > - { 0, 0 }, > > + static const std::tuple<double, uint16_t, bool, bool> testCases[] = { > > + { 7.992, 0x3ff, true, true }, > > + { 7.992, 0xbff, false, true }, > > + { 0.2, 0x01a, true, true }, > > + { -0.2, 0x7e6, true, true }, > > + { -0.8, 0x79a, true, true }, > > + { -0.4, 0x7cd, true, true }, > > + { -1.4, 0x74d, true, true }, > > + { -8, 0x400, true, true }, > > + { 0, 0, true, true }, > > }; > > > > int ret; > > for (const auto &testCase : testCases) { > > - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, > > - testCase.second); > > + double floating; > > + uint16_t fixed; > > + bool forward, backward; > > + std::tie(floating, fixed, forward, backward) = testCase; > > + if (forward && backward) > > + ret = testFullConversion<4, 7, double, uint16_t>(floating, fixed); > > + else if (forward) > > + ret = testFloatToFixed<4, 7, double, uint16_t>(floating, fixed); > > + else if (backward) > > + ret = testFixedToFloat<4, 7, uint16_t, double>(fixed, floating); > > + > > This is quite involved for only one single exception. What about keeping > the old loop for all "standard" cases and adding a > > /* special case with a superfluous one in the unused bits */ > ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992); > if (ret != TestPass) > return ret; > > below the loop? > > Do as you like. Given the complexity above, and 18 bools to determine one false condition - I think I like this suggestion quite a bit! -- Kieran > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Cheers, > Stefan > > > if (ret != TestPass) > > return ret; > > } > > -- > > 2.39.2 > >
diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp index 960ec64e9..8edcf5bf7 100644 --- a/src/ipa/rkisp1/utils.cpp +++ b/src/ipa/rkisp1/utils.cpp @@ -9,6 +9,7 @@ /** * \file utils.h + * \brief Utility functions for rkisp1 */ namespace libcamera { diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp index e48f8d362..4757ca069 100644 --- a/test/ipa/rkisp1/rkisp1-utils.cpp +++ b/test/ipa/rkisp1/rkisp1-utils.cpp @@ -7,8 +7,8 @@ #include <cmath> #include <iostream> -#include <map> #include <stdint.h> +#include <tuple> #include "../src/ipa/rkisp1/utils.h" @@ -21,53 +21,94 @@ using namespace ipa::rkisp1; class RkISP1UtilsTest : public Test { protected: - template<unsigned int IntPrec, unsigned FracPrec, typename T> - int testSingleFixedPoint(double input, T expected) + /* R for real, I for integer */ + template<unsigned int IntPrec, unsigned FracPrec, typename I, typename R> + int testFixedToFloat(I input, R expected, R *output = nullptr) { - T ret = utils::floatingToFixedPoint<IntPrec, FracPrec, T>(input); - if (ret != expected) { - cerr << "Expected " << input << " to convert to " - << expected << ", got " << ret << std::endl; + R out = utils::fixedToFloatingPoint<IntPrec, FracPrec, R>(input); + if (output) + *output = out; + R prec = 1.0 / (1 << FracPrec); + if (std::abs(out - expected) > prec) { + cerr << "Reverse conversion expected " << input + << " to convert to " << expected + << ", got " << out << std::endl; return TestFail; } - /* - * The precision check is fairly arbitrary but is based on what - * the rkisp1 is capable of in the crosstalk module. - */ - double f = utils::fixedToFloatingPoint<IntPrec, FracPrec, double>(ret); - if (std::abs(f - input) > 0.005) { - cerr << "Reverse conversion expected " << ret - << " to convert to " << input - << ", got " << f << std::endl; + return TestPass; + } + + /* R for real, I for integer */ + template<unsigned int IntPrec, unsigned FracPrec, typename R, typename I> + int testFloatToFixed(R input, I expected, I *output = nullptr) + { + I out = utils::floatingToFixedPoint<IntPrec, FracPrec, I>(input); + if (output) + *output = out; + if (out != expected) { + cerr << "Expected " << input << " to convert to " + << expected << ", got " << out << std::endl; return TestFail; } return TestPass; } + /* R for real, I for integer */ + template<unsigned int IntPrec, unsigned FracPrec, typename R, typename I> + int testFullConversion(R input, I expected) + { + I outInt; + R outReal; + int status; + + status = testFloatToFixed<IntPrec, FracPrec, R, I>(input, expected, &outInt); + if (status != TestPass) + return status; + + status = testFixedToFloat<IntPrec, FracPrec, I, R>(outInt, input, &outReal); + if (status != TestPass) + return status; + + return TestPass; + } + + int testFixedPoint() { /* * The second 7.992 test is to test that unused bits don't * affect the result. + * + * Third parameter is for testing forward, fourth parameter is + * for testing reverse. */ - std::map<double, uint16_t> testCases = { - { 7.992, 0x3ff }, - { 7.992, 0xbff }, - { 0.2, 0x01a }, - { -0.2, 0x7e6 }, - { -0.8, 0x79a }, - { -0.4, 0x7cd }, - { -1.4, 0x74d }, - { -8, 0x400 }, - { 0, 0 }, + static const std::tuple<double, uint16_t, bool, bool> testCases[] = { + { 7.992, 0x3ff, true, true }, + { 7.992, 0xbff, false, true }, + { 0.2, 0x01a, true, true }, + { -0.2, 0x7e6, true, true }, + { -0.8, 0x79a, true, true }, + { -0.4, 0x7cd, true, true }, + { -1.4, 0x74d, true, true }, + { -8, 0x400, true, true }, + { 0, 0, true, true }, }; int ret; for (const auto &testCase : testCases) { - ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, - testCase.second); + double floating; + uint16_t fixed; + bool forward, backward; + std::tie(floating, fixed, forward, backward) = testCase; + if (forward && backward) + ret = testFullConversion<4, 7, double, uint16_t>(floating, fixed); + else if (forward) + ret = testFloatToFixed<4, 7, double, uint16_t>(floating, fixed); + else if (backward) + ret = testFixedToFloat<4, 7, uint16_t, double>(fixed, floating); + if (ret != TestPass) return ret; }
There was an issue where using map to store the test cases meant that the test for ignoring unused bits was skipped because of clashing keys. Fix this by changing the test to an array of tuples. While at it, reorganize the tests so that it's possible to test only reverse conversion, which was the case here to test that multiple fixed point numbers (due to unused bits) would result in the same floating point number. While at it, also change the arbitrary floating comparison precision to be more precise. Also fix a missing documentation brief. Fixes: 9d152e9c6 ipa: rkisp1: Add a helper to convert floating-point to fixed-point Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- src/ipa/rkisp1/utils.cpp | 1 + test/ipa/rkisp1/rkisp1-utils.cpp | 97 +++++++++++++++++++++++--------- 2 files changed, 70 insertions(+), 28 deletions(-)