[PATCH v3 0/4] ipa: Move Pwl from Raspberry Pi to libipa

[Kieran Bingham]

Quoting David Plowman (2024-05-31 09:14:34)
> Hi Kieran, Paul
> 
> Yes, I did actually have a look through and it seemed like a big no-op
> for us functionally at least, so

Yes, I hope this shouldn't impact anything in the code.

I'm weary we should really make a MAINTAINERS file soon that makes sure
we automatically catch changes to files of interest so for instance,
even though this moves out of rpi/ I would still expect if there are
changes to this in the future to get your approval to make sure no one
breaks RPi. Something that enforces acks on specific components (i.e. to
prevent even accidentally merging code affecting RPi without a

*-by: * <*@raspberrypi.com>

> Acked-by: David Plowman <david.plowman at raspberrypi.com>

Thanks!

> I did have a couple of super-minor thoughts, but nothing that I think
> warrants changing anything.
> 
> I wasn't convinced by changing the name "clip" to "clamp". C++
> std::clamp clips (or clamps?) at both ends of the range, whereas I
> think our "clip" is just the top. So making them the same didn't
> really make anything clearer for me, and maybe I preferred "clip". But
> I don't really mind.
> 
> There was also Naush's point from a while ago that we use our Pwl
> implementation elsewhere so we might be wanting to keep the two in

Aha, I see that now - replying there too <done>. But probably the same
message below here.

> sync. But it's not code we touch very much these days, so we can
> probably manage anything that comes up. Maybe we'll just change our
> other version to adopt any minor name changes (e.g. Point to PointF).

That part is more tricky indeed. I still think even components like our
V4L2/MC helpers could be used outside of libcamera too - but I don't
know what bar/point we should have libcamera depend on a stack of other
libraries for the internals. That's sort of what libcamera-base already
is ... tricky ...

Thanks for the Ack, that unblocks us progressing the implementations.

--
Kieran


> 
> David
> 
> On Fri, 31 May 2024 at 08:54, Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
> >
> > Hi Naush, David,
> >
> > Do you have any objections or concerns here? If not could you provide an
> > Acked-by: tag please?
> >
> > --
> > Kieran
> >
> >
> > Quoting Paul Elder (2024-05-29 20:26:07)
> > > This patch series moves the piecewise linear function class from
> > > the Rasberry Pi IPA to libipa so that all IPAs can use it.
> > >
> > > First an addition to the geometry header is needed, to add a
> > > floating-point version of the Point class, then the pwl is copied over,
> > > and the Raspberry Pi IPA is converted to use the libipa Pwl class.
> > >
> > > The main changes in v2 are s/FPoint/PointF/g and improving the
> > > documentation.
> > >
> > > v3 has almost no change...
> > >
> > > Paul Elder (4):
> > >   libcamera: geometry: Add floating-point version of Point class
> > >   ipa: libipa: Copy pwl from rpi
> > >   ipa: libipa: pwl: Clean up Pwl class to match libcamera
> > >   ipa: rpi: controller: Use libipa's Pwl class
> > >
> > >  include/libcamera/geometry.h               |  65 ++++
> > >  src/ipa/libipa/meson.build                 |   2 +
> > >  src/ipa/libipa/pwl.cpp                     | 371 +++++++++++++++++++++
> > >  src/ipa/libipa/pwl.h                       |  98 ++++++
> > >  src/ipa/rpi/controller/cac_status.h        |   2 -
> > >  src/ipa/rpi/controller/contrast_status.h   |   4 +-
> > >  src/ipa/rpi/controller/meson.build         |   2 +-
> > >  src/ipa/rpi/controller/rpi/af.cpp          |   4 +-
> > >  src/ipa/rpi/controller/rpi/af.h            |   5 +-
> > >  src/ipa/rpi/controller/rpi/agc_channel.cpp |   8 +-
> > >  src/ipa/rpi/controller/rpi/agc_channel.h   |   7 +-
> > >  src/ipa/rpi/controller/rpi/awb.cpp         |  40 +--
> > >  src/ipa/rpi/controller/rpi/awb.h           |  23 +-
> > >  src/ipa/rpi/controller/rpi/ccm.cpp         |   4 +-
> > >  src/ipa/rpi/controller/rpi/ccm.h           |   5 +-
> > >  src/ipa/rpi/controller/rpi/contrast.cpp    |  14 +-
> > >  src/ipa/rpi/controller/rpi/contrast.h      |   5 +-
> > >  src/ipa/rpi/controller/rpi/geq.cpp         |   5 +-
> > >  src/ipa/rpi/controller/rpi/geq.h           |   4 +-
> > >  src/ipa/rpi/controller/rpi/hdr.cpp         |   8 +-
> > >  src/ipa/rpi/controller/rpi/hdr.h           |   9 +-
> > >  src/ipa/rpi/controller/rpi/tonemap.cpp     |   2 +-
> > >  src/ipa/rpi/controller/rpi/tonemap.h       |   5 +-
> > >  src/ipa/rpi/controller/tonemap_status.h    |   4 +-
> > >  src/libcamera/geometry.cpp                 | 123 ++++++-
> > >  test/geometry.cpp                          | 355 ++++++++++++++++++++
> > >  26 files changed, 1097 insertions(+), 77 deletions(-)
> > >  create mode 100644 src/ipa/libipa/pwl.cpp
> > >  create mode 100644 src/ipa/libipa/pwl.h
> > >
> > > --
> > > 2.39.2
> > >