[PATCH] test: ipa: rkisp1: utils: Fix floating and fixed point conversion test
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Jun 6 01:02:04 CEST 2024
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 at 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 at ideasonboard.com>
>
> Cheers,
> Stefan
>
> > if (ret != TestPass)
> > return ret;
> > }
> > --
> > 2.39.2
> >
More information about the libcamera-devel
mailing list