[v1] utils: Add script and schema to validate tuning files

20117 diff mbox series

Add a schema file to validate the rkisp1 tuning files. This allows us to
easily check if tuning files got outdated and can be used as implicit
documentation for the file format. For now only rkisp1 is supported.

To validate all tuning files, run
utils/validate-tuning-files.py --all

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 src/ipa/rkisp1/data/tuning-rkisp1.schema.yaml | 376 ++++++++++++++++++
 utils/validate-tuning-files.py                | 112 ++++++
 2 files changed, 488 insertions(+)
 create mode 100644 src/ipa/rkisp1/data/tuning-rkisp1.schema.yaml
 create mode 100755 utils/validate-tuning-files.py

Hi Stefan,

On Wed, May 29, 2024 at 03:15:57PM +0200, Stefan Klug wrote:
> Add a schema file to validate the rkisp1 tuning files. This allows us to
> easily check if tuning files got outdated and can be used as implicit
> documentation for the file format. For now only rkisp1 is supported.

This is amazing.

> 
> To validate all tuning files, run
> utils/validate-tuning-files.py --all
> 
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>  src/ipa/rkisp1/data/tuning-rkisp1.schema.yaml | 376 ++++++++++++++++++
>  utils/validate-tuning-files.py                | 112 ++++++
>  2 files changed, 488 insertions(+)
>  create mode 100644 src/ipa/rkisp1/data/tuning-rkisp1.schema.yaml
>  create mode 100755 utils/validate-tuning-files.py
> 
> diff --git a/src/ipa/rkisp1/data/tuning-rkisp1.schema.yaml b/src/ipa/rkisp1/data/tuning-rkisp1.schema.yaml
> new file mode 100644
> index 00000000..4f72f53e
> --- /dev/null
> +++ b/src/ipa/rkisp1/data/tuning-rkisp1.schema.yaml
> @@ -0,0 +1,376 @@
> +# SPDX-License-Identifier: CC0-1.0
> +%YAML 1.1
> +---
> +$id: https://libcamera.org/tuning-rkisp1.schema.yaml
> +$schema: https://json-schema.org/draft/2020-12/schema
> +description: Tuning file schema for the RKISP1 IPA
> +type: object
> +required: ['version', 'algorithms']
> +properties:
> +  version:
> +    const: 1
> +  algorithms:
> +    type: array
> +    items:
> +      oneOf:
> +        - $ref: '#/$defs/Agc'
> +        - $ref: '#/$defs/Awb'
> +        - $ref: '#/$defs/BlackLevelCorrection'
> +        - $ref: '#/$defs/Ccm'
> +        - $ref: '#/$defs/ColorProcessing'
> +        - $ref: '#/$defs/Dpf'
> +        - $ref: '#/$defs/Filter'
> +        - $ref: '#/$defs/GammaSensorLinearization'
> +        - $ref: '#/$defs/DefectPixelClusterCorrection'
> +        - $ref: '#/$defs/LensShadingCorrection'

I suppose these are just links to other sections in the schema so it
doesn't really apply to these directly, but I'm wondering if it's worth
consolidating these names somewhere so that we don't have to update it
in three places (tuning file parsing in C++, tuning file generator in
libtuning, and schema validation here). Although considering the former
two already duplicate and need manual synchronization of these names
maybe it's fine? Or maybe we need a new solution.

(Also if you meant for these to be in alphabetical order then
DefectPixelClusterCorrection is out of order)

> +$defs:
> +  Agc:
> +    type: object
> +    additionalProperties: false
> +    required: ['Agc']
> +    properties:
> +      Agc:
> +        type: ['object', 'null']
> +        additionalProperties: false
> +        required: ['AeMeteringMode']

I didn't think anything was required... and now I see the code that I
wrote does make it requied... I'll have to change that :)

> +        properties:
> +          AeMeteringMode:
> +            type: object
> +            additionalProperties: false
> +            required: ['MeteringCentreWeighted', 'MeteringSpot', 'MeteringMatrix']

I think oneOf is fine for metering mode? (Technically the tuning file parser
works fine with none but then what's the point)

> +            properties:

I'm not familiar at all with yaml schemas so idrk but you don't need
oneOf for maps?

> +              MeteringCentreWeighted: { $ref: '#/$defs/_AgcMeteringMode' }
> +              MeteringSpot: { $ref: '#/$defs/_AgcMeteringMode' }
> +              MeteringMatrix: { $ref: '#/$defs/_AgcMeteringMode' }
> +              MeteringCustom: { $ref: '#/$defs/_AgcMeteringMode' }
> +          AeExposureMode:
> +            type: object
> +            additionalProperties: false
> +            properties:
> +              ExposureNormal: { $ref: '#/$defs/_AgcExposureMode' }
> +              ExposureShort: { $ref: '#/$defs/_AgcExposureMode' }
> +              ExposureLong: { $ref: '#/$defs/_AgcExposureMode' }
> +              ExposureCustom: { $ref: '#/$defs/_AgcExposureMode' }
> +          AeConstraintMode:
> +            type: object
> +            additionalProperties: false
> +            properties:
> +              ConstraintNormal: { $ref: '#/$defs/_AgcConstraintMode' }
> +              ConstraintHighlight: { $ref: '#/$defs/_AgcConstraintMode' }
> +              ConstraintShadows: { $ref: '#/$defs/_AgcConstraintMode' }
> +              ConstraintCustom: { $ref: '#/$defs/_AgcConstraintMode' }
> +          relativeLuminanceTarget:
> +            type: array
> +            items:
> +              type: number
> +
> +  # Ideally these should be defined inside Agc and referenced using relative paths
> +  # but apparently this is not supported by the spec.
> +  # See https://github.com/python-jsonschema/jsonschema/issues/1265
> +  _AgcMeteringMode:
> +    type: array
> +    items:
> +      type: integer
> +    minItems: 25
> +    maxItems: 25

There's a new version that adds another level of map for v10 and v12...

It might've fell through the cracks because I removed the patch that
adds the tuning file itself.

https://patchwork.libcamera.org/patch/20141/

> +  _AgcConstraintMode:
> +    type: object
> +    additionalProperties: false
> +    properties:
> +      lower:
> +        type: object
> +        additionalProperties: false
> +        properties:
> +          qLo:
> +            type: number
> +          qHi:
> +            type: number
> +          yTarget:
> +            type: array
> +            items:
> +              type: number

This makes this patch dependent on v2 of "ipa: libipa: Change constraint
Y target to piecewise linear function".

Also it would be nice if there was a way to validate that the length is
a multiple of 2.

> +      upper:
> +        type: object
> +        additionalProperties: false
> +        properties:
> +          qLo:
> +            type: number
> +          qHi:
> +            type: number
> +          yTarget:
> +            type: array
> +            items:
> +              type: number
> +  _AgcExposureMode:
> +    type: object
> +    additionalProperties: false
> +    required: ['shutter', 'gain']
> +    properties:
> +      shutter:
> +        type: array
> +        items:
> +          type: integer
> +      gain:
> +        type: array
> +        items:
> +          type: number

Is there a way to validate that these two lengths match?

> +
> +  Awb:
> +    type: object
> +    additionalProperties: false
> +    required: ['Awb']
> +    properties:
> +      Awb: { $ref: '#/$defs/_Empty' }
> +  BlackLevelCorrection:
> +    type: object
> +    additionalProperties: false
> +    required: ['BlackLevelCorrection']
> +    properties:
> +      BlackLevelCorrection:
> +        type: object
> +        additionalProperties: false
> +        required: ['B', 'Gb', 'Gr', 'R']
> +        properties:
> +          B:
> +            type: integer
> +          Gb:
> +            type: integer
> +          Gr:
> +            type: integer
> +          R:
> +            type: integer
> +  Ccm:
> +    type: object
> +    additionalProperties: false
> +    required: ['Ccm']
> +    properties:
> +      Ccm:
> +        type: object
> +        additionalProperties: false
> +        required: ['ccms']
> +        properties:
> +          ccms:
> +            type: array
> +            items:
> +              type: object
> +              additionalProperties: false
> +              required: ['ct', 'ccm']
> +              properties:
> +                ct:
> +                  type: integer
> +                ccm:
> +                  type: array
> +                  items:
> +                    type: number
> +                  minItems: 9
> +                  maxItems: 9
> +                offsets:
> +                  type: array
> +                  items:
> +                    type: integer
> +                  minItems: 3
> +                  maxItems: 3
> +  ColorProcessing:
> +    type: object
> +    additionalProperties: false
> +    required: ['ColorProcessing']
> +    properties:
> +      ColorProcessing: { $ref: '#/$defs/_Empty' }
> +  DefectPixelClusterCorrection:
> +    type: object
> +    additionalProperties: false
> +    required: ['DefectPixelClusterCorrection']
> +    properties:
> +      DefectPixelClusterCorrection:
> +        type: object
> +        additionalProperties: false
> +        required: ['sets']
> +        properties:
> +          fixed-set:
> +            type: boolean
> +          sets:
> +            type: array
> +            maxItems: 3
> +            items:
> +              type: object
> +              additionalProperties: false
> +              # Todo clarify what is really required here ov5640 misses
> +              # 'rg-factor', 'rnd-offsets', 'rnd-threshold'

The documentation says what's required (they seem to not be the same for
all three sets), but I think we can probably just make these all
optional for the purposes of the schema (or at least at-least-one-of?).

> +              required: ['line-mad-factor', 'line-threshold', 'pg-factor', 'ro-limits']
> +              properties:
> +                line-mad-factor: { $ref: '#/$defs/_DefectPixelClusterCorrectionFactor' }
> +                line-threshold: { $ref: '#/$defs/_DefectPixelClusterCorrectionFactor' }
> +                pg-factor: { $ref: '#/$defs/_DefectPixelClusterCorrectionFactor' }
> +                rg-factor: { $ref: '#/$defs/_DefectPixelClusterCorrectionFactor' }
> +                rnd-offsets: { $ref: '#/$defs/_DefectPixelClusterCorrectionFactor' }
> +                rnd-threshold: { $ref: '#/$defs/_DefectPixelClusterCorrectionFactor' }
> +                ro-limits: { $ref: '#/$defs/_DefectPixelClusterCorrectionFactor' }
> +  _DefectPixelClusterCorrectionFactor:
> +    type: object
> +    additionalProperties: false
> +    properties:
> +      green:
> +        type: integer
> +      red-blue:
> +        type: integer
> +  Dpf:
> +    type: object
> +    additionalProperties: false
> +    required: ['Dpf']
> +    properties:
> +      Dpf:
> +        type: object
> +        additionalProperties: false
> +        required: ['DomainFilter', 'FilterStrength', 'NoiseLevelFunction']
> +        properties:
> +          DomainFilter:
> +            type: object
> +            additionalProperties: false
> +            required: ['g', 'rb']
> +            properties:
> +              g:
> +                type: array
> +                items:
> +                  type: integer
> +                minItems: 6
> +                maxItems: 6
> +              rb:
> +                type: array
> +                items:
> +                  type: integer
> +                minItems: 6
> +                maxItems: 6
> +          FilterStrength:
> +            type: object
> +            additionalProperties: false
> +            required: ['b', 'g', 'r']
> +            properties:
> +              b:
> +                type: integer
> +              g:
> +                type: integer
> +              r:
> +                type: integer
> +          NoiseLevelFunction:
> +            type: object
> +            additionalProperties: false
> +            required: ['coeff', 'scale-mode']
> +            properties:
> +              coeff:
> +                type: array
> +                items:
> +                  type: integer
> +                minItems: 17
> +                maxItems: 17
> +              scale-mode:
> +                type: string
> +                enum: ['linear', 'logarithmic']
> +  Filter:
> +    type: object
> +    additionalProperties: false
> +    required: ['Filter']
> +    properties:
> +      Filter: { $ref: '#/$defs/_Empty' }
> +  GammaSensorLinearization:
> +    type: object
> +    additionalProperties: false
> +    required: ['GammaSensorLinearization']
> +    properties:
> +      GammaSensorLinearization:
> +        type: object
> +        additionalProperties: false
> +        required: ['x-intervals', 'y']
> +        properties:
> +          x-intervals:
> +            type: array
> +            items:
> +              type: number

I think this should be integer as well.

> +            minItems: 16
> +            maxItems: 16
> +          y:
> +            type: object
> +            additionalProperties: false
> +            required: ['red', 'green', 'blue']
> +            properties:
> +              red:
> +                type: array
> +                items:
> +                  type: integer
> +                minItems: 17
> +                maxItems: 17
> +              green:
> +                type: array
> +                items:
> +                  type: integer
> +                minItems: 17
> +                maxItems: 17
> +              blue:
> +                type: array
> +                items:
> +                  type: integer
> +                minItems: 17
> +                maxItems: 17
> +  LensShadingCorrection:
> +    type: object
> +    additionalProperties: false
> +    required: ['LensShadingCorrection']
> +    properties:
> +      LensShadingCorrection:
> +        type: object
> +        additionalProperties: false
> +        required: [x-size, y-size, sets]
> +        properties:
> +          x-size:
> +            type: array
> +            items:
> +              type: number
> +            minItems: 8
> +            maxItems: 8
> +          y-size:
> +            type: array
> +            items:
> +              type: number
> +            minItems: 8
> +            maxItems: 8
> +          sets:
> +            type: array
> +            items:
> +              type: object
> +              additionalProperties: false
> +              required: ['ct', 'r', 'gr', 'gb', 'b']
> +              properties:
> +                ct:
> +                  type: integer
> +                r:
> +                  type: array
> +                  items:
> +                    type: integer
> +                  minItems: 289
> +                  maxItems: 289
> +                gr:
> +                  type: array
> +                  items:
> +                    type: integer
> +                  minItems: 289
> +                  maxItems: 289
> +                gb:
> +                  type: array
> +                  items:
> +                    type: integer
> +                  minItems: 289
> +                  maxItems: 289
> +                b:
> +                  type: array
> +                  items:
> +                    type: integer
> +                  minItems: 289
> +                  maxItems: 289
> +                # ToDo: We have some tuning files with that property. Is this needed?

Considering the lsc algorithm module in the rkisp1 IPA doesn't even
check for it, I don't think we need it.

> +                resolution:
> +                  type: string
> +  # special definition to denote algorithms that do not have any parameters
> +  _Empty:
> +    type: [ 'object', 'null' ]
> +    additionalProperties: false
> +    properties: {}
> +...
> diff --git a/utils/validate-tuning-files.py b/utils/validate-tuning-files.py
> new file mode 100755
> index 00000000..45f1d863
> --- /dev/null
> +++ b/utils/validate-tuning-files.py
> @@ -0,0 +1,112 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2024, Ideas on Board Oy
> +#
> +# Author: Stefan Klug <klug.stefan@ideasonboard.com>
> +#
> +# Validate tuning files against schema
> +
> +import argparse
> +import logging
> +import jsonschema
> +import yaml
> +import sys
> +import json
> +import os
> +from pathlib import Path

(alphabetical order)

> +
> +try:
> +    import coloredlogs
> +    coloredlogs.install(level=logging.INFO, fmt='%(levelname)s: %(message)s')
> +except ImportError:
> +    logging.basicConfig(level=logging.INFO,
> +                        format="%(levelname)s: %(message)s")

Oh that's fancy, I didn't realize this pattern.

> +
> +logger = logging.getLogger()
> +
> +
> +def do_schema_check(schema_file, files):
> +    res = True
> +    with open(schema_file, 'r') as file:
> +        schema = yaml.safe_load(file)
> +
> +    for file in files:
> +        with open(file, 'r') as f:
> +            logger.info(f"Validating file {file} against {schema_file}")
> +            data = yaml.safe_load(f)
> +            try:
> +                jsonschema.validate(instance=data, schema=schema)
> +            except jsonschema.exceptions.ValidationError as e:
> +                logging.error(f"Validation error in file {file}: {e}")
> +                res = False
> +    return res
> +
> +# Checks for the given ipa. If files is None, all files for that ipa get checked
> +
> +
> +def do_schema_check_ipa(ipa, files=None):
> +    res = True
> +
> +    if ipa != 'rkisp1':
> +        raise ValueError(f"Ipa '{ipa}' is not supported")
> +
> +    logger.info(f"Checking ipa {ipa}")
> +
> +    top_dir = os.path.dirname(os.path.dirname(os.path.abspath(__file__)))
> +    # Todo implement for other pipelines
> +    data_dir = 'src/ipa/rkisp1/data'
> +
> +    full_dir = Path(os.path.join(top_dir, data_dir))

Why not Path(top_dir) / Path(data_dir)?

> +
> +    schema_path = os.path.join(full_dir, f'tuning-{ipa}.schema.yaml')
> +    if not os.path.isfile(schema_path):
> +        raise ValueError(f"Schema file {schema_path} doesn't exist")
> +
> +    if files is None:
> +        files = [f for f in full_dir.glob(
> +            '*.yaml') if f.is_file() and not f.name.endswith('.schema.yaml')]

I was going to say instead of doing all this we could take the files as
parameters... and I see now that you do allow that and this is just a
convenience "all" option :)

I wonder if we need to check in some way that we're in a libcamera
tree or if os.path.isfile() is sufficient.

> +
> +    if not do_schema_check(schema_path, files):
> +        res = False
> +
> +    return res
> +
> +
> +def do_schema_check_all():
> +    # We only support rkisp1 for now
> +    return do_schema_check_ipa('rkisp1')
> +
> +
> +def main():
> +
> +    parser = argparse.ArgumentParser()
> +    group = parser.add_mutually_exclusive_group(required=True)
> +    group.add_argument('--all', '-a', action='store_true',
> +                       help='automatically find and check all tuning files.')
> +    group.add_argument('--schema', '-s',
> +                       help='schema file to check against')
> +    group.add_argument('--pipeline', '-p',
> +                       help='pipeline to check against (Currently only rkisp1 is supported)')
> +    parser.add_argument('files', metavar='FILE', nargs='*',
> +                        help='tuning files to check')
> +
> +    args = parser.parse_args()
> +
> +    if args.all:
> +        if args.files != []:
> +            parser.error("No files should be given when using --all")

Should we not error out here?


Thanks,

Paul

> +
> +        if not do_schema_check_all():
> +            return 1
> +        return 0
> +
> +    if not args.files:
> +        argparse.error("No files to check")
> +        return 1
> +
> +    if not do_schema_check(args.schema, args.files):
> +        return 1
> +
> +
> +if __name__ == '__main__':
> +    sys.exit(main())
> -- 
> 2.43.0
>