| Message ID | 20240530123934.2325922-1-paul.elder@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Paul, thanks for the fast update. On Thu, May 30, 2024 at 09:39:34PM +0900, Paul Elder wrote: > Add helper functions for converting between floating point and fixed > point numbers. Also add tests for them. > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > --- > Changes in v4: > - relax the test case floating point precision > - optimize the fixed -> floating point converter > > Changes in v3: > - (used to be "libcamera: utils: Add a helper to convert floating-point to fixed-point") > - move the everything from libcamera global utils to the rkisp1 ipa > > Changes in v2: > - added the reverse conversion function (fixed -> floating) > - added tests > - make the conversion code cleaner > --- > src/ipa/rkisp1/meson.build | 1 + > src/ipa/rkisp1/utils.cpp | 40 +++++++++++++++ > src/ipa/rkisp1/utils.h | 66 +++++++++++++++++++++++++ > test/ipa/meson.build | 2 + > test/ipa/rkisp1/meson.build | 15 ++++++ > test/ipa/rkisp1/rkisp1-utils.cpp | 84 ++++++++++++++++++++++++++++++++ > 6 files changed, 208 insertions(+) > create mode 100644 src/ipa/rkisp1/utils.cpp > create mode 100644 src/ipa/rkisp1/utils.h > create mode 100644 test/ipa/rkisp1/meson.build > create mode 100644 test/ipa/rkisp1/rkisp1-utils.cpp > > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build > index e813da53a..cf05cdb27 100644 > --- a/src/ipa/rkisp1/meson.build > +++ b/src/ipa/rkisp1/meson.build > @@ -8,6 +8,7 @@ ipa_name = 'ipa_rkisp1' > rkisp1_ipa_sources = files([ > 'ipa_context.cpp', > 'rkisp1.cpp', > + 'utils.cpp', > ]) > > rkisp1_ipa_sources += rkisp1_ipa_algorithms > diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp > new file mode 100644 > index 000000000..53186fdd6 > --- /dev/null > +++ b/src/ipa/rkisp1/utils.cpp > @@ -0,0 +1,40 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> > + * > + * Miscellaneous utility functions specific to rkisp1 > + */ > + > +#include "utils.h" > + > +/** > + * \file utils.h > + */ > + > +namespace libcamera { > + > +namespace ipa::rkisp1::utils { > + > +/** > + * \fn R floatingToFixedPoint(T number) > + * \brief Convert a floating point number to a fixed-point representation > + * \tparam I Bit width of the integer part of the fixed-point > + * \tparam F Bit width of the fractional part of the fixed-point > + * \tparam R Return type of the fixed-point representation > + * \tparam T Input type of the floating point representation > + * \return The converted value > + */ > + > +/** > + * \fn R fixedToFloatingPoint(T number) > + * \brief Convert a fixed-point number to a floating point representation > + * \tparam I Bit width of the integer part of the fixed-point > + * \tparam F Bit width of the fractional part of the fixed-point > + * \tparam R Return type of the floating point representation > + * \tparam T Input type of the fixed-point representation > + * \return The converted value > + */ > + > +} /* namespace ipa::rkisp1::utils */ > + > +} /* namespace libcamera */ > diff --git a/src/ipa/rkisp1/utils.h b/src/ipa/rkisp1/utils.h > new file mode 100644 > index 000000000..450f22442 > --- /dev/null > +++ b/src/ipa/rkisp1/utils.h > @@ -0,0 +1,66 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> > + * > + * Miscellaneous utility functions specific to rkisp1 > + */ > + > +#pragma once > + > +#include <cmath> > +#include <limits> > +#include <type_traits> > + > +namespace libcamera { > + > +namespace ipa::rkisp1::utils { > + > +#ifndef __DOXYGEN__ > +template<unsigned int I, unsigned int F, typename R, typename T, > + std::enable_if_t<std::is_integral_v<R> && > + std::is_floating_point_v<T>> * = nullptr> > +#else > +template<unsigned int I, unsigned int F, typename R, typename T> > +#endif > +constexpr R floatingToFixedPoint(T number) > +{ > + static_assert(sizeof(int) >= sizeof(R)); > + static_assert(I + F <= sizeof(R) * 8); > + > + /* > + * The intermediate cast to int is needed on arm platforms to properly > + * cast negative values. See > + * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/ > + */ > + R mask = (1 << (F + I)) - 1; > + R frac = static_cast<R>(static_cast<int>(std::round(number * (1 << F)))) & mask; > + > + return frac; > +} > + > +#ifndef __DOXYGEN__ > +template<unsigned int I, unsigned int F, typename R, typename T, > + std::enable_if_t<std::is_floating_point_v<R> && > + std::is_integral_v<T>> * = nullptr> > +#else > +template<unsigned int I, unsigned int F, typename R, typename T> > +#endif > +constexpr R fixedToFloatingPoint(T number) > +{ > + static_assert(sizeof(int) >= sizeof(T)); > + static_assert(I + F <= sizeof(T) * 8); > + > + /* > + * Recreate the upper bits in case of a negative number by shifting the sign > + * bit from the fixed point to the first bit of the unsigned and then right shifting > + * by the same amount which keeps the sign bit in place. > + * This can be optimized by the compiler quite well. > + */ > + int remaining_bits = sizeof(int) * 8 - (I + F); > + int t = static_cast<int>(static_cast<unsigned>(number) << remaining_bits) >> remaining_bits; > + return static_cast<R>(t) / static_cast<R>(1 << F); > +} > + > +} /* namespace ipa::rkisp1::utils */ > + > +} /* namespace libcamera */ > diff --git a/test/ipa/meson.build b/test/ipa/meson.build > index 180b0da0a..dc956284c 100644 > --- a/test/ipa/meson.build > +++ b/test/ipa/meson.build > @@ -1,5 +1,7 @@ > # SPDX-License-Identifier: CC0-1.0 > > +subdir('rkisp1') > + > ipa_test = [ > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']}, > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']}, > diff --git a/test/ipa/rkisp1/meson.build b/test/ipa/rkisp1/meson.build > new file mode 100644 > index 000000000..5ffc5dd60 > --- /dev/null > +++ b/test/ipa/rkisp1/meson.build > @@ -0,0 +1,15 @@ > +# SPDX-License-Identifier: CC0-1.0 > + > +rkisp1_ipa_test = [ > + {'name': 'rkisp1-utils', 'sources': ['rkisp1-utils.cpp']}, > +] > + > +foreach test : rkisp1_ipa_test > + exe = executable(test['name'], test['sources'], libcamera_generated_ipa_headers, > + dependencies : libcamera_private, > + link_with : [libipa, test_libraries], > + include_directories : [libipa_includes, test_includes_internal, > + '../../../src/ipa/rkisp1/']) > + > + test(test['name'], exe, suite : 'ipa') > +endforeach > diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp > new file mode 100644 > index 000000000..d23d87e0f > --- /dev/null > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp > @@ -0,0 +1,84 @@ > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > +/* > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> > + * > + * Miscellaneous utility tests > + */ > + > +#include <cmath> > +#include <iostream> > +#include <map> > + > +#include "../src/ipa/rkisp1/utils.h" > + > +#include "test.h" > + > +using namespace std; > +using namespace libcamera; > +using namespace ipa::rkisp1; > + > +class RkISP1UtilsTest : public Test > +{ > +protected: > + template<unsigned int intPrec, unsigned fracPrec, typename T> > + int testSingleFixedPoint(double input, T expected) > + { > + T ret = utils::floatingToFixedPoint<intPrec, fracPrec, T>(input); > + if (ret != expected) { > + cerr << "Expected " << input << " to convert to " > + << expected << ", got " << ret << 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 TestFail; > + } > + > + return TestPass; > + } > + > + int testFixedPoint() > + { > + /* These are the only cases that we know for certain */ Nit: This comment no longer holds :-) No need to send another patch. Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> Cheers, Stefan > + 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 }, > + }; > + > + int ret; > + for (const auto &testCase : testCases) { > + ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, > + testCase.second); > + if (ret != TestPass) > + return ret; > + } > + > + return TestPass; > + } > + > + int run() > + { > + /* fixed point conversion test */ > + if (testFixedPoint() != TestPass) > + return TestFail; > + > + return TestPass; > + } > +}; > + > +TEST_REGISTER(RkISP1UtilsTest) > -- > 2.39.2 >
Quoting Stefan Klug (2024-05-30 15:23:19) > Hi Paul, > > thanks for the fast update. > > On Thu, May 30, 2024 at 09:39:34PM +0900, Paul Elder wrote: > > Add helper functions for converting between floating point and fixed > > point numbers. Also add tests for them. > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > --- > > Changes in v4: > > - relax the test case floating point precision > > - optimize the fixed -> floating point converter > > > > Changes in v3: > > - (used to be "libcamera: utils: Add a helper to convert floating-point to fixed-point") > > - move the everything from libcamera global utils to the rkisp1 ipa > > > > Changes in v2: > > - added the reverse conversion function (fixed -> floating) > > - added tests > > - make the conversion code cleaner > > --- > > src/ipa/rkisp1/meson.build | 1 + > > src/ipa/rkisp1/utils.cpp | 40 +++++++++++++++ > > src/ipa/rkisp1/utils.h | 66 +++++++++++++++++++++++++ > > test/ipa/meson.build | 2 + > > test/ipa/rkisp1/meson.build | 15 ++++++ > > test/ipa/rkisp1/rkisp1-utils.cpp | 84 ++++++++++++++++++++++++++++++++ > > 6 files changed, 208 insertions(+) > > create mode 100644 src/ipa/rkisp1/utils.cpp > > create mode 100644 src/ipa/rkisp1/utils.h > > create mode 100644 test/ipa/rkisp1/meson.build > > create mode 100644 test/ipa/rkisp1/rkisp1-utils.cpp > > > > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build > > index e813da53a..cf05cdb27 100644 > > --- a/src/ipa/rkisp1/meson.build > > +++ b/src/ipa/rkisp1/meson.build > > @@ -8,6 +8,7 @@ ipa_name = 'ipa_rkisp1' > > rkisp1_ipa_sources = files([ > > 'ipa_context.cpp', > > 'rkisp1.cpp', > > + 'utils.cpp', > > ]) > > > > rkisp1_ipa_sources += rkisp1_ipa_algorithms > > diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp > > new file mode 100644 > > index 000000000..53186fdd6 > > --- /dev/null > > +++ b/src/ipa/rkisp1/utils.cpp > > @@ -0,0 +1,40 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> > > + * > > + * Miscellaneous utility functions specific to rkisp1 > > + */ > > + > > +#include "utils.h" > > + > > +/** > > + * \file utils.h > > + */ > > + > > +namespace libcamera { > > + > > +namespace ipa::rkisp1::utils { > > + > > +/** > > + * \fn R floatingToFixedPoint(T number) > > + * \brief Convert a floating point number to a fixed-point representation > > + * \tparam I Bit width of the integer part of the fixed-point > > + * \tparam F Bit width of the fractional part of the fixed-point > > + * \tparam R Return type of the fixed-point representation > > + * \tparam T Input type of the floating point representation > > + * \return The converted value > > + */ > > + > > +/** > > + * \fn R fixedToFloatingPoint(T number) > > + * \brief Convert a fixed-point number to a floating point representation > > + * \tparam I Bit width of the integer part of the fixed-point > > + * \tparam F Bit width of the fractional part of the fixed-point > > + * \tparam R Return type of the floating point representation > > + * \tparam T Input type of the fixed-point representation > > + * \return The converted value > > + */ > > + > > +} /* namespace ipa::rkisp1::utils */ > > + > > +} /* namespace libcamera */ > > diff --git a/src/ipa/rkisp1/utils.h b/src/ipa/rkisp1/utils.h > > new file mode 100644 > > index 000000000..450f22442 > > --- /dev/null > > +++ b/src/ipa/rkisp1/utils.h > > @@ -0,0 +1,66 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> > > + * > > + * Miscellaneous utility functions specific to rkisp1 > > + */ > > + > > +#pragma once > > + > > +#include <cmath> > > +#include <limits> > > +#include <type_traits> > > + > > +namespace libcamera { > > + > > +namespace ipa::rkisp1::utils { > > + > > +#ifndef __DOXYGEN__ > > +template<unsigned int I, unsigned int F, typename R, typename T, > > + std::enable_if_t<std::is_integral_v<R> && > > + std::is_floating_point_v<T>> * = nullptr> > > +#else > > +template<unsigned int I, unsigned int F, typename R, typename T> > > +#endif > > +constexpr R floatingToFixedPoint(T number) > > +{ > > + static_assert(sizeof(int) >= sizeof(R)); > > + static_assert(I + F <= sizeof(R) * 8); > > + > > + /* > > + * The intermediate cast to int is needed on arm platforms to properly > > + * cast negative values. See > > + * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/ > > + */ > > + R mask = (1 << (F + I)) - 1; > > + R frac = static_cast<R>(static_cast<int>(std::round(number * (1 << F)))) & mask; > > + > > + return frac; > > +} > > + > > +#ifndef __DOXYGEN__ > > +template<unsigned int I, unsigned int F, typename R, typename T, > > + std::enable_if_t<std::is_floating_point_v<R> && > > + std::is_integral_v<T>> * = nullptr> > > +#else > > +template<unsigned int I, unsigned int F, typename R, typename T> > > +#endif > > +constexpr R fixedToFloatingPoint(T number) > > +{ > > + static_assert(sizeof(int) >= sizeof(T)); > > + static_assert(I + F <= sizeof(T) * 8); > > + > > + /* > > + * Recreate the upper bits in case of a negative number by shifting the sign > > + * bit from the fixed point to the first bit of the unsigned and then right shifting > > + * by the same amount which keeps the sign bit in place. > > + * This can be optimized by the compiler quite well. > > + */ > > + int remaining_bits = sizeof(int) * 8 - (I + F); > > + int t = static_cast<int>(static_cast<unsigned>(number) << remaining_bits) >> remaining_bits; > > + return static_cast<R>(t) / static_cast<R>(1 << F); > > +} > > + > > +} /* namespace ipa::rkisp1::utils */ > > + > > +} /* namespace libcamera */ > > diff --git a/test/ipa/meson.build b/test/ipa/meson.build > > index 180b0da0a..dc956284c 100644 > > --- a/test/ipa/meson.build > > +++ b/test/ipa/meson.build > > @@ -1,5 +1,7 @@ > > # SPDX-License-Identifier: CC0-1.0 > > > > +subdir('rkisp1') > > + > > ipa_test = [ > > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']}, > > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']}, > > diff --git a/test/ipa/rkisp1/meson.build b/test/ipa/rkisp1/meson.build > > new file mode 100644 > > index 000000000..5ffc5dd60 > > --- /dev/null > > +++ b/test/ipa/rkisp1/meson.build > > @@ -0,0 +1,15 @@ > > +# SPDX-License-Identifier: CC0-1.0 > > + > > +rkisp1_ipa_test = [ > > + {'name': 'rkisp1-utils', 'sources': ['rkisp1-utils.cpp']}, > > +] > > + > > +foreach test : rkisp1_ipa_test > > + exe = executable(test['name'], test['sources'], libcamera_generated_ipa_headers, > > + dependencies : libcamera_private, > > + link_with : [libipa, test_libraries], > > + include_directories : [libipa_includes, test_includes_internal, > > + '../../../src/ipa/rkisp1/']) > > + > > + test(test['name'], exe, suite : 'ipa') > > +endforeach > > diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp > > new file mode 100644 > > index 000000000..d23d87e0f > > --- /dev/null > > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp > > @@ -0,0 +1,84 @@ > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > +/* > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> > > + * > > + * Miscellaneous utility tests > > + */ > > + > > +#include <cmath> > > +#include <iostream> > > +#include <map> > > + > > +#include "../src/ipa/rkisp1/utils.h" > > + > > +#include "test.h" > > + > > +using namespace std; > > +using namespace libcamera; > > +using namespace ipa::rkisp1; > > + > > +class RkISP1UtilsTest : public Test > > +{ > > +protected: > > + template<unsigned int intPrec, unsigned fracPrec, typename T> > > + int testSingleFixedPoint(double input, T expected) > > + { > > + T ret = utils::floatingToFixedPoint<intPrec, fracPrec, T>(input); > > + if (ret != expected) { > > + cerr << "Expected " << input << " to convert to " > > + << expected << ", got " << ret << 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 TestFail; > > + } > > + > > + return TestPass; > > + } > > + > > + int testFixedPoint() > > + { > > + /* These are the only cases that we know for certain */ > > Nit: This comment no longer holds :-) > > No need to send another patch. > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > Cheers, > Stefan > > > + std::map<double, uint16_t> testCases = { > > + { 7.992, 0x3FF }, > > + { 7.992, 0xBFF }, Is 7.992 really both expected values? How does this pass ... am I missing something obvious? I really feel like I'm mis-understanding something here ... I ... assume this passes the test right? If this passes, can you add a comment explaining what this value is testing in both cases please? -- Kieran > > + { 0.2, 0x01A }, > > + { -0.2, 0x7E6 }, > > + { -0.8, 0x79A }, > > + { -0.4, 0x7CD }, > > + { -1.4, 0x74D }, > > + { -8, 0x400 }, > > + { 0, 0 }, > > + }; > > + > > + int ret; > > + for (const auto &testCase : testCases) { > > + ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, > > + testCase.second); > > + if (ret != TestPass) > > + return ret; > > + } > > + > > + return TestPass; > > + } > > + > > + int run() > > + { > > + /* fixed point conversion test */ > > + if (testFixedPoint() != TestPass) > > + return TestFail; > > + > > + return TestPass; > > + } > > +}; > > + > > +TEST_REGISTER(RkISP1UtilsTest) > > -- > > 2.39.2 > >
Quoting Kieran Bingham (2024-05-30 23:50:43) > Quoting Stefan Klug (2024-05-30 15:23:19) > > Hi Paul, > > > > thanks for the fast update. > > > > On Thu, May 30, 2024 at 09:39:34PM +0900, Paul Elder wrote: > > > Add helper functions for converting between floating point and fixed > > > point numbers. Also add tests for them. > > > > > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> > > > --- > > > Changes in v4: > > > - relax the test case floating point precision > > > - optimize the fixed -> floating point converter > > > > > > Changes in v3: > > > - (used to be "libcamera: utils: Add a helper to convert floating-point to fixed-point") > > > - move the everything from libcamera global utils to the rkisp1 ipa > > > > > > Changes in v2: > > > - added the reverse conversion function (fixed -> floating) > > > - added tests > > > - make the conversion code cleaner > > > --- > > > src/ipa/rkisp1/meson.build | 1 + > > > src/ipa/rkisp1/utils.cpp | 40 +++++++++++++++ > > > src/ipa/rkisp1/utils.h | 66 +++++++++++++++++++++++++ > > > test/ipa/meson.build | 2 + > > > test/ipa/rkisp1/meson.build | 15 ++++++ > > > test/ipa/rkisp1/rkisp1-utils.cpp | 84 ++++++++++++++++++++++++++++++++ > > > 6 files changed, 208 insertions(+) > > > create mode 100644 src/ipa/rkisp1/utils.cpp > > > create mode 100644 src/ipa/rkisp1/utils.h > > > create mode 100644 test/ipa/rkisp1/meson.build > > > create mode 100644 test/ipa/rkisp1/rkisp1-utils.cpp > > > > > > diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build > > > index e813da53a..cf05cdb27 100644 > > > --- a/src/ipa/rkisp1/meson.build > > > +++ b/src/ipa/rkisp1/meson.build > > > @@ -8,6 +8,7 @@ ipa_name = 'ipa_rkisp1' > > > rkisp1_ipa_sources = files([ > > > 'ipa_context.cpp', > > > 'rkisp1.cpp', > > > + 'utils.cpp', > > > ]) > > > > > > rkisp1_ipa_sources += rkisp1_ipa_algorithms > > > diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp > > > new file mode 100644 > > > index 000000000..53186fdd6 > > > --- /dev/null > > > +++ b/src/ipa/rkisp1/utils.cpp > > > @@ -0,0 +1,40 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> > > > + * > > > + * Miscellaneous utility functions specific to rkisp1 > > > + */ > > > + > > > +#include "utils.h" > > > + > > > +/** > > > + * \file utils.h > > > + */ > > > + > > > +namespace libcamera { > > > + > > > +namespace ipa::rkisp1::utils { > > > + > > > +/** > > > + * \fn R floatingToFixedPoint(T number) > > > + * \brief Convert a floating point number to a fixed-point representation > > > + * \tparam I Bit width of the integer part of the fixed-point > > > + * \tparam F Bit width of the fractional part of the fixed-point > > > + * \tparam R Return type of the fixed-point representation > > > + * \tparam T Input type of the floating point representation > > > + * \return The converted value > > > + */ > > > + > > > +/** > > > + * \fn R fixedToFloatingPoint(T number) > > > + * \brief Convert a fixed-point number to a floating point representation > > > + * \tparam I Bit width of the integer part of the fixed-point > > > + * \tparam F Bit width of the fractional part of the fixed-point > > > + * \tparam R Return type of the floating point representation > > > + * \tparam T Input type of the fixed-point representation > > > + * \return The converted value > > > + */ > > > + > > > +} /* namespace ipa::rkisp1::utils */ > > > + > > > +} /* namespace libcamera */ > > > diff --git a/src/ipa/rkisp1/utils.h b/src/ipa/rkisp1/utils.h > > > new file mode 100644 > > > index 000000000..450f22442 > > > --- /dev/null > > > +++ b/src/ipa/rkisp1/utils.h > > > @@ -0,0 +1,66 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> > > > + * > > > + * Miscellaneous utility functions specific to rkisp1 > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <cmath> > > > +#include <limits> > > > +#include <type_traits> > > > + > > > +namespace libcamera { > > > + > > > +namespace ipa::rkisp1::utils { > > > + > > > +#ifndef __DOXYGEN__ > > > +template<unsigned int I, unsigned int F, typename R, typename T, > > > + std::enable_if_t<std::is_integral_v<R> && > > > + std::is_floating_point_v<T>> * = nullptr> > > > +#else > > > +template<unsigned int I, unsigned int F, typename R, typename T> > > > +#endif > > > +constexpr R floatingToFixedPoint(T number) > > > +{ > > > + static_assert(sizeof(int) >= sizeof(R)); > > > + static_assert(I + F <= sizeof(R) * 8); > > > + > > > + /* > > > + * The intermediate cast to int is needed on arm platforms to properly > > > + * cast negative values. See > > > + * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/ > > > + */ > > > + R mask = (1 << (F + I)) - 1; > > > + R frac = static_cast<R>(static_cast<int>(std::round(number * (1 << F)))) & mask; > > > + > > > + return frac; > > > +} > > > + > > > +#ifndef __DOXYGEN__ > > > +template<unsigned int I, unsigned int F, typename R, typename T, > > > + std::enable_if_t<std::is_floating_point_v<R> && > > > + std::is_integral_v<T>> * = nullptr> > > > +#else > > > +template<unsigned int I, unsigned int F, typename R, typename T> > > > +#endif > > > +constexpr R fixedToFloatingPoint(T number) > > > +{ > > > + static_assert(sizeof(int) >= sizeof(T)); > > > + static_assert(I + F <= sizeof(T) * 8); > > > + > > > + /* > > > + * Recreate the upper bits in case of a negative number by shifting the sign > > > + * bit from the fixed point to the first bit of the unsigned and then right shifting > > > + * by the same amount which keeps the sign bit in place. > > > + * This can be optimized by the compiler quite well. > > > + */ > > > + int remaining_bits = sizeof(int) * 8 - (I + F); > > > + int t = static_cast<int>(static_cast<unsigned>(number) << remaining_bits) >> remaining_bits; > > > + return static_cast<R>(t) / static_cast<R>(1 << F); > > > +} > > > + > > > +} /* namespace ipa::rkisp1::utils */ > > > + > > > +} /* namespace libcamera */ > > > diff --git a/test/ipa/meson.build b/test/ipa/meson.build > > > index 180b0da0a..dc956284c 100644 > > > --- a/test/ipa/meson.build > > > +++ b/test/ipa/meson.build > > > @@ -1,5 +1,7 @@ > > > # SPDX-License-Identifier: CC0-1.0 > > > > > > +subdir('rkisp1') > > > + > > > ipa_test = [ > > > {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']}, > > > {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']}, > > > diff --git a/test/ipa/rkisp1/meson.build b/test/ipa/rkisp1/meson.build > > > new file mode 100644 > > > index 000000000..5ffc5dd60 > > > --- /dev/null > > > +++ b/test/ipa/rkisp1/meson.build > > > @@ -0,0 +1,15 @@ > > > +# SPDX-License-Identifier: CC0-1.0 > > > + > > > +rkisp1_ipa_test = [ > > > + {'name': 'rkisp1-utils', 'sources': ['rkisp1-utils.cpp']}, > > > +] > > > + > > > +foreach test : rkisp1_ipa_test > > > + exe = executable(test['name'], test['sources'], libcamera_generated_ipa_headers, > > > + dependencies : libcamera_private, > > > + link_with : [libipa, test_libraries], > > > + include_directories : [libipa_includes, test_includes_internal, > > > + '../../../src/ipa/rkisp1/']) > > > + > > > + test(test['name'], exe, suite : 'ipa') > > > +endforeach > > > diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp > > > new file mode 100644 > > > index 000000000..d23d87e0f > > > --- /dev/null > > > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp > > > @@ -0,0 +1,84 @@ > > > +/* SPDX-License-Identifier: GPL-2.0-or-later */ > > > +/* > > > + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> > > > + * > > > + * Miscellaneous utility tests > > > + */ > > > + > > > +#include <cmath> > > > +#include <iostream> > > > +#include <map> > > > + > > > +#include "../src/ipa/rkisp1/utils.h" > > > + > > > +#include "test.h" > > > + > > > +using namespace std; > > > +using namespace libcamera; > > > +using namespace ipa::rkisp1; > > > + > > > +class RkISP1UtilsTest : public Test > > > +{ > > > +protected: > > > + template<unsigned int intPrec, unsigned fracPrec, typename T> > > > + int testSingleFixedPoint(double input, T expected) > > > + { > > > + T ret = utils::floatingToFixedPoint<intPrec, fracPrec, T>(input); > > > + if (ret != expected) { > > > + cerr << "Expected " << input << " to convert to " > > > + << expected << ", got " << ret << 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 TestFail; > > > + } > > > + > > > + return TestPass; > > > + } > > > + > > > + int testFixedPoint() > > > + { > > > + /* These are the only cases that we know for certain */ > > > > Nit: This comment no longer holds :-) > > > > No need to send another patch. > > > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com> > > > > Cheers, > > Stefan > > > > > + std::map<double, uint16_t> testCases = { > > > + { 7.992, 0x3FF }, > > > + { 7.992, 0xBFF }, > > Is 7.992 really both expected values? How does this pass ... am I > missing something obvious? > > I really feel like I'm mis-understanding something here ... I ... assume > this passes the test right? If this passes, can you add a comment > explaining what this value is testing in both cases please? I see from v3 this is testing an unused bit doesn't affect the result. Certainly needs a comment explaining that I think.. with that (and the one rejected by Stefan above removed) Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > -- > Kieran > > > > > + { 0.2, 0x01A }, > > > + { -0.2, 0x7E6 }, > > > + { -0.8, 0x79A }, > > > + { -0.4, 0x7CD }, > > > + { -1.4, 0x74D }, > > > + { -8, 0x400 }, > > > + { 0, 0 }, > > > + }; > > > + > > > + int ret; > > > + for (const auto &testCase : testCases) { > > > + ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, > > > + testCase.second); > > > > > + if (ret != TestPass) > > > + return ret; > > > + } > > > + > > > + return TestPass; > > > + } > > > + > > > + int run() > > > + { > > > + /* fixed point conversion test */ > > > + if (testFixedPoint() != TestPass) > > > + return TestFail; > > > + > > > + return TestPass; > > > + } > > > +}; > > > + > > > +TEST_REGISTER(RkISP1UtilsTest) > > > -- > > > 2.39.2 > > >
diff --git a/src/ipa/rkisp1/meson.build b/src/ipa/rkisp1/meson.build index e813da53a..cf05cdb27 100644 --- a/src/ipa/rkisp1/meson.build +++ b/src/ipa/rkisp1/meson.build @@ -8,6 +8,7 @@ ipa_name = 'ipa_rkisp1' rkisp1_ipa_sources = files([ 'ipa_context.cpp', 'rkisp1.cpp', + 'utils.cpp', ]) rkisp1_ipa_sources += rkisp1_ipa_algorithms diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp new file mode 100644 index 000000000..53186fdd6 --- /dev/null +++ b/src/ipa/rkisp1/utils.cpp @@ -0,0 +1,40 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> + * + * Miscellaneous utility functions specific to rkisp1 + */ + +#include "utils.h" + +/** + * \file utils.h + */ + +namespace libcamera { + +namespace ipa::rkisp1::utils { + +/** + * \fn R floatingToFixedPoint(T number) + * \brief Convert a floating point number to a fixed-point representation + * \tparam I Bit width of the integer part of the fixed-point + * \tparam F Bit width of the fractional part of the fixed-point + * \tparam R Return type of the fixed-point representation + * \tparam T Input type of the floating point representation + * \return The converted value + */ + +/** + * \fn R fixedToFloatingPoint(T number) + * \brief Convert a fixed-point number to a floating point representation + * \tparam I Bit width of the integer part of the fixed-point + * \tparam F Bit width of the fractional part of the fixed-point + * \tparam R Return type of the floating point representation + * \tparam T Input type of the fixed-point representation + * \return The converted value + */ + +} /* namespace ipa::rkisp1::utils */ + +} /* namespace libcamera */ diff --git a/src/ipa/rkisp1/utils.h b/src/ipa/rkisp1/utils.h new file mode 100644 index 000000000..450f22442 --- /dev/null +++ b/src/ipa/rkisp1/utils.h @@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> + * + * Miscellaneous utility functions specific to rkisp1 + */ + +#pragma once + +#include <cmath> +#include <limits> +#include <type_traits> + +namespace libcamera { + +namespace ipa::rkisp1::utils { + +#ifndef __DOXYGEN__ +template<unsigned int I, unsigned int F, typename R, typename T, + std::enable_if_t<std::is_integral_v<R> && + std::is_floating_point_v<T>> * = nullptr> +#else +template<unsigned int I, unsigned int F, typename R, typename T> +#endif +constexpr R floatingToFixedPoint(T number) +{ + static_assert(sizeof(int) >= sizeof(R)); + static_assert(I + F <= sizeof(R) * 8); + + /* + * The intermediate cast to int is needed on arm platforms to properly + * cast negative values. See + * https://embeddeduse.com/2013/08/25/casting-a-negative-float-to-an-unsigned-int/ + */ + R mask = (1 << (F + I)) - 1; + R frac = static_cast<R>(static_cast<int>(std::round(number * (1 << F)))) & mask; + + return frac; +} + +#ifndef __DOXYGEN__ +template<unsigned int I, unsigned int F, typename R, typename T, + std::enable_if_t<std::is_floating_point_v<R> && + std::is_integral_v<T>> * = nullptr> +#else +template<unsigned int I, unsigned int F, typename R, typename T> +#endif +constexpr R fixedToFloatingPoint(T number) +{ + static_assert(sizeof(int) >= sizeof(T)); + static_assert(I + F <= sizeof(T) * 8); + + /* + * Recreate the upper bits in case of a negative number by shifting the sign + * bit from the fixed point to the first bit of the unsigned and then right shifting + * by the same amount which keeps the sign bit in place. + * This can be optimized by the compiler quite well. + */ + int remaining_bits = sizeof(int) * 8 - (I + F); + int t = static_cast<int>(static_cast<unsigned>(number) << remaining_bits) >> remaining_bits; + return static_cast<R>(t) / static_cast<R>(1 << F); +} + +} /* namespace ipa::rkisp1::utils */ + +} /* namespace libcamera */ diff --git a/test/ipa/meson.build b/test/ipa/meson.build index 180b0da0a..dc956284c 100644 --- a/test/ipa/meson.build +++ b/test/ipa/meson.build @@ -1,5 +1,7 @@ # SPDX-License-Identifier: CC0-1.0 +subdir('rkisp1') + ipa_test = [ {'name': 'ipa_module_test', 'sources': ['ipa_module_test.cpp']}, {'name': 'ipa_interface_test', 'sources': ['ipa_interface_test.cpp']}, diff --git a/test/ipa/rkisp1/meson.build b/test/ipa/rkisp1/meson.build new file mode 100644 index 000000000..5ffc5dd60 --- /dev/null +++ b/test/ipa/rkisp1/meson.build @@ -0,0 +1,15 @@ +# SPDX-License-Identifier: CC0-1.0 + +rkisp1_ipa_test = [ + {'name': 'rkisp1-utils', 'sources': ['rkisp1-utils.cpp']}, +] + +foreach test : rkisp1_ipa_test + exe = executable(test['name'], test['sources'], libcamera_generated_ipa_headers, + dependencies : libcamera_private, + link_with : [libipa, test_libraries], + include_directories : [libipa_includes, test_includes_internal, + '../../../src/ipa/rkisp1/']) + + test(test['name'], exe, suite : 'ipa') +endforeach diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp new file mode 100644 index 000000000..d23d87e0f --- /dev/null +++ b/test/ipa/rkisp1/rkisp1-utils.cpp @@ -0,0 +1,84 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (C) 2024, Paul Elder <paul.elder@ideasonboard.com> + * + * Miscellaneous utility tests + */ + +#include <cmath> +#include <iostream> +#include <map> + +#include "../src/ipa/rkisp1/utils.h" + +#include "test.h" + +using namespace std; +using namespace libcamera; +using namespace ipa::rkisp1; + +class RkISP1UtilsTest : public Test +{ +protected: + template<unsigned int intPrec, unsigned fracPrec, typename T> + int testSingleFixedPoint(double input, T expected) + { + T ret = utils::floatingToFixedPoint<intPrec, fracPrec, T>(input); + if (ret != expected) { + cerr << "Expected " << input << " to convert to " + << expected << ", got " << ret << 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 TestFail; + } + + return TestPass; + } + + int testFixedPoint() + { + /* These are the only cases that we know for certain */ + 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 }, + }; + + int ret; + for (const auto &testCase : testCases) { + ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first, + testCase.second); + if (ret != TestPass) + return ret; + } + + return TestPass; + } + + int run() + { + /* fixed point conversion test */ + if (testFixedPoint() != TestPass) + return TestFail; + + return TestPass; + } +}; + +TEST_REGISTER(RkISP1UtilsTest)
Add helper functions for converting between floating point and fixed point numbers. Also add tests for them. Signed-off-by: Paul Elder <paul.elder@ideasonboard.com> --- Changes in v4: - relax the test case floating point precision - optimize the fixed -> floating point converter Changes in v3: - (used to be "libcamera: utils: Add a helper to convert floating-point to fixed-point") - move the everything from libcamera global utils to the rkisp1 ipa Changes in v2: - added the reverse conversion function (fixed -> floating) - added tests - make the conversion code cleaner --- src/ipa/rkisp1/meson.build | 1 + src/ipa/rkisp1/utils.cpp | 40 +++++++++++++++ src/ipa/rkisp1/utils.h | 66 +++++++++++++++++++++++++ test/ipa/meson.build | 2 + test/ipa/rkisp1/meson.build | 15 ++++++ test/ipa/rkisp1/rkisp1-utils.cpp | 84 ++++++++++++++++++++++++++++++++ 6 files changed, 208 insertions(+) create mode 100644 src/ipa/rkisp1/utils.cpp create mode 100644 src/ipa/rkisp1/utils.h create mode 100644 test/ipa/rkisp1/meson.build create mode 100644 test/ipa/rkisp1/rkisp1-utils.cpp