[3/4] ipa: libipa: pwl: Clean up Pwl class to match libcamera
diff mbox series

Message ID 20240405080259.1806453-4-paul.elder@ideasonboard.com
State New
Headers show
Series
  • ipa: Move Pwl from Raspberry Pi to libipa
Related show

Commit Message

Paul Elder April 5, 2024, 8:02 a.m. UTC
Clean up the Pwl class copied from the Raspberry Pi IPA to align it more
with the libcamera style.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
---
 src/ipa/libipa/pwl.cpp | 135 +++++++++++++++++++++++++++++++++--------
 src/ipa/libipa/pwl.h   | 113 ++++++++++++++--------------------
 2 files changed, 154 insertions(+), 94 deletions(-)

Comments

Naushir Patuck April 5, 2024, 9:56 a.m. UTC | #1
Hi Paul,

On Fri, 5 Apr 2024 at 09:03, Paul Elder <paul.elder@ideasonboard.com> wrote:
>
> Clean up the Pwl class copied from the Raspberry Pi IPA to align it more
> with the libcamera style.
>
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/ipa/libipa/pwl.cpp | 135 +++++++++++++++++++++++++++++++++--------
>  src/ipa/libipa/pwl.h   | 113 ++++++++++++++--------------------
>  2 files changed, 154 insertions(+), 94 deletions(-)
>
> diff --git a/src/ipa/libipa/pwl.cpp b/src/ipa/libipa/pwl.cpp
> index 09f5d65c..58925d83 100644
> --- a/src/ipa/libipa/pwl.cpp
> +++ b/src/ipa/libipa/pwl.cpp
> @@ -5,13 +5,40 @@
>   * pwl.cpp - piecewise linear functions
>   */
>
> +#include "pwl.h"
> +
>  #include <cassert>
>  #include <cmath>
> +#include <sstream>
>  #include <stdexcept>
>
> -#include "pwl.h"
> +#include <libcamera/geometry.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
>
> -int Pwl::read(const libcamera::YamlObject &params)
> +/*
> + * \enum Pwl::PerpType
> + * \brief Type of perpendicular found when inverting a piecewise linear function
> + *
> + * \var None
> + * \brief no perpendicular found
> + *
> + * \var Start
> + * \brief start of Pwl is closest point
> + *
> + * \var End
> + * \brief end of Pwl is closest point
> + *
> + * \var Vertex
> + * \brief vertex of Pwl is closest point
> + *
> + * \var Perpendicular
> + * \brief true perpendicular found
> + */
> +
> +int Pwl::readYaml(const libcamera::YamlObject &params)
>  {
>         if (!params.size() || params.size() % 2)
>                 return -EINVAL;
> @@ -29,7 +56,7 @@ int Pwl::read(const libcamera::YamlObject &params)
>                 if (!y)
>                         return -EINVAL;
>
> -               points_.push_back(Point(*x, *y));
> +               points_.push_back(FPoint(*x, *y));
>         }
>
>         return 0;
> @@ -38,13 +65,13 @@ int Pwl::read(const libcamera::YamlObject &params)
>  void Pwl::append(double x, double y, const double eps)
>  {
>         if (points_.empty() || points_.back().x + eps < x)
> -               points_.push_back(Point(x, y));
> +               points_.push_back(FPoint(x, y));
>  }
>
>  void Pwl::prepend(double x, double y, const double eps)
>  {
>         if (points_.empty() || points_.front().x - eps > x)
> -               points_.insert(points_.begin(), Point(x, y));
> +               points_.insert(points_.begin(), FPoint(x, y));
>  }
>
>  Pwl::Interval Pwl::domain() const
> @@ -65,6 +92,19 @@ bool Pwl::empty() const
>         return points_.empty();
>  }
>
> +/*
> + * \brief Evaluate the piecewise linear function
> + * \param[in] x The x value to input into the function
> + * \param[inout] spanPtr Initial guess for span
> + * \param[in] updateSpan Set to true to update spanPtr
> + *
> + * Evaluate Pwl, optionally supplying an initial guess for the
> + * "span". The "span" may be optionally be updated.  If you want to know
> + * the "span" value but don't have an initial guess you can set it to
> + * -1.
> + *
> + *  \return The result of evaluating the piecewise linear function with input \a x
> + */
>  double Pwl::eval(double x, int *spanPtr, bool updateSpan) const
>  {
>         int span = findSpan(x, spanPtr && *spanPtr != -1 ? *spanPtr : points_.size() / 2 - 1);
> @@ -94,16 +134,22 @@ int Pwl::findSpan(double x, int span) const
>         return span;
>  }
>
> -Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
> +/*
> + * Find perpendicular closest to xy, starting from span+1 so you can
> + * call it repeatedly to check for multiple closest points (set span to
> + * -1 on the first call). Also returns "pseudo" perpendiculars; see
> + * PerpType enum.
> + */
> +Pwl::PerpType Pwl::invert(FPoint const &xy, FPoint &perp, int &span,
>                           const double eps) const
>  {
>         assert(span >= -1);
>         bool prevOffEnd = false;
>         for (span = span + 1; span < (int)points_.size() - 1; span++) {
> -               Point spanVec = points_[span + 1] - points_[span];
> +               FPoint spanVec = points_[span + 1] - points_[span];
>                 double t = ((xy - points_[span]) % spanVec) / spanVec.len2();
> -               if (t < -eps) /* off the start of this span */
> -               {
> +               if (t < -eps) {
> +                       /* off the start of this span */
>                         if (span == 0) {
>                                 perp = points_[span];
>                                 return PerpType::Start;
> @@ -111,15 +157,15 @@ Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
>                                 perp = points_[span];
>                                 return PerpType::Vertex;
>                         }
> -               } else if (t > 1 + eps) /* off the end of this span */
> -               {
> +               } else if (t > 1 + eps) {
> +                       /* off the end of this span */
>                         if (span == (int)points_.size() - 2) {
>                                 perp = points_[span + 1];
>                                 return PerpType::End;
>                         }
>                         prevOffEnd = true;
> -               } else /* a true perpendicular */
> -               {
> +               } else {
> +                       /* a true perpendicular */
>                         perp = points_[span] + spanVec * t;
>                         return PerpType::Perpendicular;
>                 }
> @@ -127,25 +173,34 @@ Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
>         return PerpType::None;
>  }
>
> +/*
> + * \brief Compute the inverse function
> + * \param[out] trueInverse True of the resulting inverse is a proper/true inverse
> + * \param[in] eps Epsilon (optional)
> + * Indicate if it is a proper (true) inverse, or only a best effort (e.g.
> + * input was non-monotonic).
> + * \return The inverse piecewise linear function
> + */
>  Pwl Pwl::inverse(bool *trueInverse, const double eps) const
>  {
>         bool appended = false, prepended = false, neither = false;
>         Pwl inverse;
>
> -       for (Point const &p : points_) {
> -               if (inverse.empty())
> +       for (FPoint const &p : points_) {
> +               if (inverse.empty()) {
>                         inverse.append(p.y, p.x, eps);
> -               else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
> -                        std::abs(inverse.points_.front().x - p.y) <= eps)
> +               } else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
> +                          std::abs(inverse.points_.front().x - p.y) <= eps) {
>                         /* do nothing */;
> -               else if (p.y > inverse.points_.back().x) {
> +               } else if (p.y > inverse.points_.back().x) {
>                         inverse.append(p.y, p.x, eps);
>                         appended = true;
>                 } else if (p.y < inverse.points_.front().x) {
>                         inverse.prepend(p.y, p.x, eps);
>                         prepended = true;
> -               } else
> +               } else {
>                         neither = true;
> +               }
>         }
>
>         /*
> @@ -159,18 +214,25 @@ Pwl Pwl::inverse(bool *trueInverse, const double eps) const
>         return inverse;
>  }
>
> +/*
> + * \brief Compose two piecewise linear functions together
> + * \param[in] other The "other" piecewise linear function
> + * \param[in] eps Epsilon (optiona)
> + * The "this" function is done first, and "other" after.
> + * \return The composed piecewise linear function
> + */
>  Pwl Pwl::compose(Pwl const &other, const double eps) const
>  {
>         double thisX = points_[0].x, thisY = points_[0].y;
>         int thisSpan = 0, otherSpan = other.findSpan(thisY, 0);
>         Pwl result({ { thisX, other.eval(thisY, &otherSpan, false) } });
> +
>         while (thisSpan != (int)points_.size() - 1) {
>                 double dx = points_[thisSpan + 1].x - points_[thisSpan].x,
>                        dy = points_[thisSpan + 1].y - points_[thisSpan].y;
>                 if (std::abs(dy) > eps &&
>                     otherSpan + 1 < (int)other.points_.size() &&
> -                   points_[thisSpan + 1].y >=
> -                           other.points_[otherSpan + 1].x + eps) {
> +                   points_[thisSpan + 1].y >= other.points_[otherSpan + 1].x + eps) {
>                         /*
>                          * next control point in result will be where this
>                          * function's y reaches the next span in other
> @@ -204,18 +266,24 @@ Pwl Pwl::compose(Pwl const &other, const double eps) const
>         return result;
>  }
>
> +/* \brief Apply function to (x,y) values at every control point. */
>  void Pwl::map(std::function<void(double x, double y)> f) const
>  {
>         for (auto &pt : points_)
>                 f(pt.x, pt.y);
>  }
>
> +/*
> + * \brief Apply function to (x, y0, y1) values wherever either Pwl has a
> + * control point.
> + */
>  void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
>                std::function<void(double x, double y0, double y1)> f)
>  {
>         int span0 = 0, span1 = 0;
>         double x = std::min(pwl0.points_[0].x, pwl1.points_[0].x);
>         f(x, pwl0.eval(x, &span0, false), pwl1.eval(x, &span1, false));
> +
>         while (span0 < (int)pwl0.points_.size() - 1 ||
>                span1 < (int)pwl1.points_.size() - 1) {
>                 if (span0 == (int)pwl0.points_.size() - 1)
> @@ -230,6 +298,12 @@ void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
>         }
>  }
>
> +/*
> + * \brief Combine two Pwls
> + *
> + * Create a new Pwl where the y values are given by running f wherever either
> + * has a knot.
> + */
>  Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
>                  std::function<double(double x, double y0, double y1)> f,
>                  const double eps)
> @@ -241,6 +315,11 @@ Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
>         return result;
>  }
>
> +/*
> + * \brief Make "this" match (at least) the given domain.
> + *
> + * Any extension my be clipped or linear.
> + */
>  void Pwl::matchDomain(Interval const &domain, bool clip, const double eps)
>  {
>         int span = 0;
> @@ -258,10 +337,16 @@ Pwl &Pwl::operator*=(double d)
>         return *this;
>  }
>
> -void Pwl::debug(FILE *fp) const
> +std::string Pwl::toString() const
>  {
> -       fprintf(fp, "Pwl {\n");
> +       std::stringstream ss;
> +       ss << "Pwl { ";
>         for (auto &p : points_)
> -               fprintf(fp, "\t(%g, %g)\n", p.x, p.y);
> -       fprintf(fp, "}\n");
> +               ss << "(" << p.x << ", " << p.y << ") ";
> +       ss << "}";
> +       return ss.str();
>  }
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/pwl.h b/src/ipa/libipa/pwl.h
> index 7a6a6452..ef49e302 100644
> --- a/src/ipa/libipa/pwl.h
> +++ b/src/ipa/libipa/pwl.h
> @@ -8,116 +8,91 @@
>
>  #include <functional>
>  #include <math.h>
> +#include <string>
>  #include <vector>
>
> +#include <libcamera/geometry.h>
> +
>  #include "libcamera/internal/yaml_parser.h"
>
> +namespace libcamera {
> +
> +namespace ipa {
> +
>  class Pwl
>  {
>  public:
> +       enum class PerpType {
> +               None,
> +               Start,
> +               End,
> +               Vertex,
> +               Perpendicular,
> +       };
> +
>         struct Interval {
>                 Interval(double _start, double _end)
> -                       : start(_start), end(_end)
> -               {
> -               }
> -               double start, end;
> +                       : start(_start), end(_end) {}
> +
>                 bool contains(double value)
>                 {
>                         return value >= start && value <= end;
>                 }
> -               double clip(double value)
> +
> +               double clamp(double value)

Typically clamp implies a min/max range value.  Since we are only
using a singular value here, IMO this should still be called clip.

Thanks,
Naush


>                 {
>                         return value < start ? start
>                                              : (value > end ? end : value);
>                 }
> +
>                 double len() const { return end - start; }
> +
> +               double start, end;
>         };
> -       struct Point {
> -               Point() : x(0), y(0) {}
> -               Point(double _x, double _y)
> -                       : x(_x), y(_y) {}
> -               double x, y;
> -               Point operator-(Point const &p) const
> -               {
> -                       return Point(x - p.x, y - p.y);
> -               }
> -               Point operator+(Point const &p) const
> -               {
> -                       return Point(x + p.x, y + p.y);
> -               }
> -               double operator%(Point const &p) const
> -               {
> -                       return x * p.x + y * p.y;
> -               }
> -               Point operator*(double f) const { return Point(x * f, y * f); }
> -               Point operator/(double f) const { return Point(x / f, y / f); }
> -               double len2() const { return x * x + y * y; }
> -               double len() const { return sqrt(len2()); }
> -       };
> +
>         Pwl() {}
> -       Pwl(std::vector<Point> const &points) : points_(points) {}
> -       int read(const libcamera::YamlObject &params);
> +       Pwl(std::vector<FPoint> const &points)
> +               : points_(points) {}
> +       int readYaml(const libcamera::YamlObject &params);
> +
>         void append(double x, double y, const double eps = 1e-6);
>         void prepend(double x, double y, const double eps = 1e-6);
> +
>         Interval domain() const;
>         Interval range() const;
> +
>         bool empty() const;
> -       /*
> -        * Evaluate Pwl, optionally supplying an initial guess for the
> -        * "span". The "span" may be optionally be updated.  If you want to know
> -        * the "span" value but don't have an initial guess you can set it to
> -        * -1.
> -        */
> +
>         double eval(double x, int *spanPtr = nullptr,
>                     bool updateSpan = true) const;
> -       /*
> -        * Find perpendicular closest to xy, starting from span+1 so you can
> -        * call it repeatedly to check for multiple closest points (set span to
> -        * -1 on the first call). Also returns "pseudo" perpendiculars; see
> -        * PerpType enum.
> -        */
> -       enum class PerpType {
> -               None, /* no perpendicular found */
> -               Start, /* start of Pwl is closest point */
> -               End, /* end of Pwl is closest point */
> -               Vertex, /* vertex of Pwl is closest point */
> -               Perpendicular /* true perpendicular found */
> -       };
> -       PerpType invert(Point const &xy, Point &perp, int &span,
> +
> +       PerpType invert(FPoint const &xy, FPoint &perp, int &span,
>                         const double eps = 1e-6) const;
> -       /*
> -        * Compute the inverse function. Indicate if it is a proper (true)
> -        * inverse, or only a best effort (e.g. input was non-monotonic).
> -        */
>         Pwl inverse(bool *trueInverse = nullptr, const double eps = 1e-6) const;
> -       /* Compose two Pwls together, doing "this" first and "other" after. */
>         Pwl compose(Pwl const &other, const double eps = 1e-6) const;
> -       /* Apply function to (x,y) values at every control point. */
> +
>         void map(std::function<void(double x, double y)> f) const;
> -       /*
> -        * Apply function to (x, y0, y1) values wherever either Pwl has a
> -        * control point.
> -        */
> +
>         static void map2(Pwl const &pwl0, Pwl const &pwl1,
>                          std::function<void(double x, double y0, double y1)> f);
> -       /*
> -        * Combine two Pwls, meaning we create a new Pwl where the y values are
> -        * given by running f wherever either has a knot.
> -        */
> +
>         static Pwl
>         combine(Pwl const &pwl0, Pwl const &pwl1,
>                 std::function<double(double x, double y0, double y1)> f,
>                 const double eps = 1e-6);
> -       /*
> -        * Make "this" match (at least) the given domain. Any extension my be
> -        * clipped or linear.
> -        */
> +
>         void matchDomain(Interval const &domain, bool clip = true,
>                          const double eps = 1e-6);
> +
>         Pwl &operator*=(double d);
> -       void debug(FILE *fp = stdout) const;
> +
> +       std::string toString() const;
>
>  private:
>         int findSpan(double x, int span) const;
> -       std::vector<Point> points_;
> +       std::vector<FPoint> points_;
>  };
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> --
> 2.39.2
>
Paul Elder April 5, 2024, 2:37 p.m. UTC | #2
Hi Naush,

On Fri, Apr 05, 2024 at 10:56:58AM +0100, Naushir Patuck wrote:
> Hi Paul,
> 
> On Fri, 5 Apr 2024 at 09:03, Paul Elder <paul.elder@ideasonboard.com> wrote:
> >
> > Clean up the Pwl class copied from the Raspberry Pi IPA to align it more
> > with the libcamera style.
> >
> > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > ---
> >  src/ipa/libipa/pwl.cpp | 135 +++++++++++++++++++++++++++++++++--------
> >  src/ipa/libipa/pwl.h   | 113 ++++++++++++++--------------------
> >  2 files changed, 154 insertions(+), 94 deletions(-)
> >
> > diff --git a/src/ipa/libipa/pwl.cpp b/src/ipa/libipa/pwl.cpp
> > index 09f5d65c..58925d83 100644
> > --- a/src/ipa/libipa/pwl.cpp
> > +++ b/src/ipa/libipa/pwl.cpp
> > @@ -5,13 +5,40 @@
> >   * pwl.cpp - piecewise linear functions
> >   */
> >
> > +#include "pwl.h"
> > +
> >  #include <cassert>
> >  #include <cmath>
> > +#include <sstream>
> >  #include <stdexcept>
> >
> > -#include "pwl.h"
> > +#include <libcamera/geometry.h>
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> >
> > -int Pwl::read(const libcamera::YamlObject &params)
> > +/*
> > + * \enum Pwl::PerpType
> > + * \brief Type of perpendicular found when inverting a piecewise linear function
> > + *
> > + * \var None
> > + * \brief no perpendicular found
> > + *
> > + * \var Start
> > + * \brief start of Pwl is closest point
> > + *
> > + * \var End
> > + * \brief end of Pwl is closest point
> > + *
> > + * \var Vertex
> > + * \brief vertex of Pwl is closest point
> > + *
> > + * \var Perpendicular
> > + * \brief true perpendicular found
> > + */
> > +
> > +int Pwl::readYaml(const libcamera::YamlObject &params)
> >  {
> >         if (!params.size() || params.size() % 2)
> >                 return -EINVAL;
> > @@ -29,7 +56,7 @@ int Pwl::read(const libcamera::YamlObject &params)
> >                 if (!y)
> >                         return -EINVAL;
> >
> > -               points_.push_back(Point(*x, *y));
> > +               points_.push_back(FPoint(*x, *y));
> >         }
> >
> >         return 0;
> > @@ -38,13 +65,13 @@ int Pwl::read(const libcamera::YamlObject &params)
> >  void Pwl::append(double x, double y, const double eps)
> >  {
> >         if (points_.empty() || points_.back().x + eps < x)
> > -               points_.push_back(Point(x, y));
> > +               points_.push_back(FPoint(x, y));
> >  }
> >
> >  void Pwl::prepend(double x, double y, const double eps)
> >  {
> >         if (points_.empty() || points_.front().x - eps > x)
> > -               points_.insert(points_.begin(), Point(x, y));
> > +               points_.insert(points_.begin(), FPoint(x, y));
> >  }
> >
> >  Pwl::Interval Pwl::domain() const
> > @@ -65,6 +92,19 @@ bool Pwl::empty() const
> >         return points_.empty();
> >  }
> >
> > +/*
> > + * \brief Evaluate the piecewise linear function
> > + * \param[in] x The x value to input into the function
> > + * \param[inout] spanPtr Initial guess for span
> > + * \param[in] updateSpan Set to true to update spanPtr
> > + *
> > + * Evaluate Pwl, optionally supplying an initial guess for the
> > + * "span". The "span" may be optionally be updated.  If you want to know
> > + * the "span" value but don't have an initial guess you can set it to
> > + * -1.
> > + *
> > + *  \return The result of evaluating the piecewise linear function with input \a x
> > + */
> >  double Pwl::eval(double x, int *spanPtr, bool updateSpan) const
> >  {
> >         int span = findSpan(x, spanPtr && *spanPtr != -1 ? *spanPtr : points_.size() / 2 - 1);
> > @@ -94,16 +134,22 @@ int Pwl::findSpan(double x, int span) const
> >         return span;
> >  }
> >
> > -Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
> > +/*
> > + * Find perpendicular closest to xy, starting from span+1 so you can
> > + * call it repeatedly to check for multiple closest points (set span to
> > + * -1 on the first call). Also returns "pseudo" perpendiculars; see
> > + * PerpType enum.
> > + */
> > +Pwl::PerpType Pwl::invert(FPoint const &xy, FPoint &perp, int &span,
> >                           const double eps) const
> >  {
> >         assert(span >= -1);
> >         bool prevOffEnd = false;
> >         for (span = span + 1; span < (int)points_.size() - 1; span++) {
> > -               Point spanVec = points_[span + 1] - points_[span];
> > +               FPoint spanVec = points_[span + 1] - points_[span];
> >                 double t = ((xy - points_[span]) % spanVec) / spanVec.len2();
> > -               if (t < -eps) /* off the start of this span */
> > -               {
> > +               if (t < -eps) {
> > +                       /* off the start of this span */
> >                         if (span == 0) {
> >                                 perp = points_[span];
> >                                 return PerpType::Start;
> > @@ -111,15 +157,15 @@ Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
> >                                 perp = points_[span];
> >                                 return PerpType::Vertex;
> >                         }
> > -               } else if (t > 1 + eps) /* off the end of this span */
> > -               {
> > +               } else if (t > 1 + eps) {
> > +                       /* off the end of this span */
> >                         if (span == (int)points_.size() - 2) {
> >                                 perp = points_[span + 1];
> >                                 return PerpType::End;
> >                         }
> >                         prevOffEnd = true;
> > -               } else /* a true perpendicular */
> > -               {
> > +               } else {
> > +                       /* a true perpendicular */
> >                         perp = points_[span] + spanVec * t;
> >                         return PerpType::Perpendicular;
> >                 }
> > @@ -127,25 +173,34 @@ Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
> >         return PerpType::None;
> >  }
> >
> > +/*
> > + * \brief Compute the inverse function
> > + * \param[out] trueInverse True of the resulting inverse is a proper/true inverse
> > + * \param[in] eps Epsilon (optional)
> > + * Indicate if it is a proper (true) inverse, or only a best effort (e.g.
> > + * input was non-monotonic).
> > + * \return The inverse piecewise linear function
> > + */
> >  Pwl Pwl::inverse(bool *trueInverse, const double eps) const
> >  {
> >         bool appended = false, prepended = false, neither = false;
> >         Pwl inverse;
> >
> > -       for (Point const &p : points_) {
> > -               if (inverse.empty())
> > +       for (FPoint const &p : points_) {
> > +               if (inverse.empty()) {
> >                         inverse.append(p.y, p.x, eps);
> > -               else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
> > -                        std::abs(inverse.points_.front().x - p.y) <= eps)
> > +               } else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
> > +                          std::abs(inverse.points_.front().x - p.y) <= eps) {
> >                         /* do nothing */;
> > -               else if (p.y > inverse.points_.back().x) {
> > +               } else if (p.y > inverse.points_.back().x) {
> >                         inverse.append(p.y, p.x, eps);
> >                         appended = true;
> >                 } else if (p.y < inverse.points_.front().x) {
> >                         inverse.prepend(p.y, p.x, eps);
> >                         prepended = true;
> > -               } else
> > +               } else {
> >                         neither = true;
> > +               }
> >         }
> >
> >         /*
> > @@ -159,18 +214,25 @@ Pwl Pwl::inverse(bool *trueInverse, const double eps) const
> >         return inverse;
> >  }
> >
> > +/*
> > + * \brief Compose two piecewise linear functions together
> > + * \param[in] other The "other" piecewise linear function
> > + * \param[in] eps Epsilon (optiona)
> > + * The "this" function is done first, and "other" after.
> > + * \return The composed piecewise linear function
> > + */
> >  Pwl Pwl::compose(Pwl const &other, const double eps) const
> >  {
> >         double thisX = points_[0].x, thisY = points_[0].y;
> >         int thisSpan = 0, otherSpan = other.findSpan(thisY, 0);
> >         Pwl result({ { thisX, other.eval(thisY, &otherSpan, false) } });
> > +
> >         while (thisSpan != (int)points_.size() - 1) {
> >                 double dx = points_[thisSpan + 1].x - points_[thisSpan].x,
> >                        dy = points_[thisSpan + 1].y - points_[thisSpan].y;
> >                 if (std::abs(dy) > eps &&
> >                     otherSpan + 1 < (int)other.points_.size() &&
> > -                   points_[thisSpan + 1].y >=
> > -                           other.points_[otherSpan + 1].x + eps) {
> > +                   points_[thisSpan + 1].y >= other.points_[otherSpan + 1].x + eps) {
> >                         /*
> >                          * next control point in result will be where this
> >                          * function's y reaches the next span in other
> > @@ -204,18 +266,24 @@ Pwl Pwl::compose(Pwl const &other, const double eps) const
> >         return result;
> >  }
> >
> > +/* \brief Apply function to (x,y) values at every control point. */
> >  void Pwl::map(std::function<void(double x, double y)> f) const
> >  {
> >         for (auto &pt : points_)
> >                 f(pt.x, pt.y);
> >  }
> >
> > +/*
> > + * \brief Apply function to (x, y0, y1) values wherever either Pwl has a
> > + * control point.
> > + */
> >  void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
> >                std::function<void(double x, double y0, double y1)> f)
> >  {
> >         int span0 = 0, span1 = 0;
> >         double x = std::min(pwl0.points_[0].x, pwl1.points_[0].x);
> >         f(x, pwl0.eval(x, &span0, false), pwl1.eval(x, &span1, false));
> > +
> >         while (span0 < (int)pwl0.points_.size() - 1 ||
> >                span1 < (int)pwl1.points_.size() - 1) {
> >                 if (span0 == (int)pwl0.points_.size() - 1)
> > @@ -230,6 +298,12 @@ void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
> >         }
> >  }
> >
> > +/*
> > + * \brief Combine two Pwls
> > + *
> > + * Create a new Pwl where the y values are given by running f wherever either
> > + * has a knot.
> > + */
> >  Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
> >                  std::function<double(double x, double y0, double y1)> f,
> >                  const double eps)
> > @@ -241,6 +315,11 @@ Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
> >         return result;
> >  }
> >
> > +/*
> > + * \brief Make "this" match (at least) the given domain.
> > + *
> > + * Any extension my be clipped or linear.
> > + */
> >  void Pwl::matchDomain(Interval const &domain, bool clip, const double eps)
> >  {
> >         int span = 0;
> > @@ -258,10 +337,16 @@ Pwl &Pwl::operator*=(double d)
> >         return *this;
> >  }
> >
> > -void Pwl::debug(FILE *fp) const
> > +std::string Pwl::toString() const
> >  {
> > -       fprintf(fp, "Pwl {\n");
> > +       std::stringstream ss;
> > +       ss << "Pwl { ";
> >         for (auto &p : points_)
> > -               fprintf(fp, "\t(%g, %g)\n", p.x, p.y);
> > -       fprintf(fp, "}\n");
> > +               ss << "(" << p.x << ", " << p.y << ") ";
> > +       ss << "}";
> > +       return ss.str();
> >  }
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/ipa/libipa/pwl.h b/src/ipa/libipa/pwl.h
> > index 7a6a6452..ef49e302 100644
> > --- a/src/ipa/libipa/pwl.h
> > +++ b/src/ipa/libipa/pwl.h
> > @@ -8,116 +8,91 @@
> >
> >  #include <functional>
> >  #include <math.h>
> > +#include <string>
> >  #include <vector>
> >
> > +#include <libcamera/geometry.h>
> > +
> >  #include "libcamera/internal/yaml_parser.h"
> >
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> >  class Pwl
> >  {
> >  public:
> > +       enum class PerpType {
> > +               None,
> > +               Start,
> > +               End,
> > +               Vertex,
> > +               Perpendicular,
> > +       };
> > +
> >         struct Interval {
> >                 Interval(double _start, double _end)
> > -                       : start(_start), end(_end)
> > -               {
> > -               }
> > -               double start, end;
> > +                       : start(_start), end(_end) {}
> > +
> >                 bool contains(double value)
> >                 {
> >                         return value >= start && value <= end;
> >                 }
> > -               double clip(double value)
> > +
> > +               double clamp(double value)
> 
> Typically clamp implies a min/max range value.  Since we are only
> using a singular value here, IMO this should still be called clip.

I was under the impression that the interval itself made up the min and max
range values, which is why I changed it to clamp. I started seeing clip
used elsewhere too though and I suppose it's not that big of a deal so
I'll change it back to clip (except I have patches coming in a few
minutes that depend on this so they'll still have clamp).


Thanks,

Paul

> 
> 
> >                 {
> >                         return value < start ? start
> >                                              : (value > end ? end : value);
> >                 }
> > +
> >                 double len() const { return end - start; }
> > +
> > +               double start, end;
> >         };
> > -       struct Point {
> > -               Point() : x(0), y(0) {}
> > -               Point(double _x, double _y)
> > -                       : x(_x), y(_y) {}
> > -               double x, y;
> > -               Point operator-(Point const &p) const
> > -               {
> > -                       return Point(x - p.x, y - p.y);
> > -               }
> > -               Point operator+(Point const &p) const
> > -               {
> > -                       return Point(x + p.x, y + p.y);
> > -               }
> > -               double operator%(Point const &p) const
> > -               {
> > -                       return x * p.x + y * p.y;
> > -               }
> > -               Point operator*(double f) const { return Point(x * f, y * f); }
> > -               Point operator/(double f) const { return Point(x / f, y / f); }
> > -               double len2() const { return x * x + y * y; }
> > -               double len() const { return sqrt(len2()); }
> > -       };
> > +
> >         Pwl() {}
> > -       Pwl(std::vector<Point> const &points) : points_(points) {}
> > -       int read(const libcamera::YamlObject &params);
> > +       Pwl(std::vector<FPoint> const &points)
> > +               : points_(points) {}
> > +       int readYaml(const libcamera::YamlObject &params);
> > +
> >         void append(double x, double y, const double eps = 1e-6);
> >         void prepend(double x, double y, const double eps = 1e-6);
> > +
> >         Interval domain() const;
> >         Interval range() const;
> > +
> >         bool empty() const;
> > -       /*
> > -        * Evaluate Pwl, optionally supplying an initial guess for the
> > -        * "span". The "span" may be optionally be updated.  If you want to know
> > -        * the "span" value but don't have an initial guess you can set it to
> > -        * -1.
> > -        */
> > +
> >         double eval(double x, int *spanPtr = nullptr,
> >                     bool updateSpan = true) const;
> > -       /*
> > -        * Find perpendicular closest to xy, starting from span+1 so you can
> > -        * call it repeatedly to check for multiple closest points (set span to
> > -        * -1 on the first call). Also returns "pseudo" perpendiculars; see
> > -        * PerpType enum.
> > -        */
> > -       enum class PerpType {
> > -               None, /* no perpendicular found */
> > -               Start, /* start of Pwl is closest point */
> > -               End, /* end of Pwl is closest point */
> > -               Vertex, /* vertex of Pwl is closest point */
> > -               Perpendicular /* true perpendicular found */
> > -       };
> > -       PerpType invert(Point const &xy, Point &perp, int &span,
> > +
> > +       PerpType invert(FPoint const &xy, FPoint &perp, int &span,
> >                         const double eps = 1e-6) const;
> > -       /*
> > -        * Compute the inverse function. Indicate if it is a proper (true)
> > -        * inverse, or only a best effort (e.g. input was non-monotonic).
> > -        */
> >         Pwl inverse(bool *trueInverse = nullptr, const double eps = 1e-6) const;
> > -       /* Compose two Pwls together, doing "this" first and "other" after. */
> >         Pwl compose(Pwl const &other, const double eps = 1e-6) const;
> > -       /* Apply function to (x,y) values at every control point. */
> > +
> >         void map(std::function<void(double x, double y)> f) const;
> > -       /*
> > -        * Apply function to (x, y0, y1) values wherever either Pwl has a
> > -        * control point.
> > -        */
> > +
> >         static void map2(Pwl const &pwl0, Pwl const &pwl1,
> >                          std::function<void(double x, double y0, double y1)> f);
> > -       /*
> > -        * Combine two Pwls, meaning we create a new Pwl where the y values are
> > -        * given by running f wherever either has a knot.
> > -        */
> > +
> >         static Pwl
> >         combine(Pwl const &pwl0, Pwl const &pwl1,
> >                 std::function<double(double x, double y0, double y1)> f,
> >                 const double eps = 1e-6);
> > -       /*
> > -        * Make "this" match (at least) the given domain. Any extension my be
> > -        * clipped or linear.
> > -        */
> > +
> >         void matchDomain(Interval const &domain, bool clip = true,
> >                          const double eps = 1e-6);
> > +
> >         Pwl &operator*=(double d);
> > -       void debug(FILE *fp = stdout) const;
> > +
> > +       std::string toString() const;
> >
> >  private:
> >         int findSpan(double x, int span) const;
> > -       std::vector<Point> points_;
> > +       std::vector<FPoint> points_;
> >  };
> > +
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> > --
> > 2.39.2
> >
Naushir Patuck April 5, 2024, 3:09 p.m. UTC | #3
Hi Paul,

On Fri, 5 Apr 2024 at 15:37, Paul Elder <paul.elder@ideasonboard.com> wrote:
>
> Hi Naush,
>
> On Fri, Apr 05, 2024 at 10:56:58AM +0100, Naushir Patuck wrote:
> > Hi Paul,
> >
> > On Fri, 5 Apr 2024 at 09:03, Paul Elder <paul.elder@ideasonboard.com> wrote:
> > >
> > > Clean up the Pwl class copied from the Raspberry Pi IPA to align it more
> > > with the libcamera style.
> > >
> > > Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> > > ---
> > >  src/ipa/libipa/pwl.cpp | 135 +++++++++++++++++++++++++++++++++--------
> > >  src/ipa/libipa/pwl.h   | 113 ++++++++++++++--------------------
> > >  2 files changed, 154 insertions(+), 94 deletions(-)
> > >
> > > diff --git a/src/ipa/libipa/pwl.cpp b/src/ipa/libipa/pwl.cpp
> > > index 09f5d65c..58925d83 100644
> > > --- a/src/ipa/libipa/pwl.cpp
> > > +++ b/src/ipa/libipa/pwl.cpp
> > > @@ -5,13 +5,40 @@
> > >   * pwl.cpp - piecewise linear functions
> > >   */
> > >
> > > +#include "pwl.h"
> > > +
> > >  #include <cassert>
> > >  #include <cmath>
> > > +#include <sstream>
> > >  #include <stdexcept>
> > >
> > > -#include "pwl.h"
> > > +#include <libcamera/geometry.h>
> > > +
> > > +namespace libcamera {
> > > +
> > > +namespace ipa {
> > >
> > > -int Pwl::read(const libcamera::YamlObject &params)
> > > +/*
> > > + * \enum Pwl::PerpType
> > > + * \brief Type of perpendicular found when inverting a piecewise linear function
> > > + *
> > > + * \var None
> > > + * \brief no perpendicular found
> > > + *
> > > + * \var Start
> > > + * \brief start of Pwl is closest point
> > > + *
> > > + * \var End
> > > + * \brief end of Pwl is closest point
> > > + *
> > > + * \var Vertex
> > > + * \brief vertex of Pwl is closest point
> > > + *
> > > + * \var Perpendicular
> > > + * \brief true perpendicular found
> > > + */
> > > +
> > > +int Pwl::readYaml(const libcamera::YamlObject &params)
> > >  {
> > >         if (!params.size() || params.size() % 2)
> > >                 return -EINVAL;
> > > @@ -29,7 +56,7 @@ int Pwl::read(const libcamera::YamlObject &params)
> > >                 if (!y)
> > >                         return -EINVAL;
> > >
> > > -               points_.push_back(Point(*x, *y));
> > > +               points_.push_back(FPoint(*x, *y));
> > >         }
> > >
> > >         return 0;
> > > @@ -38,13 +65,13 @@ int Pwl::read(const libcamera::YamlObject &params)
> > >  void Pwl::append(double x, double y, const double eps)
> > >  {
> > >         if (points_.empty() || points_.back().x + eps < x)
> > > -               points_.push_back(Point(x, y));
> > > +               points_.push_back(FPoint(x, y));
> > >  }
> > >
> > >  void Pwl::prepend(double x, double y, const double eps)
> > >  {
> > >         if (points_.empty() || points_.front().x - eps > x)
> > > -               points_.insert(points_.begin(), Point(x, y));
> > > +               points_.insert(points_.begin(), FPoint(x, y));
> > >  }
> > >
> > >  Pwl::Interval Pwl::domain() const
> > > @@ -65,6 +92,19 @@ bool Pwl::empty() const
> > >         return points_.empty();
> > >  }
> > >
> > > +/*
> > > + * \brief Evaluate the piecewise linear function
> > > + * \param[in] x The x value to input into the function
> > > + * \param[inout] spanPtr Initial guess for span
> > > + * \param[in] updateSpan Set to true to update spanPtr
> > > + *
> > > + * Evaluate Pwl, optionally supplying an initial guess for the
> > > + * "span". The "span" may be optionally be updated.  If you want to know
> > > + * the "span" value but don't have an initial guess you can set it to
> > > + * -1.
> > > + *
> > > + *  \return The result of evaluating the piecewise linear function with input \a x
> > > + */
> > >  double Pwl::eval(double x, int *spanPtr, bool updateSpan) const
> > >  {
> > >         int span = findSpan(x, spanPtr && *spanPtr != -1 ? *spanPtr : points_.size() / 2 - 1);
> > > @@ -94,16 +134,22 @@ int Pwl::findSpan(double x, int span) const
> > >         return span;
> > >  }
> > >
> > > -Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
> > > +/*
> > > + * Find perpendicular closest to xy, starting from span+1 so you can
> > > + * call it repeatedly to check for multiple closest points (set span to
> > > + * -1 on the first call). Also returns "pseudo" perpendiculars; see
> > > + * PerpType enum.
> > > + */
> > > +Pwl::PerpType Pwl::invert(FPoint const &xy, FPoint &perp, int &span,
> > >                           const double eps) const
> > >  {
> > >         assert(span >= -1);
> > >         bool prevOffEnd = false;
> > >         for (span = span + 1; span < (int)points_.size() - 1; span++) {
> > > -               Point spanVec = points_[span + 1] - points_[span];
> > > +               FPoint spanVec = points_[span + 1] - points_[span];
> > >                 double t = ((xy - points_[span]) % spanVec) / spanVec.len2();
> > > -               if (t < -eps) /* off the start of this span */
> > > -               {
> > > +               if (t < -eps) {
> > > +                       /* off the start of this span */
> > >                         if (span == 0) {
> > >                                 perp = points_[span];
> > >                                 return PerpType::Start;
> > > @@ -111,15 +157,15 @@ Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
> > >                                 perp = points_[span];
> > >                                 return PerpType::Vertex;
> > >                         }
> > > -               } else if (t > 1 + eps) /* off the end of this span */
> > > -               {
> > > +               } else if (t > 1 + eps) {
> > > +                       /* off the end of this span */
> > >                         if (span == (int)points_.size() - 2) {
> > >                                 perp = points_[span + 1];
> > >                                 return PerpType::End;
> > >                         }
> > >                         prevOffEnd = true;
> > > -               } else /* a true perpendicular */
> > > -               {
> > > +               } else {
> > > +                       /* a true perpendicular */
> > >                         perp = points_[span] + spanVec * t;
> > >                         return PerpType::Perpendicular;
> > >                 }
> > > @@ -127,25 +173,34 @@ Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
> > >         return PerpType::None;
> > >  }
> > >
> > > +/*
> > > + * \brief Compute the inverse function
> > > + * \param[out] trueInverse True of the resulting inverse is a proper/true inverse
> > > + * \param[in] eps Epsilon (optional)
> > > + * Indicate if it is a proper (true) inverse, or only a best effort (e.g.
> > > + * input was non-monotonic).
> > > + * \return The inverse piecewise linear function
> > > + */
> > >  Pwl Pwl::inverse(bool *trueInverse, const double eps) const
> > >  {
> > >         bool appended = false, prepended = false, neither = false;
> > >         Pwl inverse;
> > >
> > > -       for (Point const &p : points_) {
> > > -               if (inverse.empty())
> > > +       for (FPoint const &p : points_) {
> > > +               if (inverse.empty()) {
> > >                         inverse.append(p.y, p.x, eps);
> > > -               else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
> > > -                        std::abs(inverse.points_.front().x - p.y) <= eps)
> > > +               } else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
> > > +                          std::abs(inverse.points_.front().x - p.y) <= eps) {
> > >                         /* do nothing */;
> > > -               else if (p.y > inverse.points_.back().x) {
> > > +               } else if (p.y > inverse.points_.back().x) {
> > >                         inverse.append(p.y, p.x, eps);
> > >                         appended = true;
> > >                 } else if (p.y < inverse.points_.front().x) {
> > >                         inverse.prepend(p.y, p.x, eps);
> > >                         prepended = true;
> > > -               } else
> > > +               } else {
> > >                         neither = true;
> > > +               }
> > >         }
> > >
> > >         /*
> > > @@ -159,18 +214,25 @@ Pwl Pwl::inverse(bool *trueInverse, const double eps) const
> > >         return inverse;
> > >  }
> > >
> > > +/*
> > > + * \brief Compose two piecewise linear functions together
> > > + * \param[in] other The "other" piecewise linear function
> > > + * \param[in] eps Epsilon (optiona)
> > > + * The "this" function is done first, and "other" after.
> > > + * \return The composed piecewise linear function
> > > + */
> > >  Pwl Pwl::compose(Pwl const &other, const double eps) const
> > >  {
> > >         double thisX = points_[0].x, thisY = points_[0].y;
> > >         int thisSpan = 0, otherSpan = other.findSpan(thisY, 0);
> > >         Pwl result({ { thisX, other.eval(thisY, &otherSpan, false) } });
> > > +
> > >         while (thisSpan != (int)points_.size() - 1) {
> > >                 double dx = points_[thisSpan + 1].x - points_[thisSpan].x,
> > >                        dy = points_[thisSpan + 1].y - points_[thisSpan].y;
> > >                 if (std::abs(dy) > eps &&
> > >                     otherSpan + 1 < (int)other.points_.size() &&
> > > -                   points_[thisSpan + 1].y >=
> > > -                           other.points_[otherSpan + 1].x + eps) {
> > > +                   points_[thisSpan + 1].y >= other.points_[otherSpan + 1].x + eps) {
> > >                         /*
> > >                          * next control point in result will be where this
> > >                          * function's y reaches the next span in other
> > > @@ -204,18 +266,24 @@ Pwl Pwl::compose(Pwl const &other, const double eps) const
> > >         return result;
> > >  }
> > >
> > > +/* \brief Apply function to (x,y) values at every control point. */
> > >  void Pwl::map(std::function<void(double x, double y)> f) const
> > >  {
> > >         for (auto &pt : points_)
> > >                 f(pt.x, pt.y);
> > >  }
> > >
> > > +/*
> > > + * \brief Apply function to (x, y0, y1) values wherever either Pwl has a
> > > + * control point.
> > > + */
> > >  void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
> > >                std::function<void(double x, double y0, double y1)> f)
> > >  {
> > >         int span0 = 0, span1 = 0;
> > >         double x = std::min(pwl0.points_[0].x, pwl1.points_[0].x);
> > >         f(x, pwl0.eval(x, &span0, false), pwl1.eval(x, &span1, false));
> > > +
> > >         while (span0 < (int)pwl0.points_.size() - 1 ||
> > >                span1 < (int)pwl1.points_.size() - 1) {
> > >                 if (span0 == (int)pwl0.points_.size() - 1)
> > > @@ -230,6 +298,12 @@ void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
> > >         }
> > >  }
> > >
> > > +/*
> > > + * \brief Combine two Pwls
> > > + *
> > > + * Create a new Pwl where the y values are given by running f wherever either
> > > + * has a knot.
> > > + */
> > >  Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
> > >                  std::function<double(double x, double y0, double y1)> f,
> > >                  const double eps)
> > > @@ -241,6 +315,11 @@ Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
> > >         return result;
> > >  }
> > >
> > > +/*
> > > + * \brief Make "this" match (at least) the given domain.
> > > + *
> > > + * Any extension my be clipped or linear.
> > > + */
> > >  void Pwl::matchDomain(Interval const &domain, bool clip, const double eps)
> > >  {
> > >         int span = 0;
> > > @@ -258,10 +337,16 @@ Pwl &Pwl::operator*=(double d)
> > >         return *this;
> > >  }
> > >
> > > -void Pwl::debug(FILE *fp) const
> > > +std::string Pwl::toString() const
> > >  {
> > > -       fprintf(fp, "Pwl {\n");
> > > +       std::stringstream ss;
> > > +       ss << "Pwl { ";
> > >         for (auto &p : points_)
> > > -               fprintf(fp, "\t(%g, %g)\n", p.x, p.y);
> > > -       fprintf(fp, "}\n");
> > > +               ss << "(" << p.x << ", " << p.y << ") ";
> > > +       ss << "}";
> > > +       return ss.str();
> > >  }
> > > +
> > > +} /* namespace ipa */
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/ipa/libipa/pwl.h b/src/ipa/libipa/pwl.h
> > > index 7a6a6452..ef49e302 100644
> > > --- a/src/ipa/libipa/pwl.h
> > > +++ b/src/ipa/libipa/pwl.h
> > > @@ -8,116 +8,91 @@
> > >
> > >  #include <functional>
> > >  #include <math.h>
> > > +#include <string>
> > >  #include <vector>
> > >
> > > +#include <libcamera/geometry.h>
> > > +
> > >  #include "libcamera/internal/yaml_parser.h"
> > >
> > > +namespace libcamera {
> > > +
> > > +namespace ipa {
> > > +
> > >  class Pwl
> > >  {
> > >  public:
> > > +       enum class PerpType {
> > > +               None,
> > > +               Start,
> > > +               End,
> > > +               Vertex,
> > > +               Perpendicular,
> > > +       };
> > > +
> > >         struct Interval {
> > >                 Interval(double _start, double _end)
> > > -                       : start(_start), end(_end)
> > > -               {
> > > -               }
> > > -               double start, end;
> > > +                       : start(_start), end(_end) {}
> > > +
> > >                 bool contains(double value)
> > >                 {
> > >                         return value >= start && value <= end;
> > >                 }
> > > -               double clip(double value)
> > > +
> > > +               double clamp(double value)
> >
> > Typically clamp implies a min/max range value.  Since we are only
> > using a singular value here, IMO this should still be called clip.
>
> I was under the impression that the interval itself made up the min and max
> range values, which is why I changed it to clamp. I started seeing clip
> used elsewhere too though and I suppose it's not that big of a deal so
> I'll change it back to clip (except I have patches coming in a few
> minutes that depend on this so they'll still have clamp).

You are right, this is a clamp operation, I just didn't read the code
right.  I'd be happy to change the clip to clamp like you've already
done.

Regards,
Naush

>
>
> Thanks,
>
> Paul
>
> >
> >
> > >                 {
> > >                         return value < start ? start
> > >                                              : (value > end ? end : value);
> > >                 }
> > > +
> > >                 double len() const { return end - start; }
> > > +
> > > +               double start, end;
> > >         };
> > > -       struct Point {
> > > -               Point() : x(0), y(0) {}
> > > -               Point(double _x, double _y)
> > > -                       : x(_x), y(_y) {}
> > > -               double x, y;
> > > -               Point operator-(Point const &p) const
> > > -               {
> > > -                       return Point(x - p.x, y - p.y);
> > > -               }
> > > -               Point operator+(Point const &p) const
> > > -               {
> > > -                       return Point(x + p.x, y + p.y);
> > > -               }
> > > -               double operator%(Point const &p) const
> > > -               {
> > > -                       return x * p.x + y * p.y;
> > > -               }
> > > -               Point operator*(double f) const { return Point(x * f, y * f); }
> > > -               Point operator/(double f) const { return Point(x / f, y / f); }
> > > -               double len2() const { return x * x + y * y; }
> > > -               double len() const { return sqrt(len2()); }
> > > -       };
> > > +
> > >         Pwl() {}
> > > -       Pwl(std::vector<Point> const &points) : points_(points) {}
> > > -       int read(const libcamera::YamlObject &params);
> > > +       Pwl(std::vector<FPoint> const &points)
> > > +               : points_(points) {}
> > > +       int readYaml(const libcamera::YamlObject &params);
> > > +
> > >         void append(double x, double y, const double eps = 1e-6);
> > >         void prepend(double x, double y, const double eps = 1e-6);
> > > +
> > >         Interval domain() const;
> > >         Interval range() const;
> > > +
> > >         bool empty() const;
> > > -       /*
> > > -        * Evaluate Pwl, optionally supplying an initial guess for the
> > > -        * "span". The "span" may be optionally be updated.  If you want to know
> > > -        * the "span" value but don't have an initial guess you can set it to
> > > -        * -1.
> > > -        */
> > > +
> > >         double eval(double x, int *spanPtr = nullptr,
> > >                     bool updateSpan = true) const;
> > > -       /*
> > > -        * Find perpendicular closest to xy, starting from span+1 so you can
> > > -        * call it repeatedly to check for multiple closest points (set span to
> > > -        * -1 on the first call). Also returns "pseudo" perpendiculars; see
> > > -        * PerpType enum.
> > > -        */
> > > -       enum class PerpType {
> > > -               None, /* no perpendicular found */
> > > -               Start, /* start of Pwl is closest point */
> > > -               End, /* end of Pwl is closest point */
> > > -               Vertex, /* vertex of Pwl is closest point */
> > > -               Perpendicular /* true perpendicular found */
> > > -       };
> > > -       PerpType invert(Point const &xy, Point &perp, int &span,
> > > +
> > > +       PerpType invert(FPoint const &xy, FPoint &perp, int &span,
> > >                         const double eps = 1e-6) const;
> > > -       /*
> > > -        * Compute the inverse function. Indicate if it is a proper (true)
> > > -        * inverse, or only a best effort (e.g. input was non-monotonic).
> > > -        */
> > >         Pwl inverse(bool *trueInverse = nullptr, const double eps = 1e-6) const;
> > > -       /* Compose two Pwls together, doing "this" first and "other" after. */
> > >         Pwl compose(Pwl const &other, const double eps = 1e-6) const;
> > > -       /* Apply function to (x,y) values at every control point. */
> > > +
> > >         void map(std::function<void(double x, double y)> f) const;
> > > -       /*
> > > -        * Apply function to (x, y0, y1) values wherever either Pwl has a
> > > -        * control point.
> > > -        */
> > > +
> > >         static void map2(Pwl const &pwl0, Pwl const &pwl1,
> > >                          std::function<void(double x, double y0, double y1)> f);
> > > -       /*
> > > -        * Combine two Pwls, meaning we create a new Pwl where the y values are
> > > -        * given by running f wherever either has a knot.
> > > -        */
> > > +
> > >         static Pwl
> > >         combine(Pwl const &pwl0, Pwl const &pwl1,
> > >                 std::function<double(double x, double y0, double y1)> f,
> > >                 const double eps = 1e-6);
> > > -       /*
> > > -        * Make "this" match (at least) the given domain. Any extension my be
> > > -        * clipped or linear.
> > > -        */
> > > +
> > >         void matchDomain(Interval const &domain, bool clip = true,
> > >                          const double eps = 1e-6);
> > > +
> > >         Pwl &operator*=(double d);
> > > -       void debug(FILE *fp = stdout) const;
> > > +
> > > +       std::string toString() const;
> > >
> > >  private:
> > >         int findSpan(double x, int span) const;
> > > -       std::vector<Point> points_;
> > > +       std::vector<FPoint> points_;
> > >  };
> > > +
> > > +} /* namespace ipa */
> > > +
> > > +} /* namespace libcamera */
> > > --
> > > 2.39.2
> > >
Stefan Klug April 15, 2024, 12:46 p.m. UTC | #4
Hi Paul,

thanks for the patch.

On Fri, Apr 05, 2024 at 05:02:58PM +0900, Paul Elder wrote:
> Clean up the Pwl class copied from the Raspberry Pi IPA to align it more
> with the libcamera style.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> ---
>  src/ipa/libipa/pwl.cpp | 135 +++++++++++++++++++++++++++++++++--------
>  src/ipa/libipa/pwl.h   | 113 ++++++++++++++--------------------
>  2 files changed, 154 insertions(+), 94 deletions(-)
> 
> diff --git a/src/ipa/libipa/pwl.cpp b/src/ipa/libipa/pwl.cpp
> index 09f5d65c..58925d83 100644
> --- a/src/ipa/libipa/pwl.cpp
> +++ b/src/ipa/libipa/pwl.cpp
> @@ -5,13 +5,40 @@
>   * pwl.cpp - piecewise linear functions

If we are serious with the copyright, this would be the chance to add
Ideas on Board :-)

>   */
>  
> +#include "pwl.h"
> +
>  #include <cassert>
>  #include <cmath>
> +#include <sstream>
>  #include <stdexcept>
>  
> -#include "pwl.h"
> +#include <libcamera/geometry.h>
> +
> +namespace libcamera {
> +
> +namespace ipa {
>  
> -int Pwl::read(const libcamera::YamlObject &params)
> +/*
> + * \enum Pwl::PerpType
> + * \brief Type of perpendicular found when inverting a piecewise linear function
> + *
> + * \var None
> + * \brief no perpendicular found
> + *
> + * \var Start
> + * \brief start of Pwl is closest point
> + *
> + * \var End
> + * \brief end of Pwl is closest point
> + *
> + * \var Vertex
> + * \brief vertex of Pwl is closest point
> + *
> + * \var Perpendicular
> + * \brief true perpendicular found
> + */
> +
> +int Pwl::readYaml(const libcamera::YamlObject &params)
>  {
>  	if (!params.size() || params.size() % 2)
>  		return -EINVAL;
> @@ -29,7 +56,7 @@ int Pwl::read(const libcamera::YamlObject &params)
>  		if (!y)
>  			return -EINVAL;
>  
> -		points_.push_back(Point(*x, *y));
> +		points_.push_back(FPoint(*x, *y));
>  	}
>  
>  	return 0;
> @@ -38,13 +65,13 @@ int Pwl::read(const libcamera::YamlObject &params)
>  void Pwl::append(double x, double y, const double eps)
>  {
>  	if (points_.empty() || points_.back().x + eps < x)
> -		points_.push_back(Point(x, y));
> +		points_.push_back(FPoint(x, y));
>  }
>  
>  void Pwl::prepend(double x, double y, const double eps)
>  {
>  	if (points_.empty() || points_.front().x - eps > x)
> -		points_.insert(points_.begin(), Point(x, y));
> +		points_.insert(points_.begin(), FPoint(x, y));
>  }
>  
>  Pwl::Interval Pwl::domain() const
> @@ -65,6 +92,19 @@ bool Pwl::empty() const
>  	return points_.empty();
>  }
>  
> +/*
> + * \brief Evaluate the piecewise linear function
> + * \param[in] x The x value to input into the function
> + * \param[inout] spanPtr Initial guess for span
> + * \param[in] updateSpan Set to true to update spanPtr
> + *
> + * Evaluate Pwl, optionally supplying an initial guess for the
> + * "span". The "span" may be optionally be updated.  If you want to know
> + * the "span" value but don't have an initial guess you can set it to
> + * -1.
> + *
> + *  \return The result of evaluating the piecewise linear function with input \a x

Question to the native speakers: How would you phrase that? To me "...
evaluate pwl at position x ..." sound more natural.

> + */
>  double Pwl::eval(double x, int *spanPtr, bool updateSpan) const
>  {
>  	int span = findSpan(x, spanPtr && *spanPtr != -1 ? *spanPtr : points_.size() / 2 - 1);
> @@ -94,16 +134,22 @@ int Pwl::findSpan(double x, int span) const
>  	return span;
>  }
>  
> -Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
> +/*

Docs for the params is missing.

> + * Find perpendicular closest to xy, starting from span+1 so you can
> + * call it repeatedly to check for multiple closest points (set span to
> + * -1 on the first call). Also returns "pseudo" perpendiculars; see
> + * PerpType enum.
> + */
> +Pwl::PerpType Pwl::invert(FPoint const &xy, FPoint &perp, int &span,
>  			  const double eps) const
>  {
>  	assert(span >= -1);
>  	bool prevOffEnd = false;
>  	for (span = span + 1; span < (int)points_.size() - 1; span++) {
> -		Point spanVec = points_[span + 1] - points_[span];
> +		FPoint spanVec = points_[span + 1] - points_[span];
>  		double t = ((xy - points_[span]) % spanVec) / spanVec.len2();
> -		if (t < -eps) /* off the start of this span */
> -		{
> +		if (t < -eps) {
> +			/* off the start of this span */
>  			if (span == 0) {
>  				perp = points_[span];
>  				return PerpType::Start;
> @@ -111,15 +157,15 @@ Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
>  				perp = points_[span];
>  				return PerpType::Vertex;
>  			}
> -		} else if (t > 1 + eps) /* off the end of this span */
> -		{
> +		} else if (t > 1 + eps) {
> +			/* off the end of this span */
>  			if (span == (int)points_.size() - 2) {
>  				perp = points_[span + 1];
>  				return PerpType::End;
>  			}
>  			prevOffEnd = true;
> -		} else /* a true perpendicular */
> -		{
> +		} else {
> +			/* a true perpendicular */
>  			perp = points_[span] + spanVec * t;
>  			return PerpType::Perpendicular;
>  		}
> @@ -127,25 +173,34 @@ Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
>  	return PerpType::None;
>  }
>  
> +/*
> + * \brief Compute the inverse function
> + * \param[out] trueInverse True of the resulting inverse is a proper/true inverse

s/of/if/ , maybe s/resulting inverse/result/

> + * \param[in] eps Epsilon (optional)
> + * Indicate if it is a proper (true) inverse, or only a best effort (e.g.
> + * input was non-monotonic).

I believe there should be empty lines around the description.

> + * \return The inverse piecewise linear function
> + */
>  Pwl Pwl::inverse(bool *trueInverse, const double eps) const
>  {
>  	bool appended = false, prepended = false, neither = false;
>  	Pwl inverse;
>  
> -	for (Point const &p : points_) {
> -		if (inverse.empty())
> +	for (FPoint const &p : points_) {
> +		if (inverse.empty()) {
>  			inverse.append(p.y, p.x, eps);
> -		else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
> -			 std::abs(inverse.points_.front().x - p.y) <= eps)
> +		} else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
> +			   std::abs(inverse.points_.front().x - p.y) <= eps) {
>  			/* do nothing */;
> -		else if (p.y > inverse.points_.back().x) {
> +		} else if (p.y > inverse.points_.back().x) {
>  			inverse.append(p.y, p.x, eps);
>  			appended = true;
>  		} else if (p.y < inverse.points_.front().x) {
>  			inverse.prepend(p.y, p.x, eps);
>  			prepended = true;
> -		} else
> +		} else {
>  			neither = true;
> +		}
>  	}
>  
>  	/*
> @@ -159,18 +214,25 @@ Pwl Pwl::inverse(bool *trueInverse, const double eps) const
>  	return inverse;
>  }
>  
> +/*
> + * \brief Compose two piecewise linear functions together
> + * \param[in] other The "other" piecewise linear function
> + * \param[in] eps Epsilon (optiona)

empty line

> + * The "this" function is done first, and "other" after.

empty line

> + * \return The composed piecewise linear function
> + */
>  Pwl Pwl::compose(Pwl const &other, const double eps) const
>  {
>  	double thisX = points_[0].x, thisY = points_[0].y;
>  	int thisSpan = 0, otherSpan = other.findSpan(thisY, 0);
>  	Pwl result({ { thisX, other.eval(thisY, &otherSpan, false) } });
> +
>  	while (thisSpan != (int)points_.size() - 1) {
>  		double dx = points_[thisSpan + 1].x - points_[thisSpan].x,
>  		       dy = points_[thisSpan + 1].y - points_[thisSpan].y;
>  		if (std::abs(dy) > eps &&
>  		    otherSpan + 1 < (int)other.points_.size() &&
> -		    points_[thisSpan + 1].y >=
> -			    other.points_[otherSpan + 1].x + eps) {
> +		    points_[thisSpan + 1].y >= other.points_[otherSpan + 1].x + eps) {
>  			/*
>  			 * next control point in result will be where this
>  			 * function's y reaches the next span in other
> @@ -204,18 +266,24 @@ Pwl Pwl::compose(Pwl const &other, const double eps) const
>  	return result;
>  }
>  
> +/* \brief Apply function to (x,y) values at every control point. */
>  void Pwl::map(std::function<void(double x, double y)> f) const
>  {
>  	for (auto &pt : points_)
>  		f(pt.x, pt.y);
>  }
>  
> +/*
> + * \brief Apply function to (x, y0, y1) values wherever either Pwl has a
> + * control point.
> + */
>  void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
>  	       std::function<void(double x, double y0, double y1)> f)
>  {
>  	int span0 = 0, span1 = 0;
>  	double x = std::min(pwl0.points_[0].x, pwl1.points_[0].x);
>  	f(x, pwl0.eval(x, &span0, false), pwl1.eval(x, &span1, false));
> +
>  	while (span0 < (int)pwl0.points_.size() - 1 ||
>  	       span1 < (int)pwl1.points_.size() - 1) {
>  		if (span0 == (int)pwl0.points_.size() - 1)
> @@ -230,6 +298,12 @@ void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
>  	}
>  }
>  
> +/*
> + * \brief Combine two Pwls
> + *
> + * Create a new Pwl where the y values are given by running f wherever either
> + * has a knot.
> + */
>  Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
>  		 std::function<double(double x, double y0, double y1)> f,
>  		 const double eps)
> @@ -241,6 +315,11 @@ Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
>  	return result;
>  }
>  
> +/*
> + * \brief Make "this" match (at least) the given domain.
> + *

Without looking at the source, I would not understand what this function does.

> + * Any extension my be clipped or linear.

s/my/may/ 

> + */
>  void Pwl::matchDomain(Interval const &domain, bool clip, const double eps)
>  {
>  	int span = 0;
> @@ -258,10 +337,16 @@ Pwl &Pwl::operator*=(double d)
>  	return *this;
>  }
>  
> -void Pwl::debug(FILE *fp) const
> +std::string Pwl::toString() const

In similar cases we overload the << operator. Is there any particular
reason not to do that here?

>  {
> -	fprintf(fp, "Pwl {\n");
> +	std::stringstream ss;
> +	ss << "Pwl { ";
>  	for (auto &p : points_)
> -		fprintf(fp, "\t(%g, %g)\n", p.x, p.y);
> -	fprintf(fp, "}\n");
> +		ss << "(" << p.x << ", " << p.y << ") ";
> +	ss << "}";
> +	return ss.str();
>  }
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
> diff --git a/src/ipa/libipa/pwl.h b/src/ipa/libipa/pwl.h
> index 7a6a6452..ef49e302 100644
> --- a/src/ipa/libipa/pwl.h
> +++ b/src/ipa/libipa/pwl.h
> @@ -8,116 +8,91 @@
>  
>  #include <functional>
>  #include <math.h>
> +#include <string>
>  #include <vector>
>  
> +#include <libcamera/geometry.h>
> +
>  #include "libcamera/internal/yaml_parser.h"
>  
> +namespace libcamera {
> +
> +namespace ipa {
> +
>  class Pwl
>  {
>  public:
> +	enum class PerpType {
> +		None,
> +		Start,
> +		End,
> +		Vertex,
> +		Perpendicular,
> +	};
> +
>  	struct Interval {
>  		Interval(double _start, double _end)
> -			: start(_start), end(_end)
> -		{
> -		}
> -		double start, end;
> +			: start(_start), end(_end) {}
> +
>  		bool contains(double value)
>  		{
>  			return value >= start && value <= end;
>  		}
> -		double clip(double value)
> +
> +		double clamp(double value)
>  		{
>  			return value < start ? start
>  					     : (value > end ? end : value);
>  		}
> +
>  		double len() const { return end - start; }
> +
> +		double start, end;
>  	};
> -	struct Point {
> -		Point() : x(0), y(0) {}
> -		Point(double _x, double _y)
> -			: x(_x), y(_y) {}
> -		double x, y;
> -		Point operator-(Point const &p) const
> -		{
> -			return Point(x - p.x, y - p.y);
> -		}
> -		Point operator+(Point const &p) const
> -		{
> -			return Point(x + p.x, y + p.y);
> -		}
> -		double operator%(Point const &p) const
> -		{
> -			return x * p.x + y * p.y;
> -		}
> -		Point operator*(double f) const { return Point(x * f, y * f); }
> -		Point operator/(double f) const { return Point(x / f, y / f); }
> -		double len2() const { return x * x + y * y; }
> -		double len() const { return sqrt(len2()); }
> -	};
> +
>  	Pwl() {}
> -	Pwl(std::vector<Point> const &points) : points_(points) {}
> -	int read(const libcamera::YamlObject &params);
> +	Pwl(std::vector<FPoint> const &points)
> +		: points_(points) {}
> +	int readYaml(const libcamera::YamlObject &params);
> +
>  	void append(double x, double y, const double eps = 1e-6);
>  	void prepend(double x, double y, const double eps = 1e-6);
> +
>  	Interval domain() const;
>  	Interval range() const;
> +
>  	bool empty() const;
> -	/*
> -	 * Evaluate Pwl, optionally supplying an initial guess for the
> -	 * "span". The "span" may be optionally be updated.  If you want to know
> -	 * the "span" value but don't have an initial guess you can set it to
> -	 * -1.
> -	 */
> +
>  	double eval(double x, int *spanPtr = nullptr,
>  		    bool updateSpan = true) const;
> -	/*
> -	 * Find perpendicular closest to xy, starting from span+1 so you can
> -	 * call it repeatedly to check for multiple closest points (set span to
> -	 * -1 on the first call). Also returns "pseudo" perpendiculars; see
> -	 * PerpType enum.
> -	 */
> -	enum class PerpType {
> -		None, /* no perpendicular found */
> -		Start, /* start of Pwl is closest point */
> -		End, /* end of Pwl is closest point */
> -		Vertex, /* vertex of Pwl is closest point */
> -		Perpendicular /* true perpendicular found */
> -	};
> -	PerpType invert(Point const &xy, Point &perp, int &span,
> +
> +	PerpType invert(FPoint const &xy, FPoint &perp, int &span,
>  			const double eps = 1e-6) const;
> -	/*
> -	 * Compute the inverse function. Indicate if it is a proper (true)
> -	 * inverse, or only a best effort (e.g. input was non-monotonic).
> -	 */
>  	Pwl inverse(bool *trueInverse = nullptr, const double eps = 1e-6) const;
> -	/* Compose two Pwls together, doing "this" first and "other" after. */
>  	Pwl compose(Pwl const &other, const double eps = 1e-6) const;
> -	/* Apply function to (x,y) values at every control point. */
> +
>  	void map(std::function<void(double x, double y)> f) const;
> -	/*
> -	 * Apply function to (x, y0, y1) values wherever either Pwl has a
> -	 * control point.
> -	 */
> +
>  	static void map2(Pwl const &pwl0, Pwl const &pwl1,
>  			 std::function<void(double x, double y0, double y1)> f);
> -	/*
> -	 * Combine two Pwls, meaning we create a new Pwl where the y values are
> -	 * given by running f wherever either has a knot.
> -	 */
> +
>  	static Pwl
>  	combine(Pwl const &pwl0, Pwl const &pwl1,
>  		std::function<double(double x, double y0, double y1)> f,
>  		const double eps = 1e-6);
> -	/*
> -	 * Make "this" match (at least) the given domain. Any extension my be
> -	 * clipped or linear.
> -	 */
> +
>  	void matchDomain(Interval const &domain, bool clip = true,
>  			 const double eps = 1e-6);
> +
>  	Pwl &operator*=(double d);
> -	void debug(FILE *fp = stdout) const;
> +
> +	std::string toString() const;
>  
>  private:
>  	int findSpan(double x, int span) const;
> -	std::vector<Point> points_;
> +	std::vector<FPoint> points_;
>  };
> +
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */


I feel bad to say it, but a few initial tests would be gerat, as it
lands in public API :-)

Cheers,
Stefan

> -- 
> 2.39.2
>

Patch
diff mbox series

diff --git a/src/ipa/libipa/pwl.cpp b/src/ipa/libipa/pwl.cpp
index 09f5d65c..58925d83 100644
--- a/src/ipa/libipa/pwl.cpp
+++ b/src/ipa/libipa/pwl.cpp
@@ -5,13 +5,40 @@ 
  * pwl.cpp - piecewise linear functions
  */
 
+#include "pwl.h"
+
 #include <cassert>
 #include <cmath>
+#include <sstream>
 #include <stdexcept>
 
-#include "pwl.h"
+#include <libcamera/geometry.h>
+
+namespace libcamera {
+
+namespace ipa {
 
-int Pwl::read(const libcamera::YamlObject &params)
+/*
+ * \enum Pwl::PerpType
+ * \brief Type of perpendicular found when inverting a piecewise linear function
+ *
+ * \var None
+ * \brief no perpendicular found
+ *
+ * \var Start
+ * \brief start of Pwl is closest point
+ *
+ * \var End
+ * \brief end of Pwl is closest point
+ *
+ * \var Vertex
+ * \brief vertex of Pwl is closest point
+ *
+ * \var Perpendicular
+ * \brief true perpendicular found
+ */
+
+int Pwl::readYaml(const libcamera::YamlObject &params)
 {
 	if (!params.size() || params.size() % 2)
 		return -EINVAL;
@@ -29,7 +56,7 @@  int Pwl::read(const libcamera::YamlObject &params)
 		if (!y)
 			return -EINVAL;
 
-		points_.push_back(Point(*x, *y));
+		points_.push_back(FPoint(*x, *y));
 	}
 
 	return 0;
@@ -38,13 +65,13 @@  int Pwl::read(const libcamera::YamlObject &params)
 void Pwl::append(double x, double y, const double eps)
 {
 	if (points_.empty() || points_.back().x + eps < x)
-		points_.push_back(Point(x, y));
+		points_.push_back(FPoint(x, y));
 }
 
 void Pwl::prepend(double x, double y, const double eps)
 {
 	if (points_.empty() || points_.front().x - eps > x)
-		points_.insert(points_.begin(), Point(x, y));
+		points_.insert(points_.begin(), FPoint(x, y));
 }
 
 Pwl::Interval Pwl::domain() const
@@ -65,6 +92,19 @@  bool Pwl::empty() const
 	return points_.empty();
 }
 
+/*
+ * \brief Evaluate the piecewise linear function
+ * \param[in] x The x value to input into the function
+ * \param[inout] spanPtr Initial guess for span
+ * \param[in] updateSpan Set to true to update spanPtr
+ *
+ * Evaluate Pwl, optionally supplying an initial guess for the
+ * "span". The "span" may be optionally be updated.  If you want to know
+ * the "span" value but don't have an initial guess you can set it to
+ * -1.
+ *
+ *  \return The result of evaluating the piecewise linear function with input \a x
+ */
 double Pwl::eval(double x, int *spanPtr, bool updateSpan) const
 {
 	int span = findSpan(x, spanPtr && *spanPtr != -1 ? *spanPtr : points_.size() / 2 - 1);
@@ -94,16 +134,22 @@  int Pwl::findSpan(double x, int span) const
 	return span;
 }
 
-Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
+/*
+ * Find perpendicular closest to xy, starting from span+1 so you can
+ * call it repeatedly to check for multiple closest points (set span to
+ * -1 on the first call). Also returns "pseudo" perpendiculars; see
+ * PerpType enum.
+ */
+Pwl::PerpType Pwl::invert(FPoint const &xy, FPoint &perp, int &span,
 			  const double eps) const
 {
 	assert(span >= -1);
 	bool prevOffEnd = false;
 	for (span = span + 1; span < (int)points_.size() - 1; span++) {
-		Point spanVec = points_[span + 1] - points_[span];
+		FPoint spanVec = points_[span + 1] - points_[span];
 		double t = ((xy - points_[span]) % spanVec) / spanVec.len2();
-		if (t < -eps) /* off the start of this span */
-		{
+		if (t < -eps) {
+			/* off the start of this span */
 			if (span == 0) {
 				perp = points_[span];
 				return PerpType::Start;
@@ -111,15 +157,15 @@  Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
 				perp = points_[span];
 				return PerpType::Vertex;
 			}
-		} else if (t > 1 + eps) /* off the end of this span */
-		{
+		} else if (t > 1 + eps) {
+			/* off the end of this span */
 			if (span == (int)points_.size() - 2) {
 				perp = points_[span + 1];
 				return PerpType::End;
 			}
 			prevOffEnd = true;
-		} else /* a true perpendicular */
-		{
+		} else {
+			/* a true perpendicular */
 			perp = points_[span] + spanVec * t;
 			return PerpType::Perpendicular;
 		}
@@ -127,25 +173,34 @@  Pwl::PerpType Pwl::invert(Point const &xy, Point &perp, int &span,
 	return PerpType::None;
 }
 
+/*
+ * \brief Compute the inverse function
+ * \param[out] trueInverse True of the resulting inverse is a proper/true inverse
+ * \param[in] eps Epsilon (optional)
+ * Indicate if it is a proper (true) inverse, or only a best effort (e.g.
+ * input was non-monotonic).
+ * \return The inverse piecewise linear function
+ */
 Pwl Pwl::inverse(bool *trueInverse, const double eps) const
 {
 	bool appended = false, prepended = false, neither = false;
 	Pwl inverse;
 
-	for (Point const &p : points_) {
-		if (inverse.empty())
+	for (FPoint const &p : points_) {
+		if (inverse.empty()) {
 			inverse.append(p.y, p.x, eps);
-		else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
-			 std::abs(inverse.points_.front().x - p.y) <= eps)
+		} else if (std::abs(inverse.points_.back().x - p.y) <= eps ||
+			   std::abs(inverse.points_.front().x - p.y) <= eps) {
 			/* do nothing */;
-		else if (p.y > inverse.points_.back().x) {
+		} else if (p.y > inverse.points_.back().x) {
 			inverse.append(p.y, p.x, eps);
 			appended = true;
 		} else if (p.y < inverse.points_.front().x) {
 			inverse.prepend(p.y, p.x, eps);
 			prepended = true;
-		} else
+		} else {
 			neither = true;
+		}
 	}
 
 	/*
@@ -159,18 +214,25 @@  Pwl Pwl::inverse(bool *trueInverse, const double eps) const
 	return inverse;
 }
 
+/*
+ * \brief Compose two piecewise linear functions together
+ * \param[in] other The "other" piecewise linear function
+ * \param[in] eps Epsilon (optiona)
+ * The "this" function is done first, and "other" after.
+ * \return The composed piecewise linear function
+ */
 Pwl Pwl::compose(Pwl const &other, const double eps) const
 {
 	double thisX = points_[0].x, thisY = points_[0].y;
 	int thisSpan = 0, otherSpan = other.findSpan(thisY, 0);
 	Pwl result({ { thisX, other.eval(thisY, &otherSpan, false) } });
+
 	while (thisSpan != (int)points_.size() - 1) {
 		double dx = points_[thisSpan + 1].x - points_[thisSpan].x,
 		       dy = points_[thisSpan + 1].y - points_[thisSpan].y;
 		if (std::abs(dy) > eps &&
 		    otherSpan + 1 < (int)other.points_.size() &&
-		    points_[thisSpan + 1].y >=
-			    other.points_[otherSpan + 1].x + eps) {
+		    points_[thisSpan + 1].y >= other.points_[otherSpan + 1].x + eps) {
 			/*
 			 * next control point in result will be where this
 			 * function's y reaches the next span in other
@@ -204,18 +266,24 @@  Pwl Pwl::compose(Pwl const &other, const double eps) const
 	return result;
 }
 
+/* \brief Apply function to (x,y) values at every control point. */
 void Pwl::map(std::function<void(double x, double y)> f) const
 {
 	for (auto &pt : points_)
 		f(pt.x, pt.y);
 }
 
+/*
+ * \brief Apply function to (x, y0, y1) values wherever either Pwl has a
+ * control point.
+ */
 void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
 	       std::function<void(double x, double y0, double y1)> f)
 {
 	int span0 = 0, span1 = 0;
 	double x = std::min(pwl0.points_[0].x, pwl1.points_[0].x);
 	f(x, pwl0.eval(x, &span0, false), pwl1.eval(x, &span1, false));
+
 	while (span0 < (int)pwl0.points_.size() - 1 ||
 	       span1 < (int)pwl1.points_.size() - 1) {
 		if (span0 == (int)pwl0.points_.size() - 1)
@@ -230,6 +298,12 @@  void Pwl::map2(Pwl const &pwl0, Pwl const &pwl1,
 	}
 }
 
+/*
+ * \brief Combine two Pwls
+ *
+ * Create a new Pwl where the y values are given by running f wherever either
+ * has a knot.
+ */
 Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
 		 std::function<double(double x, double y0, double y1)> f,
 		 const double eps)
@@ -241,6 +315,11 @@  Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
 	return result;
 }
 
+/*
+ * \brief Make "this" match (at least) the given domain.
+ *
+ * Any extension my be clipped or linear.
+ */
 void Pwl::matchDomain(Interval const &domain, bool clip, const double eps)
 {
 	int span = 0;
@@ -258,10 +337,16 @@  Pwl &Pwl::operator*=(double d)
 	return *this;
 }
 
-void Pwl::debug(FILE *fp) const
+std::string Pwl::toString() const
 {
-	fprintf(fp, "Pwl {\n");
+	std::stringstream ss;
+	ss << "Pwl { ";
 	for (auto &p : points_)
-		fprintf(fp, "\t(%g, %g)\n", p.x, p.y);
-	fprintf(fp, "}\n");
+		ss << "(" << p.x << ", " << p.y << ") ";
+	ss << "}";
+	return ss.str();
 }
+
+} /* namespace ipa */
+
+} /* namespace libcamera */
diff --git a/src/ipa/libipa/pwl.h b/src/ipa/libipa/pwl.h
index 7a6a6452..ef49e302 100644
--- a/src/ipa/libipa/pwl.h
+++ b/src/ipa/libipa/pwl.h
@@ -8,116 +8,91 @@ 
 
 #include <functional>
 #include <math.h>
+#include <string>
 #include <vector>
 
+#include <libcamera/geometry.h>
+
 #include "libcamera/internal/yaml_parser.h"
 
+namespace libcamera {
+
+namespace ipa {
+
 class Pwl
 {
 public:
+	enum class PerpType {
+		None,
+		Start,
+		End,
+		Vertex,
+		Perpendicular,
+	};
+
 	struct Interval {
 		Interval(double _start, double _end)
-			: start(_start), end(_end)
-		{
-		}
-		double start, end;
+			: start(_start), end(_end) {}
+
 		bool contains(double value)
 		{
 			return value >= start && value <= end;
 		}
-		double clip(double value)
+
+		double clamp(double value)
 		{
 			return value < start ? start
 					     : (value > end ? end : value);
 		}
+
 		double len() const { return end - start; }
+
+		double start, end;
 	};
-	struct Point {
-		Point() : x(0), y(0) {}
-		Point(double _x, double _y)
-			: x(_x), y(_y) {}
-		double x, y;
-		Point operator-(Point const &p) const
-		{
-			return Point(x - p.x, y - p.y);
-		}
-		Point operator+(Point const &p) const
-		{
-			return Point(x + p.x, y + p.y);
-		}
-		double operator%(Point const &p) const
-		{
-			return x * p.x + y * p.y;
-		}
-		Point operator*(double f) const { return Point(x * f, y * f); }
-		Point operator/(double f) const { return Point(x / f, y / f); }
-		double len2() const { return x * x + y * y; }
-		double len() const { return sqrt(len2()); }
-	};
+
 	Pwl() {}
-	Pwl(std::vector<Point> const &points) : points_(points) {}
-	int read(const libcamera::YamlObject &params);
+	Pwl(std::vector<FPoint> const &points)
+		: points_(points) {}
+	int readYaml(const libcamera::YamlObject &params);
+
 	void append(double x, double y, const double eps = 1e-6);
 	void prepend(double x, double y, const double eps = 1e-6);
+
 	Interval domain() const;
 	Interval range() const;
+
 	bool empty() const;
-	/*
-	 * Evaluate Pwl, optionally supplying an initial guess for the
-	 * "span". The "span" may be optionally be updated.  If you want to know
-	 * the "span" value but don't have an initial guess you can set it to
-	 * -1.
-	 */
+
 	double eval(double x, int *spanPtr = nullptr,
 		    bool updateSpan = true) const;
-	/*
-	 * Find perpendicular closest to xy, starting from span+1 so you can
-	 * call it repeatedly to check for multiple closest points (set span to
-	 * -1 on the first call). Also returns "pseudo" perpendiculars; see
-	 * PerpType enum.
-	 */
-	enum class PerpType {
-		None, /* no perpendicular found */
-		Start, /* start of Pwl is closest point */
-		End, /* end of Pwl is closest point */
-		Vertex, /* vertex of Pwl is closest point */
-		Perpendicular /* true perpendicular found */
-	};
-	PerpType invert(Point const &xy, Point &perp, int &span,
+
+	PerpType invert(FPoint const &xy, FPoint &perp, int &span,
 			const double eps = 1e-6) const;
-	/*
-	 * Compute the inverse function. Indicate if it is a proper (true)
-	 * inverse, or only a best effort (e.g. input was non-monotonic).
-	 */
 	Pwl inverse(bool *trueInverse = nullptr, const double eps = 1e-6) const;
-	/* Compose two Pwls together, doing "this" first and "other" after. */
 	Pwl compose(Pwl const &other, const double eps = 1e-6) const;
-	/* Apply function to (x,y) values at every control point. */
+
 	void map(std::function<void(double x, double y)> f) const;
-	/*
-	 * Apply function to (x, y0, y1) values wherever either Pwl has a
-	 * control point.
-	 */
+
 	static void map2(Pwl const &pwl0, Pwl const &pwl1,
 			 std::function<void(double x, double y0, double y1)> f);
-	/*
-	 * Combine two Pwls, meaning we create a new Pwl where the y values are
-	 * given by running f wherever either has a knot.
-	 */
+
 	static Pwl
 	combine(Pwl const &pwl0, Pwl const &pwl1,
 		std::function<double(double x, double y0, double y1)> f,
 		const double eps = 1e-6);
-	/*
-	 * Make "this" match (at least) the given domain. Any extension my be
-	 * clipped or linear.
-	 */
+
 	void matchDomain(Interval const &domain, bool clip = true,
 			 const double eps = 1e-6);
+
 	Pwl &operator*=(double d);
-	void debug(FILE *fp = stdout) const;
+
+	std::string toString() const;
 
 private:
 	int findSpan(double x, int span) const;
-	std::vector<Point> points_;
+	std::vector<FPoint> points_;
 };
+
+} /* namespace ipa */
+
+} /* namespace libcamera */