[v3] ipa: rkisp1: Add a helper to convert floating-point to fixed-point

20125 diff mbox series

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 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

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@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@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@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@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
>

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@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@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@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@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
> >