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

20163 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>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
Changes in v5:
- fix test comments
- add documentation for the actual parameters (only template parameters
  were documented previously)

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         | 42 +++++++++++++++
 src/ipa/rkisp1/utils.h           | 66 ++++++++++++++++++++++++
 test/ipa/meson.build             |  2 +
 test/ipa/rkisp1/meson.build      | 15 ++++++
 test/ipa/rkisp1/rkisp1-utils.cpp | 87 ++++++++++++++++++++++++++++++++
 6 files changed, 213 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,

Thank you for the patch, and sorry for the late review.

On Fri, May 31, 2024 at 03:51:21PM +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>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> Changes in v5:
> - fix test comments
> - add documentation for the actual parameters (only template parameters
>   were documented previously)
> 
> 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         | 42 +++++++++++++++
>  src/ipa/rkisp1/utils.h           | 66 ++++++++++++++++++++++++
>  test/ipa/meson.build             |  2 +
>  test/ipa/rkisp1/meson.build      | 15 ++++++
>  test/ipa/rkisp1/rkisp1-utils.cpp | 87 ++++++++++++++++++++++++++++++++
>  6 files changed, 213 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..960ec64e9
> --- /dev/null
> +++ b/src/ipa/rkisp1/utils.cpp
> @@ -0,0 +1,42 @@
> +/* 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

Missing \brief

> + */
> +
> +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
> + * \param number The floating point number to convert to fixed point
> + * \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
> + * \param number The fixed point number to convert to floating point
> + * \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@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..ed6f44e9b
> --- /dev/null
> +++ b/test/ipa/rkisp1/rkisp1-utils.cpp
> @@ -0,0 +1,87 @@
> +/* 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.

Shouldn't it be based on the fracPrec value instead ?

> +		 */
> +		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()
> +	{
> +		/*
> +		 * The second 7.992 test is to test that unused bits don't
> +		 * affect the result.
> +		 */
> +		std::map<double, uint16_t> testCases = {
> +			{ 7.992, 0x3FF },
> +			{ 7.992, 0xBFF },

I had a bit of a WTF moment, not understanding how the same double could
convert to two different fixed point values. Then I realized you're
using a std::map here, which doesn't support multiple items with the
same key.

What you need is a

		std::vector<std::pair<double, uint16_t>> testCases = {

(don't forget to include vector and utility instead of map). With that,
the test will fail.

As this patch has been merged already, could you send patches to fix
those issues ?

> +			{   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)

Hi


2024. június 2., vasárnap 2:05 keltezéssel, Laurent Pinchart <laurent.pinchart@ideasonboard.com> írta:

> Hi Paul,
> 
> Thank you for the patch, and sorry for the late review.
> 
> On Fri, May 31, 2024 at 03:51:21PM +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>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> >
> > ---
> > Changes in v5:
> > - fix test comments
> > - add documentation for the actual parameters (only template parameters
> >   were documented previously)
> >
> > 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
> > ---
> [...]
> > +	int testFixedPoint()
> > +	{
> > +		/*
> > +		 * The second 7.992 test is to test that unused bits don't
> > +		 * affect the result.
> > +		 */
> > +		std::map<double, uint16_t> testCases = {
> > +			{ 7.992, 0x3FF },
> > +			{ 7.992, 0xBFF },
> 
> I had a bit of a WTF moment, not understanding how the same double could
> convert to two different fixed point values. Then I realized you're
> using a std::map here, which doesn't support multiple items with the
> same key.
> 
> What you need is a
> 
> 		std::vector<std::pair<double, uint16_t>> testCases = {
> 
> (don't forget to include vector and utility instead of map). With that,
> the test will fail.
> 
> As this patch has been merged already, could you send patches to fix
> those issues ?

I think

  static const std::pair<double, uint16_t> testCases[] = { ... };

is more than sufficient here.


Regards,
Barnabás Pőcze


> 
> > +			{   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)
> 
> --
> Regards,
> 
> Laurent Pinchart
>

On Sun, Jun 02, 2024 at 12:10:17AM +0000, Barnabás Pőcze wrote:
> 2024. június 2., vasárnap 2:05 keltezéssel, Laurent Pinchart írta:
> 
> > Hi Paul,
> > 
> > Thank you for the patch, and sorry for the late review.
> > 
> > On Fri, May 31, 2024 at 03:51:21PM +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>
> > > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > >
> > > ---
> > > Changes in v5:
> > > - fix test comments
> > > - add documentation for the actual parameters (only template parameters
> > >   were documented previously)
> > >
> > > 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
> > > ---
> > [...]
> > > +	int testFixedPoint()
> > > +	{
> > > +		/*
> > > +		 * The second 7.992 test is to test that unused bits don't
> > > +		 * affect the result.
> > > +		 */
> > > +		std::map<double, uint16_t> testCases = {
> > > +			{ 7.992, 0x3FF },
> > > +			{ 7.992, 0xBFF },
> > 
> > I had a bit of a WTF moment, not understanding how the same double could
> > convert to two different fixed point values. Then I realized you're
> > using a std::map here, which doesn't support multiple items with the
> > same key.
> > 
> > What you need is a
> > 
> > 		std::vector<std::pair<double, uint16_t>> testCases = {
> > 
> > (don't forget to include vector and utility instead of map). With that,
> > the test will fail.
> > 
> > As this patch has been merged already, could you send patches to fix
> > those issues ?
> 
> I think
> 
>   static const std::pair<double, uint16_t> testCases[] = { ... };
> 
> is more than sufficient here.

Yes, or an std::array<>. As it's a test, I don't mind much either way.

> > > +			{   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)