[PATCH v8 3/4] ipa: libipa: pwl: Clean up Pwl class to match libcamera
Paul Elder
paul.elder at ideasonboard.com
Wed Jun 12 09:56:18 CEST 2024
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 at ideasonboard.com>
> > Reviewed-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > Acked-by: David Plowman <david.plowman at raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham at 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 ¶ms)
> > +/**
> > + * \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 ¶ms)
> > {
> > if (!params.size() || params.size() % 2)
> > return -EINVAL;
> > @@ -24,64 +125,109 @@ int Pwl::read(const libcamera::YamlObject ¶ms)
> > 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 ¶ms);
> > + 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 ¶ms);
> > +
> > 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 at ideasonboard.com>
>
> > std::vector<Point> points_;
> > };
> >
> > -} /* namespace RPiController */
> > +} /* namespace ipa */
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list