[PATCH] test: ipa: rkisp1: utils: Fix floating and fixed point conversion test

[Stefan Klug]

Hi Paul,

thanks for the patch.

On Mon, Jun 03, 2024 at 09:53:24PM +0900, Paul Elder wrote:
> There was an issue where using map to store the test cases meant that
> the test for ignoring unused bits was skipped because of clashing keys.
> Fix this by changing the test to an array of tuples.
> 
> While at it, reorganize the tests so that it's possible to test only
> reverse conversion, which was the case here to test that multiple fixed
> point numbers (due to unused bits) would result in the same floating
> point number.
> 
> While at it, also change the arbitrary floating comparison precision to
> be more precise.
> 
> Also fix a missing documentation brief.
> 
> Fixes: 9d152e9c6 ipa: rkisp1: Add a helper to convert floating-point to
> fixed-point
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>  src/ipa/rkisp1/utils.cpp         |  1 +
>  test/ipa/rkisp1/rkisp1-utils.cpp | 97 +++++++++++++++++++++++---------
>  2 files changed, 70 insertions(+), 28 deletions(-)
> 
> diff --git a/src/ipa/rkisp1/utils.cpp b/src/ipa/rkisp1/utils.cpp
> index 960ec64e9..8edcf5bf7 100644
> --- a/src/ipa/rkisp1/utils.cpp
> +++ b/src/ipa/rkisp1/utils.cpp
> @@ -9,6 +9,7 @@
>  
>  /**
>   * \file utils.h
> + * \brief Utility functions for rkisp1
>   */
>  
>  namespace libcamera {
> diff --git a/test/ipa/rkisp1/rkisp1-utils.cpp b/test/ipa/rkisp1/rkisp1-utils.cpp
> index e48f8d362..4757ca069 100644
> --- a/test/ipa/rkisp1/rkisp1-utils.cpp
> +++ b/test/ipa/rkisp1/rkisp1-utils.cpp
> @@ -7,8 +7,8 @@
>  
>  #include <cmath>
>  #include <iostream>
> -#include <map>
>  #include <stdint.h>
> +#include <tuple>
>  
>  #include "../src/ipa/rkisp1/utils.h"
>  
> @@ -21,53 +21,94 @@ using namespace ipa::rkisp1;
>  class RkISP1UtilsTest : public Test
>  {
>  protected:
> -	template<unsigned int IntPrec, unsigned FracPrec, typename T>
> -	int testSingleFixedPoint(double input, T expected)
> +	/* R for real, I for integer */
> +	template<unsigned int IntPrec, unsigned FracPrec, typename I, typename R>
> +	int testFixedToFloat(I input, R expected, R *output = nullptr)
>  	{
> -		T ret = utils::floatingToFixedPoint<IntPrec, FracPrec, T>(input);
> -		if (ret != expected) {
> -			cerr << "Expected " << input << " to convert to "
> -			     << expected << ", got " << ret << std::endl;
> +		R out = utils::fixedToFloatingPoint<IntPrec, FracPrec, R>(input);
> +		if (output)
> +			*output = out;
> +		R prec = 1.0 / (1 << FracPrec);
> +		if (std::abs(out - expected) > prec) {
> +			cerr << "Reverse conversion expected " << input
> +			     << " to convert to " << expected
> +			     << ", got " << out << 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 TestPass;
> +	}
> +
> +	/* R for real, I for integer */
> +	template<unsigned int IntPrec, unsigned FracPrec, typename R, typename I>
> +	int testFloatToFixed(R input, I expected, I *output = nullptr)
> +	{
> +		I out = utils::floatingToFixedPoint<IntPrec, FracPrec, I>(input);
> +		if (output)
> +			*output = out;
> +		if (out != expected) {
> +			cerr << "Expected " << input << " to convert to "
> +			     << expected << ", got " << out << std::endl;
>  			return TestFail;
>  		}
>  
>  		return TestPass;
>  	}
>  
> +	/* R for real, I for integer */
> +	template<unsigned int IntPrec, unsigned FracPrec, typename R, typename I>
> +	int testFullConversion(R input, I expected)
> +	{
> +		I outInt;
> +		R outReal;
> +		int status;
> +
> +		status = testFloatToFixed<IntPrec, FracPrec, R, I>(input, expected, &outInt);
> +		if (status != TestPass)
> +			return status;
> +
> +		status = testFixedToFloat<IntPrec, FracPrec, I, R>(outInt, input, &outReal);
> +		if (status != TestPass)
> +			return status;
> +
> +		return TestPass;
> +	}
> +
> +
>  	int testFixedPoint()
>  	{
>  		/*
>  		 * The second 7.992 test is to test that unused bits don't
>  		 * affect the result.
> +		 *
> +		 * Third parameter is for testing forward, fourth parameter is
> +		 * for testing reverse.
>  		 */
> -		std::map<double, uint16_t> testCases = {
> -			{ 7.992, 0x3ff },
> -			{ 7.992, 0xbff },
> -			{   0.2, 0x01a },
> -			{  -0.2, 0x7e6 },
> -			{  -0.8, 0x79a },
> -			{  -0.4, 0x7cd },
> -			{  -1.4, 0x74d },
> -			{    -8, 0x400 },
> -			{     0, 0 },
> +		static const std::tuple<double, uint16_t, bool, bool> testCases[] = {
> +			{ 7.992, 0x3ff,  true, true },
> +			{ 7.992, 0xbff, false, true },
> +			{   0.2, 0x01a,  true, true },
> +			{  -0.2, 0x7e6,  true, true },
> +			{  -0.8, 0x79a,  true, true },
> +			{  -0.4, 0x7cd,  true, true },
> +			{  -1.4, 0x74d,  true, true },
> +			{    -8, 0x400,  true, true },
> +			{     0,     0,  true, true },
>  		};
>  
>  		int ret;
>  		for (const auto &testCase : testCases) {
> -			ret = testSingleFixedPoint<4, 7, uint16_t>(testCase.first,
> -								   testCase.second);
> +			double floating;
> +			uint16_t fixed;
> +			bool forward, backward;
> +			std::tie(floating, fixed, forward, backward) = testCase;
> +			if (forward && backward)
> +				ret = testFullConversion<4, 7, double, uint16_t>(floating, fixed);
> +			else if (forward)
> +				ret = testFloatToFixed<4, 7, double, uint16_t>(floating, fixed);
> +			else if (backward)
> +				ret = testFixedToFloat<4, 7, uint16_t, double>(fixed, floating);
> +

This is quite involved for only one single exception. What about keeping
the old loop for all "standard" cases and adding a 

/* special case with a superfluous one in the unused bits */
ret = testFixedToFloat<4, 7, uint16_t, double>(0xbff, 7.992);
if (ret != TestPass)
	return ret;

below the loop?

Do as you like.

Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com> 

Cheers,
Stefan

>  			if (ret != TestPass)
>  				return ret;
>  		}
> -- 
> 2.39.2
>