[PATCH v4 20/23] tuning: rkisp1: Add some static modules

[Paul Elder]

On Fri, Jul 05, 2024 at 09:54:12PM +0200, Stefan Klug wrote:
> Hi Dan,
> 
> Thank you for the review.
> 
> On Fri, Jul 05, 2024 at 08:19:04PM +0100, Dan Scally wrote:
> > Hi Stefan
> > 
> > On 05/07/2024 15:41, Stefan Klug wrote:
> > > Add awb, blc, cproc, filter, and gamma to the tuning file.  These don't
> > > need any configuration.
> > > 
> > > At the moment there are no inter-module dependencies in the tuning
> > > process. We can therefore safely sort them alphabetically. As soon as
> > > the first dependency gets introduced (most likely lsc -> ccm) we will
> > > see how to solve that.
> > > 
> > > The output order controls the order of processing in the IPA. It is now
> > > also in alphabetical order which happens to be no change for the modules
> > > that existed previously. For the others, there is no need for a specific
> > > order.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug at ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> > > ---
> > >   utils/tuning/rkisp1.py | 21 +++++++++++++++++----
> > >   1 file changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py
> > > index fae35783c656..0d279a39ab1b 100755
> > > --- a/utils/tuning/rkisp1.py
> > > +++ b/utils/tuning/rkisp1.py
> > > @@ -15,11 +15,24 @@ from libtuning.generators import YamlOutput
> > >   from libtuning.modules.lsc import LSCRkISP1
> > >   from libtuning.modules.agc import AGCRkISP1
> > >   from libtuning.modules.ccm import CCMRkISP1
> > > -
> > > +from libtuning.modules.static import StaticModule
> > >   coloredlogs.install(level=logging.INFO, fmt='%(name)s %(levelname)s %(message)s')
> > > +awb = StaticModule('Awb')
> > > +blc = StaticModule('BlackLevelCorrection')
> > > +color_processing = StaticModule('ColorProcessing')
> > > +filter = StaticModule('Filter')
> > > +gamma_out = StaticModule('GammaOutCorrection', {'gamma': 2.2})
> > > +
> > >   tuner = lt.Tuner('RkISP1')
> > > +tuner.add(AGCRkISP1(debug=[lt.Debug.Plot]))
> > > +tuner.add(awb)
> > > +tuner.add(blc)
> > > +tuner.add(CCMRkISP1(debug=[lt.Debug.Plot]))
> > > +tuner.add(color_processing)
> > > +tuner.add(filter)
> > > +tuner.add(gamma_out)
> > >   tuner.add(LSCRkISP1(
> > 
> > This is nitpicking so feel free to ignore me, but I think it'd be better to
> > be consistent in using variables for the modules or not.
> 
> Honestly I don't know why I didn't do that. At first I wanted to add the
> static modules the same way as the others, but I couldn't do that
> because multiple entries of the StaticModule class in the output_order
> would be detected as error. I then (unwillingly) had to go for the
> intermediate variables. But I never thought about doing the same with
> the others even though I really disliked the asymmetry.

It would be a lot of duplicate text even if you changed output_order
validation since you'd have:

tuner.add(StaticModule('BlackLevelCorrection'))
...
tuner.set_output_order([... , StaticModule('BlackLevelCorrection'), ...])

Whereas for the others there are no extra parameters so it's less of an
issue just duplicating the class.


Paul

> 
> So thanks for the hint :-). I'll add that as a separate patch if that's
> ok for you.
> 
> Cheers,
> Stefan
> 
> > 
> > Reviewed-by: Daniel Scally <dan.scally at ideasonboard.com>
> > 
> > >             debug=[lt.Debug.Plot],
> > >             # This is for the actual LSC tuning, and is part of the base LSC
> > > @@ -39,11 +52,11 @@ tuner.add(LSCRkISP1(
> > >             # values.  This can also be a custom function.
> > >             smoothing_function=lt.smoothing.MedianBlur(3),
> > >             ))
> > > -tuner.add(AGCRkISP1(debug=[lt.Debug.Plot]))
> > > -tuner.add(CCMRkISP1(debug=[lt.Debug.Plot]))
> > > +
> > >   tuner.set_input_parser(YamlParser())
> > >   tuner.set_output_formatter(YamlOutput())
> > > -tuner.set_output_order([AGCRkISP1, CCMRkISP1, LSCRkISP1])
> > > +tuner.set_output_order([AGCRkISP1, awb, blc, CCMRkISP1, color_processing,
> > > +                        filter, gamma_out, LSCRkISP1])
> > >   if __name__ == '__main__':
> > >       sys.exit(tuner.run(sys.argv))