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

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

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)

-- 
Regards,

Laurent Pinchart