ipa: rpi: Use std::abs()

21373 diff mbox series

As explained in the coding style document, usage of std::abs() is
preferred over abs() or fabs() as it picks the correct function based on
the argument type. Replace calls to abs() and fabs() with std::abs() in
the Raspberry Pi algorithms.

This fixes a reported warning from clang:

../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value]
        if (abs(denominator) > eps) {
            ^
../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead
        if (abs(denominator) > eps) {
            ^~~
            std::abs

Reported-by: Maarten Lankhorst <dev@lankhorst.se>
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
Naush, David, I have only compile-tested this patch, could you give it a
try to make sure it doesn't break anything ?
---
 src/ipa/rpi/controller/rpi/alsc.cpp | 18 +++++++++---------
 src/ipa/rpi/controller/rpi/awb.cpp  |  3 ++-
 2 files changed, 11 insertions(+), 10 deletions(-)


base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824

Hi all,


On Wed, 25 Sept 2024 at 23:12, Laurent Pinchart <
laurent.pinchart@ideasonboard.com> wrote:

> As explained in the coding style document, usage of std::abs() is
> preferred over abs() or fabs() as it picks the correct function based on
> the argument type. Replace calls to abs() and fabs() with std::abs() in
> the Raspberry Pi algorithms.
>
> This fixes a reported warning from clang:
>
> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute
> value function 'abs' when argument is of floating point type
> [-Werror,-Wabsolute-value]
>         if (abs(denominator) > eps) {
>             ^
> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs'
> instead
>         if (abs(denominator) > eps) {
>             ^~~
>             std::abs
>
> Reported-by: Maarten Lankhorst <dev@lankhorst.se>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Naush, David, I have only compile-tested this patch, could you give it a
> try to make sure it doesn't break anything ?
>

I've done some brief testing and nothing seems to go wrong running with
these changes.

Tested-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

---
>  src/ipa/rpi/controller/rpi/alsc.cpp | 18 +++++++++---------
>  src/ipa/rpi/controller/rpi/awb.cpp  |  3 ++-
>  2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/src/ipa/rpi/controller/rpi/alsc.cpp
> b/src/ipa/rpi/controller/rpi/alsc.cpp
> index 161fd45526ec..21edb819ad1c 100644
> --- a/src/ipa/rpi/controller/rpi/alsc.cpp
> +++ b/src/ipa/rpi/controller/rpi/alsc.cpp
> @@ -6,8 +6,8 @@
>   */
>
>  #include <algorithm>
> +#include <cmath>
>  #include <functional>
> -#include <math.h>
>  #include <numeric>
>  #include <vector>
>
> @@ -252,12 +252,12 @@ static bool compareModes(CameraMode const &cm0,
> CameraMode const &cm1)
>          */
>         if (cm0.transform != cm1.transform)
>                 return true;
> -       int leftDiff = abs(cm0.cropX - cm1.cropX);
> -       int topDiff = abs(cm0.cropY - cm1.cropY);
> -       int rightDiff = fabs(cm0.cropX + cm0.scaleX * cm0.width -
> -                            cm1.cropX - cm1.scaleX * cm1.width);
> -       int bottomDiff = fabs(cm0.cropY + cm0.scaleY * cm0.height -
> -                             cm1.cropY - cm1.scaleY * cm1.height);
> +       int leftDiff = std::abs(cm0.cropX - cm1.cropX);
> +       int topDiff = std::abs(cm0.cropY - cm1.cropY);
> +       int rightDiff = std::abs(cm0.cropX + cm0.scaleX * cm0.width -
> +                                cm1.cropX - cm1.scaleX * cm1.width);
> +       int bottomDiff = std::abs(cm0.cropY + cm0.scaleY * cm0.height -
> +                                 cm1.cropY - cm1.scaleY * cm1.height);
>         /*
>          * These thresholds are a rather arbitrary amount chosen to trigger
>          * when carrying on with the previously calculated tables might be
> @@ -732,7 +732,7 @@ static double gaussSeidel2Sor(const
> SparseArray<double> &M, double omega,
>         double maxDiff = 0;
>         for (i = 0; i < XY; i++) {
>                 lambda[i] = oldLambda[i] + (lambda[i] - oldLambda[i]) *
> omega;
> -               if (fabs(lambda[i] - oldLambda[i]) > fabs(maxDiff))
> +               if (std::abs(lambda[i] - oldLambda[i]) > std::abs(maxDiff))
>                         maxDiff = lambda[i] - oldLambda[i];
>         }
>         return maxDiff;
> @@ -764,7 +764,7 @@ static void runMatrixIterations(const Array2D<double>
> &C,
>         constructM(C, W, M);
>         double lastMaxDiff = std::numeric_limits<double>::max();
>         for (unsigned int i = 0; i < nIter; i++) {
> -               double maxDiff = fabs(gaussSeidel2Sor(M, omega, lambda,
> lambdaBound));
> +               double maxDiff = std::abs(gaussSeidel2Sor(M, omega,
> lambda, lambdaBound));
>                 if (maxDiff < threshold) {
>                         LOG(RPiAlsc, Debug)
>                                 << "Stop after " << i + 1 << " iterations";
> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp
> b/src/ipa/rpi/controller/rpi/awb.cpp
> index f45525bce2d1..e5d51092605e 100644
> --- a/src/ipa/rpi/controller/rpi/awb.cpp
> +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> @@ -6,6 +6,7 @@
>   */
>
>  #include <assert.h>
> +#include <cmath>
>  #include <functional>
>
>  #include <libcamera/base/log.h>
> @@ -505,7 +506,7 @@ static double interpolateQuadatric(ipa::Pwl::Point
> const &a, ipa::Pwl::Point con
>         const double eps = 1e-3;
>         ipa::Pwl::Point ca = c - a, ba = b - a;
>         double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x());
> -       if (abs(denominator) > eps) {
> +       if (std::abs(denominator) > eps) {
>                 double numerator = ba.y() * ca.x() * ca.x() - ca.y() *
> ba.x() * ba.x();
>                 double result = numerator / denominator + a.x();
>                 return std::max(a.x(), std::min(c.x(), result));
>
> base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824
> --
> Regards,
>
> Laurent Pinchart
>
>

Quoting Laurent Pinchart (2024-09-25 23:12:10)
> As explained in the coding style document, usage of std::abs() is
> preferred over abs() or fabs() as it picks the correct function based on
> the argument type. Replace calls to abs() and fabs() with std::abs() in
> the Raspberry Pi algorithms.
> 
> This fixes a reported warning from clang:
> 
> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: error: using integer absolute value function 'abs' when argument is of floating point type [-Werror,-Wabsolute-value]
>         if (abs(denominator) > eps) {
>             ^
> ../src/ipa/rpi/controller/rpi/awb.cpp:508:6: note: use function 'std::abs' instead
>         if (abs(denominator) > eps) {
>             ^~~
>             std::abs
> 
> Reported-by: Maarten Lankhorst <dev@lankhorst.se>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Naush, David, I have only compile-tested this patch, could you give it a
> try to make sure it doesn't break anything ?
> ---
>  src/ipa/rpi/controller/rpi/alsc.cpp | 18 +++++++++---------
>  src/ipa/rpi/controller/rpi/awb.cpp  |  3 ++-
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/src/ipa/rpi/controller/rpi/alsc.cpp b/src/ipa/rpi/controller/rpi/alsc.cpp
> index 161fd45526ec..21edb819ad1c 100644
> --- a/src/ipa/rpi/controller/rpi/alsc.cpp
> +++ b/src/ipa/rpi/controller/rpi/alsc.cpp
> @@ -6,8 +6,8 @@
>   */
>  
>  #include <algorithm>
> +#include <cmath>
>  #include <functional>
> -#include <math.h>

Oh, I see - this is where we're converting more of the math.h users too.



>  #include <numeric>
>  #include <vector>
>  
> @@ -252,12 +252,12 @@ static bool compareModes(CameraMode const &cm0, CameraMode const &cm1)
>          */
>         if (cm0.transform != cm1.transform)
>                 return true;
> -       int leftDiff = abs(cm0.cropX - cm1.cropX);
> -       int topDiff = abs(cm0.cropY - cm1.cropY);
> -       int rightDiff = fabs(cm0.cropX + cm0.scaleX * cm0.width -
> -                            cm1.cropX - cm1.scaleX * cm1.width);
> -       int bottomDiff = fabs(cm0.cropY + cm0.scaleY * cm0.height -
> -                             cm1.cropY - cm1.scaleY * cm1.height);
> +       int leftDiff = std::abs(cm0.cropX - cm1.cropX);
> +       int topDiff = std::abs(cm0.cropY - cm1.cropY);
> +       int rightDiff = std::abs(cm0.cropX + cm0.scaleX * cm0.width -
> +                                cm1.cropX - cm1.scaleX * cm1.width);
> +       int bottomDiff = std::abs(cm0.cropY + cm0.scaleY * cm0.height -
> +                                 cm1.cropY - cm1.scaleY * cm1.height);
>         /*
>          * These thresholds are a rather arbitrary amount chosen to trigger
>          * when carrying on with the previously calculated tables might be
> @@ -732,7 +732,7 @@ static double gaussSeidel2Sor(const SparseArray<double> &M, double omega,
>         double maxDiff = 0;
>         for (i = 0; i < XY; i++) {
>                 lambda[i] = oldLambda[i] + (lambda[i] - oldLambda[i]) * omega;
> -               if (fabs(lambda[i] - oldLambda[i]) > fabs(maxDiff))
> +               if (std::abs(lambda[i] - oldLambda[i]) > std::abs(maxDiff))
>                         maxDiff = lambda[i] - oldLambda[i];
>         }
>         return maxDiff;
> @@ -764,7 +764,7 @@ static void runMatrixIterations(const Array2D<double> &C,
>         constructM(C, W, M);
>         double lastMaxDiff = std::numeric_limits<double>::max();
>         for (unsigned int i = 0; i < nIter; i++) {
> -               double maxDiff = fabs(gaussSeidel2Sor(M, omega, lambda, lambdaBound));
> +               double maxDiff = std::abs(gaussSeidel2Sor(M, omega, lambda, lambdaBound));
>                 if (maxDiff < threshold) {
>                         LOG(RPiAlsc, Debug)
>                                 << "Stop after " << i + 1 << " iterations";
> diff --git a/src/ipa/rpi/controller/rpi/awb.cpp b/src/ipa/rpi/controller/rpi/awb.cpp
> index f45525bce2d1..e5d51092605e 100644
> --- a/src/ipa/rpi/controller/rpi/awb.cpp
> +++ b/src/ipa/rpi/controller/rpi/awb.cpp
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <assert.h>
> +#include <cmath>
>  #include <functional>
>  
>  #include <libcamera/base/log.h>
> @@ -505,7 +506,7 @@ static double interpolateQuadatric(ipa::Pwl::Point const &a, ipa::Pwl::Point con
>         const double eps = 1e-3;
>         ipa::Pwl::Point ca = c - a, ba = b - a;
>         double denominator = 2 * (ba.y() * ca.x() - ca.y() * ba.x());
> -       if (abs(denominator) > eps) {
> +       if (std::abs(denominator) > eps) {

Looks like an easy/straight-forward change.


Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>                 double numerator = ba.y() * ca.x() * ca.x() - ca.y() * ba.x() * ba.x();
>                 double result = numerator / denominator + a.x();
>                 return std::max(a.x(), std::min(c.x(), result));
> 
> base-commit: f2088eb91fd6477b152233b9031cb115ca1ae824
> -- 
> Regards,
> 
> Laurent Pinchart
>