[{"id":25755,"web_url":"https://patchwork.libcamera.org/comment/25755/","msgid":"","date":"2022-11-09T12:39:36","subject":"Re: [libcamera-devel] [PATCH v2 11/11] [WIP] utils: tuning: Add\n\ttuning script for rkisp1","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Sat, Oct 22, 2022 at 03:23:10PM +0900, Paul Elder via libcamera-devel wrote:\n> Add a tuning script for rkisp1 that uses libtuning.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v2:\n> - add SPDX and copyright\n> - s/average_functions/average/\n> - update the script to work with the new rkisp1 alsc module\n> \n> As of v2, this runs, but only with LSC. Parabolic gradient support is\n> put on hold, and instead the sectors are divided linearly. Is this what\n> we want?\n\nIs mentioned previously, I don't think we want a fixed function to\ncompute the sector sizes. We want sizes that will minimize the errors.\nHonestly, I would hardcode the linear sizes for now, and drop parametric\ngradient support as I don't think it will ever be used, but that's not a\nblocker.\n\n> Technically this script is complete in the sense that it works, but it's\n> still WIP because it only support LSC. What should we do about this?\n> Should it be \"complete\" at this stage and we can add the other modules\n> later, or should we keep it WIP until the other modules are added?\n> \n> v1:\n> This won't run as we're missing the necessary parser and generator for\n> yaml, parabolic gradient support, and multiple green support in the LSC\n> module, hence the DNI. As soon as those are added though, this *should*\n> work.\n> ---\n> utils/tuning/rkisp1.py | 58 ++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 58 insertions(+)\n> create mode 100755 utils/tuning/rkisp1.py\n> \n> diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py\n> new file mode 100755\n> index 00000000..cb692dd1\n> --- /dev/null\n> +++ b/utils/tuning/rkisp1.py\n> @@ -0,0 +1,58 @@\n> +#!/usr/bin/env python3\n> +# SPDX-License-Identifier: GPL-2.0-or-later\n> +#\n> +# Copyright (C) 2022, Paul Elder \n> +#\n> +# rkisp1.py - Tuning script for rkisp1\n> +\n> +import sys\n> +\n> +import libtuning as lt\n> +from libtuning.parsers import YamlParser\n> +from libtuning.generators import YamlOutput\n> +from libtuning.modules.alsc import ALSCRkISP1\n\nShould be named LSCRkISP1 as mentioned elsewhere. Same below.\n\n> +\n> +tuner = lt.Camera('RkISP1')\n> +tuner.add(ALSCRkISP1(\n> + # This can support other debug options (I can't think of any rn\n\ns/rn/right now/\n\n> + # but for future-proofing)\n\ns/$/./\n\nSame for other sentences below.\n\n> + debug=[lt.Debug.Plot],\n> +\n> + # This is for the actual LSC tuning, and is part of the base\n> + # ALSC module. rkisp1's table size (16x16 programmed as mirrored\n> + # 8x8) is separate, and is hardcoded in its specific ALSC tuning\n\nIt's only the sector sizes that are mirrored, not necessarily the\nvalues.\n\n> + # module\n> + sector_shape=(17, 17),\n> +\n> + # Other functions might include Circular, Hyperbolic, Gaussian,\n> + # Linear, Exponential, Logarithmic, etc\n> + # Of course, both need not be the same function\n> + # Some functions would need a sector_x_parameter (eg. sigma for Gaussian)\n> + # Alternatively: sector_x_sizes = [] ? I don't think it would work tho\n\nDrop this, per the comment above.\n\n> + sector_x_gradient=lt.gradient.Linear(lt.Remainder.DistributeFront),\n> + sector_y_gradient=lt.gradient.Linear(lt.Remainder.DistributeFront),\n> +\n> + # This is the function that will be used to average the pixels in each sector\n> + # This can also be a custom function.\n> + sector_average_function=lt.average.Mean(),\n> +\n> + # This is the function that will be used to smooth the color ratio values\n> + # This can also be a custom function.\n> + smoothing_function=lt.smoothing.MedianBlur(3),\n> +\n> + # Do we need a flag to enable/disable calculating sigmas? afaik\n> + # the rkisp1 IPA doesn't use it? But we could output it anyway\n\nIt's for the RPi ALSC only, so it shouldn't be part of the base.\n\n> + # and algorithms can decide whether or not if they want to use\n> + # it. Oh well, no flag for now, we can always add it later if\n> + # there's a big demand, plus it's only two to three values and\n> + # not an entire table.\n> + ))\n> +tuner.setInputType(YamlParser)\n> +tuner.setOutputType(YamlOutput)\n> +tuner.setOutputOrder([ALSCRkISP1])\n> +\n> +# Maybe this should be wrapped in an if __main__ = '__main__' ? That way the\n\nIt's a good practice, so I would do that, and drop this comment.\n\n> +# developer can control which tuner they want to be executed based on another\n> +# layer of arguments? But I was thinking that this would handle *all* arguments\n> +# based on the modules' and the input/output configurations.\n> +tuner.run(sys.argv)","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 01705BE08B\n\tfor ;\n\tWed, 9 Nov 2022 12:39:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4FFD46307B;\n\tWed, 9 Nov 2022 13:39:58 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 010936305E\n\tfor ;\n\tWed, 9 Nov 2022 13:39:55 +0100 (CET)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 670044AF;\n\tWed, 9 Nov 2022 13:39:55 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1667997598;\n\tbh=J9zv2XWWNoKHzeVFLCoiP5wbED4YU6SEhs5MoCKpL/8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=w0VptzOV7LRO2e3WIEMP9KHJq0Q+7Fc9FNUAjUuM82i6h2+PjBEYsvbz9DnwqsCbX\n\tQo6HFDVQ6MDFgipwGIrjRkCuyOjxtZv4pIk81qITgqKCtyE/cZMS5rKvgL9cluxIUy\n\tcIaHWx53JPwE2u2MyC7uAxCHhuWGdk1N0zBz/qytc0CkfmGK2snyl1yHI8svwEAuoN\n\tnXlcY6HyW69vofLE9tKtNLgMgNEPodvm5VELEtM7gvVXXtfnphDNZmGB2Ri3ZtHHAR\n\td2NS9Rf1k6IXMSt/bz4Dze7gXKAsd9tQensPG68dk/+bAiyuwXbr6VYd1ZROSCyWCb\n\tkflaoUxm+U2mQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1667997595;\n\tbh=J9zv2XWWNoKHzeVFLCoiP5wbED4YU6SEhs5MoCKpL/8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=iPbp0ycQfVWMQFkjIOJMFOGpllaHrPlMHPWJ4Ts66kGX1AjjExVdk8b5K3gPzlS3f\n\tDJjwy+wvgDt4ZVoHgErMln/Bz762VMrotlUcejrqco57h7XV6S4f4+CRKMBou6vVG3\n\tO4qPVbi8ZRlf8ImBrEOkjb8W5P68+OZBdh1TIDnE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"iPbp0ycQ\"; dkim-atps=neutral","Date":"Wed, 9 Nov 2022 14:39:36 +0200","To":"Paul Elder ","Message-ID":"","References":"<20221022062310.2545463-1-paul.elder@ideasonboard.com>\n\t<20221022062310.2545463-12-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20221022062310.2545463-12-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v2 11/11] [WIP] utils: tuning: Add\n\ttuning script for rkisp1","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]