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

[Laurent Pinchart]

Hi Jacopo,

On Mon, Aug 03, 2020 at 04:43:26PM +0200, Jacopo Mondi wrote:
> On Mon, Aug 03, 2020 at 05:25:47PM +0300, Laurent Pinchart wrote:
> > 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.

Ack on either option, as I said, pick the one you like best :-)

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