[libcamera-devel] [PATCH v2 05/11] utils: libtuning: parsers: Add raspberrypi parser

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Sat, Oct 22, 2022 at 03:23:04PM +0900, Paul Elder via libcamera-devel wrote:
> Add a parser to libtuning for parsing configuration files that are the
> same format as raspberrypi's ctt's configuration files.
> 
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
> 
> ---
> Changes in v2:
> - fix python errors
> - fix style
> - add SPDX and copyright
> - don't put black level in the processed config, as raspberrypi ctt's
>   config format uses -1 as a magical value to tell ctt to use the value
>   from the image, but in our Image loading function it already does, and
>   uses this config value to override it if its present
> - Don't validate module config in parser, instead libtuning will do it;
>   parser just converts the key from string to module instance
> ---
>  utils/tuning/libtuning/parsers/__init__.py    |  5 +
>  .../libtuning/parsers/raspberrypi_parser.py   | 91 +++++++++++++++++++
>  2 files changed, 96 insertions(+)
>  create mode 100644 utils/tuning/libtuning/parsers/raspberrypi_parser.py
> 
> diff --git a/utils/tuning/libtuning/parsers/__init__.py b/utils/tuning/libtuning/parsers/__init__.py
> index e69de29b..9d20d2fc 100644
> --- a/utils/tuning/libtuning/parsers/__init__.py
> +++ b/utils/tuning/libtuning/parsers/__init__.py
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright (C) 2022, Paul Elder <paul.elder at ideasonboard.com>
> +
> +from libtuning.parsers.raspberrypi_parser import RaspberryPiParser
> diff --git a/utils/tuning/libtuning/parsers/raspberrypi_parser.py b/utils/tuning/libtuning/parsers/raspberrypi_parser.py
> new file mode 100644
> index 00000000..196c5b36
> --- /dev/null
> +++ b/utils/tuning/libtuning/parsers/raspberrypi_parser.py
> @@ -0,0 +1,91 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright (C) 2022, Paul Elder <paul.elder at ideasonboard.com>
> +
> +from .parser import Parser
> +
> +import json
> +import numbers
> +
> +import libtuning.utils as utils
> +
> +
> +class RaspberryPiParser(Parser):
> +    def __init__(self):
> +        super().__init__()
> +
> +    # The string in the 'disable' and 'plot' lists are formatted as
> +    # 'rpi.{module_name}'.
> +    # @brief Enumerate, as a module, @a listt if its value exists in @a dictt
> +    #        and it is the name of a valid module in @a modules
> +    def enumerate_rpi_modules(self, listt, dictt, modules):

Following your naming convention, shouldn't this start with an _ ?

> +        for x in listt:
> +            name = x.replace('rpi.', '')
> +            if name not in dictt:
> +                continue
> +            module = utils.get_module_by_typename(modules, name)
> +            if module is not None:
> +                yield module
> +
> +    def _validMacbethOption(self, value):

snake_case

> +        if not isinstance(value, dict):
> +            return False
> +
> +        if list(value.keys()) != ['small', 'show']:
> +            return False
> +
> +        for val in value.values():
> +            if not isinstance(val, numbers.Number):
> +                return False
> +
> +        return True
> +
> +    def _parse(self, config_file, modules):
> +        with open(config_file, 'r') as config_json:
> +            config = json.load(config_json)
> +
> +        disable = []
> +        for module in self.enumerate_rpi_modules(config['disable'], config, modules):
> +            disable.append(module)
> +            # Remove the disabled module's config too
> +            config.pop(module.name)
> +        config.pop('disable')
> +
> +        # The raspberrypi config format has 'plot' map to a list of module
> +        # names which should be plotted.  libtuning has each module contain the

s/  libtuning/ libtuning/

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

> +        # plot information in itself so do this conversion.
> +
> +        for module in self.enumerate_rpi_modules(config['plot'], config, modules):
> +            # It's fine to set the value of a potentially disabled module, as
> +            # the object still exists at this point
> +            module.appendValue('debug', 'plot')
> +        config.pop('plot')
> +
> +        # Convert the keys from module name to module instance
> +
> +        new_config = {}
> +
> +        for module_name in config:
> +            module = utils.get_module_by_typename(modules, module_name)
> +            if module is not None:
> +                new_config[module] = config[module_name]
> +
> +        new_config['general'] = {}
> +
> +        if 'blacklevel' in config:
> +            if not isinstance(config['blacklevel'], numbers.Number):
> +                raise TypeError('Config "blacklevel" must be a number')
> +            # Raspberry Pi's ctt config has magic blacklevel value -1 to mean
> +            # "get it from the image metadata". Since we already do that in
> +            # Image, don't save it to the config here.
> +            if config['blacklevel'] >= 0:
> +                new_config['general']['blacklevel'] = config['blacklevel']
> +
> +        if 'macbeth' in config:
> +            if not self._validMacbethOption(config['macbeth']):
> +                raise TypeError('Config "macbeth" must be a dict: {"small": number, "show": number}')
> +            new_config['general']['macbeth'] = config['macbeth']
> +        else:
> +            new_config['general']['macbeth'] = {'small': 0, 'show': 0}
> +
> +        return new_config, disable

-- 
Regards,

Laurent Pinchart