[{"id":31093,"web_url":"https://patchwork.libcamera.org/comment/31093/","msgid":"<172553362684.129190.1409560271298823815@ping.linuxembedded.co.uk>","date":"2024-09-05T10:53:46","subject":"Re: [RFC/PATCH v1 0/8] Implement polynomial lsc loader","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Stefan Klug (2024-08-26 16:21:58)\n> Hi all,\n> \n> This series is not yet completely polished, as I'd like to get some\n> feedback on the overall direction first. It is based on my colour\n> temperature series [1].\n> \n> Polynomial functions allow a relatively precise representation of the\n> lensshading. The formula used here is from the DNG specification [2]\n> page 110. Initial implementations in libtuning show that the overall\n> error is quite low, but it is not yet clear if the ability to model the\n> lens shading is sufficient for all cases.\n> \n> The huge benefit of polynomials is that they can be easily resampled for\n> arbitrary crop rectangles. In case of the rkisp1 the hardware is\n> configured using a 17x17 grid of gains. Resampling these for a\n> arbitrary crop rectangle on the sensor leads to a degradation in\n> quality. This can either be mitigated using a higher resolution grid as\n> basis or by describing the lens shading using polynomials. This series\n> implements the latter.\n> \n> Patches 1-3 replace the matrix interpolator with a generic interpolator\n> class. This is only refactoring without functional changes.\n> \n> Patch 4 Uses the new interpolator for the current lsc algorithm.\n> \n> Patch 5-8 Implement a loader for polynomial lens shading coefficients.\n> Sampling for the current sensor crop rectangle is done at load time.\n> \n> Questions I have in mind:\n> - libipa/polynomial.h is only used inside the loader. Is that worth a\n> own file, or should it be defined in lsc.cpp?\n\nHaving discussed this briefly, My question is 'is this a generic\npolynomial helper class' or is it ... quite specific.\n\nI think it's quite specific as this class expliitly handles the DNG\npolynomial for vignetting only ...\n\nNow the next part ... should it be in (rkisp1/lsc.cpp)... I think that's\na quick no ... because I would entirely expect this to be usable by\nother IPAs, so I certainly think this deserves a helper class in libipa.\n\nBut I'd call it something that reflects what it implements. Into\nbikeshedding but perhaps, polynomial-vignetting, polynomial-dng, or\npolynomial-lsc. I don't think it matters, as long as it can't be\ninterpretted as a 'generic polynomial helper class'. If there's a more\nspecific suited name from the dng spec, (which I haven't read) then\nanything from that would be fine too.\n\n\n\n> - lsc.cpp now contains several helper classes on the top. Are these fine\n> there, or should they be moved into another file?\n\nI see:\n\n- Helpers to interpolate an RKISP1 specific vector.\n\nSeems fine to keep that in rkisp1/lsc.cpp to me.\n\n- LscPolynomialLoader\n- LscClassicLoader\n\nI'd be fine keeping those in that specific file too, as they are just\nhandling the specific loading details. \n\n> I'm happy for any feedback.\n\nI look forward to a non-rfc posting with a few more cleanups to progress\nthe series then ;-)\n\n--\nKieran\n\n\n> Best regards,\n> Stefan\n> \n> [1] https://patchwork.libcamera.org/project/libcamera/list/?series=4514\n> [1] https://helpx.adobe.com/content/dam/help/en/photoshop/pdf/DNG_Spec_1_7_1_0.pdf\n> \n> \n> Stefan Klug (8):\n> ipa: libipa: Add generic Interpolator class\n> ipa: rkisp1: Use generic Interpolator class\n> ipa: rkisp1: Remove MatrixInterpolator\n> ipa: rkisp1: Use interpolator in lsc\n> ipa: rkisp1: Move loader functions into helper class\n> ipa: libipa: Add lsc polynomial class\n> ipa: rkisp1: Add sensor info to context\n> ipa: rkisp1: Add polynomial LSC loader\n> \n> src/ipa/libipa/interpolator.cpp | 139 +++++++++\n> src/ipa/libipa/interpolator.h | 139 +++++++++\n> src/ipa/libipa/matrix_interpolator.cpp | 110 -------\n> src/ipa/libipa/matrix_interpolator.h | 122 --------\n> src/ipa/libipa/meson.build | 6 +-\n> src/ipa/libipa/polynomial.cpp | 23 ++\n> src/ipa/libipa/polynomial.h | 107 +++++++\n> src/ipa/rkisp1/algorithms/awb.cpp | 4 +-\n> src/ipa/rkisp1/algorithms/awb.h | 5 +-\n> src/ipa/rkisp1/algorithms/ccm.cpp | 18 +-\n> src/ipa/rkisp1/algorithms/ccm.h | 6 +-\n> src/ipa/rkisp1/algorithms/lsc.cpp | 405 +++++++++++++++----------\n> src/ipa/rkisp1/algorithms/lsc.h | 13 +-\n> src/ipa/rkisp1/ipa_context.h | 2 +\n> src/ipa/rkisp1/rkisp1.cpp | 4 +-\n> 15 files changed, 684 insertions(+), 419 deletions(-)\n> create mode 100644 src/ipa/libipa/interpolator.cpp\n> create mode 100644 src/ipa/libipa/interpolator.h\n> delete mode 100644 src/ipa/libipa/matrix_interpolator.cpp\n> delete mode 100644 src/ipa/libipa/matrix_interpolator.h\n> create mode 100644 src/ipa/libipa/polynomial.cpp\n> create mode 100644 src/ipa/libipa/polynomial.h\n> \n> -- \n> 2.43.0\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id C0AEDBF415\n\tfor ;\n\tThu, 5 Sep 2024 10:53:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C3542634CB;\n\tThu, 5 Sep 2024 12:53:51 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CB67D6345D\n\tfor ;\n\tThu, 5 Sep 2024 12:53:49 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6A992AD0;\n\tThu, 5 Sep 2024 12:52:36 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"j+tAn8UR\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1725533556;\n\tbh=Hd7imisEVg03vDTDBbuxX86YNu7m9HnYXipWxIvmUj0=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=j+tAn8UREgtzuMW9WsULClI7zf3ApjTSeB2RYNEULw9I8cFiDYlcuu+mcywZCHPsV\n\tm9rbhx0O6l2NKVFOUIfOsyo/EckAPnFjvEVZXELnyEEaDQ2BpFo2OapjoKOwln6eS7\n\t5Hw/FCSr86cDFY5/oRXKH9NwY3QGxpmtLGjAfvMM=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240826152224.362773-1-stefan.klug@ideasonboard.com>","References":"<20240826152224.362773-1-stefan.klug@ideasonboard.com>","Subject":"Re: [RFC/PATCH v1 0/8] Implement polynomial lsc loader","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Thu, 05 Sep 2024 11:53:46 +0100","Message-ID":"<172553362684.129190.1409560271298823815@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]