[libcamera-devel] [PATCH 03/10] libcamera: properties: Generate libcamera properties
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Dec 4 16:40:12 CET 2019
Hi Jacopo,
Thank you for the patch.
On Wed, Dec 04, 2019 at 02:20:59PM +0100, Jacopo Mondi wrote:
> Re-use the Control generation infrastructure to generate libcamera properties.
>
> Introduce three additional files, one that enumerates the properties ids
> (include/libcamera/property_ids.h) and one the defines Control<> instances,
> one for each property (src/libcamera/property_ids.cpp) and provide
> properties definitions in src/libcamera/property_ids.yaml
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> include/libcamera/meson.build | 9 ++++++
> include/libcamera/property_ids.h.in | 33 ++++++++++++++++++++++
> src/libcamera/meson.build | 6 ++++
> src/libcamera/property_ids.cpp.in | 43 +++++++++++++++++++++++++++++
> src/libcamera/property_ids.yaml | 34 +++++++++++++++++++++++
> 5 files changed, 125 insertions(+)
> create mode 100644 include/libcamera/property_ids.h.in
> create mode 100644 src/libcamera/property_ids.cpp.in
> create mode 100644 src/libcamera/property_ids.yaml
>
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 99abf0609940..0ec32ad84c96 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -31,7 +31,16 @@ control_ids_h = custom_target('control_ids_h',
> install : true,
> install_dir : join_paths('include', include_dir))
>
> +property_ids_h = custom_target('property_ids_h',
> + input : files('../../src/libcamera/property_ids.yaml', 'property_ids.h.in'),
It would be lovely if we could avoid the ../.., but that's not a problem
introduced by this patch. I wonder if it would make sense to move the
yaml files to a separate directory at some point. No need to care about
it now, it's just a random comment.
> + output : 'property_ids.h',
> + depend_files : gen_controls,
> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],
> + install : true,
> + install_dir : join_paths('include', include_dir))
> +
> libcamera_api += control_ids_h
> +libcamera_api += property_ids_h
If we want to follow the 0, 1, N rule, this could be
control_headers_sources = [
'control_ids.h',
'property_ids.h',
]
control_headers = []
foreach header : control_header_sources
inputs = files('../../src/libcamera/' + header + '.yaml', header + '.h.in')
control_headers += custom_target(header + '_h',
input : inputs,
output : header + '.h',
depend_files : gen_controls,
command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],
install : true,
install_dir : join_paths('include', include_dir))
endforeach
(untested) and something similar for the .cpp files. But maybe that's
overkill ? Do you think we'll have a third generated file for metadata ?
>
> gen_header = files('gen-header.sh')
>
> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in
> new file mode 100644
> index 000000000000..62799b3e8c54
> --- /dev/null
> +++ b/include/libcamera/property_ids.h.in
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * property_ids.h : Property ID list
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#ifndef __LIBCAMERA_PROPERTY_IDS_H__
> +#define __LIBCAMERA_PROPERTY_IDS_H__
> +
> +#include <stdint.h>
> +
> +#include <libcamera/controls.h>
> +
> +namespace libcamera {
> +
> +namespace properties {
> +
> +enum {
> +${ids}
> +};
> +
> +${controls}
> +
> +extern const ControlIdMap properties;
> +
> +} /* namespace propertiess */
> +
> +} /* namespace libcamera */
> +
> +#endif // __LIBCAMERA_PROPERTY_IDS_H__
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index c4f965bd7413..abd7046bd95d 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -73,7 +73,13 @@ control_ids_cpp = custom_target('control_ids_cpp',
> depend_files : gen_controls,
> command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])
>
> +property_ids_cpp = custom_target('property_ids_cpp',
> + input : files('property_ids.yaml', 'property_ids.cpp.in'),
> + output : 'property_ids.cpp',
> + depend_files : gen_controls,
> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])
Missing blank line.
> libcamera_sources += control_ids_cpp
> +libcamera_sources += property_ids_cpp
> libcamera_sources += control_ids_h
Shouldn't you add property_ids_h here ? And let's keep them
alphabetically sorted.
>
> gen_version = join_paths(meson.source_root(), 'utils', 'gen-version.sh')
> diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in
> new file mode 100644
> index 000000000000..635a56f7d647
> --- /dev/null
> +++ b/src/libcamera/property_ids.cpp.in
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2019, Google Inc.
> + *
> + * property_ids.cpp : Property ID list
> + *
> + * This file is auto-generated. Do not edit.
> + */
> +
> +#include <libcamera/property_ids.h>
> +
> +/**
> + * \file property_ids.h
> + * \brief Camera property identifiers
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \brief Namespace for libcamera controls
s/controls/properties/
I wonder if we could find a single term that would encompass what we
call controls, properties, and later metadata.
> + */
> +namespace properties {
> +
> +${controls_doc}
> +
> +#ifndef __DOXYGEN__
> +/*
> + * Keep the properties definitions hidden from doxygen as it incorrectly parses
> + * them as functions.
> + */
> +${controls_def}
> +#endif
> +
> +/**
> + * \brief List of all supported libcamera properties
> + */
> +extern const ControlIdMap properties {
> +${controls_map}
> +};
> +
> +} /* namespace properties */
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml
> new file mode 100644
> index 000000000000..61be2ab5298c
> --- /dev/null
> +++ b/src/libcamera/property_ids.yaml
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: LGPL-2.1-or-later
> +#
> +# Copyright (C) 2019, Google Inc.
> +#
> +%YAML 1.2
> +---
> +controls:
> + - Location:
> + type: int32_t
> + description: |
> + Camera mounting location
> + values:
> + - name: "CAMERA_LOCATION_FRONT"
> + value: 0
> + description: |
> + The camera is mounted on the front side of the device, facing the
> + user
> + - name: "CAMERA_LOCATION_BACK"
> + value: 1
> + description: |
> + The camera is mounted on the back facing side of the device
> + - name: "CAMERA_LOCATION_EXTERNAL"
> + value: 2
> + description: |
> + The camera is attached to the device in a way that allows it to
> + move freely
I think we can make this a bit more yaml by encoding the name directly
as the key:
values:
- CAMERA_LOCATION_FRONT:
value: 0
description: |
The camera is mounted on the front side of the device, facing the
user
- CAMERA_LOCATION_BACK:
value: 1
description: |
The camera is mounted on the back facing side of the device
- CAMERA_LOCATION_EXTERNAL
value: 2
description: |
The camera is attached to the device in a way that allows it to
move freely
What do you think ? "values" could also be renamed to "items" to match
the naming used in DT bindings (I know they're unrelated, but there may
be a value in using a similar scheme where applicable).
> +
> + - Rotation:
> + type: int32_t
> + description: |
> + Camera mounting rotation expressed as counterclockwise rotation degrees
> + towards the axis perpendicular to the sensor surface and directed away
> + from it
Given that we will have to expose more precise information in order to
support depth-related cameras later, would it make sense to already go
for a full rotation matrix ?
> +...
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list