[{"id":30572,"web_url":"https://patchwork.libcamera.org/comment/30572/","msgid":"<172284354447.2725865.10429864367889564314@ping.linuxembedded.co.uk>","date":"2024-08-05T07:39:04","subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","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-05 08:06:33)\n> Change the first parameter of Tuner.add() to be a list of modules\n> instead of a single module. This allows more compact code and is in sync\n> with Tuner.set_output_order().\n> \n> Signed-off-by: Stefan Klug \n> ---\n> utils/tuning/libtuning/libtuning.py | 4 ++--\n> utils/tuning/raspberrypi_alsc_only.py | 2 +-\n> utils/tuning/rkisp1.py | 10 +---------\n> 3 files changed, 4 insertions(+), 12 deletions(-)\n> \n> diff --git a/utils/tuning/libtuning/libtuning.py b/utils/tuning/libtuning/libtuning.py\n> index e7c63535fefd..a167e39ef08e 100644\n> --- a/utils/tuning/libtuning/libtuning.py\n> +++ b/utils/tuning/libtuning/libtuning.py\n> @@ -94,8 +94,8 @@ class Tuner(object):\n> self.config = {}\n> self.output = {}\n> \n> - def add(self, module):\n> - self.modules.append(module)\n> + def add(self, modules):\n> + self.modules.extend(modules)\n\nI guess it already supported a list, it's just we're only calling a\nsingle module and it had only been used that way.\n\nThe pluralisation here is certainly worthwhile though to convey this.\n\n> \n> def set_input_parser(self, parser):\n> self.parser = parser\n> diff --git a/utils/tuning/raspberrypi_alsc_only.py b/utils/tuning/raspberrypi_alsc_only.py\n> index 777d800765e0..fe2b5b8b5a24 100755\n> --- a/utils/tuning/raspberrypi_alsc_only.py\n> +++ b/utils/tuning/raspberrypi_alsc_only.py\n> @@ -14,7 +14,7 @@ from libtuning.generators import RaspberryPiOutput\n> from raspberrypi.alsc import ALSC\n> \n> tuner = lt.Tuner('Raspberry Pi (ALSC only)')\n> -tuner.add(ALSC)\n> +tuner.add([ALSC])\n\nDoes this one need to be changed though? (It doesn't hurt I believe), I\nsee we're in a module called _alsc_only.py, so this won't be expected to\nbe added to..\n\nBut still ... it keeps the usages consistent...\n\n> tuner.set_input_parser(RaspberryPiParser())\n> tuner.set_output_formatter(RaspberryPiOutput())\n> tuner.set_output_order([ALSC])\n> diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py\n> index 5d7a69fc4a13..f5c42a61d15e 100755\n> --- a/utils/tuning/rkisp1.py\n> +++ b/utils/tuning/rkisp1.py\n> @@ -45,15 +45,7 @@ lsc = LSCRkISP1(debug=[lt.Debug.Plot],\n> smoothing_function=lt.smoothing.MedianBlur(3),)\n> \n> tuner = lt.Tuner('RkISP1')\n> -tuner.add(agc)\n> -tuner.add(awb)\n> -tuner.add(blc)\n> -tuner.add(ccm)\n> -tuner.add(color_processing)\n> -tuner.add(filter)\n> -tuner.add(gamma_out)\n> -tuner.add(lsc)\n> -\n> +tuner.add([agc, awb, blc, ccm, color_processing, filter, gamma_out, lsc])\n\n\nKeeping them on separate lines would make it easier to add new entries\n... but there's a finite number of items we'll add here and once\n'feature complete' this line won't change - so it's not quite comparable\nto other lists that we might otherwise extend, so :\n\nReviewed-by: Kieran Bingham \n\n> tuner.set_input_parser(YamlParser())\n> tuner.set_output_formatter(YamlOutput())\n> tuner.set_output_order([agc, awb, blc, ccm, color_processing,\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 CF19EC323E\n\tfor ;\n\tMon, 5 Aug 2024 07:39:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 730C863381;\n\tMon, 5 Aug 2024 09:39:08 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CE916195A\n\tfor ;\n\tMon, 5 Aug 2024 09:39:07 +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 076241A2;\n\tMon, 5 Aug 2024 09:38:15 +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=\"WhclIcf6\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722843496;\n\tbh=CVrFRlPfMj43MZpQIz5fCqWNbiQ8LpZJJfVGG8LUvTY=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=WhclIcf6D7KanjbImFbAuXZazpM/9gooVPuZeHn60mwcnD2vQHQqVN3SPlTzOGMpQ\n\t3/QXe9VGmqQyjAhIL0SsuXHwwaANQB21XnXhjpQ8LIFdbdChYrlAOLQhZDLxpTx0/X\n\tUweh8rAWrREkod4JDHxwTYPczo+quaKAzNZVykmA=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20240805070644.1536232-3-stefan.klug@ideasonboard.com>","References":"<20240805070644.1536232-1-stefan.klug@ideasonboard.com>\n\t<20240805070644.1536232-3-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","From":"Kieran Bingham ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Mon, 05 Aug 2024 08:39:04 +0100","Message-ID":"<172284354447.2725865.10429864367889564314@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\" "}},{"id":30575,"web_url":"https://patchwork.libcamera.org/comment/30575/","msgid":"","date":"2024-08-05T08:55:23","subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the review.\n\nOn Mon, Aug 05, 2024 at 08:39:04AM +0100, Kieran Bingham wrote:\n> Quoting Stefan Klug (2024-08-05 08:06:33)\n> > Change the first parameter of Tuner.add() to be a list of modules\n> > instead of a single module. This allows more compact code and is in sync\n> > with Tuner.set_output_order().\n> > \n> > Signed-off-by: Stefan Klug \n> > ---\n> > utils/tuning/libtuning/libtuning.py | 4 ++--\n> > utils/tuning/raspberrypi_alsc_only.py | 2 +-\n> > utils/tuning/rkisp1.py | 10 +---------\n> > 3 files changed, 4 insertions(+), 12 deletions(-)\n> > \n> > diff --git a/utils/tuning/libtuning/libtuning.py b/utils/tuning/libtuning/libtuning.py\n> > index e7c63535fefd..a167e39ef08e 100644\n> > --- a/utils/tuning/libtuning/libtuning.py\n> > +++ b/utils/tuning/libtuning/libtuning.py\n> > @@ -94,8 +94,8 @@ class Tuner(object):\n> > self.config = {}\n> > self.output = {}\n> > \n> > - def add(self, module):\n> > - self.modules.append(module)\n> > + def add(self, modules):\n> > + self.modules.extend(modules)\n> \n> I guess it already supported a list, it's just we're only calling a\n> single module and it had only been used that way.\n\nNot really, it now calls extend() instead of append() on the list.\n\n> \n> The pluralisation here is certainly worthwhile though to convey this.\n> \n> > \n> > def set_input_parser(self, parser):\n> > self.parser = parser\n> > diff --git a/utils/tuning/raspberrypi_alsc_only.py b/utils/tuning/raspberrypi_alsc_only.py\n> > index 777d800765e0..fe2b5b8b5a24 100755\n> > --- a/utils/tuning/raspberrypi_alsc_only.py\n> > +++ b/utils/tuning/raspberrypi_alsc_only.py\n> > @@ -14,7 +14,7 @@ from libtuning.generators import RaspberryPiOutput\n> > from raspberrypi.alsc import ALSC\n> > \n> > tuner = lt.Tuner('Raspberry Pi (ALSC only)')\n> > -tuner.add(ALSC)\n> > +tuner.add([ALSC])\n> \n> Does this one need to be changed though? (It doesn't hurt I believe), I\n> see we're in a module called _alsc_only.py, so this won't be expected to\n> be added to..\n\nAs noted above, this is actually required (If we don't want to go for\nmore magic).\n\nRegards,\nStefan\n\n> \n> But still ... it keeps the usages consistent...\n> \n> > tuner.set_input_parser(RaspberryPiParser())\n> > tuner.set_output_formatter(RaspberryPiOutput())\n> > tuner.set_output_order([ALSC])\n> > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py\n> > index 5d7a69fc4a13..f5c42a61d15e 100755\n> > --- a/utils/tuning/rkisp1.py\n> > +++ b/utils/tuning/rkisp1.py\n> > @@ -45,15 +45,7 @@ lsc = LSCRkISP1(debug=[lt.Debug.Plot],\n> > smoothing_function=lt.smoothing.MedianBlur(3),)\n> > \n> > tuner = lt.Tuner('RkISP1')\n> > -tuner.add(agc)\n> > -tuner.add(awb)\n> > -tuner.add(blc)\n> > -tuner.add(ccm)\n> > -tuner.add(color_processing)\n> > -tuner.add(filter)\n> > -tuner.add(gamma_out)\n> > -tuner.add(lsc)\n> > -\n> > +tuner.add([agc, awb, blc, ccm, color_processing, filter, gamma_out, lsc])\n> \n> \n> Keeping them on separate lines would make it easier to add new entries\n> ... but there's a finite number of items we'll add here and once\n> 'feature complete' this line won't change - so it's not quite comparable\n> to other lists that we might otherwise extend, so :\n> \n> Reviewed-by: Kieran Bingham \n> \n> > tuner.set_input_parser(YamlParser())\n> > tuner.set_output_formatter(YamlOutput())\n> > tuner.set_output_order([agc, awb, blc, ccm, color_processing,\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 797BBBE173\n\tfor ;\n\tMon, 5 Aug 2024 08:55:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 663736191F;\n\tMon, 5 Aug 2024 10:55:29 +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 1C0156191F\n\tfor ;\n\tMon, 5 Aug 2024 10:55:27 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:30ff:e7b4:349c:44ce])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B712F581;\n\tMon, 5 Aug 2024 10:54:35 +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=\"a3Y5/yg1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722848075;\n\tbh=oLUlv8SNQyJf5nudxGHRgea+Q0bQi0qM2pGkp/c69vA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=a3Y5/yg19p5iASKKBuit4AC3E+6IUK/egf9gBz7P03vqbbgtwO9SbstCePZVtu/Ij\n\t4gpqdvuVh9gruURXo2AUHQVBy45LxdHUe4DqJWvplE6KnExbjxIECwkxUAjkDZZeuj\n\tfBgSZlbPK08J/osVvvrqy7WrsjB7TPtSgfKUaxfE=","Date":"Mon, 5 Aug 2024 10:55:23 +0200","From":"Stefan Klug ","To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","Message-ID":"","References":"<20240805070644.1536232-1-stefan.klug@ideasonboard.com>\n\t<20240805070644.1536232-3-stefan.klug@ideasonboard.com>\n\t<172284354447.2725865.10429864367889564314@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<172284354447.2725865.10429864367889564314@ping.linuxembedded.co.uk>","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":30576,"web_url":"https://patchwork.libcamera.org/comment/30576/","msgid":"<172284888060.2725865.920490467391422402@ping.linuxembedded.co.uk>","date":"2024-08-05T09:08:00","subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","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-05 09:55:23)\n> Hi Kieran,\n> \n> Thank you for the review.\n> \n> On Mon, Aug 05, 2024 at 08:39:04AM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2024-08-05 08:06:33)\n> > > Change the first parameter of Tuner.add() to be a list of modules\n> > > instead of a single module. This allows more compact code and is in sync\n> > > with Tuner.set_output_order().\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > utils/tuning/libtuning/libtuning.py | 4 ++--\n> > > utils/tuning/raspberrypi_alsc_only.py | 2 +-\n> > > utils/tuning/rkisp1.py | 10 +---------\n> > > 3 files changed, 4 insertions(+), 12 deletions(-)\n> > > \n> > > diff --git a/utils/tuning/libtuning/libtuning.py b/utils/tuning/libtuning/libtuning.py\n> > > index e7c63535fefd..a167e39ef08e 100644\n> > > --- a/utils/tuning/libtuning/libtuning.py\n> > > +++ b/utils/tuning/libtuning/libtuning.py\n> > > @@ -94,8 +94,8 @@ class Tuner(object):\n> > > self.config = {}\n> > > self.output = {}\n> > > \n> > > - def add(self, module):\n> > > - self.modules.append(module)\n> > > + def add(self, modules):\n> > > + self.modules.extend(modules)\n> > \n> > I guess it already supported a list, it's just we're only calling a\n> > single module and it had only been used that way.\n> \n> Not really, it now calls extend() instead of append() on the list.\n\nOhh I completley missed that !\n\n> > \n> > The pluralisation here is certainly worthwhile though to convey this.\n> > \n> > > \n> > > def set_input_parser(self, parser):\n> > > self.parser = parser\n> > > diff --git a/utils/tuning/raspberrypi_alsc_only.py b/utils/tuning/raspberrypi_alsc_only.py\n> > > index 777d800765e0..fe2b5b8b5a24 100755\n> > > --- a/utils/tuning/raspberrypi_alsc_only.py\n> > > +++ b/utils/tuning/raspberrypi_alsc_only.py\n> > > @@ -14,7 +14,7 @@ from libtuning.generators import RaspberryPiOutput\n> > > from raspberrypi.alsc import ALSC\n> > > \n> > > tuner = lt.Tuner('Raspberry Pi (ALSC only)')\n> > > -tuner.add(ALSC)\n> > > +tuner.add([ALSC])\n> > \n> > Does this one need to be changed though? (It doesn't hurt I believe), I\n> > see we're in a module called _alsc_only.py, so this won't be expected to\n> > be added to..\n> \n> As noted above, this is actually required (If we don't want to go for\n> more magic).\n\nYup. It's clearer now thanks.\n\n\n> \n> Regards,\n> Stefan\n> \n> > \n> > But still ... it keeps the usages consistent...\n> > \n> > > tuner.set_input_parser(RaspberryPiParser())\n> > > tuner.set_output_formatter(RaspberryPiOutput())\n> > > tuner.set_output_order([ALSC])\n> > > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py\n> > > index 5d7a69fc4a13..f5c42a61d15e 100755\n> > > --- a/utils/tuning/rkisp1.py\n> > > +++ b/utils/tuning/rkisp1.py\n> > > @@ -45,15 +45,7 @@ lsc = LSCRkISP1(debug=[lt.Debug.Plot],\n> > > smoothing_function=lt.smoothing.MedianBlur(3),)\n> > > \n> > > tuner = lt.Tuner('RkISP1')\n> > > -tuner.add(agc)\n> > > -tuner.add(awb)\n> > > -tuner.add(blc)\n> > > -tuner.add(ccm)\n> > > -tuner.add(color_processing)\n> > > -tuner.add(filter)\n> > > -tuner.add(gamma_out)\n> > > -tuner.add(lsc)\n> > > -\n> > > +tuner.add([agc, awb, blc, ccm, color_processing, filter, gamma_out, lsc])\n> > \n> > \n> > Keeping them on separate lines would make it easier to add new entries\n> > ... but there's a finite number of items we'll add here and once\n> > 'feature complete' this line won't change - so it's not quite comparable\n> > to other lists that we might otherwise extend, so :\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > tuner.set_input_parser(YamlParser())\n> > > tuner.set_output_formatter(YamlOutput())\n> > > tuner.set_output_order([agc, awb, blc, ccm, color_processing,\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 06E8FC323E\n\tfor ;\n\tMon, 5 Aug 2024 09:08:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 499316195A;\n\tMon, 5 Aug 2024 11:08:05 +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 B83A76191F\n\tfor ;\n\tMon, 5 Aug 2024 11:08:03 +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 4BC341A2;\n\tMon, 5 Aug 2024 11:07:12 +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=\"ebM4q2YC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1722848832;\n\tbh=qPoNQnZnE5iZuHeLbWaWWxeLS+NluLM3819raT54Nno=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=ebM4q2YCVT2wwBwTfuvCFA5y8T5gHD6MnBfHVgWJsYdkVf9kV8E7SnsLVwP/DUYAE\n\tcUkIvk4bhZe5napT1VDIffxx4vonAejPZClhUwJVdZ5cuZi8nHfLFEUdEZZiGIKIWR\n\tOgl02T+jI2VXrRkIKDM8CSut+G/dJYa+avOAurv8=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240805070644.1536232-1-stefan.klug@ideasonboard.com>\n\t<20240805070644.1536232-3-stefan.klug@ideasonboard.com>\n\t<172284354447.2725865.10429864367889564314@ping.linuxembedded.co.uk>\n\t","Subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","From":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","To":"Stefan Klug ","Date":"Mon, 05 Aug 2024 10:08:00 +0100","Message-ID":"<172284888060.2725865.920490467391422402@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\" "}},{"id":30805,"web_url":"https://patchwork.libcamera.org/comment/30805/","msgid":"","date":"2024-08-14T08:00:17","subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"On Mon, Aug 05, 2024 at 10:55:23AM +0200, Stefan Klug wrote:\n> Hi Kieran,\n> \n> Thank you for the review.\n> \n> On Mon, Aug 05, 2024 at 08:39:04AM +0100, Kieran Bingham wrote:\n> > Quoting Stefan Klug (2024-08-05 08:06:33)\n> > > Change the first parameter of Tuner.add() to be a list of modules\n> > > instead of a single module. This allows more compact code and is in sync\n> > > with Tuner.set_output_order().\n\nHm true. I got the idea from my university days of working with\npytorch...\n\n> > > \n> > > Signed-off-by: Stefan Klug \n> > > ---\n> > > utils/tuning/libtuning/libtuning.py | 4 ++--\n> > > utils/tuning/raspberrypi_alsc_only.py | 2 +-\n> > > utils/tuning/rkisp1.py | 10 +---------\n> > > 3 files changed, 4 insertions(+), 12 deletions(-)\n> > > \n> > > diff --git a/utils/tuning/libtuning/libtuning.py b/utils/tuning/libtuning/libtuning.py\n> > > index e7c63535fefd..a167e39ef08e 100644\n> > > --- a/utils/tuning/libtuning/libtuning.py\n> > > +++ b/utils/tuning/libtuning/libtuning.py\n> > > @@ -94,8 +94,8 @@ class Tuner(object):\n> > > self.config = {}\n> > > self.output = {}\n> > > \n> > > - def add(self, module):\n> > > - self.modules.append(module)\n> > > + def add(self, modules):\n> > > + self.modules.extend(modules)\n> > \n> > I guess it already supported a list, it's just we're only calling a\n> > single module and it had only been used that way.\n> \n> Not really, it now calls extend() instead of append() on the list.\n> \n> > \n> > The pluralisation here is certainly worthwhile though to convey this.\n> > \n> > > \n> > > def set_input_parser(self, parser):\n> > > self.parser = parser\n> > > diff --git a/utils/tuning/raspberrypi_alsc_only.py b/utils/tuning/raspberrypi_alsc_only.py\n> > > index 777d800765e0..fe2b5b8b5a24 100755\n> > > --- a/utils/tuning/raspberrypi_alsc_only.py\n> > > +++ b/utils/tuning/raspberrypi_alsc_only.py\n> > > @@ -14,7 +14,7 @@ from libtuning.generators import RaspberryPiOutput\n> > > from raspberrypi.alsc import ALSC\n> > > \n> > > tuner = lt.Tuner('Raspberry Pi (ALSC only)')\n> > > -tuner.add(ALSC)\n> > > +tuner.add([ALSC])\n> > \n> > Does this one need to be changed though? (It doesn't hurt I believe), I\n> > see we're in a module called _alsc_only.py, so this won't be expected to\n> > be added to..\n> \n> As noted above, this is actually required (If we don't want to go for\n> more magic).\n\nWouldn't the only magic be isinstance(modules, list)? (which tbh I'd\nprefer but idk if there's much demand for both)\n\n\nPaul\n\n> \n> > \n> > But still ... it keeps the usages consistent...\n> > \n> > > tuner.set_input_parser(RaspberryPiParser())\n> > > tuner.set_output_formatter(RaspberryPiOutput())\n> > > tuner.set_output_order([ALSC])\n> > > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py\n> > > index 5d7a69fc4a13..f5c42a61d15e 100755\n> > > --- a/utils/tuning/rkisp1.py\n> > > +++ b/utils/tuning/rkisp1.py\n> > > @@ -45,15 +45,7 @@ lsc = LSCRkISP1(debug=[lt.Debug.Plot],\n> > > smoothing_function=lt.smoothing.MedianBlur(3),)\n> > > \n> > > tuner = lt.Tuner('RkISP1')\n> > > -tuner.add(agc)\n> > > -tuner.add(awb)\n> > > -tuner.add(blc)\n> > > -tuner.add(ccm)\n> > > -tuner.add(color_processing)\n> > > -tuner.add(filter)\n> > > -tuner.add(gamma_out)\n> > > -tuner.add(lsc)\n> > > -\n> > > +tuner.add([agc, awb, blc, ccm, color_processing, filter, gamma_out, lsc])\n> > \n> > \n> > Keeping them on separate lines would make it easier to add new entries\n> > ... but there's a finite number of items we'll add here and once\n> > 'feature complete' this line won't change - so it's not quite comparable\n> > to other lists that we might otherwise extend, so :\n> > \n> > Reviewed-by: Kieran Bingham \n> > \n> > > tuner.set_input_parser(YamlParser())\n> > > tuner.set_output_formatter(YamlOutput())\n> > > tuner.set_output_order([agc, awb, blc, ccm, color_processing,\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 3C5E5BDB13\n\tfor ;\n\tWed, 14 Aug 2024 08:00:28 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 24377633B5;\n\tWed, 14 Aug 2024 10:00:27 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 541FC61946\n\tfor ;\n\tWed, 14 Aug 2024 10:00:25 +0200 (CEST)","from pyrite.rasen.tech (h175-177-049-024.catv02.itscom.jp\n\t[175.177.49.24])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 75A676B5;\n\tWed, 14 Aug 2024 09:59:26 +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=\"ad/qWXF3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1723622367;\n\tbh=Y2iQVMHBUTqHFbpl4/Ms4siuQxTjQ4X48E8G1BSJuwY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ad/qWXF3aRQ0fUcri5AUxoBS/SefpibuN5aCECLLnBkV9WVmPCYRPTweUSGaEDodc\n\tckha2z5hSxTsRtT5lA+zaylZfHvgR3jTvLPxI0goY3eeZIf8jog8AK5WSzr0YfN7nh\n\t+yN071+Y5XfarTdeEgmxjec3waUgy/PP7QMi0bTk=","Date":"Wed, 14 Aug 2024 17:00:17 +0900","From":"Paul Elder ","To":"Stefan Klug ","Cc":"Kieran Bingham ,\n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","Message-ID":"","References":"<20240805070644.1536232-1-stefan.klug@ideasonboard.com>\n\t<20240805070644.1536232-3-stefan.klug@ideasonboard.com>\n\t<172284354447.2725865.10429864367889564314@ping.linuxembedded.co.uk>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"","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":30807,"web_url":"https://patchwork.libcamera.org/comment/30807/","msgid":"","date":"2024-08-14T09:03:53","subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"On Wed, Aug 14, 2024 at 05:00:17PM +0900, Paul Elder wrote:\n> On Mon, Aug 05, 2024 at 10:55:23AM +0200, Stefan Klug wrote:\n> > Hi Kieran,\n> > \n> > Thank you for the review.\n> > \n> > On Mon, Aug 05, 2024 at 08:39:04AM +0100, Kieran Bingham wrote:\n> > > Quoting Stefan Klug (2024-08-05 08:06:33)\n> > > > Change the first parameter of Tuner.add() to be a list of modules\n> > > > instead of a single module. This allows more compact code and is in sync\n> > > > with Tuner.set_output_order().\n> \n> Hm true. I got the idea from my university days of working with\n> pytorch...\n> \n> > > > \n> > > > Signed-off-by: Stefan Klug \n> > > > ---\n> > > > utils/tuning/libtuning/libtuning.py | 4 ++--\n> > > > utils/tuning/raspberrypi_alsc_only.py | 2 +-\n> > > > utils/tuning/rkisp1.py | 10 +---------\n> > > > 3 files changed, 4 insertions(+), 12 deletions(-)\n> > > > \n> > > > diff --git a/utils/tuning/libtuning/libtuning.py b/utils/tuning/libtuning/libtuning.py\n> > > > index e7c63535fefd..a167e39ef08e 100644\n> > > > --- a/utils/tuning/libtuning/libtuning.py\n> > > > +++ b/utils/tuning/libtuning/libtuning.py\n> > > > @@ -94,8 +94,8 @@ class Tuner(object):\n> > > > self.config = {}\n> > > > self.output = {}\n> > > > \n> > > > - def add(self, module):\n> > > > - self.modules.append(module)\n> > > > + def add(self, modules):\n> > > > + self.modules.extend(modules)\n> > > \n> > > I guess it already supported a list, it's just we're only calling a\n> > > single module and it had only been used that way.\n> > \n> > Not really, it now calls extend() instead of append() on the list.\n> > \n> > > \n> > > The pluralisation here is certainly worthwhile though to convey this.\n> > > \n> > > > \n> > > > def set_input_parser(self, parser):\n> > > > self.parser = parser\n> > > > diff --git a/utils/tuning/raspberrypi_alsc_only.py b/utils/tuning/raspberrypi_alsc_only.py\n> > > > index 777d800765e0..fe2b5b8b5a24 100755\n> > > > --- a/utils/tuning/raspberrypi_alsc_only.py\n> > > > +++ b/utils/tuning/raspberrypi_alsc_only.py\n> > > > @@ -14,7 +14,7 @@ from libtuning.generators import RaspberryPiOutput\n> > > > from raspberrypi.alsc import ALSC\n> > > > \n> > > > tuner = lt.Tuner('Raspberry Pi (ALSC only)')\n> > > > -tuner.add(ALSC)\n> > > > +tuner.add([ALSC])\n> > > \n> > > Does this one need to be changed though? (It doesn't hurt I believe), I\n> > > see we're in a module called _alsc_only.py, so this won't be expected to\n> > > be added to..\n> > \n> > As noted above, this is actually required (If we don't want to go for\n> > more magic).\n> \n> Wouldn't the only magic be isinstance(modules, list)? (which tbh I'd\n> prefer but idk if there's much demand for both)\n\nYes, that would be the magic. I'll implement it and post a v2.\n\nCheers,\nStefan\n\n> \n> \n> Paul\n> \n> > \n> > > \n> > > But still ... it keeps the usages consistent...\n> > > \n> > > > tuner.set_input_parser(RaspberryPiParser())\n> > > > tuner.set_output_formatter(RaspberryPiOutput())\n> > > > tuner.set_output_order([ALSC])\n> > > > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py\n> > > > index 5d7a69fc4a13..f5c42a61d15e 100755\n> > > > --- a/utils/tuning/rkisp1.py\n> > > > +++ b/utils/tuning/rkisp1.py\n> > > > @@ -45,15 +45,7 @@ lsc = LSCRkISP1(debug=[lt.Debug.Plot],\n> > > > smoothing_function=lt.smoothing.MedianBlur(3),)\n> > > > \n> > > > tuner = lt.Tuner('RkISP1')\n> > > > -tuner.add(agc)\n> > > > -tuner.add(awb)\n> > > > -tuner.add(blc)\n> > > > -tuner.add(ccm)\n> > > > -tuner.add(color_processing)\n> > > > -tuner.add(filter)\n> > > > -tuner.add(gamma_out)\n> > > > -tuner.add(lsc)\n> > > > -\n> > > > +tuner.add([agc, awb, blc, ccm, color_processing, filter, gamma_out, lsc])\n> > > \n> > > \n> > > Keeping them on separate lines would make it easier to add new entries\n> > > ... but there's a finite number of items we'll add here and once\n> > > 'feature complete' this line won't change - so it's not quite comparable\n> > > to other lists that we might otherwise extend, so :\n> > > \n> > > Reviewed-by: Kieran Bingham \n> > > \n> > > > tuner.set_input_parser(YamlParser())\n> > > > tuner.set_output_formatter(YamlOutput())\n> > > > tuner.set_output_order([agc, awb, blc, ccm, color_processing,\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 7EE2FC323E\n\tfor ;\n\tWed, 14 Aug 2024 09:04:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 03B18633B5;\n\tWed, 14 Aug 2024 11:03:58 +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 2F41961946\n\tfor ;\n\tWed, 14 Aug 2024 11:03:57 +0200 (CEST)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:8a6:aa2:ebee:5ae5])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6EF246B5;\n\tWed, 14 Aug 2024 11:02:59 +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=\"IyHHAUtN\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1723626179;\n\tbh=MCNuQc28I4PLSRnWviOW/FoXV66bzEI8Ub+mABkmvQY=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IyHHAUtNIRxCDXkIgWjIEWYIhDwerjs/mkEPRG5UFMOja7qMq8uumobhrPTBUjVkS\n\tpNNyEhg3XyrX5wnDFoW/WhnjKprNT3aMFxXujmdgAgirn+UxYivyVjewTCvakIWGIc\n\tBO+Pd01q3XZFm/3aomJUpO/EATpgXrFtmokvMEqQ=","Date":"Wed, 14 Aug 2024 11:03:53 +0200","From":"Stefan Klug ","To":"Paul Elder ","Cc":"Kieran Bingham , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a\n\tlist of modules","Message-ID":"","References":"<20240805070644.1536232-1-stefan.klug@ideasonboard.com>\n\t<20240805070644.1536232-3-stefan.klug@ideasonboard.com>\n\t<172284354447.2725865.10429864367889564314@ping.linuxembedded.co.uk>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","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\" "}}]