[libcamera-devel] [PATCH v3 03/10] libcamera: properties: Generate libcamera properties
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Dec 9 18:47:37 CET 2019
Hi Jacopo,
Thanks for your work.
On 2019-12-09 17:34:39 +0100, Jacopo Mondi wrote:
> Re-use the Control generation infrastructure to generate libcamera properties.
>
> Introduce three additional files:
> - include/libcamera/property_ids.h
> Defines the properties ids
>
> - src/libcamera/property_ids.cpp
> Defines the properties Control<> instances
>
> - src/libcamera/property_ids.yaml
> Provide properties definitions
>
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
> include/libcamera/meson.build | 26 +++++++++++------
> include/libcamera/property_ids.h.in | 33 ++++++++++++++++++++++
> src/libcamera/meson.build | 21 ++++++++------
> src/libcamera/property_ids.cpp.in | 43 +++++++++++++++++++++++++++++
> src/libcamera/property_ids.yaml | 34 +++++++++++++++++++++++
> 5 files changed, 140 insertions(+), 17 deletions(-)
> 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..25f503660960 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -23,15 +23,23 @@ install_headers(libcamera_api,
>
> gen_controls = files('../../src/libcamera/gen-controls.py')
>
> -control_ids_h = custom_target('control_ids_h',
> - input : files('../../src/libcamera/control_ids.yaml', 'control_ids.h.in'),
> - output : 'control_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
> +control_source_files = [
> + 'control_ids',
> + 'property_ids',
> +]
> +
> +control_headers = []
> +
> +foreach header : control_source_files
> + input_files = files('../../src/libcamera/' + header +'.yaml', header + '.h.in')
> + control_headers += custom_target(header + '_h',
> + input : input_files,
> + output : header + '.h',
> + depend_files : gen_controls,
> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],
> + install : true,
> + install_dir : join_paths('include', include_dir))
> +endforeach
I still think this should be split in two :-)
The reason I say that is not that I don't understand what you do, rather
that it takes me much longer time to review patches which do multiple
things.
Consider if this was split in two where the first changes the 'style'
and the commit message states that there is no functional change. As
soon as I verify there is no functional change and I like the direction
of the patch I will tag it and move along. In this case I notice
something is changing in the large chunk I need to understand what
changed and why, and for the rest of the patch I keep having this in the
back of my head that this change was needed in this patch consuming
review time.
In some cases it's unavoidable to have such patches but in this case
it's trivial to split it in two. I will not withhold my tag from patches
like this as I already spend the extra time understanding them so the
time is already consumed. But I will keep writing rants like this to
vent my frustration ;-)
>
> 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..14aff6e5fc13 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -67,14 +67,19 @@ endif
>
> gen_controls = files('gen-controls.py')
>
> -control_ids_cpp = custom_target('control_ids_cpp',
> - input : files('control_ids.yaml', 'control_ids.cpp.in'),
> - output : 'control_ids.cpp',
> - depend_files : gen_controls,
> - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])
> -
> -libcamera_sources += control_ids_cpp
> -libcamera_sources += control_ids_h
> +control_sources = []
> +
> +foreach source : control_source_files
> + input_files = files(source +'.yaml', source + '.cpp.in')
> + control_sources += custom_target(source + '_cpp',
> + input : input_files,
> + output : source + '.cpp',
> + depend_files : gen_controls,
> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])
> +endforeach
> +
> +libcamera_sources += control_headers
> +libcamera_sources += control_sources
>
> 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..bfdd823f63b0
> --- /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 properties
> + */
> +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..811c300c6b0a
> --- /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
> + enum:
> + - CameraLocationFront:
> + value: 0
> + description: |
> + The camera is mounted on the front side of the device, facing the
> + user
> + - CameraLocationBack:
> + value: 1
> + description: |
> + The camera is mounted on the back facing side of the device
> + - CameraLocationExternal:
> + value: 2
> + description: |
> + The camera is attached to the device in a way that allows it to
> + move freely
> +
> + - 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
I know this is the controversy in this series. I'm would find this
control much more useful if the description contained a definition on
how a camera is rotated if a read back 0 from this control. Is it
"level" in portrait or landscape mode then for example?
> +...
> --
> 2.24.0
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list