[libcamera-devel,v2,11/11,WIP] utils: tuning: Add tuning script for rkisp1

17662 diff mbox series

Add a tuning script for rkisp1 that uses libtuning.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- add SPDX and copyright
- s/average_functions/average/
- update the script to work with the new rkisp1 alsc module

As of v2, this runs, but only with LSC. Parabolic gradient support is
put on hold, and instead the sectors are divided linearly. Is this what
we want?

Technically this script is complete in the sense that it works, but it's
still WIP because it only support LSC. What should we do about this?
Should it be "complete" at this stage and we can add the other modules
later, or should we keep it WIP until the other modules are added?

v1:
This won't run as we're missing the necessary parser and generator for
yaml, parabolic gradient support, and multiple green support in the LSC
module, hence the DNI. As soon as those are added though, this *should*
work.
---
 utils/tuning/rkisp1.py | 58 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100755 utils/tuning/rkisp1.py

Hi Paul,

Thank you for the patch.

On Sat, Oct 22, 2022 at 03:23:10PM +0900, Paul Elder via libcamera-devel wrote:
> Add a tuning script for rkisp1 that uses libtuning.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - add SPDX and copyright
> - s/average_functions/average/
> - update the script to work with the new rkisp1 alsc module
> 
> As of v2, this runs, but only with LSC. Parabolic gradient support is
> put on hold, and instead the sectors are divided linearly. Is this what
> we want?

Is mentioned previously, I don't think we want a fixed function to
compute the sector sizes. We want sizes that will minimize the errors.
Honestly, I would hardcode the linear sizes for now, and drop parametric
gradient support as I don't think it will ever be used, but that's not a
blocker.

> Technically this script is complete in the sense that it works, but it's
> still WIP because it only support LSC. What should we do about this?
> Should it be "complete" at this stage and we can add the other modules
> later, or should we keep it WIP until the other modules are added?
> 
> v1:
> This won't run as we're missing the necessary parser and generator for
> yaml, parabolic gradient support, and multiple green support in the LSC
> module, hence the DNI. As soon as those are added though, this *should*
> work.
> ---
>  utils/tuning/rkisp1.py | 58 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100755 utils/tuning/rkisp1.py
> 
> diff --git a/utils/tuning/rkisp1.py b/utils/tuning/rkisp1.py
> new file mode 100755
> index 00000000..cb692dd1
> --- /dev/null
> +++ b/utils/tuning/rkisp1.py
> @@ -0,0 +1,58 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright (C) 2022, Paul Elder <paul.elder@ideasonboard.com>
> +#
> +# rkisp1.py - Tuning script for rkisp1
> +
> +import sys
> +
> +import libtuning as lt
> +from libtuning.parsers import YamlParser
> +from libtuning.generators import YamlOutput
> +from libtuning.modules.alsc import ALSCRkISP1

Should be named LSCRkISP1 as mentioned elsewhere. Same below.

> +
> +tuner = lt.Camera('RkISP1')
> +tuner.add(ALSCRkISP1(
> +          # This can support other debug options (I can't think of any rn

s/rn/right now/

> +          # but for future-proofing)

s/$/./

Same for other sentences below.

> +          debug=[lt.Debug.Plot],
> +
> +          # This is for the actual LSC tuning, and is part of the base
> +          # ALSC module. rkisp1's table size (16x16 programmed as mirrored
> +          # 8x8) is separate, and is hardcoded in its specific ALSC tuning

It's only the sector sizes that are mirrored, not necessarily the
values.

> +          # module
> +          sector_shape=(17, 17),
> +
> +          # Other functions might include Circular, Hyperbolic, Gaussian,
> +          # Linear, Exponential, Logarithmic, etc
> +          # Of course, both need not be the same function
> +          # Some functions would need a sector_x_parameter (eg. sigma for Gaussian)
> +          # Alternatively: sector_x_sizes = [] ? I don't think it would work tho

Drop this, per the comment above.

> +          sector_x_gradient=lt.gradient.Linear(lt.Remainder.DistributeFront),
> +          sector_y_gradient=lt.gradient.Linear(lt.Remainder.DistributeFront),
> +
> +          # This is the function that will be used to average the pixels in each sector
> +          # This can also be a custom function.
> +          sector_average_function=lt.average.Mean(),
> +
> +          # This is the function that will be used to smooth the color ratio values
> +          # This can also be a custom function.
> +          smoothing_function=lt.smoothing.MedianBlur(3),
> +
> +          # Do we need a flag to enable/disable calculating sigmas? afaik
> +          # the rkisp1 IPA doesn't use it? But we could output it anyway

It's for the RPi ALSC only, so it shouldn't be part of the base.

> +          # and algorithms can decide whether or not if they want to use
> +          # it. Oh well, no flag for now, we can always add it later if
> +          # there's a big demand, plus it's only two to three values and
> +          # not an entire table.
> +          ))
> +tuner.setInputType(YamlParser)
> +tuner.setOutputType(YamlOutput)
> +tuner.setOutputOrder([ALSCRkISP1])
> +
> +# Maybe this should be wrapped in an if __main__ = '__main__' ? That way the

It's a good practice, so I would do that, and drop this comment.

> +# developer can control which tuner they want to be executed based on another
> +# layer of arguments? But I was thinking that this would handle *all* arguments
> +# based on the modules' and the input/output configurations.
> +tuner.run(sys.argv)