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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart