[v3,0/4] ipa: Move Pwl from Raspberry Pi to libipa
mbox series

Message ID 20240529192612.814515-1-paul.elder@ideasonboard.com
Headers show
Series
  • ipa: Move Pwl from Raspberry Pi to libipa
Related show

Message

Paul Elder May 29, 2024, 7:26 p.m. UTC
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

Comments

Kieran Bingham May 31, 2024, 7:54 a.m. UTC | #1
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
>
David Plowman May 31, 2024, 8:14 a.m. UTC | #2
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

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

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

David

On Fri, 31 May 2024 at 08:54, Kieran Bingham
<kieran.bingham@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
> >
Kieran Bingham May 31, 2024, 11:12 a.m. UTC | #3
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@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@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
> > >