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

[Barnabás Pőcze]

Hi


2024. június 2., vasárnap 2:05 keltezéssel, Laurent Pinchart <laurent.pinchart at 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 at ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at 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
>