[libcamera-devel,03/10] libcamera: properties: Generate libcamera properties

2387 diff mbox series

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@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

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@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 ?

> +...

Hi Laurent,

On Wed, Dec 04, 2019 at 05:40:12PM +0200, Laurent Pinchart wrote:
> 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@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...

> > + */
> > +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 ?

> > +...
>
> --
> Regards,
>
> Laurent Pinchart

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@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 ?

> > > +...