[PATCH v1 2/2] utils: tuning: Change Tuner.add() to accept a list of modules

Stefan Klug stefan.klug at ideasonboard.com
Mon Aug 5 10:55:23 CEST 2024


Hi Kieran,

Thank you for the review.

On Mon, Aug 05, 2024 at 08:39:04AM +0100, Kieran Bingham wrote:
> Quoting Stefan Klug (2024-08-05 08:06:33)
> > Change the first parameter of Tuner.add() to be a list of modules
> > instead of a single module. This allows more compact code and is in sync
> > with Tuner.set_output_order().
> > 
> > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > ---
> >  utils/tuning/libtuning/libtuning.py   |  4 ++--
> >  utils/tuning/raspberrypi_alsc_only.py |  2 +-
> >  utils/tuning/rkisp1.py                | 10 +---------
> >  3 files changed, 4 insertions(+), 12 deletions(-)
> > 
> > diff --git a/utils/tuning/libtuning/libtuning.py b/utils/tuning/libtuning/libtuning.py
> > index e7c63535fefd..a167e39ef08e 100644
> > --- a/utils/tuning/libtuning/libtuning.py
> > +++ b/utils/tuning/libtuning/libtuning.py
> > @@ -94,8 +94,8 @@ class Tuner(object):
> >          self.config = {}
> >          self.output = {}
> >  
> > -    def add(self, module):
> > -        self.modules.append(module)
> > +    def add(self, modules):
> > +        self.modules.extend(modules)
> 
> I guess it already supported a list, it's just we're only calling a
> single module and it had only been used that way.

Not really, it now calls extend() instead of append() on the list.

> 
> The pluralisation here is certainly worthwhile though to convey this.
> 
> >  
> >      def set_input_parser(self, parser):
> >          self.parser = parser
> > diff --git a/utils/tuning/raspberrypi_alsc_only.py b/utils/tuning/raspberrypi_alsc_only.py
> > index 777d800765e0..fe2b5b8b5a24 100755
> > --- a/utils/tuning/raspberrypi_alsc_only.py
> > +++ b/utils/tuning/raspberrypi_alsc_only.py
> > @@ -14,7 +14,7 @@ from libtuning.generators import RaspberryPiOutput
> >  from raspberrypi.alsc import ALSC
> >  
> >  tuner = lt.Tuner('Raspberry Pi (ALSC only)')
> > -tuner.add(ALSC)
> > +tuner.add([ALSC])
> 
> Does this one need to be changed though? (It doesn't hurt I believe), I
> see we're in a module called _alsc_only.py, so this won't be expected to
> be added to..

As noted above, this is actually required (If we don't want to go for
more magic).

Regards,
Stefan

> 
> But still ... it keeps the usages consistent...
> 
> >  tuner.set_input_parser(RaspberryPiParser())
> >  tuner.set_output_formatter(RaspberryPiOutput())
> >  tuner.set_output_order([ALSC])
> > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py
> > index 5d7a69fc4a13..f5c42a61d15e 100755
> > --- a/utils/tuning/rkisp1.py
> > +++ b/utils/tuning/rkisp1.py
> > @@ -45,15 +45,7 @@ lsc = LSCRkISP1(debug=[lt.Debug.Plot],
> >                  smoothing_function=lt.smoothing.MedianBlur(3),)
> >  
> >  tuner = lt.Tuner('RkISP1')
> > -tuner.add(agc)
> > -tuner.add(awb)
> > -tuner.add(blc)
> > -tuner.add(ccm)
> > -tuner.add(color_processing)
> > -tuner.add(filter)
> > -tuner.add(gamma_out)
> > -tuner.add(lsc)
> > -
> > +tuner.add([agc, awb, blc, ccm, color_processing, filter, gamma_out, lsc])
> 
> 
> Keeping them on separate lines would make it easier to add new entries
> ... but there's a finite number of items we'll add here and once
> 'feature complete' this line won't change - so it's not quite comparable
> to other lists that we might otherwise extend, so :
> 
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> 
> >  tuner.set_input_parser(YamlParser())
> >  tuner.set_output_formatter(YamlOutput())
> >  tuner.set_output_order([agc, awb, blc, ccm, color_processing,
> > -- 
> > 2.43.0
> >


More information about the libcamera-devel mailing list