[libcamera-devel] [PATCH v5 06/10] ipa: raspberry: replace abs() with std::abs()

[Naushir Patuck]

On Fri, 28 Oct 2022 at 11:27, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Quoting Nicholas Roth via libcamera-devel (2022-10-28 04:17:22)
> > From: Nicholas Roth <nicholas at rothemail.net>
> >
> > pwl.cpp uses abs() instead of std::abs(), which causes unexpected
> > behavior in the Clang compiler used for Android. Replace with
> > C++-standard absolute value function std::abs(), which supports
> > double-precision absolute values in a standard way.
> >
> > Signed-off-by: Nicholas Roth <nicholas at rothemail.net>
>
> From Jacopo on v1:
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> And from me:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> Though we really might have to do a bit of extra validation here:
>
>
> https://stackoverflow.com/questions/21392627/abs-vs-stdabs-what-does-the-reference-say
>
> Seems a bit worrying.
>
> Naush - any idea here?


I think this code change ought to be safe - we are including cmath header,
and dy is a double,
so the compiler uses the double abs(double arg) signature.

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


>
>
>
> > ---
> >  src/ipa/raspberrypi/controller/pwl.cpp | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp
> b/src/ipa/raspberrypi/controller/pwl.cpp
> > index c59f5fa1..70c2e24b 100644
> > --- a/src/ipa/raspberrypi/controller/pwl.cpp
> > +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include <cassert>
> > +#include <cmath>
> >  #include <stdexcept>
> >
> >  #include "pwl.h"
> > @@ -168,7 +169,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> const
> >         while (thisSpan != (int)points_.size() - 1) {
> >                 double dx = points_[thisSpan + 1].x -
> points_[thisSpan].x,
> >                        dy = points_[thisSpan + 1].y -
> points_[thisSpan].y;
> > -               if (abs(dy) > eps &&
> > +               if (std::abs(dy) > eps &&
> >                     otherSpan + 1 < (int)other.points_.size() &&
> >                     points_[thisSpan + 1].y >=
> >                             other.points_[otherSpan + 1].x + eps) {
> > @@ -181,7 +182,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> const
> >                                  points_[thisSpan].y) *
> >                                         dx / dy;
> >                         thisY = other.points_[++otherSpan].x;
> > -               } else if (abs(dy) > eps && otherSpan > 0 &&
> > +               } else if (std::abs(dy) > eps && otherSpan > 0 &&
> >                            points_[thisSpan + 1].y <=
> >                                    other.points_[otherSpan - 1].x - eps)
> {
> >                         /*
> > --
> > 2.34.1
> >
>

On Fri, 28 Oct 2022 at 11:27, Kieran Bingham <
kieran.bingham at ideasonboard.com> wrote:

> Quoting Nicholas Roth via libcamera-devel (2022-10-28 04:17:22)
> > From: Nicholas Roth <nicholas at rothemail.net>
> >
> > pwl.cpp uses abs() instead of std::abs(), which causes unexpected
> > behavior in the Clang compiler used for Android. Replace with
> > C++-standard absolute value function std::abs(), which supports
> > double-precision absolute values in a standard way.
> >
> > Signed-off-by: Nicholas Roth <nicholas at rothemail.net>
>
> From Jacopo on v1:
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> And from me:
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> Though we really might have to do a bit of extra validation here:
>
>
> https://stackoverflow.com/questions/21392627/abs-vs-stdabs-what-does-the-reference-say
>
> Seems a bit worrying.
>
> Naush - any idea here?
>
>
> > ---
> >  src/ipa/raspberrypi/controller/pwl.cpp | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp
> b/src/ipa/raspberrypi/controller/pwl.cpp
> > index c59f5fa1..70c2e24b 100644
> > --- a/src/ipa/raspberrypi/controller/pwl.cpp
> > +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include <cassert>
> > +#include <cmath>
> >  #include <stdexcept>
> >
> >  #include "pwl.h"
> > @@ -168,7 +169,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> const
> >         while (thisSpan != (int)points_.size() - 1) {
> >                 double dx = points_[thisSpan + 1].x -
> points_[thisSpan].x,
> >                        dy = points_[thisSpan + 1].y -
> points_[thisSpan].y;
> > -               if (abs(dy) > eps &&
> > +               if (std::abs(dy) > eps &&
> >                     otherSpan + 1 < (int)other.points_.size() &&
> >                     points_[thisSpan + 1].y >=
> >                             other.points_[otherSpan + 1].x + eps) {
> > @@ -181,7 +182,7 @@ Pwl Pwl::compose(Pwl const &other, const double eps)
> const
> >                                  points_[thisSpan].y) *
> >                                         dx / dy;
> >                         thisY = other.points_[++otherSpan].x;
> > -               } else if (abs(dy) > eps && otherSpan > 0 &&
> > +               } else if (std::abs(dy) > eps && otherSpan > 0 &&
> >                            points_[thisSpan + 1].y <=
> >                                    other.points_[otherSpan - 1].x - eps)
> {
> >                         /*
> > --
> > 2.34.1
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20221028/922c8ba3/attachment.htm>