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

20258 diff mbox series

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

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

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