[libcamera-devel] [PATCH 03/10] libcamera: properties: Generate libcamera properties
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Dec 5 15:52:03 CET 2019
Hi Jacopo,
On Thu, Dec 05, 2019 at 11:22:39AM +0100, Jacopo Mondi wrote:
> On Wed, Dec 04, 2019 at 05:40:12PM +0200, Laurent Pinchart wrote:
> > 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 ?
>
> I would say yes, as we now have controls and properties, a metadata
> file would probably be the best option. We might end up unifying all
> of them one day if we'll have to, but having controls and properties
> seprately defined and metadata in one of the two does not make much
> sense to me.
>
> I'll give the above proposal a try.
>
> > >
> > > 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.
>
> Thanks
>
> > > 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.
>
> I'll fix
>
> > > 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.
>
> Good question... I tried thinking a bit of that, but I have no answer
> at the moment. Also, the three are semantically different things, so
> having them separate makes sense to me, even if a blanket term would
> help when referring to all of them. Android seems to use metadata,
> which to me is strictly related to information produced by the camera
> and associated with an image. V4L2 has control, which might work, but
> controls are the tunable part of a request as of now, so it's easy to
> confuse... We could s/controls/switches/ and use 'controls' as a
> generic term, but I'm not too excited by that.. what do you think ?
>
> Not sure to be honest. Let's keep the separate for the moment...
OK. Let's revisit it if we find a better term.
> > > + */
> > > +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
>
> I'll trust your judgment on the 'more yaml' part, I know very few
> things on that. I'll try to use this.
>
> > 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).
>
> To be honest, I don't see much value in this.. But at the same time I
> don't feel strong about 'values' either... If you prefer 'items' I can
> indeed use that one..
>
> > > +
> > > + - 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 ?
>
> Ah, that painful rotation matrix :)
>
> As per the discussion we had on the kernel side, I consider it an
> overkill for 2D cameras, and I would make it a property specific for
> 3D cameras while still having the simpler "rotation" for simpler use cases.
> As per the discussion on the kernel, having vendors provide a 9-values
> matrix just to say "my camera is mounted upside down" is not so nice
> in my opinion.
>
> Do you think having to deal with 2 properties might cause issues ?
As we're designing something front scratch, having to deal with a single
property could be nice. We would probably need to offer some helper for
applications to get the 'simple' orientation value from the rotation
matrix though. Maybe it's then overkill and we should just go for two
properties. Any third-party opinion ?
> > > +...
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list