[PATCH v4] ipa: rkisp1: Add a helper to convert floating-point to fixed-point
Kieran Bingham
kieran.bingham at ideasonboard.com
Fri May 31 00:50:43 CEST 2024
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 at 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 at 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 at 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 at 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 at 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
> >
More information about the libcamera-devel
mailing list