| Message ID | 20240826152224.362773-1-stefan.klug@ideasonboard.com |
|---|---|
| Headers | show |
| Series |
|
| Related | show |
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 >