[RFC/PATCH v1 0/8] Implement polynomial lsc loader

[Kieran Bingham]

Quoting Stefan Klug (2024-08-26 16:21:58)
> Hi all,
> 
> This series is not yet completely polished, as I'd like to get some
> feedback on the overall direction first. It is based on my colour
> temperature series [1].
> 
> Polynomial functions allow a relatively precise representation of the
> lensshading. The formula used here is from the DNG specification [2]
> page 110. Initial implementations in libtuning show that the overall
> error is quite low, but it is not yet clear if the ability to model the
> lens shading is sufficient for all cases.
> 
> The huge benefit of polynomials is that they can be easily resampled for
> arbitrary crop rectangles. In case of the rkisp1 the hardware is
> configured using a 17x17 grid of gains.  Resampling these for a
> arbitrary crop rectangle on the sensor leads to a degradation in
> quality. This can either be mitigated using a higher resolution grid as
> basis or by describing the lens shading using polynomials. This series
> implements the latter.
> 
> Patches 1-3 replace the matrix interpolator with a generic interpolator
> class. This is only refactoring without functional changes.
> 
> Patch 4 Uses the new interpolator for the current lsc algorithm.
> 
> Patch 5-8 Implement a loader for polynomial lens shading coefficients.
> Sampling for the current sensor crop rectangle is done at load time.
> 
> Questions I have in mind:
> - libipa/polynomial.h is only used inside the loader. Is that worth a
>   own file, or should it be defined in lsc.cpp?

Having discussed this briefly, My question is 'is this a generic
polynomial helper class' or is it ... quite specific.

I think it's quite specific as this class expliitly handles the DNG
polynomial for vignetting only ...

Now the next part ... should it be in (rkisp1/lsc.cpp)... I think that's
a quick no ... because I would entirely expect this to be usable by
other IPAs, so I certainly think this deserves a helper class in libipa.

But I'd call it something that reflects what it implements. Into
bikeshedding but perhaps, polynomial-vignetting, polynomial-dng, or
polynomial-lsc. I don't think it matters, as long as it can't be
interpretted as a 'generic polynomial helper class'. If there's a more
specific suited name from the dng spec, (which I haven't read) then
anything from that would be fine too.



> - lsc.cpp now contains several helper classes on the top. Are these fine
>   there, or should they be moved into another file?

I see:

- Helpers to interpolate an RKISP1 specific vector.

Seems fine to keep that in rkisp1/lsc.cpp to me.

- LscPolynomialLoader
- LscClassicLoader

I'd be fine keeping those in that specific file too, as they are just
handling the specific loading details. 

> I'm happy for any feedback.

I look forward to a non-rfc posting with a few more cleanups to progress
the series then ;-)

--
Kieran


> Best regards,
> Stefan
> 
> [1] https://patchwork.libcamera.org/project/libcamera/list/?series=4514
> [1] https://helpx.adobe.com/content/dam/help/en/photoshop/pdf/DNG_Spec_1_7_1_0.pdf
> 
> 
> Stefan Klug (8):
>   ipa: libipa: Add generic Interpolator class
>   ipa: rkisp1: Use generic Interpolator class
>   ipa: rkisp1: Remove MatrixInterpolator
>   ipa: rkisp1: Use interpolator in lsc
>   ipa: rkisp1: Move loader functions into helper class
>   ipa: libipa: Add lsc polynomial class
>   ipa: rkisp1: Add sensor info to context
>   ipa: rkisp1: Add polynomial LSC loader
> 
>  src/ipa/libipa/interpolator.cpp        | 139 +++++++++
>  src/ipa/libipa/interpolator.h          | 139 +++++++++
>  src/ipa/libipa/matrix_interpolator.cpp | 110 -------
>  src/ipa/libipa/matrix_interpolator.h   | 122 --------
>  src/ipa/libipa/meson.build             |   6 +-
>  src/ipa/libipa/polynomial.cpp          |  23 ++
>  src/ipa/libipa/polynomial.h            | 107 +++++++
>  src/ipa/rkisp1/algorithms/awb.cpp      |   4 +-
>  src/ipa/rkisp1/algorithms/awb.h        |   5 +-
>  src/ipa/rkisp1/algorithms/ccm.cpp      |  18 +-
>  src/ipa/rkisp1/algorithms/ccm.h        |   6 +-
>  src/ipa/rkisp1/algorithms/lsc.cpp      | 405 +++++++++++++++----------
>  src/ipa/rkisp1/algorithms/lsc.h        |  13 +-
>  src/ipa/rkisp1/ipa_context.h           |   2 +
>  src/ipa/rkisp1/rkisp1.cpp              |   4 +-
>  15 files changed, 684 insertions(+), 419 deletions(-)
>  create mode 100644 src/ipa/libipa/interpolator.cpp
>  create mode 100644 src/ipa/libipa/interpolator.h
>  delete mode 100644 src/ipa/libipa/matrix_interpolator.cpp
>  delete mode 100644 src/ipa/libipa/matrix_interpolator.h
>  create mode 100644 src/ipa/libipa/polynomial.cpp
>  create mode 100644 src/ipa/libipa/polynomial.h
> 
> -- 
> 2.43.0
>