[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:53:03 CEST 2024
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 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?
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 at 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
> > >
More information about the libcamera-devel
mailing list