| Message ID | 20201207180121.6374-5-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, Thank you for your patch. On Mon, 7 Dec 2020 at 18:02, David Plowman <david.plowman@raspberrypi.com> 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@raspberrypi.com> > Reviewed-by: Naushir Patuck <naush@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) { > + 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. > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
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@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() ? Reviewed-by: Laurent Pinchart <laurent.pinchart@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.
Hi Laurent Thanks for the review. On Tue, 8 Dec 2020 at 11:46, Laurent Pinchart <laurent.pinchart@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@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@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
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) { + 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.
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@raspberrypi.com> --- src/ipa/raspberrypi/controller/pwl.cpp | 27 ++++++++++++++++++++++++++ src/ipa/raspberrypi/controller/pwl.hpp | 3 +++ 2 files changed, 30 insertions(+)