[PATCH v3] ipa: rkisp1: Add a helper to convert floating-point to fixed-point
Paul Elder
paul.elder at ideasonboard.com
Thu May 30 14:23:27 CEST 2024
On Thu, May 30, 2024 at 01:16:56PM +0200, Stefan Klug wrote:
> Hi Paul,
>
> thanks for the patch.
>
> On Thu, May 30, 2024 at 04:21:44AM +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 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 | 67 +++++++++++++++++++++++++++
> > test/ipa/meson.build | 2 +
> > test/ipa/rkisp1/meson.build | 15 ++++++
> > test/ipa/rkisp1/rkisp1-utils.cpp | 78 ++++++++++++++++++++++++++++++++
> > 6 files changed, 203 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..7d647d6d7
> > --- /dev/null
> > +++ b/src/ipa/rkisp1/utils.h
> > @@ -0,0 +1,67 @@
> > +/* 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);
> > +
> > + /*
> > + * Create a number with all upper bits set including the sign bit
> > + * of the fixed point number. We need an extra cast as the compiler
> > + * won't let us left-shift negative numbers.
> > + */
> > + int upper_ones = static_cast<int>(std::numeric_limits<unsigned int>::max() << (I + F - 1));
> > + if (number & upper_ones)
> > + return static_cast<R>(upper_ones | number) / static_cast<R>(1 << F);
> > +
> > + return static_cast<R>(number) / static_cast<R>(1 << F);
>
> Sorry for beeing a pest here. This fails in some cases. I wrote that
> here https://patchwork.libcamera.org/patch/19935/#29340 with a fixed
> (and possibly faster) version.
This version works fine for 0xBFF that you mention below, but indeed
your version there is more optimizable so I'll go with that.
>
> > +}
> > +
> > +} /* 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..c0c05dc68
> > --- /dev/null
> > +++ b/test/ipa/rkisp1/rkisp1-utils.cpp
> > @@ -0,0 +1,78 @@
> > +/* 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.001) {
> > + 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 },
> > + { -8, 0x400 },
> > + { 0, 0 },
>
> Could you include the two cases that led to additional rewrites of the
> function? So (-0.4, -1.4) and one test, where the value is positive
> but has a 1 in the uneeded bits (e.g. 0xBFF should result in 7.992)
>
Aaand I can't get -1.4 to pass the test...
Paul
> If ever someone touches that code again, we shouldn't fall into these
> traps.
>
> > + };
> > +
> > + 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