[libcamera-devel,PATCH/RFC,0/2] libcamera: Add Control instances for V4L2 controls
mbox series

Message ID 20210208225429.31627-1-laurent.pinchart@ideasonboard.com
Headers show
Series
  • libcamera: Add Control instances for V4L2 controls
Related show

Message

Laurent Pinchart Feb. 8, 2021, 10:54 p.m. UTC 
Hello,

This patch series attempts to extend Control generation for V4L2
controls. I've been toying with the idea for some time and wondered how
it would look like, we now have one answer to that question.

The patches are quite self-explicit, so I won't repeat their commit
messages here. The tentative improvements are in patch 2/2, and its
commit message lists remaining issues and ideas for potential further
improvements. I'm not entirely convinced with the end result yet, but
maybe that's because this is only an RFC and doesn't handle the RPi
controls yet.

Laurent Pinchart (2):
  libcamera: Generate Control instances for V4L2 controls
  libcamera: Use V4L2 Control instances

 include/libcamera/internal/meson.build        |  29 +++-
 .../libcamera/internal/v4l2_control_ids.h.in  |  29 ++++
 include/libcamera/internal/v4l2_controls.h    |   2 +
 src/ipa/ipu3/ipu3.cpp                         |  10 +-
 src/ipa/raspberrypi/raspberrypi.cpp           |  25 ++--
 src/ipa/rkisp1/rkisp1.cpp                     |  10 +-
 src/libcamera/camera_sensor.cpp               |  36 ++---
 src/libcamera/meson.build                     |   8 +-
 src/libcamera/pipeline/ipu3/ipu3.cpp          |   2 +-
 .../pipeline/raspberrypi/raspberrypi.cpp      |  16 +--
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp  |  29 ++--
 src/libcamera/pipeline/vimc/vimc.cpp          |  10 +-
 src/libcamera/v4l2_control_ids.cpp.in         |  34 +++++
 src/libcamera/v4l2_control_ids.yaml           |  62 +++++++++
 utils/gen-v4l2-controls.py                    | 126 ++++++++++++++++++
 utils/meson.build                             |   1 +
 16 files changed, 351 insertions(+), 78 deletions(-)
 create mode 100644 include/libcamera/internal/v4l2_control_ids.h.in
 create mode 100644 src/libcamera/v4l2_control_ids.cpp.in
 create mode 100644 src/libcamera/v4l2_control_ids.yaml
 create mode 100755 utils/gen-v4l2-controls.py

Comments

Kieran Bingham Feb. 11, 2021, 2:41 p.m. UTC | #1 
Hi Laurent,

On 08/02/2021 22:54, Laurent Pinchart wrote:
> The ControlList class is used for both libcamera controls and V4L2
> controls, with very different APIs. For libcamera controls, all controls
> have a corresponding Control instance, which can be used to access a
> ControlList with a type-safe API. For V4L2 controls, on the other hand,
> only numerical IDs are available, leading to the usage of an unsafe API.
> 
> Improve handling of V4L2 controls by generating Control instances for
> each V4L2 control used by libcamera, using a similar code generation
> mechanism as for libcamera controls.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  include/libcamera/internal/meson.build        |  29 +++-
>  .../libcamera/internal/v4l2_control_ids.h.in  |  29 ++++
>  include/libcamera/internal/v4l2_controls.h    |   2 +
>  src/libcamera/meson.build                     |   8 +-
>  src/libcamera/v4l2_control_ids.cpp.in         |  34 +++++
>  src/libcamera/v4l2_control_ids.yaml           |  62 +++++++++
>  utils/gen-v4l2-controls.py                    | 126 ++++++++++++++++++
>  utils/meson.build                             |   1 +
>  8 files changed, 283 insertions(+), 8 deletions(-)
>  create mode 100644 include/libcamera/internal/v4l2_control_ids.h.in
>  create mode 100644 src/libcamera/v4l2_control_ids.cpp.in
>  create mode 100644 src/libcamera/v4l2_control_ids.yaml
>  create mode 100755 utils/gen-v4l2-controls.py
> 
> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build
> index a1071721967e..f96eec9602d8 100644
> --- a/include/libcamera/internal/meson.build
> +++ b/include/libcamera/internal/meson.build
> @@ -2,13 +2,6 @@
>  
>  subdir('tracepoints')
>  
> -libcamera_tracepoint_header = custom_target(
> -    'tp_header',
> -    input: ['tracepoints.h.in', tracepoint_files],
> -    output: 'tracepoints.h',
> -    command: [gen_tracepoints_header, '@OUTPUT@', '@INPUT@'],
> -)
> -
>  libcamera_internal_headers = files([
>      'bayer_format.h',
>      'buffer.h',
> @@ -49,3 +42,25 @@ libcamera_internal_headers = files([
>      'v4l2_subdevice.h',
>      'v4l2_videodevice.h',
>  ])
> +
> +#
> +# Generate headers from templates.
> +#
> +
> +v4l2_control_header = custom_target(
> +    'v4l2_control_ids_h',
> +    input : files('../../../src/libcamera/v4l2_control_ids.yaml', 'v4l2_control_ids.h.in'),
> +    output : 'v4l2_control_ids.h',
> +    command : [gen_v4l2_controls, '-o', '@OUTPUT@', '@INPUT@'],
> +)
> +
> +libcamera_internal_headers += v4l2_control_header
> +
> +tracepoint_header = custom_target(
> +    'tp_header',
> +    input: ['tracepoints.h.in', tracepoint_files],
> +    output: 'tracepoints.h',
> +    command: [gen_tracepoints_header, '@OUTPUT@', '@INPUT@'],
> +)
> +
> +libcamera_internal_headers += tracepoint_header
> diff --git a/include/libcamera/internal/v4l2_control_ids.h.in b/include/libcamera/internal/v4l2_control_ids.h.in
> new file mode 100644
> index 000000000000..3f00449c9adf
> --- /dev/null
> +++ b/include/libcamera/internal/v4l2_control_ids.h.in
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2021, Google Inc.
> + *
> + * v4l2_control_ids.h - V4L2 Control ID list
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#ifndef __LIBCAMERA_V4L2_CONTROL_IDS_H__
> +#define __LIBCAMERA_V4L2_CONTROL_IDS_H__
> +
> +#include <stdint.h>
> +
> +#include <libcamera/controls.h>
> +
> +namespace libcamera {
> +
> +namespace v4l2 {
> +
> +#ifndef __DOXYGEN__
> +${controls}
> +#endif
> +
> +} /* namespace v4l2 */
> +
> +} /* namespace libcamera */
> +
> +#endif /* __LIBCAMERA_V4L2_CONTROL_IDS_H__ */
> diff --git a/include/libcamera/internal/v4l2_controls.h b/include/libcamera/internal/v4l2_controls.h
> index 0851b8ddb128..c54aed00cfae 100644
> --- a/include/libcamera/internal/v4l2_controls.h
> +++ b/include/libcamera/internal/v4l2_controls.h
> @@ -12,6 +12,8 @@
>  
>  #include <libcamera/controls.h>
>  
> +#include "libcamera/internal/v4l2_control_ids.h"
> +
>  namespace libcamera {
>  
>  class V4L2ControlId : public ControlId
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index ebce19d90c1e..36f79e11e1ac 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -57,7 +57,7 @@ libcamera_sources = files([
>  ])
>  
>  libcamera_sources += libcamera_public_headers
> -libcamera_sources += libcamera_tracepoint_header
> +libcamera_sources += libcamera_internal_headers
>  
>  includes = [
>      libcamera_includes,
> @@ -97,6 +97,12 @@ foreach source : control_source_files
>                                       command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])
>  endforeach
>  
> +control_sources += custom_target('v4l2_control_ids_cpp',
> +                                 input : files('v4l2_control_ids.yaml',
> +                                               'v4l2_control_ids.cpp.in'),
> +                                 output : 'v4l2_control_ids.cpp',
> +                                 command : [gen_v4l2_controls, '-o', '@OUTPUT@', '@INPUT@'])
> +
>  libcamera_sources += control_sources
>  
>  gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')
> diff --git a/src/libcamera/v4l2_control_ids.cpp.in b/src/libcamera/v4l2_control_ids.cpp.in
> new file mode 100644
> index 000000000000..6c57a5b0e4eb
> --- /dev/null
> +++ b/src/libcamera/v4l2_control_ids.cpp.in
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2031, Google Inc.

Please please please let me borrow your time machine ...

I promise I will only use it for ... good investments.

> + *
> + * v4l2_control_ids.cpp : V4L2 Control ID list
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#include <linux/v4l2-controls.h>
> +
> +#include <libcamera/controls.h>
> +
> +#include "libcamera/internal/v4l2_control_ids.h"
> +
> +/**
> + * \file v4l2_control_ids.h
> + * \brief V4L2 control identifiers
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \brief Namespace for V4L2 controls
> + */
> +namespace v4l2 {
> +
> +#ifndef __DOXYGEN__
> +${controls_def}
> +#endif
> +
> +} /* namespace v4l2 */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/v4l2_control_ids.yaml b/src/libcamera/v4l2_control_ids.yaml
> new file mode 100644
> index 000000000000..ec0adb21695f
> --- /dev/null
> +++ b/src/libcamera/v4l2_control_ids.yaml
> @@ -0,0 +1,62 @@
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +#
> +# Copyright (C) 2021, Google Inc.
> +#
> +%YAML 1.2
> +---
> +
> +controls:
> +  - V4L2_CID_ANALOGUE_GAIN:
> +      type: int32_t
> +
> +  - V4L2_CID_BLUE_BALANCE:
> +      type: int32_t
> +
> +  - V4L2_CID_BRIGHTNESS:
> +      type: int32_t
> +
> +  - V4L2_CID_CAMERA_ORIENTATION:
> +      type: int32_t
> +
> +  - V4L2_CID_CAMERA_SENSOR_ROTATION:
> +      type: int32_t
> +
> +  - V4L2_CID_CONTRAST:
> +      type: int32_t
> +
> +  - V4L2_CID_DIGITAL_GAIN:
> +      type: int32_t
> +
> +  - V4L2_CID_EXPOSURE:
> +      type: int32_t
> +
> +  - V4L2_CID_EXPOSURE_ABSOLUTE:
> +      type: int32_t
> +
> +  - V4L2_CID_EXPOSURE_AUTO:
> +      type: int32_t
> +
> +  - V4L2_CID_GAIN:
> +      type: int32_t
> +
> +  - V4L2_CID_HBLANK:
> +      type: int32_t
> +
> +  - V4L2_CID_HFLIP:
> +      type: int32_t
> +
> +  - V4L2_CID_PIXEL_RATE:
> +      type: int64_t
> +
> +  - V4L2_CID_RED_BALANCE:
> +      type: int32_t
> +
> +  - V4L2_CID_SATURATION:
> +      type: int32_t
> +
> +  - V4L2_CID_VBLANK:
> +      type: int32_t
> +
> +  - V4L2_CID_VFLIP:
> +      type: int32_t

My earlier concern about this is that now we need to have a specific
definition for every v4l2 control, but perhaps there's nothing wrong
with adding a new declaration to this list when it's required.

It's not public API so it's internal only, which even means the ordering
can change to keep the list sorted.

Otherwise, we could generate the list from the v4l2 header -but that
then defines more controls than are needed.

So ... this probably works.


This also means we have a centralised list of V4L2 controls to
reference, and from the looks of 2/2 means we don't have to directly
pull in v4l2 kernel headers into IPAs which feels like a bit of a win.

I'm not 100% sure I like the idea of 'wrapping' V4L2_CID_EXPOSURE to
v4l2::EXPOSURE, because I'm weary that the name change (or rather prefix
substitution) would make it harder to find references to specific
controls but ... I can't see how that would actually be a problem yet.

I'd be quite happy to see  this progress, so as I expect it is already
functional:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>



> +...
> diff --git a/utils/gen-v4l2-controls.py b/utils/gen-v4l2-controls.py
> new file mode 100755
> index 000000000000..2eb964f6f459
> --- /dev/null
> +++ b/utils/gen-v4l2-controls.py
> @@ -0,0 +1,126 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +# Copyright (C) 2021, Google Inc.
> +#
> +# Author: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +#
> +# gen-v4l2-controls.py - Generate V4L2 control definitions from YAML
> +
> +import argparse
> +import string
> +import sys
> +import yaml
> +
> +PREFIX = 'V4L2_CID_'
> +
> +
> +def generate_cpp(controls):
> +    def_template = string.Template('extern const Control<${type}> ${name}(${id_name}, "${name}");')
> +
> +    ctrls_def = []
> +
> +    for ctrl in controls:
> +        name, ctrl = ctrl.popitem()
> +
> +        if not name.startswith(PREFIX):
> +            raise RuntimeError(f'Invalid control name {name}')
> +
> +        id_name = name
> +        name = name[len(PREFIX):]
> +
> +        ctrl_type = ctrl['type']
> +        if ctrl_type == 'string':
> +            ctrl_type = 'std::string'
> +        elif ctrl.get('size'):
> +            ctrl_type = 'Span<const %s>' % ctrl_type
> +
> +        info = {
> +            'name': name,
> +            'type': ctrl_type,
> +            'id_name': id_name,
> +        }
> +
> +        ctrls_def.append(def_template.substitute(info))
> +
> +    return {
> +        'controls_def': '\n'.join(ctrls_def),
> +    }
> +
> +
> +def generate_h(controls):
> +    template = string.Template('''extern const Control<${type}> ${name};''')
> +
> +    ctrls = []
> +    draft_ctrls = []
> +    ids = []
> +
> +    for ctrl in controls:
> +        name, ctrl = ctrl.popitem()
> +
> +        if not name.startswith(PREFIX):
> +            raise RuntimeError(f'Invalid control name {name}')
> +
> +        name = name[len(PREFIX):]
> +
> +        ctrl_type = ctrl['type']
> +        if ctrl_type == 'string':
> +            ctrl_type = 'std::string'
> +        elif ctrl.get('size'):
> +            ctrl_type = 'Span<const %s>' % ctrl_type
> +
> +        info = {
> +            'name': name,
> +            'type': ctrl_type,
> +        }
> +
> +        ctrls.append(template.substitute(info))
> +
> +    return {
> +        'controls': '\n'.join(ctrls),
> +    }
> +
> +
> +def fill_template(template, data):
> +
> +    template = open(template, 'rb').read()
> +    template = template.decode('utf-8')
> +    template = string.Template(template)
> +    return template.substitute(data)
> +
> +
> +def main(argv):
> +
> +    # Parse command line arguments
> +    parser = argparse.ArgumentParser()
> +    parser.add_argument('-o', dest='output', metavar='file', type=str,
> +                        help='Output file name. Defaults to standard output if not specified.')
> +    parser.add_argument('input', type=str,
> +                        help='Input file name.')
> +    parser.add_argument('template', type=str,
> +                        help='Template file name.')
> +    args = parser.parse_args(argv[1:])
> +
> +    data = open(args.input, 'rb').read()
> +    controls = yaml.safe_load(data)['controls']
> +
> +    if args.template.endswith('.cpp.in'):
> +        data = generate_cpp(controls)
> +    elif args.template.endswith('.h.in'):
> +        data = generate_h(controls)
> +    else:
> +        raise RuntimeError('Unknown template type')
> +
> +    data = fill_template(args.template, data)
> +
> +    if args.output:
> +        output = open(args.output, 'wb')
> +        output.write(data.encode('utf-8'))
> +        output.close()
> +    else:
> +        sys.stdout.write(data)
> +
> +    return 0
> +
> +
> +if __name__ == '__main__':
> +    sys.exit(main(sys.argv))
> diff --git a/utils/meson.build b/utils/meson.build
> index 8e28ada7165a..8ff1e40dbe81 100644
> --- a/utils/meson.build
> +++ b/utils/meson.build
> @@ -9,6 +9,7 @@ py_modules += ['yaml']
>  gen_controls = files('gen-controls.py')
>  gen_formats = files('gen-formats.py')
>  gen_header = files('gen-header.sh')
> +gen_v4l2_controls = files('gen-v4l2-controls.py')
>  
>  ## Module signing
>  gen_ipa_priv_key = files('gen-ipa-priv-key.sh')
>