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

2414 diff mbox series

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

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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

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@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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Hello,
    following discussions on the 'rotation' property here defined, it
seems we're struggling to find consensus on its definition.

What I had here come from the kernel definition, and poorly applies to
a camera library control.

I'll here have another go, trying to list the possible use cases as
suggested by Laurent, in the attempt to latr find a generic enough
definition.

With this clarified, I'll re-send the series (almost fully reviewed)
along with compound controls support on top.

On Mon, Dec 09, 2019 at 05:34:39PM +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@jmondi.org>
> ---

[snip]

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


This comes straight from the DT property I am trying to push to kernel
space, but for libcamera it might be more opportune to define rotation
as the -correction- to apply to the captured images to obtain the best
possible orientation for the device the camera module is installed on.

I think what we're missing here is a definition of best possible
orientation that is generic and clear enough.

I'll try to list a few use cases
- phone/tablet
  Images should be presented to the user with their longer side
  aligned to the device's longer side, to minimize resizing/cropping.
- laptop (user facing camera)
  Images should be presented with the longer side parallel to the
  usage surface
- sport/action cameras
  The FOV should be maximed in its horizontal dimension

So it seems to me each use case has a different 'designated
orientation' and the rotation correction really depends on the device
intended usage. This rises the question, who sets this property ?
We're moving this to a CameraSensor subclass, while it really seems to
me this is -device- dependent (and yes, calls for a config file, I'm
sorry).

So I would go with:
"Rotation correction, expressed as counterclockwise rotation degrees
to apply to captured images to align their display orientation with
the device intended usage orientation. For mobile devices such as
phones or tablets, images are usually displayed with their longer side
aligned to the device longer edge."

How would that feel ?
Please note also the DT property and the proposed v4l2 control would
need some feedbacks, as ideally, the two series should go in more or
less at the same time.

Thanks
   j

Hi Jacopo,

Thank you for the patch.

On Fri, Jan 03, 2020 at 11:45:30AM +0100, Jacopo Mondi wrote:
> Hello,
>     following discussions on the 'rotation' property here defined, it
> seems we're struggling to find consensus on its definition.
> 
> What I had here come from the kernel definition, and poorly applies to
> a camera library control.
> 
> I'll here have another go, trying to list the possible use cases as
> suggested by Laurent, in the attempt to latr find a generic enough
> definition.
> 
> With this clarified, I'll re-send the series (almost fully reviewed)
> along with compound controls support on top.
> 
> On Mon, Dec 09, 2019 at 05:34:39PM +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@jmondi.org>
> > ---
> 
> [snip]
> 
> > +
> > +  - 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
> 
> This comes straight from the DT property I am trying to push to kernel
> space, but for libcamera it might be more opportune to define rotation
> as the -correction- to apply to the captured images to obtain the best
> possible orientation for the device the camera module is installed on.

This is what the Android camera HAL API does, but I don't think we can
do the same. The big difference is that an Android camera HAL
implementation can hardcode information about the device and its
display, while we don't have that luxury in libcamera.

Android defines the android.sensor.orientation property as

"Clockwise angle through which the output image needs to be rotated to
be upright on the device screen in its native orientation."

For all we know, we may not even have a screen :-) We thus need to
define the rotation relatively to the device but without involving its
screen (or at least not in the general case).

> I think what we're missing here is a definition of best possible
> orientation that is generic and clear enough.
> 
> I'll try to list a few use cases
> - phone/tablet
>   Images should be presented to the user with their longer side
>   aligned to the device's longer side, to minimize resizing/cropping.
> - laptop (user facing camera)
>   Images should be presented with the longer side parallel to the
>   usage surface
> - sport/action cameras
>   The FOV should be maximed in its horizontal dimension

This really depends on the sport, some are more vertically oriented
(although if they have to be shown on TV, horizontal orientation is
usually preferred).

You're very focussed on the end-user market in that list. How about
surveillance cameras, face scanners in security gates, computer vision
cameras for industrial applications, and even linear cameras (where the
sensor has a single line and infinitely long images are created by
moving a conveyor belt in front of the sensor) ?)

> So it seems to me each use case has a different 'designated
> orientation' and the rotation correction really depends on the device
> intended usage. This rises the question, who sets this property ?
> We're moving this to a CameraSensor subclass, while it really seems to
> me this is -device- dependent (and yes, calls for a config file, I'm
> sorry).

I still fully disagree :-) In any case that would be completely
impractical as nobody will be able to maintain an up-to-date database of
such configuration files for every platform out there, and we need a
solution for end-users.

> So I would go with:
> "Rotation correction, expressed as counterclockwise rotation degrees
> to apply to captured images to align their display orientation with
> the device intended usage orientation. For mobile devices such as
> phones or tablets, images are usually displayed with their longer side
> aligned to the device longer edge."
> 
> How would that feel ?

Let me try and make a proposal (unless somebody wants to beat me to it
:-)).

> Please note also the DT property and the proposed v4l2 control would
> need some feedbacks, as ideally, the two series should go in more or
> less at the same time.

Agreed, I think we need to come up with an agreement for the whole chain
(DT, V4L2 and libcamera) before merging any code.

Hi Laurent,

On Mon, Jan 06, 2020 at 11:35:59PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Jan 03, 2020 at 11:45:30AM +0100, Jacopo Mondi wrote:
> > Hello,
> >     following discussions on the 'rotation' property here defined, it
> > seems we're struggling to find consensus on its definition.
> >
> > What I had here come from the kernel definition, and poorly applies to
> > a camera library control.
> >
> > I'll here have another go, trying to list the possible use cases as
> > suggested by Laurent, in the attempt to latr find a generic enough
> > definition.
> >
> > With this clarified, I'll re-send the series (almost fully reviewed)
> > along with compound controls support on top.
> >
> > On Mon, Dec 09, 2019 at 05:34:39PM +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@jmondi.org>
> > > ---
> >
> > [snip]
> >
> > > +
> > > +  - 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
> >
> > This comes straight from the DT property I am trying to push to kernel
> > space, but for libcamera it might be more opportune to define rotation
> > as the -correction- to apply to the captured images to obtain the best
> > possible orientation for the device the camera module is installed on.
>
> This is what the Android camera HAL API does, but I don't think we can
> do the same. The big difference is that an Android camera HAL
> implementation can hardcode information about the device and its
> display, while we don't have that luxury in libcamera.
>
> Android defines the android.sensor.orientation property as
>
> "Clockwise angle through which the output image needs to be rotated to
> be upright on the device screen in its native orientation."
>
> For all we know, we may not even have a screen :-) We thus need to
> define the rotation relatively to the device but without involving its
> screen (or at least not in the general case).

I don't thin "best possible orientation" implies the usage of a
screen. At least, that's not what I meant with "

> > as the -correction- to apply to the captured images to obtain the best
> > possible orientation for the device the camera module is installed on.

>
> > I think what we're missing here is a definition of best possible
> > orientation that is generic and clear enough.
> >
> > I'll try to list a few use cases
> > - phone/tablet
> >   Images should be presented to the user with their longer side
> >   aligned to the device's longer side, to minimize resizing/cropping.
> > - laptop (user facing camera)
> >   Images should be presented with the longer side parallel to the
> >   usage surface
> > - sport/action cameras
> >   The FOV should be maximed in its horizontal dimension
>
> This really depends on the sport, some are more vertically oriented
> (although if they have to be shown on TV, horizontal orientation is
> usually preferred).
>
> You're very focussed on the end-user market in that list. How about
> surveillance cameras, face scanners in security gates, computer vision
> cameras for industrial applications, and even linear cameras (where the
> sensor has a single line and infinitely long images are created by
> moving a conveyor belt in front of the sensor) ?)

I wonder how can we possibly know that. Is the conveyor belt
horizontal ? Does it move vertically ? Are the pieces moved taller
than larger ? I'm still of the idea the best 'orientation' (which is
basically portrait vs landscape) depends on the use case.

>
> > So it seems to me each use case has a different 'designated
> > orientation' and the rotation correction really depends on the device
> > intended usage. This rises the question, who sets this property ?
> > We're moving this to a CameraSensor subclass, while it really seems to
> > me this is -device- dependent (and yes, calls for a config file, I'm
> > sorry).
>
> I still fully disagree :-) In any case that would be completely
> impractical as nobody will be able to maintain an up-to-date database of
> such configuration files for every platform out there, and we need a
> solution for end-users.
>

Why a database ? As and OEM you have to provide a file for your
device, why there should be a shared database ?

Otherwise we defer the decision to applications, but that kind of
defeat the whole purpose of generalizing this definition.

> > So I would go with:
> > "Rotation correction, expressed as counterclockwise rotation degrees
> > to apply to captured images to align their display orientation with
> > the device intended usage orientation. For mobile devices such as
> > phones or tablets, images are usually displayed with their longer side
> > aligned to the device longer edge."
> >
> > How would that feel ?
>
> Let me try and make a proposal (unless somebody wants to beat me to it
> :-)).
>
> > Please note also the DT property and the proposed v4l2 control would
> > need some feedbacks, as ideally, the two series should go in more or
> > less at the same time.
>
> Agreed, I think we need to come up with an agreement for the whole chain
> (DT, V4L2 and libcamera) before merging any code.
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Tue, Jan 07, 2020 at 09:39:31AM +0100, Jacopo Mondi wrote:
> On Mon, Jan 06, 2020 at 11:35:59PM +0200, Laurent Pinchart wrote:
> > On Fri, Jan 03, 2020 at 11:45:30AM +0100, Jacopo Mondi wrote:
> >> Hello,
> >>     following discussions on the 'rotation' property here defined, it
> >> seems we're struggling to find consensus on its definition.
> >>
> >> What I had here come from the kernel definition, and poorly applies to
> >> a camera library control.
> >>
> >> I'll here have another go, trying to list the possible use cases as
> >> suggested by Laurent, in the attempt to latr find a generic enough
> >> definition.
> >>
> >> With this clarified, I'll re-send the series (almost fully reviewed)
> >> along with compound controls support on top.
> >>
> >> On Mon, Dec 09, 2019 at 05:34:39PM +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@jmondi.org>
> >>> ---
> >>
> >> [snip]
> >>
> >>> +
> >>> +  - 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
> >>
> >> This comes straight from the DT property I am trying to push to kernel
> >> space, but for libcamera it might be more opportune to define rotation
> >> as the -correction- to apply to the captured images to obtain the best
> >> possible orientation for the device the camera module is installed on.
> >
> > This is what the Android camera HAL API does, but I don't think we can
> > do the same. The big difference is that an Android camera HAL
> > implementation can hardcode information about the device and its
> > display, while we don't have that luxury in libcamera.
> >
> > Android defines the android.sensor.orientation property as
> >
> > "Clockwise angle through which the output image needs to be rotated to
> > be upright on the device screen in its native orientation."
> >
> > For all we know, we may not even have a screen :-) We thus need to
> > define the rotation relatively to the device but without involving its
> > screen (or at least not in the general case).
> 
> I don't thin "best possible orientation" implies the usage of a
> screen. At least, that's not what I meant with "
> 
> >> as the -correction- to apply to the captured images to obtain the best
> >> possible orientation for the device the camera module is installed on.
> >>
> >> I think what we're missing here is a definition of best possible
> >> orientation that is generic and clear enough.
> >>
> >> I'll try to list a few use cases
> >> - phone/tablet
> >>   Images should be presented to the user with their longer side
> >>   aligned to the device's longer side, to minimize resizing/cropping.
> >> - laptop (user facing camera)
> >>   Images should be presented with the longer side parallel to the
> >>   usage surface
> >> - sport/action cameras
> >>   The FOV should be maximed in its horizontal dimension
> >
> > This really depends on the sport, some are more vertically oriented
> > (although if they have to be shown on TV, horizontal orientation is
> > usually preferred).
> >
> > You're very focussed on the end-user market in that list. How about
> > surveillance cameras, face scanners in security gates, computer vision
> > cameras for industrial applications, and even linear cameras (where the
> > sensor has a single line and infinitely long images are created by
> > moving a conveyor belt in front of the sensor) ?)
> 
> I wonder how can we possibly know that. Is the conveyor belt
> horizontal ? Does it move vertically ? Are the pieces moved taller
> than larger ? I'm still of the idea the best 'orientation' (which is
> basically portrait vs landscape) depends on the use case.

We've discussed this offline and you've posted a new version, so I won't
reply here has it would be rather long, but please let me know if any
unanswered question still needs a reply.

> >> So it seems to me each use case has a different 'designated
> >> orientation' and the rotation correction really depends on the device
> >> intended usage. This rises the question, who sets this property ?
> >> We're moving this to a CameraSensor subclass, while it really seems to
> >> me this is -device- dependent (and yes, calls for a config file, I'm
> >> sorry).
> >
> > I still fully disagree :-) In any case that would be completely
> > impractical as nobody will be able to maintain an up-to-date database of
> > such configuration files for every platform out there, and we need a
> > solution for end-users.
> 
> Why a database ? As and OEM you have to provide a file for your
> device, why there should be a shared database ?

For OEM devices shipping libcamera, that's not a problem. For other
devices where the end user will install libcamera (such as a laptop with
a regular Linux distribution), there will be no OEM writing
configuration files, and I don't think we can expect distributions to
assume this role either if the configuration has to be device-specific.

> Otherwise we defer the decision to applications, but that kind of
> defeat the whole purpose of generalizing this definition.
> 
> >> So I would go with:
> >> "Rotation correction, expressed as counterclockwise rotation degrees
> >> to apply to captured images to align their display orientation with
> >> the device intended usage orientation. For mobile devices such as
> >> phones or tablets, images are usually displayed with their longer side
> >> aligned to the device longer edge."
> >>
> >> How would that feel ?
> >
> > Let me try and make a proposal (unless somebody wants to beat me to it
> > :-)).
> >
> >> Please note also the DT property and the proposed v4l2 control would
> >> need some feedbacks, as ideally, the two series should go in more or
> >> less at the same time.
> >
> > Agreed, I think we need to come up with an agreement for the whole chain
> > (DT, V4L2 and libcamera) before merging any code.