[PATCH v3] ipa: rkisp1: Add a helper to convert floating-point to fixed-point
Stefan Klug
stefan.klug at ideasonboard.com
Thu May 30 13:16:56 CEST 2024
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.
> +}
> +
> +} /* 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)
If ever someone touches that code again, we shouldn't fall into these
traps.
Cheers,
Stefan
> + };
> +
> + 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