[{"id":30139,"web_url":"https://patchwork.libcamera.org/comment/30139/","msgid":"<20240628234753.GM30900@pendragon.ideasonboard.com>","date":"2024-06-28T23:47:53","subject":"Re: [PATCH v2 15/25] libtuning: modules: Add initial CCM module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul and Stefan,\n\nThank you for the patch.\n\nOn Fri, Jun 28, 2024 at 12:47:08PM +0200, Stefan Klug wrote:\n> From: Paul Elder \n> \n> Implement a minimal ccm calibration module. For now it doesn't take the\n> results from lsc into account and supports rkisp1 only.\n> \n> Signed-off-by: Paul Elder \n> Signed-off-by: Stefan Klug \n> ---\n> .../tuning/libtuning/modules/ccm/__init__.py | 6 +++\n> utils/tuning/libtuning/modules/ccm/ccm.py | 44 +++++++++++++++++++\n> utils/tuning/libtuning/modules/ccm/rkisp1.py | 34 ++++++++++++++\n> utils/tuning/rkisp1.py | 4 +-\n> 4 files changed, 87 insertions(+), 1 deletion(-)\n> create mode 100644 utils/tuning/libtuning/modules/ccm/__init__.py\n> create mode 100644 utils/tuning/libtuning/modules/ccm/ccm.py\n> create mode 100644 utils/tuning/libtuning/modules/ccm/rkisp1.py\n> \n> diff --git a/utils/tuning/libtuning/modules/ccm/__init__.py b/utils/tuning/libtuning/modules/ccm/__init__.py\n> new file mode 100644\n> index 000000000000..322602afe63b\n> --- /dev/null\n> +++ b/utils/tuning/libtuning/modules/ccm/__init__.py\n> @@ -0,0 +1,6 @@\n> +# SPDX-License-Identifier: GPL-2.0-or-later\n> +#\n> +# Copyright (C) 2024, Paul Elder \n> +\n> +from libtuning.modules.ccm.ccm import CCM\n> +from libtuning.modules.ccm.rkisp1 import CCMRkISP1\n> diff --git a/utils/tuning/libtuning/modules/ccm/ccm.py b/utils/tuning/libtuning/modules/ccm/ccm.py\n> new file mode 100644\n> index 000000000000..50d435ad84a3\n> --- /dev/null\n> +++ b/utils/tuning/libtuning/modules/ccm/ccm.py\n> @@ -0,0 +1,44 @@\n> +# SPDX-License-Identifier: GPL-2.0-or-later\n> +#\n> +# Copyright (C) 2024, Paul Elder \n> +# Copyright (C) 2024, Ideas on Board\n> +#\n> +# Base Ccm tuning module\n> +\n> +from ..module import Module\n> +\n> +from libtuning.ctt_ccm import ccm\n> +import logging\n> +import time\n> +\n> +import numpy as np\n\ntime and numpy don't seem needed.\n\n> +\n> +logger = logging.getLogger(__name__)\n> +\n> +\n> +class CCM(Module):\n> + type = 'ccm'\n> + hr_name = 'CCM (Base)'\n\nUnrelated to this patch, renaming this to display_name may make the code\nmore readable.\n\n> + out_name = 'GenericCCM'\n\nIf the base algorithms are not meant to be used directly, should we drop\nthe out_name ?\n\n> +\n> + def __init__(self, debug: list):\n> + super().__init__()\n> +\n> + self.debug = debug\n> +\n> + def do_calibration(self, images):\n> + logger.info('Starting CCM calibration')\n> +\n> + imgs = [img for img in images if img.macbeth is not None]\n> +\n> + # todo: take alsc calibration results into account\n\ns/alsc/LSC/\n\n> + cal_cr_list = None\n> + cal_cb_list = None\n> +\n> + try:\n> + ccms = ccm(imgs, cal_cr_list, cal_cb_list)\n> + except ArithmeticError:\n> + logger.error('CCM calibration failed')\n> + return 1\n> +\n> + return ccms\n> diff --git a/utils/tuning/libtuning/modules/ccm/rkisp1.py b/utils/tuning/libtuning/modules/ccm/rkisp1.py\n> new file mode 100644\n> index 000000000000..a74d0d93c764\n> --- /dev/null\n> +++ b/utils/tuning/libtuning/modules/ccm/rkisp1.py\n> @@ -0,0 +1,34 @@\n> +# SPDX-License-Identifier: GPL-2.0-or-later\n> +#\n> +# Copyright (C) 2024, Paul Elder \n> +# Copyright (C) 2024, Ideas on Board\n> +#\n> +# Ccm module for tuning rkisp1\n> +\n> +from .ccm import CCM\n> +\n> +import libtuning as lt\n> +import libtuning.utils as utils\n> +\n> +from numbers import Number\n> +import numpy as np\n\nApart from CCM, none of thoese look needed either.\n\n> +\n> +\n> +class CCMRkISP1(CCM):\n> + hr_name = 'Crosstalk Correction (RkISP1)'\n> + out_name = 'Ccm'\n\nThe ISP documentation mentions \"color correction\" twice and uses \"cross\ntalk\" everywhere else. In the IPA module, and in libtuning, we mostly\nuse \"ccm\". I think that's fine, and I think using the name from the ISP\ndocumentation in hr_name is good.\n\n> +\n> + def __init__(self, **kwargs):\n> + super().__init__(**kwargs)\n> +\n> + # We don't actually need anything from the config file\n\ns/actually //\ns/$/./\n\n> + def validate_config(self, config: dict) -> bool:\n> + return True\n> +\n> + def process(self, config: dict, images: list, outputs: dict) -> dict:\n> + output = {}\n> +\n> + ctms = self.do_calibration(images)\n\nHere, however, I would name the variable ccms.\n\nReviewed-by: Laurent Pinchart \n\n> + output['ccms'] = ctms\n> +\n> + return output\n> diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py\n> index 2606e07a93f7..fae35783c656 100755\n> --- a/utils/tuning/rkisp1.py\n> +++ b/utils/tuning/rkisp1.py\n> @@ -14,6 +14,7 @@ from libtuning.parsers import YamlParser\n> from libtuning.generators import YamlOutput\n> from libtuning.modules.lsc import LSCRkISP1\n> from libtuning.modules.agc import AGCRkISP1\n> +from libtuning.modules.ccm import CCMRkISP1\n> \n> \n> coloredlogs.install(level=logging.INFO, fmt='%(name)s %(levelname)s %(message)s')\n> @@ -39,9 +40,10 @@ tuner.add(LSCRkISP1(\n> smoothing_function=lt.smoothing.MedianBlur(3),\n> ))\n> tuner.add(AGCRkISP1(debug=[lt.Debug.Plot]))\n> +tuner.add(CCMRkISP1(debug=[lt.Debug.Plot]))\n> tuner.set_input_parser(YamlParser())\n> tuner.set_output_formatter(YamlOutput())\n> -tuner.set_output_order([AGCRkISP1, LSCRkISP1])\n> +tuner.set_output_order([AGCRkISP1, CCMRkISP1, LSCRkISP1])\n> \n> if __name__ == '__main__':\n> sys.exit(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 6404DBDB1D\n\tfor ;\n\tFri, 28 Jun 2024 23:48:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6009B62C99;\n\tSat, 29 Jun 2024 01:48:16 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CEFB3619E8\n\tfor ;\n\tSat, 29 Jun 2024 01:48:14 +0200 (CEST)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 7784763B;\n\tSat, 29 Jun 2024 01:47:49 +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=\"E/B3x0cj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719618469;\n\tbh=zL1DIsG8M9pWHbr8lHLKXvKZ/5MPrFkYedZ3nh0CRGg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E/B3x0cjw51u7GSenDz5q5cfvzw73hQc2jD5mMLXJqt9IMfsqg7zkQMTOXDG+gGRQ\n\tfOvDxVbWtcAjTOhVmkNqJIZvCMZtiOj7YJhmjz2CPTe0ZI4/ZifJIZMpIPu2orXm9+\n\tRMBvWiQ340MUpIZjrx1/eEOgrT3diH5mMEcYCFl8=","Date":"Sat, 29 Jun 2024 02:47:53 +0300","From":"Laurent Pinchart ","To":"Stefan Klug ","Cc":"libcamera-devel@lists.libcamera.org,\n\tPaul Elder ","Subject":"Re: [PATCH v2 15/25] libtuning: modules: Add initial CCM module","Message-ID":"<20240628234753.GM30900@pendragon.ideasonboard.com>","References":"<20240628104828.2928109-1-stefan.klug@ideasonboard.com>\n\t<20240628104828.2928109-16-stefan.klug@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240628104828.2928109-16-stefan.klug@ideasonboard.com>","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\" "}},{"id":30200,"web_url":"https://patchwork.libcamera.org/comment/30200/","msgid":"<53jdu5won6wfpjk7ea5akxtfuofiybhg4jfjcswnaosrbn4fia@3t4zzrerpeom>","date":"2024-07-02T09:15:07","subject":"Re: [PATCH v2 15/25] libtuning: modules: Add initial CCM module","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Laurent,\n\nOn Sat, Jun 29, 2024 at 02:47:53AM +0300, Laurent Pinchart wrote:\n> Hi Paul and Stefan,\n> \n> Thank you for the patch.\n> \n> On Fri, Jun 28, 2024 at 12:47:08PM +0200, Stefan Klug wrote:\n> > From: Paul Elder \n> > \n> > Implement a minimal ccm calibration module. For now it doesn't take the\n> > results from lsc into account and supports rkisp1 only.\n> > \n> > Signed-off-by: Paul Elder \n> > Signed-off-by: Stefan Klug \n> > ---\n> > .../tuning/libtuning/modules/ccm/__init__.py | 6 +++\n> > utils/tuning/libtuning/modules/ccm/ccm.py | 44 +++++++++++++++++++\n> > utils/tuning/libtuning/modules/ccm/rkisp1.py | 34 ++++++++++++++\n> > utils/tuning/rkisp1.py | 4 +-\n> > 4 files changed, 87 insertions(+), 1 deletion(-)\n> > create mode 100644 utils/tuning/libtuning/modules/ccm/__init__.py\n> > create mode 100644 utils/tuning/libtuning/modules/ccm/ccm.py\n> > create mode 100644 utils/tuning/libtuning/modules/ccm/rkisp1.py\n> > \n> > diff --git a/utils/tuning/libtuning/modules/ccm/__init__.py b/utils/tuning/libtuning/modules/ccm/__init__.py\n> > new file mode 100644\n> > index 000000000000..322602afe63b\n> > --- /dev/null\n> > +++ b/utils/tuning/libtuning/modules/ccm/__init__.py\n> > @@ -0,0 +1,6 @@\n> > +# SPDX-License-Identifier: GPL-2.0-or-later\n> > +#\n> > +# Copyright (C) 2024, Paul Elder \n> > +\n> > +from libtuning.modules.ccm.ccm import CCM\n> > +from libtuning.modules.ccm.rkisp1 import CCMRkISP1\n> > diff --git a/utils/tuning/libtuning/modules/ccm/ccm.py b/utils/tuning/libtuning/modules/ccm/ccm.py\n> > new file mode 100644\n> > index 000000000000..50d435ad84a3\n> > --- /dev/null\n> > +++ b/utils/tuning/libtuning/modules/ccm/ccm.py\n> > @@ -0,0 +1,44 @@\n> > +# SPDX-License-Identifier: GPL-2.0-or-later\n> > +#\n> > +# Copyright (C) 2024, Paul Elder \n> > +# Copyright (C) 2024, Ideas on Board\n> > +#\n> > +# Base Ccm tuning module\n> > +\n> > +from ..module import Module\n> > +\n> > +from libtuning.ctt_ccm import ccm\n> > +import logging\n> > +import time\n> > +\n> > +import numpy as np\n> \n> time and numpy don't seem needed.\n> \n> > +\n> > +logger = logging.getLogger(__name__)\n> > +\n> > +\n> > +class CCM(Module):\n> > + type = 'ccm'\n> > + hr_name = 'CCM (Base)'\n> \n> Unrelated to this patch, renaming this to display_name may make the code\n> more readable.\n\nYes, that sound good.\n\n> \n> > + out_name = 'GenericCCM'\n> \n> If the base algorithms are not meant to be used directly, should we drop\n> the out_name ?\n\nI think we should question that again. Why not a using the base algo\ndirectly? If the content is really generic (and ccm is a likely\ncandidate) I don't see much benefit in the derived class. I made a\nmental note for later :-)\n\nThe rest is fixed locally.\n\nRegards,\nStefan\n\n> \n> > +\n> > + def __init__(self, debug: list):\n> > + super().__init__()\n> > +\n> > + self.debug = debug\n> > +\n> > + def do_calibration(self, images):\n> > + logger.info('Starting CCM calibration')\n> > +\n> > + imgs = [img for img in images if img.macbeth is not None]\n> > +\n> > + # todo: take alsc calibration results into account\n> \n> s/alsc/LSC/\n> \n> > + cal_cr_list = None\n> > + cal_cb_list = None\n> > +\n> > + try:\n> > + ccms = ccm(imgs, cal_cr_list, cal_cb_list)\n> > + except ArithmeticError:\n> > + logger.error('CCM calibration failed')\n> > + return 1\n> > +\n> > + return ccms\n> > diff --git a/utils/tuning/libtuning/modules/ccm/rkisp1.py b/utils/tuning/libtuning/modules/ccm/rkisp1.py\n> > new file mode 100644\n> > index 000000000000..a74d0d93c764\n> > --- /dev/null\n> > +++ b/utils/tuning/libtuning/modules/ccm/rkisp1.py\n> > @@ -0,0 +1,34 @@\n> > +# SPDX-License-Identifier: GPL-2.0-or-later\n> > +#\n> > +# Copyright (C) 2024, Paul Elder \n> > +# Copyright (C) 2024, Ideas on Board\n> > +#\n> > +# Ccm module for tuning rkisp1\n> > +\n> > +from .ccm import CCM\n> > +\n> > +import libtuning as lt\n> > +import libtuning.utils as utils\n> > +\n> > +from numbers import Number\n> > +import numpy as np\n> \n> Apart from CCM, none of thoese look needed either.\n> \n> > +\n> > +\n> > +class CCMRkISP1(CCM):\n> > + hr_name = 'Crosstalk Correction (RkISP1)'\n> > + out_name = 'Ccm'\n> \n> The ISP documentation mentions \"color correction\" twice and uses \"cross\n> talk\" everywhere else. In the IPA module, and in libtuning, we mostly\n> use \"ccm\". I think that's fine, and I think using the name from the ISP\n> documentation in hr_name is good.\n> \n> > +\n> > + def __init__(self, **kwargs):\n> > + super().__init__(**kwargs)\n> > +\n> > + # We don't actually need anything from the config file\n> \n> s/actually //\n> s/$/./\n> \n> > + def validate_config(self, config: dict) -> bool:\n> > + return True\n> > +\n> > + def process(self, config: dict, images: list, outputs: dict) -> dict:\n> > + output = {}\n> > +\n> > + ctms = self.do_calibration(images)\n> \n> Here, however, I would name the variable ccms.\n> \n> Reviewed-by: Laurent Pinchart \n> \n> > + output['ccms'] = ctms\n> > +\n> > + return output\n> > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py\n> > index 2606e07a93f7..fae35783c656 100755\n> > --- a/utils/tuning/rkisp1.py\n> > +++ b/utils/tuning/rkisp1.py\n> > @@ -14,6 +14,7 @@ from libtuning.parsers import YamlParser\n> > from libtuning.generators import YamlOutput\n> > from libtuning.modules.lsc import LSCRkISP1\n> > from libtuning.modules.agc import AGCRkISP1\n> > +from libtuning.modules.ccm import CCMRkISP1\n> > \n> > \n> > coloredlogs.install(level=logging.INFO, fmt='%(name)s %(levelname)s %(message)s')\n> > @@ -39,9 +40,10 @@ tuner.add(LSCRkISP1(\n> > smoothing_function=lt.smoothing.MedianBlur(3),\n> > ))\n> > tuner.add(AGCRkISP1(debug=[lt.Debug.Plot]))\n> > +tuner.add(CCMRkISP1(debug=[lt.Debug.Plot]))\n> > tuner.set_input_parser(YamlParser())\n> > tuner.set_output_formatter(YamlOutput())\n> > -tuner.set_output_order([AGCRkISP1, LSCRkISP1])\n> > +tuner.set_output_order([AGCRkISP1, CCMRkISP1, LSCRkISP1])\n> > \n> > if __name__ == '__main__':\n> > sys.exit(tuner.run(sys.argv))\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","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 17120BEFBE\n\tfor ;\n\tTue, 2 Jul 2024 09:15:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19D9662E01;\n\tTue, 2 Jul 2024 11:15:12 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7FA4F619C8\n\tfor ;\n\tTue, 2 Jul 2024 11:15:10 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:15de:d83a:d962:e44])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 155F9664;\n\tTue, 2 Jul 2024 11:14:43 +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=\"tl+5iQSv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1719911683;\n\tbh=pR+J7OthRgLIROIN0JLXFpALwZQEj5AXI6K+JWtDN3k=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tl+5iQSv1nyPeubacASL8sNPO/LL9iCZ+o09nQbdaugFYKvUXxC720qHNG8ibxQkE\n\tcBpIVX3KfZz2hZ4sAooRAttmhC0KcKUP29D37DtrrIF+FmPRhnzZ5cCOHrePZt+Ve4\n\tOTcLD4K4vEswT6/eeevUN4QON6qu6+/+yA7+XYiE=","Date":"Tue, 2 Jul 2024 11:15:07 +0200","From":"Stefan Klug ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, \n\tPaul Elder ","Subject":"Re: [PATCH v2 15/25] libtuning: modules: Add initial CCM module","Message-ID":"<53jdu5won6wfpjk7ea5akxtfuofiybhg4jfjcswnaosrbn4fia@3t4zzrerpeom>","References":"<20240628104828.2928109-1-stefan.klug@ideasonboard.com>\n\t<20240628104828.2928109-16-stefan.klug@ideasonboard.com>\n\t<20240628234753.GM30900@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240628234753.GM30900@pendragon.ideasonboard.com>","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\" "}}]