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

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

Commit Message

Paul Elder June 11, 2024, 1:24 p.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>
Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
Acked-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

---
Changes in v8:
- use the updated Vector interface
- remove unused functions (prepend, invert, extendDomain)
- improve class documentation
- checkstyle
- s/PointF/Point/
- make inverse() return pair instead of output parameter
- fix const order
- fix includes

No change in v7

Changes in v6:
- move adding pwl to meson here

Changes in v5:
- fix documentation order
- fix some typos
- add the Vector-based PointF

Changes in v4:
- update to apply to new copy of pwl
- add documentation
- fix doxygen

No change in v3

Changes in v2:
- s/FPoint/PointF/g
- improve documentation
- s/matchDomain/extendDomain/
---
 src/ipa/libipa/meson.build |   2 +
 src/ipa/libipa/pwl.cpp     | 372 ++++++++++++++++++++++++++-----------
 src/ipa/libipa/pwl.h       | 133 +++++--------
 3 files changed, 311 insertions(+), 196 deletions(-)

Comments

Laurent Pinchart June 11, 2024, 10:14 p.m. UTC | #1
Hi Paul,

Thank you for the patch.

On Tue, Jun 11, 2024 at 10:24:29PM +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>
> Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> Acked-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> ---
> Changes in v8:
> - use the updated Vector interface
> - remove unused functions (prepend, invert, extendDomain)
> - improve class documentation
> - checkstyle
> - s/PointF/Point/
> - make inverse() return pair instead of output parameter
> - fix const order
> - fix includes
> 
> No change in v7
> 
> Changes in v6:
> - move adding pwl to meson here
> 
> Changes in v5:
> - fix documentation order
> - fix some typos
> - add the Vector-based PointF
> 
> Changes in v4:
> - update to apply to new copy of pwl
> - add documentation
> - fix doxygen
> 
> No change in v3
> 
> Changes in v2:
> - s/FPoint/PointF/g
> - improve documentation
> - s/matchDomain/extendDomain/
> ---
>  src/ipa/libipa/meson.build |   2 +
>  src/ipa/libipa/pwl.cpp     | 372 ++++++++++++++++++++++++++-----------
>  src/ipa/libipa/pwl.h       | 133 +++++--------
>  3 files changed, 311 insertions(+), 196 deletions(-)
> 
> diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> index 8b0c8fff901b..3669f8939d3b 100644
> --- a/src/ipa/libipa/meson.build
> +++ b/src/ipa/libipa/meson.build
> @@ -8,6 +8,7 @@ libipa_headers = files([
>      'fc_queue.h',
>      'histogram.h',
>      'module.h',
> +    'pwl.h',
>      'vector.h',
>  ])
>  
> @@ -19,6 +20,7 @@ libipa_sources = files([
>      'fc_queue.cpp',
>      'histogram.cpp',
>      'module.cpp',
> +    'pwl.cpp',
>      'vector.cpp',
>  ])
>  
> diff --git a/src/ipa/libipa/pwl.cpp b/src/ipa/libipa/pwl.cpp
> index e39123767aa6..4dc59981708d 100644
> --- a/src/ipa/libipa/pwl.cpp
> +++ b/src/ipa/libipa/pwl.cpp
> @@ -1,19 +1,120 @@
>  /* SPDX-License-Identifier: BSD-2-Clause */
>  /*
>   * Copyright (C) 2019, Raspberry Pi Ltd
> + * Copyright (C) 2024, Ideas on Board Oy
>   *
> - * piecewise linear functions
> + * Piecewise linear functions
>   */
>  
> -#include <cassert>
> +#include "pwl.h"
> +
> +#include <assert.h>
>  #include <cmath>
> +#include <sstream>
>  #include <stdexcept>
>  
> -#include "pwl.h"
> +#include <libcamera/geometry.h>

Unless I'm missing something, this isn't needed.

> +
> +/**
> + * \file pwl.h
> + * \brief Piecewise linear functions
> + */
> +
> +namespace libcamera {
> +
> +namespace ipa {
> +
> +/**
> + * \class Pwl
> + * \brief Describe a univariate piecewise linear function in two-dimensional
> + * real space
> + *
> + * A piecewise linear function is a univariate function that maps reals to
> + * reals, and it is composed of multiple straight-line segments.
> + *
> + * While a mathematical piecewise linear function would usually be defined by
> + * a list of linear functions and for which values of the domain they apply,
> + * this Pwl class is instead defined by a list of points at which these line
> + * segments intersect. These intersecting points are known as knots.
> + *
> + * https://en.wikipedia.org/wiki/Piecewise_linear_function
> + *
> + * A consequence of the Pwl class being defined by knots instead of linear
> + * functions is that the values of the piecewise linear function past the ends
> + * of the function are constants as opposed to linear functions. In a
> + * mathematical piecewise linear function that is defined by multiple linear
> + * functions, the ends of the function are also linear functions and hence grow
> + * to infinity (or negative infinity). However, since this Pwl class is defined
> + * by knots, the y-value of the leftmost and rightmost knots will hold for all
> + * x values to negative infinity and positive infinity, respectively.

Nice documentation, I especially like the part about what happens
outside of the defined segments, that was not evident.

> + */
> +
> +/**
> + * \typedef Pwl::Point
> + * \brief Describe a point in two-dimensional real space
> + */
> +
> +/**
> + * \class Pwl::Interval
> + * \brief Describe an interval in one-dimensional real space
> + */
> +
> +/**
> + * \fn Pwl::Interval::Interval(double _start, double _end)
> + * \brief Construct an interval
> + * \param _start Start of the interval
> + * \param _end End of the interval
> + */
> +
> +/**
> + * \fn Pwl::Interval::contains
> + * \brief Check if a given value falls within the interval
> + * \param value Value to check

 * \return True if the value falls within the interval, including its
 * bounds, or false otherwise

> + */
> +
> +/**
> + * \fn Pwl::Interval::clamp
> + * \brief Clamp a value such that it is within the interval
> + * \param value Value to clamp

 * \return The clamped value

> + */
> +
> +/**
> + * \fn Pwl::Interval::length
> + * \brief Compute the length of the interval

 * \return The length of the interval

> + */
> +
> +/**
> + * \var Pwl::Interval::start
> + * \brief Start of the interval
> + */
>  
> -using namespace RPiController;
> +/**
> + * \var Pwl::Interval::end
> + * \brief End of the interval
> + */
>  
> -int Pwl::read(const libcamera::YamlObject &params)
> +/**
> + * \fn Pwl::Pwl(std::vector<Point> const &points)
> + * \brief Construct a piecewise linear function from a list of 2D points
> + * \param points Vector of points from which to construct the piecewise linear function
> + *
> + * \a points must be in ascending order of x-value.
> + */
> +
> +/**
> + * \brief Populate the piecewise linear function from yaml data
> + * \param params Yaml data to populate the piecewise linear function with
> + *
> + * Any existing points in the piecewise linear function will *not* be
> + * overwritten.

It sounds like a bit of an off behaviour, compared to clearing the PWL
first. Does anything depends on it ?

> + *
> + * The yaml data is expected to be a list with an even number of numerical
> + * elements. These will be parsed in pairs into x and y points in the piecewise
> + * linear function, and added in order. x must be monotonically increasing.
> + *
> + * \return 0 on success, negative error code otherwise
> + */
> +int Pwl::readYaml(const libcamera::YamlObject &params)
>  {
>  	if (!params.size() || params.size() % 2)
>  		return -EINVAL;
> @@ -24,64 +125,109 @@ int Pwl::read(const libcamera::YamlObject &params)
>  		auto x = it->get<double>();
>  		if (!x)
>  			return -EINVAL;
> -		if (it != list.begin() && *x <= points_.back().x)
> +		if (it != list.begin() && *x <= points_.back().x())
>  			return -EINVAL;
>  
>  		auto y = (++it)->get<double>();
>  		if (!y)
>  			return -EINVAL;
>  
> -		points_.push_back(Point(*x, *y));
> +		points_.push_back(Point({ *x, *y }));
>  	}
>  
>  	return 0;
>  }
>  
> +/**
> + * \brief Append a point to the end of the piecewise linear function
> + * \param x x-coordinate of the point to add to the piecewise linear function
> + * \param y y-coordinate of the point to add to the piecewise linear function
> + * \param eps Epsilon for the minimum x distance between points (optional)
> + *
> + * The point's x-coordinate must be greater than the x-coordinate of the last
> + * (= greatest) point already in the piecewise linear function.
> + */
>  void Pwl::append(double x, double y, const double eps)
>  {
> -	if (points_.empty() || points_.back().x + eps < x)
> -		points_.push_back(Point(x, y));
> +	if (points_.empty() || points_.back().x() + eps < x)
> +		points_.push_back(Point({ x, y }));
>  }
>  
> +/**
> + * \brief Prepend a point to the beginning of the piecewise linear function
> + * \param x x-coordinate of the point to add to the piecewise linear function
> + * \param y y-coordinate of the point to add to the piecewise linear function
> + * \param eps Epsilon for the minimum x distance between points (optional)
> + *
> + * The point's x-coordinate must be less than the x-coordinate of the first
> + * (= smallest) point already in the piecewise linear function.
> + */
>  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));
> +	if (points_.empty() || points_.front().x() - eps > x)
> +		points_.insert(points_.begin(), Point({ x, y }));
>  }
>  
> +/**
> + * \brief Get the domain of the piecewise linear function
> + * \return An interval representing the domain
> + */
>  Pwl::Interval Pwl::domain() const
>  {
> -	return Interval(points_[0].x, points_[points_.size() - 1].x);
> +	return Interval(points_[0].x(), points_[points_.size() - 1].x());
>  }
>  
> +/**
> + * \brief Get the range of the piecewise linear function
> + * \return An interval representing the range
> + */
>  Pwl::Interval Pwl::range() const
>  {
> -	double lo = points_[0].y, hi = lo;
> +	double lo = points_[0].y(), hi = lo;
>  	for (auto &p : points_)
> -		lo = std::min(lo, p.y), hi = std::max(hi, p.y);
> +		lo = std::min(lo, p.y()), hi = std::max(hi, p.y());
>  	return Interval(lo, hi);
>  }
>  
> +/**
> + * \brief Check if the piecewise linear function is empty
> + * \return True if there are no points in the function, false otherwise
> + */
>  bool Pwl::empty() const
>  {
>  	return points_.empty();
>  }
>  
> -double Pwl::eval(double x, int *spanPtr, bool updateSpan) const
> +/**
> + * \brief Evaluate the piecewise linear function
> + * \param[in] x The x value to input into the function
> + * \param[inout] span Initial guess for span
> + * \param[in] updateSpan Set to true to update span
> + *
> + * 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 at position \a x
> + */
> +double Pwl::eval(double x, int *span, bool updateSpan) const
>  {
> -	int span = findSpan(x, spanPtr && *spanPtr != -1 ? *spanPtr : points_.size() / 2 - 1);
> -	if (spanPtr && updateSpan)
> -		*spanPtr = span;
> -	return points_[span].y +
> -	       (x - points_[span].x) * (points_[span + 1].y - points_[span].y) /
> -		       (points_[span + 1].x - points_[span].x);
> +	int index = findSpan(x, span && *span != -1
> +					? *span
> +					: points_.size() / 2 - 1);
> +	if (span && updateSpan)
> +		*span = index;
> +	return points_[index].y() +
> +	       (x - points_[index].x()) * (points_[index + 1].y() - points_[index].y()) /
> +		       (points_[index + 1].x() - points_[index].x());
>  }
>  
>  int Pwl::findSpan(double x, int span) const
>  {
>  	/*
>  	 * Pwls are generally small, so linear search may well be faster than
> -	 * binary, though could review this if large PWls start turning up.
> +	 * binary, though could review this if large Pwls start turning up.
>  	 */
>  	int lastSpan = points_.size() - 2;
>  	/*
> @@ -89,65 +235,43 @@ int Pwl::findSpan(double x, int span) const
>  	 * control point
>  	 */
>  	span = std::max(0, std::min(lastSpan, span));
> -	while (span < lastSpan && x >= points_[span + 1].x)
> +	while (span < lastSpan && x >= points_[span + 1].x())
>  		span++;
> -	while (span && x < points_[span].x)
> +	while (span && x < points_[span].x())
>  		span--;
>  	return span;
>  }
>  
> -Pwl::PerpType Pwl::invert(Point const &xy, Point &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];
> -		double t = ((xy - points_[span]) % spanVec) / spanVec.len2();
> -		if (t < -eps) /* off the start of this span */
> -		{
> -			if (span == 0) {
> -				perp = points_[span];
> -				return PerpType::Start;
> -			} else if (prevOffEnd) {
> -				perp = points_[span];
> -				return PerpType::Vertex;
> -			}
> -		} 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 */
> -		{
> -			perp = points_[span] + spanVec * t;
> -			return PerpType::Perpendicular;
> -		}
> -	}
> -	return PerpType::None;
> -}
> -
> -Pwl Pwl::inverse(bool *trueInverse, const double eps) const
> +/**
> + * \brief Compute the inverse function
> + * \param[in] eps Epsilon for the minimum x distance between points (optional)
> + *
> + * The output includes whether the resulting inverse function is a proper
> + * (true) inverse, or only a best effort (e.g. input was non-monotonic).
> + *
> + * \return A pair of the inverse piecewise linear function, and whether or not
> + * the result is a proper/true inverse
> + */
> +std::pair<Pwl, bool> Pwl::inverse(const double eps) const
>  {
>  	bool appended = false, prepended = false, neither = false;
>  	Pwl inverse;
>  
>  	for (Point 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)
> +		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) {
>  			/* do nothing */;
> -		else if (p.y > inverse.points_.back().x) {
> -			inverse.append(p.y, p.x, eps);
> +		} 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);
> +		} else if (p.y() < inverse.points_.front().x()) {
> +			inverse.prepend(p.y(), p.x(), eps);
>  			prepended = true;
> -		} else
> +		} else {
>  			neither = true;
> +		}
>  	}
>  
>  	/*
> @@ -155,50 +279,58 @@ Pwl Pwl::inverse(bool *trueInverse, const double eps) const
>  	 * onto both ends of the inverse, or if there were points that couldn't
>  	 * go on either.
>  	 */
> -	if (trueInverse)
> -		*trueInverse = !(neither || (appended && prepended));
> +	bool trueInverse = !(neither || (appended && prepended));
>  
> -	return inverse;
> +	return { inverse, trueInverse };
>  }
>  
> +/**
> + * \brief Compose two piecewise linear functions together
> + * \param[in] other The "other" piecewise linear function
> + * \param[in] eps Epsilon for the minimum x distance between points (optional)
> + *
> + * 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;
> +	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) } });
> +	Pwl result({ Point({ 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;
> +		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
>  			 */
> -			thisX = points_[thisSpan].x +
> -				(other.points_[otherSpan + 1].x -
> -				 points_[thisSpan].y) *
> +			thisX = points_[thisSpan].x() +
> +				(other.points_[otherSpan + 1].x() -
> +				 points_[thisSpan].y()) *
>  					dx / dy;
> -			thisY = other.points_[++otherSpan].x;
> +			thisY = other.points_[++otherSpan].x();
>  		} else if (std::abs(dy) > eps && otherSpan > 0 &&
> -			   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 previous span in other
>  			 */
> -			thisX = points_[thisSpan].x +
> -				(other.points_[otherSpan + 1].x -
> -				 points_[thisSpan].y) *
> +			thisX = points_[thisSpan].x() +
> +				(other.points_[otherSpan + 1].x() -
> +				 points_[thisSpan].y()) *
>  					dx / dy;
> -			thisY = other.points_[--otherSpan].x;
> +			thisY = other.points_[--otherSpan].x();
>  		} else {
>  			/* we stay in the same span in other */
>  			thisSpan++;
> -			thisX = points_[thisSpan].x,
> -			thisY = points_[thisSpan].y;
> +			thisX = points_[thisSpan].x(),
> +			thisY = points_[thisSpan].y();
>  		}
>  		result.append(thisX, other.eval(thisY, &otherSpan, false),
>  			      eps);
> @@ -206,32 +338,47 @@ Pwl Pwl::compose(Pwl const &other, const double eps) const
>  	return result;
>  }
>  
> +/**
> + * \brief Apply function to (x,y) values at every control point
> + * \param f Function to be applied
> + */
>  void Pwl::map(std::function<void(double x, double y)> f) const
>  {
>  	for (auto &pt : points_)
> -		f(pt.x, pt.y);
> +		f(pt.x(), pt.y());
>  }
>  
> +/**
> + * \brief Apply function to (x, y0, y1) values wherever either Pwl has a
> + * control point.

Missing \param

> + */
>  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);
> +	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)
> -			x = pwl1.points_[++span1].x;
> +			x = pwl1.points_[++span1].x();
>  		else if (span1 == (int)pwl1.points_.size() - 1)
> -			x = pwl0.points_[++span0].x;
> -		else if (pwl0.points_[span0 + 1].x > pwl1.points_[span1 + 1].x)
> -			x = pwl1.points_[++span1].x;
> +			x = pwl0.points_[++span0].x();
> +		else if (pwl0.points_[span0 + 1].x() > pwl1.points_[span1 + 1].x())
> +			x = pwl1.points_[++span1].x();
>  		else
> -			x = pwl0.points_[++span0].x;
> +			x = pwl0.points_[++span0].x();
>  		f(x, pwl0.eval(x, &span0, false), pwl1.eval(x, &span1, false));
>  	}
>  }
>  
> +/**
> + * \brief Combine two Pwls

Missing \param

> + *
> + * Create a new Pwl where the y values are given by running f wherever either
> + * has a knot.

Missing \return

> + */
>  Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
>  		 std::function<double(double x, double y0, double y1)> f,
>  		 const double eps)
> @@ -243,27 +390,32 @@ Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
>  	return result;
>  }
>  
> -void Pwl::matchDomain(Interval const &domain, bool clip, const double eps)
> -{
> -	int span = 0;
> -	prepend(domain.start, eval(clip ? points_[0].x : domain.start, &span),
> -		eps);
> -	span = points_.size() - 2;
> -	append(domain.end, eval(clip ? points_.back().x : domain.end, &span),
> -	       eps);
> -}
> -
> +/**
> + * \brief Multiply the piecewise linear function
> + * \param d Scalar multiplier to multiply the function by
> + * \return This function, after it has been multiplied by \a d
> + */
>  Pwl &Pwl::operator*=(double d)
>  {
>  	for (auto &pt : points_)
> -		pt.y *= d;
> +		pt[1] *= d;

If you add non-const x() and y() accessors to the Vector class that
return a reference, you could use

		pt.y() *= d;

Up to you.

>  	return *this;
>  }
>  
> -void Pwl::debug(FILE *fp) const
> +/**
> + * \brief Assemble and return a string describing the piecewise linear function
> + * \return A string describing the piecewise linear function
> + */
> +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 7d5e7e4d3fda..a2cbad6c1597 100644
> --- a/src/ipa/libipa/pwl.h
> +++ b/src/ipa/libipa/pwl.h
> @@ -2,126 +2,87 @@
>  /*
>   * Copyright (C) 2019, Raspberry Pi Ltd
>   *
> - * piecewise linear functions interface
> + * Piecewise linear functions interface
>   */
>  #pragma once
>  
> +#include <algorithm>
> +#include <cmath>
>  #include <functional>
> -#include <math.h>
> +#include <string>
> +#include <utility>
>  #include <vector>
>  
> +#include <libcamera/geometry.h>

Unless I'm missing something, this isn't needed.

> +
>  #include "libcamera/internal/yaml_parser.h"
>  
> -namespace RPiController {
> +#include "vector.h"
> +
> +namespace libcamera {
> +
> +namespace ipa {
>  
>  class Pwl
>  {
>  public:
> +	using Point = Vector<double, 2>;
> +
>  	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)
> -		{
> -			return value < start ? start
> -					     : (value > end ? end : value);
> -		}
> -		double len() const { return end - start; }
> -	};
> -	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
> +
> +		double clamp(double value)
>  		{
> -			return x * p.x + y * p.y;
> +			return std::clamp(value, start, end);
>  		}
> -		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()); }
> +
> +		double length() const { return end - start; }
> +
> +		double start, end;
>  	};
> +
>  	Pwl() {}

	Pwl() = default;

> -	Pwl(std::vector<Point> const &points) : points_(points) {}
> -	int read(const libcamera::YamlObject &params);
> +	Pwl(const std::vector<Point> &points)
> +		: points_(points) {}

	Pwl(const std::vector<Point> &points)
		: points_(points)
	{
	}

but I think it would be better to not make the constructor inline. You
can move the implementation to the .cpp file. Same for the default
constructor.

> +	int readYaml(const libcamera::YamlObject &params);
> +
>  	void append(double x, double y, const double eps = 1e-6);

Is there a reason to qualify eps with const but not x and y ? I would
qualify them all, or none (likely none). Same for other functions using
eps, I think you can drop the const qualifier.

> -	void prepend(double x, double y, const double eps = 1e-6);
> +
> +	bool empty() const;
>  	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,
> +
> +	double eval(double x, int *span = 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,
> -			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. */
> +
> +	std::pair<Pwl, bool> inverse(const double eps = 1e-6) const;
> +	Pwl compose(const Pwl &other, const double eps = 1e-6) const;
> +
>  	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,
> +	combine(const Pwl &pwl0, const Pwl &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:
> +	void prepend(double x, double y, const double eps = 1e-6);
> +	static void map2(const Pwl &pwl0, const Pwl &pwl1,
> +			 std::function<void(double x, double y0, double y1)> f);

We usually put the static functions first or last, but not in the
middle.

>  	int findSpan(double x, int span) const;

And a blank line here to separate functions from variables.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	std::vector<Point> points_;
>  };
>  
> -} /* namespace RPiController */
> +} /* namespace ipa */
> +
> +} /* namespace libcamera */
Paul Elder June 12, 2024, 7:56 a.m. UTC | #2
On Wed, Jun 12, 2024 at 01:14:41AM +0300, Laurent Pinchart wrote:
> Hi Paul,
> 
> Thank you for the patch.
> 
> On Tue, Jun 11, 2024 at 10:24:29PM +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>
> > Reviewed-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > Acked-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > ---
> > Changes in v8:
> > - use the updated Vector interface
> > - remove unused functions (prepend, invert, extendDomain)
> > - improve class documentation
> > - checkstyle
> > - s/PointF/Point/
> > - make inverse() return pair instead of output parameter
> > - fix const order
> > - fix includes
> > 
> > No change in v7
> > 
> > Changes in v6:
> > - move adding pwl to meson here
> > 
> > Changes in v5:
> > - fix documentation order
> > - fix some typos
> > - add the Vector-based PointF
> > 
> > Changes in v4:
> > - update to apply to new copy of pwl
> > - add documentation
> > - fix doxygen
> > 
> > No change in v3
> > 
> > Changes in v2:
> > - s/FPoint/PointF/g
> > - improve documentation
> > - s/matchDomain/extendDomain/
> > ---
> >  src/ipa/libipa/meson.build |   2 +
> >  src/ipa/libipa/pwl.cpp     | 372 ++++++++++++++++++++++++++-----------
> >  src/ipa/libipa/pwl.h       | 133 +++++--------
> >  3 files changed, 311 insertions(+), 196 deletions(-)
> > 
> > diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
> > index 8b0c8fff901b..3669f8939d3b 100644
> > --- a/src/ipa/libipa/meson.build
> > +++ b/src/ipa/libipa/meson.build
> > @@ -8,6 +8,7 @@ libipa_headers = files([
> >      'fc_queue.h',
> >      'histogram.h',
> >      'module.h',
> > +    'pwl.h',
> >      'vector.h',
> >  ])
> >  
> > @@ -19,6 +20,7 @@ libipa_sources = files([
> >      'fc_queue.cpp',
> >      'histogram.cpp',
> >      'module.cpp',
> > +    'pwl.cpp',
> >      'vector.cpp',
> >  ])
> >  
> > diff --git a/src/ipa/libipa/pwl.cpp b/src/ipa/libipa/pwl.cpp
> > index e39123767aa6..4dc59981708d 100644
> > --- a/src/ipa/libipa/pwl.cpp
> > +++ b/src/ipa/libipa/pwl.cpp
> > @@ -1,19 +1,120 @@
> >  /* SPDX-License-Identifier: BSD-2-Clause */
> >  /*
> >   * Copyright (C) 2019, Raspberry Pi Ltd
> > + * Copyright (C) 2024, Ideas on Board Oy
> >   *
> > - * piecewise linear functions
> > + * Piecewise linear functions
> >   */
> >  
> > -#include <cassert>
> > +#include "pwl.h"
> > +
> > +#include <assert.h>
> >  #include <cmath>
> > +#include <sstream>
> >  #include <stdexcept>
> >  
> > -#include "pwl.h"
> > +#include <libcamera/geometry.h>
> 
> Unless I'm missing something, this isn't needed.
> 
> > +
> > +/**
> > + * \file pwl.h
> > + * \brief Piecewise linear functions
> > + */
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> > +
> > +/**
> > + * \class Pwl
> > + * \brief Describe a univariate piecewise linear function in two-dimensional
> > + * real space
> > + *
> > + * A piecewise linear function is a univariate function that maps reals to
> > + * reals, and it is composed of multiple straight-line segments.
> > + *
> > + * While a mathematical piecewise linear function would usually be defined by
> > + * a list of linear functions and for which values of the domain they apply,
> > + * this Pwl class is instead defined by a list of points at which these line
> > + * segments intersect. These intersecting points are known as knots.
> > + *
> > + * https://en.wikipedia.org/wiki/Piecewise_linear_function
> > + *
> > + * A consequence of the Pwl class being defined by knots instead of linear
> > + * functions is that the values of the piecewise linear function past the ends
> > + * of the function are constants as opposed to linear functions. In a
> > + * mathematical piecewise linear function that is defined by multiple linear
> > + * functions, the ends of the function are also linear functions and hence grow
> > + * to infinity (or negative infinity). However, since this Pwl class is defined
> > + * by knots, the y-value of the leftmost and rightmost knots will hold for all
> > + * x values to negative infinity and positive infinity, respectively.
> 
> Nice documentation, I especially like the part about what happens
> outside of the defined segments, that was not evident.
> 
> > + */
> > +
> > +/**
> > + * \typedef Pwl::Point
> > + * \brief Describe a point in two-dimensional real space
> > + */
> > +
> > +/**
> > + * \class Pwl::Interval
> > + * \brief Describe an interval in one-dimensional real space
> > + */
> > +
> > +/**
> > + * \fn Pwl::Interval::Interval(double _start, double _end)
> > + * \brief Construct an interval
> > + * \param _start Start of the interval
> > + * \param _end End of the interval
> > + */
> > +
> > +/**
> > + * \fn Pwl::Interval::contains
> > + * \brief Check if a given value falls within the interval
> > + * \param value Value to check
> 
>  * \return True if the value falls within the interval, including its
>  * bounds, or false otherwise
> 
> > + */
> > +
> > +/**
> > + * \fn Pwl::Interval::clamp
> > + * \brief Clamp a value such that it is within the interval
> > + * \param value Value to clamp
> 
>  * \return The clamped value
> 
> > + */
> > +
> > +/**
> > + * \fn Pwl::Interval::length
> > + * \brief Compute the length of the interval
> 
>  * \return The length of the interval
> 
> > + */
> > +
> > +/**
> > + * \var Pwl::Interval::start
> > + * \brief Start of the interval
> > + */
> >  
> > -using namespace RPiController;
> > +/**
> > + * \var Pwl::Interval::end
> > + * \brief End of the interval
> > + */
> >  
> > -int Pwl::read(const libcamera::YamlObject &params)
> > +/**
> > + * \fn Pwl::Pwl(std::vector<Point> const &points)
> > + * \brief Construct a piecewise linear function from a list of 2D points
> > + * \param points Vector of points from which to construct the piecewise linear function
> > + *
> > + * \a points must be in ascending order of x-value.
> > + */
> > +
> > +/**
> > + * \brief Populate the piecewise linear function from yaml data
> > + * \param params Yaml data to populate the piecewise linear function with
> > + *
> > + * Any existing points in the piecewise linear function will *not* be
> > + * overwritten.
> 
> It sounds like a bit of an off behaviour, compared to clearing the PWL
> first. Does anything depends on it ?
> 

Actually, from what I can tell everything else assumes it's cleared, so
I'll clear it first.


> > + *
> > + * The yaml data is expected to be a list with an even number of numerical
> > + * elements. These will be parsed in pairs into x and y points in the piecewise
> > + * linear function, and added in order. x must be monotonically increasing.
> > + *
> > + * \return 0 on success, negative error code otherwise
> > + */
> > +int Pwl::readYaml(const libcamera::YamlObject &params)
> >  {
> >  	if (!params.size() || params.size() % 2)
> >  		return -EINVAL;
> > @@ -24,64 +125,109 @@ int Pwl::read(const libcamera::YamlObject &params)
> >  		auto x = it->get<double>();
> >  		if (!x)
> >  			return -EINVAL;
> > -		if (it != list.begin() && *x <= points_.back().x)
> > +		if (it != list.begin() && *x <= points_.back().x())
> >  			return -EINVAL;
> >  
> >  		auto y = (++it)->get<double>();
> >  		if (!y)
> >  			return -EINVAL;
> >  
> > -		points_.push_back(Point(*x, *y));
> > +		points_.push_back(Point({ *x, *y }));
> >  	}
> >  
> >  	return 0;
> >  }
> >  
> > +/**
> > + * \brief Append a point to the end of the piecewise linear function
> > + * \param x x-coordinate of the point to add to the piecewise linear function
> > + * \param y y-coordinate of the point to add to the piecewise linear function
> > + * \param eps Epsilon for the minimum x distance between points (optional)
> > + *
> > + * The point's x-coordinate must be greater than the x-coordinate of the last
> > + * (= greatest) point already in the piecewise linear function.
> > + */
> >  void Pwl::append(double x, double y, const double eps)
> >  {
> > -	if (points_.empty() || points_.back().x + eps < x)
> > -		points_.push_back(Point(x, y));
> > +	if (points_.empty() || points_.back().x() + eps < x)
> > +		points_.push_back(Point({ x, y }));
> >  }
> >  
> > +/**
> > + * \brief Prepend a point to the beginning of the piecewise linear function
> > + * \param x x-coordinate of the point to add to the piecewise linear function
> > + * \param y y-coordinate of the point to add to the piecewise linear function
> > + * \param eps Epsilon for the minimum x distance between points (optional)
> > + *
> > + * The point's x-coordinate must be less than the x-coordinate of the first
> > + * (= smallest) point already in the piecewise linear function.
> > + */
> >  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));
> > +	if (points_.empty() || points_.front().x() - eps > x)
> > +		points_.insert(points_.begin(), Point({ x, y }));
> >  }
> >  
> > +/**
> > + * \brief Get the domain of the piecewise linear function
> > + * \return An interval representing the domain
> > + */
> >  Pwl::Interval Pwl::domain() const
> >  {
> > -	return Interval(points_[0].x, points_[points_.size() - 1].x);
> > +	return Interval(points_[0].x(), points_[points_.size() - 1].x());
> >  }
> >  
> > +/**
> > + * \brief Get the range of the piecewise linear function
> > + * \return An interval representing the range
> > + */
> >  Pwl::Interval Pwl::range() const
> >  {
> > -	double lo = points_[0].y, hi = lo;
> > +	double lo = points_[0].y(), hi = lo;
> >  	for (auto &p : points_)
> > -		lo = std::min(lo, p.y), hi = std::max(hi, p.y);
> > +		lo = std::min(lo, p.y()), hi = std::max(hi, p.y());
> >  	return Interval(lo, hi);
> >  }
> >  
> > +/**
> > + * \brief Check if the piecewise linear function is empty
> > + * \return True if there are no points in the function, false otherwise
> > + */
> >  bool Pwl::empty() const
> >  {
> >  	return points_.empty();
> >  }
> >  
> > -double Pwl::eval(double x, int *spanPtr, bool updateSpan) const
> > +/**
> > + * \brief Evaluate the piecewise linear function
> > + * \param[in] x The x value to input into the function
> > + * \param[inout] span Initial guess for span
> > + * \param[in] updateSpan Set to true to update span
> > + *
> > + * 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 at position \a x
> > + */
> > +double Pwl::eval(double x, int *span, bool updateSpan) const
> >  {
> > -	int span = findSpan(x, spanPtr && *spanPtr != -1 ? *spanPtr : points_.size() / 2 - 1);
> > -	if (spanPtr && updateSpan)
> > -		*spanPtr = span;
> > -	return points_[span].y +
> > -	       (x - points_[span].x) * (points_[span + 1].y - points_[span].y) /
> > -		       (points_[span + 1].x - points_[span].x);
> > +	int index = findSpan(x, span && *span != -1
> > +					? *span
> > +					: points_.size() / 2 - 1);
> > +	if (span && updateSpan)
> > +		*span = index;
> > +	return points_[index].y() +
> > +	       (x - points_[index].x()) * (points_[index + 1].y() - points_[index].y()) /
> > +		       (points_[index + 1].x() - points_[index].x());
> >  }
> >  
> >  int Pwl::findSpan(double x, int span) const
> >  {
> >  	/*
> >  	 * Pwls are generally small, so linear search may well be faster than
> > -	 * binary, though could review this if large PWls start turning up.
> > +	 * binary, though could review this if large Pwls start turning up.
> >  	 */
> >  	int lastSpan = points_.size() - 2;
> >  	/*
> > @@ -89,65 +235,43 @@ int Pwl::findSpan(double x, int span) const
> >  	 * control point
> >  	 */
> >  	span = std::max(0, std::min(lastSpan, span));
> > -	while (span < lastSpan && x >= points_[span + 1].x)
> > +	while (span < lastSpan && x >= points_[span + 1].x())
> >  		span++;
> > -	while (span && x < points_[span].x)
> > +	while (span && x < points_[span].x())
> >  		span--;
> >  	return span;
> >  }
> >  
> > -Pwl::PerpType Pwl::invert(Point const &xy, Point &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];
> > -		double t = ((xy - points_[span]) % spanVec) / spanVec.len2();
> > -		if (t < -eps) /* off the start of this span */
> > -		{
> > -			if (span == 0) {
> > -				perp = points_[span];
> > -				return PerpType::Start;
> > -			} else if (prevOffEnd) {
> > -				perp = points_[span];
> > -				return PerpType::Vertex;
> > -			}
> > -		} 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 */
> > -		{
> > -			perp = points_[span] + spanVec * t;
> > -			return PerpType::Perpendicular;
> > -		}
> > -	}
> > -	return PerpType::None;
> > -}
> > -
> > -Pwl Pwl::inverse(bool *trueInverse, const double eps) const
> > +/**
> > + * \brief Compute the inverse function
> > + * \param[in] eps Epsilon for the minimum x distance between points (optional)
> > + *
> > + * The output includes whether the resulting inverse function is a proper
> > + * (true) inverse, or only a best effort (e.g. input was non-monotonic).
> > + *
> > + * \return A pair of the inverse piecewise linear function, and whether or not
> > + * the result is a proper/true inverse
> > + */
> > +std::pair<Pwl, bool> Pwl::inverse(const double eps) const
> >  {
> >  	bool appended = false, prepended = false, neither = false;
> >  	Pwl inverse;
> >  
> >  	for (Point 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)
> > +		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) {
> >  			/* do nothing */;
> > -		else if (p.y > inverse.points_.back().x) {
> > -			inverse.append(p.y, p.x, eps);
> > +		} 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);
> > +		} else if (p.y() < inverse.points_.front().x()) {
> > +			inverse.prepend(p.y(), p.x(), eps);
> >  			prepended = true;
> > -		} else
> > +		} else {
> >  			neither = true;
> > +		}
> >  	}
> >  
> >  	/*
> > @@ -155,50 +279,58 @@ Pwl Pwl::inverse(bool *trueInverse, const double eps) const
> >  	 * onto both ends of the inverse, or if there were points that couldn't
> >  	 * go on either.
> >  	 */
> > -	if (trueInverse)
> > -		*trueInverse = !(neither || (appended && prepended));
> > +	bool trueInverse = !(neither || (appended && prepended));
> >  
> > -	return inverse;
> > +	return { inverse, trueInverse };
> >  }
> >  
> > +/**
> > + * \brief Compose two piecewise linear functions together
> > + * \param[in] other The "other" piecewise linear function
> > + * \param[in] eps Epsilon for the minimum x distance between points (optional)
> > + *
> > + * 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;
> > +	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) } });
> > +	Pwl result({ Point({ 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;
> > +		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
> >  			 */
> > -			thisX = points_[thisSpan].x +
> > -				(other.points_[otherSpan + 1].x -
> > -				 points_[thisSpan].y) *
> > +			thisX = points_[thisSpan].x() +
> > +				(other.points_[otherSpan + 1].x() -
> > +				 points_[thisSpan].y()) *
> >  					dx / dy;
> > -			thisY = other.points_[++otherSpan].x;
> > +			thisY = other.points_[++otherSpan].x();
> >  		} else if (std::abs(dy) > eps && otherSpan > 0 &&
> > -			   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 previous span in other
> >  			 */
> > -			thisX = points_[thisSpan].x +
> > -				(other.points_[otherSpan + 1].x -
> > -				 points_[thisSpan].y) *
> > +			thisX = points_[thisSpan].x() +
> > +				(other.points_[otherSpan + 1].x() -
> > +				 points_[thisSpan].y()) *
> >  					dx / dy;
> > -			thisY = other.points_[--otherSpan].x;
> > +			thisY = other.points_[--otherSpan].x();
> >  		} else {
> >  			/* we stay in the same span in other */
> >  			thisSpan++;
> > -			thisX = points_[thisSpan].x,
> > -			thisY = points_[thisSpan].y;
> > +			thisX = points_[thisSpan].x(),
> > +			thisY = points_[thisSpan].y();
> >  		}
> >  		result.append(thisX, other.eval(thisY, &otherSpan, false),
> >  			      eps);
> > @@ -206,32 +338,47 @@ Pwl Pwl::compose(Pwl const &other, const double eps) const
> >  	return result;
> >  }
> >  
> > +/**
> > + * \brief Apply function to (x,y) values at every control point
> > + * \param f Function to be applied
> > + */
> >  void Pwl::map(std::function<void(double x, double y)> f) const
> >  {
> >  	for (auto &pt : points_)
> > -		f(pt.x, pt.y);
> > +		f(pt.x(), pt.y());
> >  }
> >  
> > +/**
> > + * \brief Apply function to (x, y0, y1) values wherever either Pwl has a
> > + * control point.
> 
> Missing \param
> 
> > + */
> >  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);
> > +	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)
> > -			x = pwl1.points_[++span1].x;
> > +			x = pwl1.points_[++span1].x();
> >  		else if (span1 == (int)pwl1.points_.size() - 1)
> > -			x = pwl0.points_[++span0].x;
> > -		else if (pwl0.points_[span0 + 1].x > pwl1.points_[span1 + 1].x)
> > -			x = pwl1.points_[++span1].x;
> > +			x = pwl0.points_[++span0].x();
> > +		else if (pwl0.points_[span0 + 1].x() > pwl1.points_[span1 + 1].x())
> > +			x = pwl1.points_[++span1].x();
> >  		else
> > -			x = pwl0.points_[++span0].x;
> > +			x = pwl0.points_[++span0].x();
> >  		f(x, pwl0.eval(x, &span0, false), pwl1.eval(x, &span1, false));
> >  	}
> >  }
> >  
> > +/**
> > + * \brief Combine two Pwls
> 
> Missing \param
> 
> > + *
> > + * Create a new Pwl where the y values are given by running f wherever either
> > + * has a knot.
> 
> Missing \return
> 
> > + */
> >  Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
> >  		 std::function<double(double x, double y0, double y1)> f,
> >  		 const double eps)
> > @@ -243,27 +390,32 @@ Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
> >  	return result;
> >  }
> >  
> > -void Pwl::matchDomain(Interval const &domain, bool clip, const double eps)
> > -{
> > -	int span = 0;
> > -	prepend(domain.start, eval(clip ? points_[0].x : domain.start, &span),
> > -		eps);
> > -	span = points_.size() - 2;
> > -	append(domain.end, eval(clip ? points_.back().x : domain.end, &span),
> > -	       eps);
> > -}
> > -
> > +/**
> > + * \brief Multiply the piecewise linear function
> > + * \param d Scalar multiplier to multiply the function by
> > + * \return This function, after it has been multiplied by \a d
> > + */
> >  Pwl &Pwl::operator*=(double d)
> >  {
> >  	for (auto &pt : points_)
> > -		pt.y *= d;
> > +		pt[1] *= d;
> 
> If you add non-const x() and y() accessors to the Vector class that
> return a reference, you could use
> 
> 		pt.y() *= d;
> 
> Up to you.

Eeh I'll go without it.


Paul

> 
> >  	return *this;
> >  }
> >  
> > -void Pwl::debug(FILE *fp) const
> > +/**
> > + * \brief Assemble and return a string describing the piecewise linear function
> > + * \return A string describing the piecewise linear function
> > + */
> > +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 7d5e7e4d3fda..a2cbad6c1597 100644
> > --- a/src/ipa/libipa/pwl.h
> > +++ b/src/ipa/libipa/pwl.h
> > @@ -2,126 +2,87 @@
> >  /*
> >   * Copyright (C) 2019, Raspberry Pi Ltd
> >   *
> > - * piecewise linear functions interface
> > + * Piecewise linear functions interface
> >   */
> >  #pragma once
> >  
> > +#include <algorithm>
> > +#include <cmath>
> >  #include <functional>
> > -#include <math.h>
> > +#include <string>
> > +#include <utility>
> >  #include <vector>
> >  
> > +#include <libcamera/geometry.h>
> 
> Unless I'm missing something, this isn't needed.
> 
> > +
> >  #include "libcamera/internal/yaml_parser.h"
> >  
> > -namespace RPiController {
> > +#include "vector.h"
> > +
> > +namespace libcamera {
> > +
> > +namespace ipa {
> >  
> >  class Pwl
> >  {
> >  public:
> > +	using Point = Vector<double, 2>;
> > +
> >  	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)
> > -		{
> > -			return value < start ? start
> > -					     : (value > end ? end : value);
> > -		}
> > -		double len() const { return end - start; }
> > -	};
> > -	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
> > +
> > +		double clamp(double value)
> >  		{
> > -			return x * p.x + y * p.y;
> > +			return std::clamp(value, start, end);
> >  		}
> > -		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()); }
> > +
> > +		double length() const { return end - start; }
> > +
> > +		double start, end;
> >  	};
> > +
> >  	Pwl() {}
> 
> 	Pwl() = default;
> 
> > -	Pwl(std::vector<Point> const &points) : points_(points) {}
> > -	int read(const libcamera::YamlObject &params);
> > +	Pwl(const std::vector<Point> &points)
> > +		: points_(points) {}
> 
> 	Pwl(const std::vector<Point> &points)
> 		: points_(points)
> 	{
> 	}
> 
> but I think it would be better to not make the constructor inline. You
> can move the implementation to the .cpp file. Same for the default
> constructor.
> 
> > +	int readYaml(const libcamera::YamlObject &params);
> > +
> >  	void append(double x, double y, const double eps = 1e-6);
> 
> Is there a reason to qualify eps with const but not x and y ? I would
> qualify them all, or none (likely none). Same for other functions using
> eps, I think you can drop the const qualifier.
> 
> > -	void prepend(double x, double y, const double eps = 1e-6);
> > +
> > +	bool empty() const;
> >  	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,
> > +
> > +	double eval(double x, int *span = 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,
> > -			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. */
> > +
> > +	std::pair<Pwl, bool> inverse(const double eps = 1e-6) const;
> > +	Pwl compose(const Pwl &other, const double eps = 1e-6) const;
> > +
> >  	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,
> > +	combine(const Pwl &pwl0, const Pwl &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:
> > +	void prepend(double x, double y, const double eps = 1e-6);
> > +	static void map2(const Pwl &pwl0, const Pwl &pwl1,
> > +			 std::function<void(double x, double y0, double y1)> f);
> 
> We usually put the static functions first or last, but not in the
> middle.
> 
> >  	int findSpan(double x, int span) const;
> 
> And a blank line here to separate functions from variables.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> >  	std::vector<Point> points_;
> >  };
> >  
> > -} /* namespace RPiController */
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Patch
diff mbox series

diff --git a/src/ipa/libipa/meson.build b/src/ipa/libipa/meson.build
index 8b0c8fff901b..3669f8939d3b 100644
--- a/src/ipa/libipa/meson.build
+++ b/src/ipa/libipa/meson.build
@@ -8,6 +8,7 @@  libipa_headers = files([
     'fc_queue.h',
     'histogram.h',
     'module.h',
+    'pwl.h',
     'vector.h',
 ])
 
@@ -19,6 +20,7 @@  libipa_sources = files([
     'fc_queue.cpp',
     'histogram.cpp',
     'module.cpp',
+    'pwl.cpp',
     'vector.cpp',
 ])
 
diff --git a/src/ipa/libipa/pwl.cpp b/src/ipa/libipa/pwl.cpp
index e39123767aa6..4dc59981708d 100644
--- a/src/ipa/libipa/pwl.cpp
+++ b/src/ipa/libipa/pwl.cpp
@@ -1,19 +1,120 @@ 
 /* SPDX-License-Identifier: BSD-2-Clause */
 /*
  * Copyright (C) 2019, Raspberry Pi Ltd
+ * Copyright (C) 2024, Ideas on Board Oy
  *
- * piecewise linear functions
+ * Piecewise linear functions
  */
 
-#include <cassert>
+#include "pwl.h"
+
+#include <assert.h>
 #include <cmath>
+#include <sstream>
 #include <stdexcept>
 
-#include "pwl.h"
+#include <libcamera/geometry.h>
+
+/**
+ * \file pwl.h
+ * \brief Piecewise linear functions
+ */
+
+namespace libcamera {
+
+namespace ipa {
+
+/**
+ * \class Pwl
+ * \brief Describe a univariate piecewise linear function in two-dimensional
+ * real space
+ *
+ * A piecewise linear function is a univariate function that maps reals to
+ * reals, and it is composed of multiple straight-line segments.
+ *
+ * While a mathematical piecewise linear function would usually be defined by
+ * a list of linear functions and for which values of the domain they apply,
+ * this Pwl class is instead defined by a list of points at which these line
+ * segments intersect. These intersecting points are known as knots.
+ *
+ * https://en.wikipedia.org/wiki/Piecewise_linear_function
+ *
+ * A consequence of the Pwl class being defined by knots instead of linear
+ * functions is that the values of the piecewise linear function past the ends
+ * of the function are constants as opposed to linear functions. In a
+ * mathematical piecewise linear function that is defined by multiple linear
+ * functions, the ends of the function are also linear functions and hence grow
+ * to infinity (or negative infinity). However, since this Pwl class is defined
+ * by knots, the y-value of the leftmost and rightmost knots will hold for all
+ * x values to negative infinity and positive infinity, respectively.
+ */
+
+/**
+ * \typedef Pwl::Point
+ * \brief Describe a point in two-dimensional real space
+ */
+
+/**
+ * \class Pwl::Interval
+ * \brief Describe an interval in one-dimensional real space
+ */
+
+/**
+ * \fn Pwl::Interval::Interval(double _start, double _end)
+ * \brief Construct an interval
+ * \param _start Start of the interval
+ * \param _end End of the interval
+ */
+
+/**
+ * \fn Pwl::Interval::contains
+ * \brief Check if a given value falls within the interval
+ * \param value Value to check
+ */
+
+/**
+ * \fn Pwl::Interval::clamp
+ * \brief Clamp a value such that it is within the interval
+ * \param value Value to clamp
+ */
+
+/**
+ * \fn Pwl::Interval::length
+ * \brief Compute the length of the interval
+ */
+
+/**
+ * \var Pwl::Interval::start
+ * \brief Start of the interval
+ */
 
-using namespace RPiController;
+/**
+ * \var Pwl::Interval::end
+ * \brief End of the interval
+ */
 
-int Pwl::read(const libcamera::YamlObject &params)
+/**
+ * \fn Pwl::Pwl(std::vector<Point> const &points)
+ * \brief Construct a piecewise linear function from a list of 2D points
+ * \param points Vector of points from which to construct the piecewise linear function
+ *
+ * \a points must be in ascending order of x-value.
+ */
+
+/**
+ * \brief Populate the piecewise linear function from yaml data
+ * \param params Yaml data to populate the piecewise linear function with
+ *
+ * Any existing points in the piecewise linear function will *not* be
+ * overwritten.
+ *
+ * The yaml data is expected to be a list with an even number of numerical
+ * elements. These will be parsed in pairs into x and y points in the piecewise
+ * linear function, and added in order. x must be monotonically increasing.
+ *
+ * \return 0 on success, negative error code otherwise
+ */
+int Pwl::readYaml(const libcamera::YamlObject &params)
 {
 	if (!params.size() || params.size() % 2)
 		return -EINVAL;
@@ -24,64 +125,109 @@  int Pwl::read(const libcamera::YamlObject &params)
 		auto x = it->get<double>();
 		if (!x)
 			return -EINVAL;
-		if (it != list.begin() && *x <= points_.back().x)
+		if (it != list.begin() && *x <= points_.back().x())
 			return -EINVAL;
 
 		auto y = (++it)->get<double>();
 		if (!y)
 			return -EINVAL;
 
-		points_.push_back(Point(*x, *y));
+		points_.push_back(Point({ *x, *y }));
 	}
 
 	return 0;
 }
 
+/**
+ * \brief Append a point to the end of the piecewise linear function
+ * \param x x-coordinate of the point to add to the piecewise linear function
+ * \param y y-coordinate of the point to add to the piecewise linear function
+ * \param eps Epsilon for the minimum x distance between points (optional)
+ *
+ * The point's x-coordinate must be greater than the x-coordinate of the last
+ * (= greatest) point already in the piecewise linear function.
+ */
 void Pwl::append(double x, double y, const double eps)
 {
-	if (points_.empty() || points_.back().x + eps < x)
-		points_.push_back(Point(x, y));
+	if (points_.empty() || points_.back().x() + eps < x)
+		points_.push_back(Point({ x, y }));
 }
 
+/**
+ * \brief Prepend a point to the beginning of the piecewise linear function
+ * \param x x-coordinate of the point to add to the piecewise linear function
+ * \param y y-coordinate of the point to add to the piecewise linear function
+ * \param eps Epsilon for the minimum x distance between points (optional)
+ *
+ * The point's x-coordinate must be less than the x-coordinate of the first
+ * (= smallest) point already in the piecewise linear function.
+ */
 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));
+	if (points_.empty() || points_.front().x() - eps > x)
+		points_.insert(points_.begin(), Point({ x, y }));
 }
 
+/**
+ * \brief Get the domain of the piecewise linear function
+ * \return An interval representing the domain
+ */
 Pwl::Interval Pwl::domain() const
 {
-	return Interval(points_[0].x, points_[points_.size() - 1].x);
+	return Interval(points_[0].x(), points_[points_.size() - 1].x());
 }
 
+/**
+ * \brief Get the range of the piecewise linear function
+ * \return An interval representing the range
+ */
 Pwl::Interval Pwl::range() const
 {
-	double lo = points_[0].y, hi = lo;
+	double lo = points_[0].y(), hi = lo;
 	for (auto &p : points_)
-		lo = std::min(lo, p.y), hi = std::max(hi, p.y);
+		lo = std::min(lo, p.y()), hi = std::max(hi, p.y());
 	return Interval(lo, hi);
 }
 
+/**
+ * \brief Check if the piecewise linear function is empty
+ * \return True if there are no points in the function, false otherwise
+ */
 bool Pwl::empty() const
 {
 	return points_.empty();
 }
 
-double Pwl::eval(double x, int *spanPtr, bool updateSpan) const
+/**
+ * \brief Evaluate the piecewise linear function
+ * \param[in] x The x value to input into the function
+ * \param[inout] span Initial guess for span
+ * \param[in] updateSpan Set to true to update span
+ *
+ * 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 at position \a x
+ */
+double Pwl::eval(double x, int *span, bool updateSpan) const
 {
-	int span = findSpan(x, spanPtr && *spanPtr != -1 ? *spanPtr : points_.size() / 2 - 1);
-	if (spanPtr && updateSpan)
-		*spanPtr = span;
-	return points_[span].y +
-	       (x - points_[span].x) * (points_[span + 1].y - points_[span].y) /
-		       (points_[span + 1].x - points_[span].x);
+	int index = findSpan(x, span && *span != -1
+					? *span
+					: points_.size() / 2 - 1);
+	if (span && updateSpan)
+		*span = index;
+	return points_[index].y() +
+	       (x - points_[index].x()) * (points_[index + 1].y() - points_[index].y()) /
+		       (points_[index + 1].x() - points_[index].x());
 }
 
 int Pwl::findSpan(double x, int span) const
 {
 	/*
 	 * Pwls are generally small, so linear search may well be faster than
-	 * binary, though could review this if large PWls start turning up.
+	 * binary, though could review this if large Pwls start turning up.
 	 */
 	int lastSpan = points_.size() - 2;
 	/*
@@ -89,65 +235,43 @@  int Pwl::findSpan(double x, int span) const
 	 * control point
 	 */
 	span = std::max(0, std::min(lastSpan, span));
-	while (span < lastSpan && x >= points_[span + 1].x)
+	while (span < lastSpan && x >= points_[span + 1].x())
 		span++;
-	while (span && x < points_[span].x)
+	while (span && x < points_[span].x())
 		span--;
 	return span;
 }
 
-Pwl::PerpType Pwl::invert(Point const &xy, Point &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];
-		double t = ((xy - points_[span]) % spanVec) / spanVec.len2();
-		if (t < -eps) /* off the start of this span */
-		{
-			if (span == 0) {
-				perp = points_[span];
-				return PerpType::Start;
-			} else if (prevOffEnd) {
-				perp = points_[span];
-				return PerpType::Vertex;
-			}
-		} 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 */
-		{
-			perp = points_[span] + spanVec * t;
-			return PerpType::Perpendicular;
-		}
-	}
-	return PerpType::None;
-}
-
-Pwl Pwl::inverse(bool *trueInverse, const double eps) const
+/**
+ * \brief Compute the inverse function
+ * \param[in] eps Epsilon for the minimum x distance between points (optional)
+ *
+ * The output includes whether the resulting inverse function is a proper
+ * (true) inverse, or only a best effort (e.g. input was non-monotonic).
+ *
+ * \return A pair of the inverse piecewise linear function, and whether or not
+ * the result is a proper/true inverse
+ */
+std::pair<Pwl, bool> Pwl::inverse(const double eps) const
 {
 	bool appended = false, prepended = false, neither = false;
 	Pwl inverse;
 
 	for (Point 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)
+		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) {
 			/* do nothing */;
-		else if (p.y > inverse.points_.back().x) {
-			inverse.append(p.y, p.x, eps);
+		} 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);
+		} else if (p.y() < inverse.points_.front().x()) {
+			inverse.prepend(p.y(), p.x(), eps);
 			prepended = true;
-		} else
+		} else {
 			neither = true;
+		}
 	}
 
 	/*
@@ -155,50 +279,58 @@  Pwl Pwl::inverse(bool *trueInverse, const double eps) const
 	 * onto both ends of the inverse, or if there were points that couldn't
 	 * go on either.
 	 */
-	if (trueInverse)
-		*trueInverse = !(neither || (appended && prepended));
+	bool trueInverse = !(neither || (appended && prepended));
 
-	return inverse;
+	return { inverse, trueInverse };
 }
 
+/**
+ * \brief Compose two piecewise linear functions together
+ * \param[in] other The "other" piecewise linear function
+ * \param[in] eps Epsilon for the minimum x distance between points (optional)
+ *
+ * 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;
+	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) } });
+	Pwl result({ Point({ 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;
+		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
 			 */
-			thisX = points_[thisSpan].x +
-				(other.points_[otherSpan + 1].x -
-				 points_[thisSpan].y) *
+			thisX = points_[thisSpan].x() +
+				(other.points_[otherSpan + 1].x() -
+				 points_[thisSpan].y()) *
 					dx / dy;
-			thisY = other.points_[++otherSpan].x;
+			thisY = other.points_[++otherSpan].x();
 		} else if (std::abs(dy) > eps && otherSpan > 0 &&
-			   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 previous span in other
 			 */
-			thisX = points_[thisSpan].x +
-				(other.points_[otherSpan + 1].x -
-				 points_[thisSpan].y) *
+			thisX = points_[thisSpan].x() +
+				(other.points_[otherSpan + 1].x() -
+				 points_[thisSpan].y()) *
 					dx / dy;
-			thisY = other.points_[--otherSpan].x;
+			thisY = other.points_[--otherSpan].x();
 		} else {
 			/* we stay in the same span in other */
 			thisSpan++;
-			thisX = points_[thisSpan].x,
-			thisY = points_[thisSpan].y;
+			thisX = points_[thisSpan].x(),
+			thisY = points_[thisSpan].y();
 		}
 		result.append(thisX, other.eval(thisY, &otherSpan, false),
 			      eps);
@@ -206,32 +338,47 @@  Pwl Pwl::compose(Pwl const &other, const double eps) const
 	return result;
 }
 
+/**
+ * \brief Apply function to (x,y) values at every control point
+ * \param f Function to be applied
+ */
 void Pwl::map(std::function<void(double x, double y)> f) const
 {
 	for (auto &pt : points_)
-		f(pt.x, pt.y);
+		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);
+	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)
-			x = pwl1.points_[++span1].x;
+			x = pwl1.points_[++span1].x();
 		else if (span1 == (int)pwl1.points_.size() - 1)
-			x = pwl0.points_[++span0].x;
-		else if (pwl0.points_[span0 + 1].x > pwl1.points_[span1 + 1].x)
-			x = pwl1.points_[++span1].x;
+			x = pwl0.points_[++span0].x();
+		else if (pwl0.points_[span0 + 1].x() > pwl1.points_[span1 + 1].x())
+			x = pwl1.points_[++span1].x();
 		else
-			x = pwl0.points_[++span0].x;
+			x = pwl0.points_[++span0].x();
 		f(x, pwl0.eval(x, &span0, false), pwl1.eval(x, &span1, false));
 	}
 }
 
+/**
+ * \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)
@@ -243,27 +390,32 @@  Pwl Pwl::combine(Pwl const &pwl0, Pwl const &pwl1,
 	return result;
 }
 
-void Pwl::matchDomain(Interval const &domain, bool clip, const double eps)
-{
-	int span = 0;
-	prepend(domain.start, eval(clip ? points_[0].x : domain.start, &span),
-		eps);
-	span = points_.size() - 2;
-	append(domain.end, eval(clip ? points_.back().x : domain.end, &span),
-	       eps);
-}
-
+/**
+ * \brief Multiply the piecewise linear function
+ * \param d Scalar multiplier to multiply the function by
+ * \return This function, after it has been multiplied by \a d
+ */
 Pwl &Pwl::operator*=(double d)
 {
 	for (auto &pt : points_)
-		pt.y *= d;
+		pt[1] *= d;
 	return *this;
 }
 
-void Pwl::debug(FILE *fp) const
+/**
+ * \brief Assemble and return a string describing the piecewise linear function
+ * \return A string describing the piecewise linear function
+ */
+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 7d5e7e4d3fda..a2cbad6c1597 100644
--- a/src/ipa/libipa/pwl.h
+++ b/src/ipa/libipa/pwl.h
@@ -2,126 +2,87 @@ 
 /*
  * Copyright (C) 2019, Raspberry Pi Ltd
  *
- * piecewise linear functions interface
+ * Piecewise linear functions interface
  */
 #pragma once
 
+#include <algorithm>
+#include <cmath>
 #include <functional>
-#include <math.h>
+#include <string>
+#include <utility>
 #include <vector>
 
+#include <libcamera/geometry.h>
+
 #include "libcamera/internal/yaml_parser.h"
 
-namespace RPiController {
+#include "vector.h"
+
+namespace libcamera {
+
+namespace ipa {
 
 class Pwl
 {
 public:
+	using Point = Vector<double, 2>;
+
 	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)
-		{
-			return value < start ? start
-					     : (value > end ? end : value);
-		}
-		double len() const { return end - start; }
-	};
-	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
+
+		double clamp(double value)
 		{
-			return x * p.x + y * p.y;
+			return std::clamp(value, start, end);
 		}
-		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()); }
+
+		double length() const { return end - start; }
+
+		double start, end;
 	};
+
 	Pwl() {}
-	Pwl(std::vector<Point> const &points) : points_(points) {}
-	int read(const libcamera::YamlObject &params);
+	Pwl(const std::vector<Point> &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);
+
+	bool empty() const;
 	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,
+
+	double eval(double x, int *span = 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,
-			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. */
+
+	std::pair<Pwl, bool> inverse(const double eps = 1e-6) const;
+	Pwl compose(const Pwl &other, const double eps = 1e-6) const;
+
 	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,
+	combine(const Pwl &pwl0, const Pwl &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:
+	void prepend(double x, double y, const double eps = 1e-6);
+	static void map2(const Pwl &pwl0, const Pwl &pwl1,
+			 std::function<void(double x, double y0, double y1)> f);
 	int findSpan(double x, int span) const;
 	std::vector<Point> points_;
 };
 
-} /* namespace RPiController */
+} /* namespace ipa */
+
+} /* namespace libcamera */