[libcamera-devel] [PATCH] libcamera: ipu3: Fix compilation issues with gcc5

[Jacopo Mondi]

Hi Laurent,

On Mon, Aug 03, 2020 at 05:25:47PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Mon, Aug 03, 2020 at 04:18:37PM +0200, Jacopo Mondi wrote:
> > GCC5 does not provide prototypes for the math library functions in
> > the std:: namespace.
> >
> > Remove the namespace specifier to fix build errors reported by
> > that compiler version.
> >
> > Fixes: 968ab9bad0ed ("libcamera: ipu3: imgu: Calculate ImgU pipe configuration")
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >
> > Laurent, can you test with gcc and confirm this fixes the issue ?
>
> It does fix the compilation issue, but... see below.
>
> > ---
> >  src/libcamera/pipeline/ipu3/imgu.cpp | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/imgu.cpp b/src/libcamera/pipeline/ipu3/imgu.cpp
> > index b7593ceb3672..671a798e69d0 100644
> > --- a/src/libcamera/pipeline/ipu3/imgu.cpp
> > +++ b/src/libcamera/pipeline/ipu3/imgu.cpp
> > @@ -94,7 +94,7 @@ float findScaleFactor(float sf, const std::vector<float> &range,
> >  	float bestDiff = std::numeric_limits<float>::max();
> >  	unsigned int index = 0;
> >  	for (unsigned int i = 0; i < range.size(); ++i) {
> > -		float diff = std::abs(sf - range[i]);
> > +		float diff = abs(sf - range[i]);
>
> The abs() function in the global namespace is, according to
> https://en.cppreference.com/w/c/numeric/math/abs,
>
> 	int abs( int n );
>
> This will thus cast the argument to int, which is likely not what you
> want. You need to use
>
> 	float fabsf( float arg );
>
> instead (https://en.cppreference.com/w/c/numeric/math/fabs).
>
> Same below.
>
> The math functions in the std:: namespace don't suffer for this, as
> multiple overloads are defined:
>
> int       abs( int n );
> long      abs( long n );
> long long abs( long long n );
>
> (https://en.cppreference.com/w/cpp/numeric/math/abs)
>
> float       abs( float arg );
> double      abs( double arg );
> long double abs( long double arg );
>
> (https://en.cppreference.com/w/cpp/numeric/math/fabs)
>
> but they require <cmath>, not <math.h>.
>
> It's up to you, but if you keep using math.h, you need fabsf() when the
> argument and result are floats. Similarly, fmod() should become fmodf(),
> otherwise you'll cast the argument to double and cast the result back to
> float.

We had a rule, to use <foo.h> and not <cfoo>

All the rest of the code uses <math.h> but I don't want to try&shoot
and introduce implicit casting errors in this rather fragile code.

With an ack, I'll use for this module <cmath>, contrary to the rest of
the codebase.

>
> >  		if (diff < bestDiff) {
> >  			bestDiff = diff;
> >  			index = i;
> > @@ -112,7 +112,7 @@ bool isSameRatio(const Size &in, const Size &out)
> >  	float inRatio = static_cast<float>(in.width) / in.height;
> >  	float outRatio = static_cast<float>(out.width) / out.height;
> >
> > -	if (std::abs(inRatio - outRatio) > 0.1)
> > +	if (abs(inRatio - outRatio) > 0.1)
> >  		return false;
> >
> >  	return true;
> > @@ -136,7 +136,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> >  		while (ifHeight >= minIFHeight && ifHeight / bdsSF >= minBDSHeight) {
> >
> >  			bdsHeight = ifHeight / bdsSF;
> > -			if (std::fmod(bdsHeight, 1.0) == 0) {
> > +			if (fmod(bdsHeight, 1.0) == 0) {
> >  				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> >
> >  				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > @@ -152,7 +152,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> >  		while (ifHeight <= iif.height && ifHeight / bdsSF >= minBDSHeight) {
> >
> >  			bdsHeight = ifHeight / bdsSF;
> > -			if (std::fmod(bdsHeight, 1.0) == 0) {
> > +			if (fmod(bdsHeight, 1.0) == 0) {
> >  				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> >
> >  				if (!(bdsIntHeight % BDS_ALIGN_H)) {
> > @@ -176,7 +176,7 @@ void calculateBDSHeight(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc
> >  		while (ifHeight > minIFHeight && ifHeight / bdsSF >= minBDSHeight) {
> >
> >  			bdsHeight = ifHeight / bdsSF;
> > -			if (std::fmod(ifHeight, 1.0) == 0 && std::fmod(bdsHeight, 1.0) == 0) {
> > +			if (fmod(ifHeight, 1.0) == 0 && fmod(bdsHeight, 1.0) == 0) {
> >  				unsigned int bdsIntHeight = static_cast<unsigned int>(bdsHeight);
> >
> >  				if (!(ifHeight % IF_ALIGN_H) &&
> > @@ -199,7 +199,7 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa
> >  	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> >  		float bdsWidth = static_cast<float>(iif.width) / sf;
> >
> > -		if (std::fmod(bdsWidth, 1.0) == 0) {
> > +		if (fmod(bdsWidth, 1.0) == 0) {
> >  			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> >  			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> >  				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
> > @@ -212,7 +212,7 @@ void calculateBDS(ImgUDevice::Pipe *pipe, const Size &iif, const Size &gdc, floa
> >  	while (sf <= BDS_SF_MAX && sf >= BDS_SF_MIN) {
> >  		float bdsWidth = static_cast<float>(iif.width) / sf;
> >
> > -		if (std::fmod(bdsWidth, 1.0) == 0) {
> > +		if (fmod(bdsWidth, 1.0) == 0) {
> >  			unsigned int bdsIntWidth = static_cast<unsigned int>(bdsWidth);
> >  			if (!(bdsIntWidth % BDS_ALIGN_W) && bdsWidth >= minBDSWidth)
> >  				calculateBDSHeight(pipe, iif, gdc, bdsIntWidth, sf);
>
> --
> Regards,
>
> Laurent Pinchart