[libcamera-devel] [PATCH v3 03/10] libcamera: properties: Generate libcamera properties
Jacopo Mondi
jacopo at jmondi.org
Tue Dec 10 09:33:31 CET 2019
Hi Niklas,
On Mon, Dec 09, 2019 at 06:47:37PM +0100, Niklas Söderlund wrote:
> 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
Why ? There are no new files to add, why change the style ?
> 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
Yes, and to know the direction you have to jump through the series
> 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 ;-)
>
It took you longer writing this rant down than reding that 20 lines
change. I feel like we're again arguing just for the sake of it, and I'm
kind of tired of that.
> >
> > 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?
>
Please read my reply to the previous version.
Thanks
j
> > +...
> > --
> > 2.24.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/20191210/a90374c8/attachment.sig>
More information about the libcamera-devel
mailing list