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

20609 diff mbox series

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@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
---
 utils/tuning/rkisp1.py | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

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@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@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.

Reviewed-by: Daniel Scally <dan.scally@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))

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@ideasonboard.com>
> > Reviewed-by: Paul Elder <paul.elder@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.

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@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))

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@ideasonboard.com>
> > > Reviewed-by: Paul Elder <paul.elder@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@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))