[libcamera-devel] [PATCH v2 4/6] src: ipa: raspberrypi: Compute inverse of piecewise linear function

[David Plowman]

Hi Laurent

Thanks for the review.

On Tue, 8 Dec 2020 at 11:46, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Mon, Dec 07, 2020 at 06:01:19PM +0000, David Plowman wrote:
> > Add a method to the piecewise linear function (Pwl) class to compute
> > the inverse of a given Pwl. If the input function is non-monotonic we
> > can only produce a best effort "pseudo" inverse, and we signal this to
> > the caller.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/ipa/raspberrypi/controller/pwl.cpp | 27 ++++++++++++++++++++++++++
> >  src/ipa/raspberrypi/controller/pwl.hpp |  3 +++
> >  2 files changed, 30 insertions(+)
> >
> > diff --git a/src/ipa/raspberrypi/controller/pwl.cpp b/src/ipa/raspberrypi/controller/pwl.cpp
> > index aa134a1f..70206418 100644
> > --- a/src/ipa/raspberrypi/controller/pwl.cpp
> > +++ b/src/ipa/raspberrypi/controller/pwl.cpp
> > @@ -114,6 +114,33 @@ Pwl::PerpType Pwl::Invert(Point const &xy, Point &perp, int &span,
> >       return PerpType::None;
> >  }
> >
> > +Pwl Pwl::Inverse(bool *true_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 (p.y > inverse.points_.back().x - eps) {
>
> Shouldn't this be + eps (and - eps below), based on Pwl::Append() and
> Pwm::Prepend() ?

Yes, as always these numerical things are a bit icky. So if I had

else if (p.y > inverse.points_.back().x + eps) {

then a point that is eps/2 beyond back().x would end up triggering the
"neither" case, which seems harsh. I guess there's a possibility the
input function was constructed with a smaller value for eps. On
balance I felt that "within eps of the end" was probably good enough
to accept the point, and either create a new segment, or drop it
without complaint.

Though on reflection I can see an argument for explicitly checking the
"within eps of the start/end" case and then deliberately doing nothing
(not setting appended/prepended), so I might add that.

Thanks
David

>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>
> > +                     inverse.Append(p.y, p.x, eps);
> > +                     appended = true;
> > +             } else if (p.y < inverse.points_.front().x + eps) {
> > +                     inverse.Prepend(p.y, p.x, eps);
> > +                     prepended = true;
> > +             } else
> > +                     neither = true;
> > +     }
> > +
> > +     // This is not a proper inverse if we found ourselves putting points
> > +     // onto both ends of the inverse, or if there were points that couldn't
> > +     // go on either.
> > +     if (true_inverse)
> > +             *true_inverse = !(neither || (appended && prepended));
> > +
> > +     return inverse;
> > +}
> > +
> >  Pwl Pwl::Compose(Pwl const &other, const double eps) const
> >  {
> >       double this_x = points_[0].x, this_y = points_[0].y;
> > diff --git a/src/ipa/raspberrypi/controller/pwl.hpp b/src/ipa/raspberrypi/controller/pwl.hpp
> > index 4f168551..484672f6 100644
> > --- a/src/ipa/raspberrypi/controller/pwl.hpp
> > +++ b/src/ipa/raspberrypi/controller/pwl.hpp
> > @@ -80,6 +80,9 @@ public:
> >       };
> >       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 *true_inverse = 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.
>
> --
> Regards,
>
> Laurent Pinchart