[libcamera-devel] [PATCH v2 03/10] libcamera: properties: Generate libcamera properties
Jacopo Mondi
jacopo at jmondi.org
Sun Dec 8 19:15:14 CET 2019
Hi Niklas,
On Fri, Dec 06, 2019 at 10:42:03PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2019-12-05 21:43:43 +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
>
> This is also hard for me to parse, how about
>
> 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.
>
I agree, thanks
> >
> > 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 would have split this commit in two and added the foreach loop in a
> separate patch.
Yes, but, really, why ?
Is this something logically not connected with the introduction of two
new files in the build system ? Is this hard to parse ?
Would you prefer one patch that introduce the new files, and one patch
that integrates them in the build system ? The first patch would not
even compile the newly added items...
>
> >
> > 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
>
> Same here the foreach conversion could be put together with the one
> above in a separate patch.
>
> > +
> > +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..6ab5be83bc49
> > --- /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:
> > + - 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
> > +
> > + - 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 writing this description is really hard. This is the best one I
> seen so far and not sure I can do better. Only missing peace of
> information is where 0 degrees rotation really means. On a hand held
> device is that if the camera of a handheld device is not rotated while
> holding it in portrait or landscape mode?
I think I'm missing the point that both you and Laurent rised here.
To me it's not a matter of the device where the camera in installed,
but how the camera sensor is supposed to be mounted. I'm -sure- it has
a designated upright position, with its pixel (0,0) in its top left
corner (or maybe not, but that depends on the sensor, and I'm sure it
is specified in the manual).
That said, this documentation comes from the DT bindings property I'm
pushing upstream, and while all those details on the sensor mounting
position are fair to be known at that level, for libcamera we might
need something different, I agree. I still don't get what's wrong with
the DT property description though (not the point you have rised).
Thanks
j
>
> > +...
> > --
> > 2.23.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20191208/4889a51b/attachment.sig>
More information about the libcamera-devel
mailing list