[libcamera-devel,v5,02/12] libcamera: camera: Introduce Orientation

18965 diff mbox series

Introduce the Orientation enumeration which describes the possible 2D
transformations that can be applied to an image using two basic plane
transformations.

Add to the CameraConfiguration class a new member 'orientation' which is
used to specify the image orientation in the memory buffers delivered to
applications.

The enumeration values follow the ones defined by the EXIF specification at
revision 2.32, Tag 274 'orientation'.

The newly introduced field is meant to replace
CameraConfiguration::transform which is not removed yet not to break
compilation.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/camera.h      |  2 +
 include/libcamera/meson.build   |  1 +
 include/libcamera/orientation.h | 28 ++++++++++++
 src/libcamera/camera.cpp        | 18 +++++++-
 src/libcamera/meson.build       |  1 +
 src/libcamera/orientation.cpp   | 78 +++++++++++++++++++++++++++++++++
 6 files changed, 127 insertions(+), 1 deletion(-)
 create mode 100644 include/libcamera/orientation.h
 create mode 100644 src/libcamera/orientation.cpp

Hi Jacopo,

Thank you for the patch.

On Fri, Sep 01, 2023 at 05:02:05PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Introduce the Orientation enumeration which describes the possible 2D
> transformations that can be applied to an image using two basic plane
> transformations.
> 
> Add to the CameraConfiguration class a new member 'orientation' which is
> used to specify the image orientation in the memory buffers delivered to
> applications.
> 
> The enumeration values follow the ones defined by the EXIF specification at
> revision 2.32, Tag 274 'orientation'.
> 
> The newly introduced field is meant to replace
> CameraConfiguration::transform which is not removed yet not to break
> compilation.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/camera.h      |  2 +
>  include/libcamera/meson.build   |  1 +
>  include/libcamera/orientation.h | 28 ++++++++++++
>  src/libcamera/camera.cpp        | 18 +++++++-
>  src/libcamera/meson.build       |  1 +
>  src/libcamera/orientation.cpp   | 78 +++++++++++++++++++++++++++++++++
>  6 files changed, 127 insertions(+), 1 deletion(-)
>  create mode 100644 include/libcamera/orientation.h
>  create mode 100644 src/libcamera/orientation.cpp
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 004bc89455f5..6e9342773962 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -19,6 +19,7 @@
>  #include <libcamera/base/signal.h>
>  
>  #include <libcamera/controls.h>
> +#include <libcamera/orientation.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  #include <libcamera/transform.h>
> @@ -67,6 +68,7 @@ public:
>  	std::size_t size() const;
>  
>  	Transform transform;
> +	Orientation orientation;
>  
>  protected:
>  	CameraConfiguration();
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index 408b7acf152c..a24c50d66a82 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -12,6 +12,7 @@ libcamera_public_headers = files([
>      'framebuffer_allocator.h',
>      'geometry.h',
>      'logging.h',
> +    'orientation.h',
>      'pixel_format.h',
>      'request.h',
>      'stream.h',
> diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h
> new file mode 100644
> index 000000000000..63ac4aba07ce
> --- /dev/null
> +++ b/include/libcamera/orientation.h
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Ideas On Board Oy
> + *
> + * orientation.h - Image orientation
> + */
> +
> +#pragma once
> +
> +#include <iostream>
> +
> +namespace libcamera {
> +
> +enum class Orientation {
> +	/* EXIF tag 274 starts from '1' */
> +	rotate0 = 1,

Enumerators should start with an uppercase letter.

> +	rotate0Flip,
> +	rotate180,
> +	rotate180Flip,
> +	rotate90Flip,
> +	rotate270,

I think this should be rotate90.

> +	rotate270Flip,
> +	rotate90,

And here, rotate270.

> +};
> +
> +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);
> +
> +} /* namespace libcamera */
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 0eecee766f00..d4ad4a553752 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)
>   * \brief Create an empty camera configuration
>   */
>  CameraConfiguration::CameraConfiguration()
> -	: transform(Transform::Identity), config_({})
> +	: transform(Transform::Identity), orientation(Orientation::rotate0),
> +	  config_({})
>  {
>  }
>  
> @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>   * may adjust this field at its discretion if the selection is not supported.
>   */
>  
> +/**
> + * \var CameraConfiguration::orientation
> + * \brief The desired orientation of the images produced by the camera
> + *
> + * The orientation field is a user-specified 2D plane transformation that

s/field/member/

The word "field" in the C++ specification is used for bit-field only.
"fields" of a class or structure are named "members" (or "data member"
if you want to differentiate them from member functions).

> + * specifies how the application wants the camera images to be rotated in
> + * the memory buffers.

As the whole point of this series is to move from transform to
orientation, can we try to avoid using "transformation" to define the
orientation ? How about the following ?

 * The orientation specifies how the application wants images to be oriented in
 * memory buffers. The camera will rotate and flip images as appropriate to
 * obtain the desired orientation.

> + *
> + * If the application requested orientation cannot be obtained the validate()

s/obtained/obtained,/

> + * function will Adjust this field to the actual orientation of the images

s/this field/the orientation member/

> + * as produced by the camera pipeline.

I recall we discussed that, in this case, the camera shouldn't try to
pick a value it considers the closest to the requested orientation, but
don't transform the image at all. Is that right ? If so, let's document
it explicitly:

 * If the orientation requested by the application cannot be obtained, the
 * camera will not rotate or flip the images, and the validate() function will
 * Adjust this value to the native image orientation produced by the camera.

> + *
> + * By default the orientation field is set to Orientation::rotate0.

s/field/member/

> + */
> +
>  /**
>   * \var CameraConfiguration::config_
>   * \brief The vector of stream configurations
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index b24f82965764..d0e26f6b4141 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -34,6 +34,7 @@ libcamera_sources = files([
>      'mapped_framebuffer.cpp',
>      'media_device.cpp',
>      'media_object.cpp',
> +    'orientation.cpp',
>      'pipeline_handler.cpp',
>      'pixel_format.cpp',
>      'process.cpp',
> diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp
> new file mode 100644
> index 000000000000..f2ee14dd4182
> --- /dev/null
> +++ b/src/libcamera/orientation.cpp
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2023, Ideas On Board Oy
> + *
> + * orientation.cpp - Image orientation
> + */
> +
> +#include <libcamera/orientation.h>
> +
> +#include <array>
> +#include <string>
> +
> +/**
> + * \file libcamera/orientation.h
> + * \brief Image orientation definition
> + */
> +
> +namespace libcamera {
> +
> +/**
> + * \enum Orientation
> + * \brief The image orientation in a memory buffer
> + *
> + * The Orientation enumeration describes the orientation of the images
> + * produced by the camera pipeline as they get received by the application
> + * inside memory buffers.
> + *

From here to ...

> + * All the possible 2D plane transformations of an image are expressed as the
> + * combination of two basic operations:
> + *
> + *   'a': rotate the image 90 degrees clockwise
> + *   'b': flip the image horizontally (mirroring)
> + *
> + * plus an identity operator 'e'.
> + *
> + * The composition of operations 'a' and 'b' according to the canonical function
> + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied
> + * next) gives origin to a symmetric group of order 8, or dihedral group.
> + *
> + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg
> + * as an example of the 2D plane transformation and how they originate from
> + * the composition of the 'a' and 'b' operations.
> + *
> + * The image orientation expressed using the Orientation enumeration can be
> + * then inferred by applying a multiple of a 90 degrees rotation from the origin
> + * and then applying any horizontal mirroring to a base image assume to be
> + * naturally oriented (no rotation and no mirroring applied).

... here, is this a leftover of a previous implementation, or copied
from somewhere else ? It doesn't seem to match the enumeration.

> + *
> + * The enumeration numerical values follow the ones defined by the EXIF
> + * Specification version 2.32, Tag 274 "Orientation", while the names of the
> + * enumerated values report the rotation and mirroring operations performed.
> + *
> + * In example Orientation::rotate90Flip describes the image transformation

s/In example/For example,/

> + * obtained by rotating 90 degrees clockwise first and then applying an
> + * horizontal mirroring.
> + */
> +
> +/**
> + * \brief Prints human-friendly names for Orientation items

Let's be consistent with the other operator<<() implementations:

 * \brief Insert a text representation of an Orientation into an output stream

> + * \param[in] out The output stream
> + * \param[in] orientation The Orientation item

s/ item//

> + * \return The output stream \a out
> + */
> +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)
> +{
> +	constexpr std::array<const char *, 9> orientationNames = {
> +		"", /* Orientation starts counting from 1. */
> +		"rotate0", "rotate0Flip",
> +		"rotate180", "rotate180Flip",
> +		"rotate90Flip", "rotate270",
> +		"rotate270Flip", "rotate90",
> +	};
> +
> +	out << orientationNames[static_cast<unsigned int>(orientation)];

You can write

	/* The Orientation enumeration starts counting from 1. */
	out << orientationNames[static_cast<unsigned int>(orientation) - 1];

And drop the first entry from the array.

> +	return out;
> +}
> +
> +} /* namespace libcamera */

Hi Laurent

On Wed, Oct 18, 2023 at 09:37:10PM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Sep 01, 2023 at 05:02:05PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > Introduce the Orientation enumeration which describes the possible 2D
> > transformations that can be applied to an image using two basic plane
> > transformations.
> >
> > Add to the CameraConfiguration class a new member 'orientation' which is
> > used to specify the image orientation in the memory buffers delivered to
> > applications.
> >
> > The enumeration values follow the ones defined by the EXIF specification at
> > revision 2.32, Tag 274 'orientation'.
> >
> > The newly introduced field is meant to replace
> > CameraConfiguration::transform which is not removed yet not to break
> > compilation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > ---
> >  include/libcamera/camera.h      |  2 +
> >  include/libcamera/meson.build   |  1 +
> >  include/libcamera/orientation.h | 28 ++++++++++++
> >  src/libcamera/camera.cpp        | 18 +++++++-
> >  src/libcamera/meson.build       |  1 +
> >  src/libcamera/orientation.cpp   | 78 +++++++++++++++++++++++++++++++++
> >  6 files changed, 127 insertions(+), 1 deletion(-)
> >  create mode 100644 include/libcamera/orientation.h
> >  create mode 100644 src/libcamera/orientation.cpp
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 004bc89455f5..6e9342773962 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -19,6 +19,7 @@
> >  #include <libcamera/base/signal.h>
> >
> >  #include <libcamera/controls.h>
> > +#include <libcamera/orientation.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >  #include <libcamera/transform.h>
> > @@ -67,6 +68,7 @@ public:
> >  	std::size_t size() const;
> >
> >  	Transform transform;
> > +	Orientation orientation;
> >
> >  protected:
> >  	CameraConfiguration();
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index 408b7acf152c..a24c50d66a82 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -12,6 +12,7 @@ libcamera_public_headers = files([
> >      'framebuffer_allocator.h',
> >      'geometry.h',
> >      'logging.h',
> > +    'orientation.h',
> >      'pixel_format.h',
> >      'request.h',
> >      'stream.h',
> > diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h
> > new file mode 100644
> > index 000000000000..63ac4aba07ce
> > --- /dev/null
> > +++ b/include/libcamera/orientation.h
> > @@ -0,0 +1,28 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Ideas On Board Oy
> > + *
> > + * orientation.h - Image orientation
> > + */
> > +
> > +#pragma once
> > +
> > +#include <iostream>
> > +
> > +namespace libcamera {
> > +
> > +enum class Orientation {
> > +	/* EXIF tag 274 starts from '1' */
> > +	rotate0 = 1,
>
> Enumerators should start with an uppercase letter.
>
> > +	rotate0Flip,
> > +	rotate180,
> > +	rotate180Flip,
> > +	rotate90Flip,
> > +	rotate270,
>
> I think this should be rotate90.
>

Ah!

The EXIF tag 6 is described as

The 0th row is the visual right-hand side of the image, and the 0th
column is the visual top.

> > +	rotate270Flip,
> > +	rotate90,
>
> And here, rotate270.

EXIF tag 8

The 0th row is the visual left-hand side of the image, and the 0th
column is the visual bottom.

I'll correct both

>
> > +};
> > +
> > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);
> > +
> > +} /* namespace libcamera */
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 0eecee766f00..d4ad4a553752 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)
> >   * \brief Create an empty camera configuration
> >   */
> >  CameraConfiguration::CameraConfiguration()
> > -	: transform(Transform::Identity), config_({})
> > +	: transform(Transform::Identity), orientation(Orientation::rotate0),
> > +	  config_({})
> >  {
> >  }
> >
> > @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> >   * may adjust this field at its discretion if the selection is not supported.
> >   */
> >
> > +/**
> > + * \var CameraConfiguration::orientation
> > + * \brief The desired orientation of the images produced by the camera
> > + *
> > + * The orientation field is a user-specified 2D plane transformation that
>
> s/field/member/
>
> The word "field" in the C++ specification is used for bit-field only.
> "fields" of a class or structure are named "members" (or "data member"
> if you want to differentiate them from member functions).
>
> > + * specifies how the application wants the camera images to be rotated in
> > + * the memory buffers.
>
> As the whole point of this series is to move from transform to
> orientation, can we try to avoid using "transformation" to define the
> orientation ? How about the following ?
>
>  * The orientation specifies how the application wants images to be oriented in
>  * memory buffers. The camera will rotate and flip images as appropriate to
>  * obtain the desired orientation.
>

I think there's difference between the Transform we want to move away
from and a 2d plane transformation, which is a specific term, but if
just seeing "transformation" spelled out irritates you.. ok...


> > + *
> > + * If the application requested orientation cannot be obtained the validate()
>
> s/obtained/obtained,/
>
> > + * function will Adjust this field to the actual orientation of the images
>
> s/this field/the orientation member/
>
> > + * as produced by the camera pipeline.
>
> I recall we discussed that, in this case, the camera shouldn't try to
> pick a value it considers the closest to the requested orientation, but
> don't transform the image at all. Is that right ? If so, let's document
> it explicitly:

Yes, that's what the proposed documentation says

 * If the application requested orientation cannot be obtained the validate()
 * function will Adjust this field to the actual orientation of the images
 * as produced by the camera pipeline.

>
>  * If the orientation requested by the application cannot be obtained, the
>  * camera will not rotate or flip the images, and the validate() function will
>  * Adjust this value to the native image orientation produced by the camera.
>

The two says the same thing, but.. ok

> > + *
> > + * By default the orientation field is set to Orientation::rotate0.
>
> s/field/member/
>
> > + */
> > +
> >  /**
> >   * \var CameraConfiguration::config_
> >   * \brief The vector of stream configurations
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index b24f82965764..d0e26f6b4141 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -34,6 +34,7 @@ libcamera_sources = files([
> >      'mapped_framebuffer.cpp',
> >      'media_device.cpp',
> >      'media_object.cpp',
> > +    'orientation.cpp',
> >      'pipeline_handler.cpp',
> >      'pixel_format.cpp',
> >      'process.cpp',
> > diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp
> > new file mode 100644
> > index 000000000000..f2ee14dd4182
> > --- /dev/null
> > +++ b/src/libcamera/orientation.cpp
> > @@ -0,0 +1,78 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2023, Ideas On Board Oy
> > + *
> > + * orientation.cpp - Image orientation
> > + */
> > +
> > +#include <libcamera/orientation.h>
> > +
> > +#include <array>
> > +#include <string>
> > +
> > +/**
> > + * \file libcamera/orientation.h
> > + * \brief Image orientation definition
> > + */
> > +
> > +namespace libcamera {
> > +
> > +/**
> > + * \enum Orientation
> > + * \brief The image orientation in a memory buffer
> > + *
> > + * The Orientation enumeration describes the orientation of the images
> > + * produced by the camera pipeline as they get received by the application
> > + * inside memory buffers.
> > + *
>
> From here to ...
>
> > + * All the possible 2D plane transformations of an image are expressed as the
> > + * combination of two basic operations:
> > + *
> > + *   'a': rotate the image 90 degrees clockwise
> > + *   'b': flip the image horizontally (mirroring)
> > + *
> > + * plus an identity operator 'e'.
> > + *
> > + * The composition of operations 'a' and 'b' according to the canonical function
> > + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied
> > + * next) gives origin to a symmetric group of order 8, or dihedral group.
> > + *
> > + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg
> > + * as an example of the 2D plane transformation and how they originate from
> > + * the composition of the 'a' and 'b' operations.
> > + *
> > + * The image orientation expressed using the Orientation enumeration can be
> > + * then inferred by applying a multiple of a 90 degrees rotation from the origin
> > + * and then applying any horizontal mirroring to a base image assume to be
> > + * naturally oriented (no rotation and no mirroring applied).
>
> ... here, is this a leftover of a previous implementation, or copied
> from somewhere else ? It doesn't seem to match the enumeration.

Why it doesn't match the enumeration ? The enumerated members are
named as "90 degrees rotation" + "optional horizontal mirroring".

What is wrong here ?

>
> > + *
> > + * The enumeration numerical values follow the ones defined by the EXIF
> > + * Specification version 2.32, Tag 274 "Orientation", while the names of the
> > + * enumerated values report the rotation and mirroring operations performed.
> > + *
> > + * In example Orientation::rotate90Flip describes the image transformation
>
> s/In example/For example,/
>
> > + * obtained by rotating 90 degrees clockwise first and then applying an
> > + * horizontal mirroring.
> > + */
> > +
> > +/**
> > + * \brief Prints human-friendly names for Orientation items
>
> Let's be consistent with the other operator<<() implementations:
>
>  * \brief Insert a text representation of an Orientation into an output stream
>
> > + * \param[in] out The output stream
> > + * \param[in] orientation The Orientation item
>
> s/ item//
>
> > + * \return The output stream \a out
> > + */
> > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)
> > +{
> > +	constexpr std::array<const char *, 9> orientationNames = {
> > +		"", /* Orientation starts counting from 1. */
> > +		"rotate0", "rotate0Flip",
> > +		"rotate180", "rotate180Flip",
> > +		"rotate90Flip", "rotate270",
> > +		"rotate270Flip", "rotate90",
> > +	};
> > +
> > +	out << orientationNames[static_cast<unsigned int>(orientation)];
>
> You can write
>
> 	/* The Orientation enumeration starts counting from 1. */
> 	out << orientationNames[static_cast<unsigned int>(orientation) - 1];
>
> And drop the first entry from the array.
>
> > +	return out;
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Thu, Oct 19, 2023 at 10:19:11AM +0200, Jacopo Mondi wrote:
> On Wed, Oct 18, 2023 at 09:37:10PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > On Fri, Sep 01, 2023 at 05:02:05PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > Introduce the Orientation enumeration which describes the possible 2D
> > > transformations that can be applied to an image using two basic plane
> > > transformations.
> > >
> > > Add to the CameraConfiguration class a new member 'orientation' which is
> > > used to specify the image orientation in the memory buffers delivered to
> > > applications.
> > >
> > > The enumeration values follow the ones defined by the EXIF specification at
> > > revision 2.32, Tag 274 'orientation'.
> > >
> > > The newly introduced field is meant to replace
> > > CameraConfiguration::transform which is not removed yet not to break
> > > compilation.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > ---
> > >  include/libcamera/camera.h      |  2 +
> > >  include/libcamera/meson.build   |  1 +
> > >  include/libcamera/orientation.h | 28 ++++++++++++
> > >  src/libcamera/camera.cpp        | 18 +++++++-
> > >  src/libcamera/meson.build       |  1 +
> > >  src/libcamera/orientation.cpp   | 78 +++++++++++++++++++++++++++++++++
> > >  6 files changed, 127 insertions(+), 1 deletion(-)
> > >  create mode 100644 include/libcamera/orientation.h
> > >  create mode 100644 src/libcamera/orientation.cpp
> > >
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 004bc89455f5..6e9342773962 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -19,6 +19,7 @@
> > >  #include <libcamera/base/signal.h>
> > >
> > >  #include <libcamera/controls.h>
> > > +#include <libcamera/orientation.h>
> > >  #include <libcamera/request.h>
> > >  #include <libcamera/stream.h>
> > >  #include <libcamera/transform.h>
> > > @@ -67,6 +68,7 @@ public:
> > >  	std::size_t size() const;
> > >
> > >  	Transform transform;
> > > +	Orientation orientation;
> > >
> > >  protected:
> > >  	CameraConfiguration();
> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > index 408b7acf152c..a24c50d66a82 100644
> > > --- a/include/libcamera/meson.build
> > > +++ b/include/libcamera/meson.build
> > > @@ -12,6 +12,7 @@ libcamera_public_headers = files([
> > >      'framebuffer_allocator.h',
> > >      'geometry.h',
> > >      'logging.h',
> > > +    'orientation.h',
> > >      'pixel_format.h',
> > >      'request.h',
> > >      'stream.h',
> > > diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h
> > > new file mode 100644
> > > index 000000000000..63ac4aba07ce
> > > --- /dev/null
> > > +++ b/include/libcamera/orientation.h
> > > @@ -0,0 +1,28 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2023, Ideas On Board Oy
> > > + *
> > > + * orientation.h - Image orientation
> > > + */
> > > +
> > > +#pragma once
> > > +
> > > +#include <iostream>
> > > +
> > > +namespace libcamera {
> > > +
> > > +enum class Orientation {
> > > +	/* EXIF tag 274 starts from '1' */
> > > +	rotate0 = 1,
> >
> > Enumerators should start with an uppercase letter.
> >
> > > +	rotate0Flip,
> > > +	rotate180,
> > > +	rotate180Flip,
> > > +	rotate90Flip,
> > > +	rotate270,
> >
> > I think this should be rotate90.
> 
> Ah!
> 
> The EXIF tag 6 is described as
> 
> The 0th row is the visual right-hand side of the image, and the 0th
> column is the visual top.

Looks like I've misread it then, 270 is right.

> > > +	rotate270Flip,
> > > +	rotate90,
> >
> > And here, rotate270.
> 
> EXIF tag 8
> 
> The 0th row is the visual left-hand side of the image, and the 0th
> column is the visual bottom.
> 
> I'll correct both

No need to :-)

> > > +};
> > > +
> > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);
> > > +
> > > +} /* namespace libcamera */
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 0eecee766f00..d4ad4a553752 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)
> > >   * \brief Create an empty camera configuration
> > >   */
> > >  CameraConfiguration::CameraConfiguration()
> > > -	: transform(Transform::Identity), config_({})
> > > +	: transform(Transform::Identity), orientation(Orientation::rotate0),
> > > +	  config_({})
> > >  {
> > >  }
> > >
> > > @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> > >   * may adjust this field at its discretion if the selection is not supported.
> > >   */
> > >
> > > +/**
> > > + * \var CameraConfiguration::orientation
> > > + * \brief The desired orientation of the images produced by the camera
> > > + *
> > > + * The orientation field is a user-specified 2D plane transformation that
> >
> > s/field/member/
> >
> > The word "field" in the C++ specification is used for bit-field only.
> > "fields" of a class or structure are named "members" (or "data member"
> > if you want to differentiate them from member functions).
> >
> > > + * specifies how the application wants the camera images to be rotated in
> > > + * the memory buffers.
> >
> > As the whole point of this series is to move from transform to
> > orientation, can we try to avoid using "transformation" to define the
> > orientation ? How about the following ?
> >
> >  * The orientation specifies how the application wants images to be oriented in
> >  * memory buffers. The camera will rotate and flip images as appropriate to
> >  * obtain the desired orientation.
> 
> I think there's difference between the Transform we want to move away
> from and a 2d plane transformation, which is a specific term, but if
> just seeing "transformation" spelled out irritates you.. ok...

Yuo're right that the word "transformation" has a more general meaning,
I didn't think about it during the review. We could indeed keep using
it, when it's clear we're not referring to the Transform enum.

> > > + *
> > > + * If the application requested orientation cannot be obtained the validate()
> >
> > s/obtained/obtained,/
> >
> > > + * function will Adjust this field to the actual orientation of the images
> >
> > s/this field/the orientation member/
> >
> > > + * as produced by the camera pipeline.
> >
> > I recall we discussed that, in this case, the camera shouldn't try to
> > pick a value it considers the closest to the requested orientation, but
> > don't transform the image at all. Is that right ? If so, let's document
> > it explicitly:
> 
> Yes, that's what the proposed documentation says
> 
>  * If the application requested orientation cannot be obtained the validate()
>  * function will Adjust this field to the actual orientation of the images
>  * as produced by the camera pipeline.

"as produced by the camera pipeline" means that the orientation field
gets adjusted to reflect what the camera produces, but it doesn't tell
if the camera can/should still apply a transformation.

> >  * If the orientation requested by the application cannot be obtained, the
> >  * camera will not rotate or flip the images, and the validate() function will
> >  * Adjust this value to the native image orientation produced by the camera.
> 
> The two says the same thing, but.. ok
> 
> > > + *
> > > + * By default the orientation field is set to Orientation::rotate0.
> >
> > s/field/member/
> >
> > > + */
> > > +
> > >  /**
> > >   * \var CameraConfiguration::config_
> > >   * \brief The vector of stream configurations
> > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > index b24f82965764..d0e26f6b4141 100644
> > > --- a/src/libcamera/meson.build
> > > +++ b/src/libcamera/meson.build
> > > @@ -34,6 +34,7 @@ libcamera_sources = files([
> > >      'mapped_framebuffer.cpp',
> > >      'media_device.cpp',
> > >      'media_object.cpp',
> > > +    'orientation.cpp',
> > >      'pipeline_handler.cpp',
> > >      'pixel_format.cpp',
> > >      'process.cpp',
> > > diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp
> > > new file mode 100644
> > > index 000000000000..f2ee14dd4182
> > > --- /dev/null
> > > +++ b/src/libcamera/orientation.cpp
> > > @@ -0,0 +1,78 @@
> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > +/*
> > > + * Copyright (C) 2023, Ideas On Board Oy
> > > + *
> > > + * orientation.cpp - Image orientation
> > > + */
> > > +
> > > +#include <libcamera/orientation.h>
> > > +
> > > +#include <array>
> > > +#include <string>
> > > +
> > > +/**
> > > + * \file libcamera/orientation.h
> > > + * \brief Image orientation definition
> > > + */
> > > +
> > > +namespace libcamera {
> > > +
> > > +/**
> > > + * \enum Orientation
> > > + * \brief The image orientation in a memory buffer
> > > + *
> > > + * The Orientation enumeration describes the orientation of the images
> > > + * produced by the camera pipeline as they get received by the application
> > > + * inside memory buffers.
> > > + *
> >
> > From here to ...
> >
> > > + * All the possible 2D plane transformations of an image are expressed as the
> > > + * combination of two basic operations:
> > > + *
> > > + *   'a': rotate the image 90 degrees clockwise
> > > + *   'b': flip the image horizontally (mirroring)
> > > + *
> > > + * plus an identity operator 'e'.
> > > + *
> > > + * The composition of operations 'a' and 'b' according to the canonical function
> > > + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied
> > > + * next) gives origin to a symmetric group of order 8, or dihedral group.
> > > + *
> > > + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg
> > > + * as an example of the 2D plane transformation and how they originate from
> > > + * the composition of the 'a' and 'b' operations.
> > > + *
> > > + * The image orientation expressed using the Orientation enumeration can be
> > > + * then inferred by applying a multiple of a 90 degrees rotation from the origin
> > > + * and then applying any horizontal mirroring to a base image assume to be
> > > + * naturally oriented (no rotation and no mirroring applied).
> >
> > ... here, is this a leftover of a previous implementation, or copied
> > from somewhere else ? It doesn't seem to match the enumeration.
> 
> Why it doesn't match the enumeration ? The enumerated members are
> named as "90 degrees rotation" + "optional horizontal mirroring".
> 
> What is wrong here ?

The text above is not incorrect, but reading it, it appears barely
connected to the enumerators, and I think it's also overly complicated.

> > > + *
> > > + * The enumeration numerical values follow the ones defined by the EXIF
> > > + * Specification version 2.32, Tag 274 "Orientation", while the names of the
> > > + * enumerated values report the rotation and mirroring operations performed.
> > > + *
> > > + * In example Orientation::rotate90Flip describes the image transformation
> >
> > s/In example/For example,/
> >
> > > + * obtained by rotating 90 degrees clockwise first and then applying an
> > > + * horizontal mirroring.
> > > + */
> > > +
> > > +/**
> > > + * \brief Prints human-friendly names for Orientation items
> >
> > Let's be consistent with the other operator<<() implementations:
> >
> >  * \brief Insert a text representation of an Orientation into an output stream
> >
> > > + * \param[in] out The output stream
> > > + * \param[in] orientation The Orientation item
> >
> > s/ item//
> >
> > > + * \return The output stream \a out
> > > + */
> > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)
> > > +{
> > > +	constexpr std::array<const char *, 9> orientationNames = {
> > > +		"", /* Orientation starts counting from 1. */
> > > +		"rotate0", "rotate0Flip",
> > > +		"rotate180", "rotate180Flip",
> > > +		"rotate90Flip", "rotate270",
> > > +		"rotate270Flip", "rotate90",
> > > +	};
> > > +
> > > +	out << orientationNames[static_cast<unsigned int>(orientation)];
> >
> > You can write
> >
> > 	/* The Orientation enumeration starts counting from 1. */
> > 	out << orientationNames[static_cast<unsigned int>(orientation) - 1];
> >
> > And drop the first entry from the array.
> >
> > > +	return out;
> > > +}
> > > +
> > > +} /* namespace libcamera */

Hi Laurent

On Thu, Oct 19, 2023 at 11:37:52AM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Thu, Oct 19, 2023 at 10:19:11AM +0200, Jacopo Mondi wrote:
> > On Wed, Oct 18, 2023 at 09:37:10PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > On Fri, Sep 01, 2023 at 05:02:05PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > > Introduce the Orientation enumeration which describes the possible 2D
> > > > transformations that can be applied to an image using two basic plane
> > > > transformations.
> > > >
> > > > Add to the CameraConfiguration class a new member 'orientation' which is
> > > > used to specify the image orientation in the memory buffers delivered to
> > > > applications.
> > > >
> > > > The enumeration values follow the ones defined by the EXIF specification at
> > > > revision 2.32, Tag 274 'orientation'.
> > > >
> > > > The newly introduced field is meant to replace
> > > > CameraConfiguration::transform which is not removed yet not to break
> > > > compilation.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > > ---
> > > >  include/libcamera/camera.h      |  2 +
> > > >  include/libcamera/meson.build   |  1 +
> > > >  include/libcamera/orientation.h | 28 ++++++++++++
> > > >  src/libcamera/camera.cpp        | 18 +++++++-
> > > >  src/libcamera/meson.build       |  1 +
> > > >  src/libcamera/orientation.cpp   | 78 +++++++++++++++++++++++++++++++++
> > > >  6 files changed, 127 insertions(+), 1 deletion(-)
> > > >  create mode 100644 include/libcamera/orientation.h
> > > >  create mode 100644 src/libcamera/orientation.cpp
> > > >
> > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > > index 004bc89455f5..6e9342773962 100644
> > > > --- a/include/libcamera/camera.h
> > > > +++ b/include/libcamera/camera.h
> > > > @@ -19,6 +19,7 @@
> > > >  #include <libcamera/base/signal.h>
> > > >
> > > >  #include <libcamera/controls.h>
> > > > +#include <libcamera/orientation.h>
> > > >  #include <libcamera/request.h>
> > > >  #include <libcamera/stream.h>
> > > >  #include <libcamera/transform.h>
> > > > @@ -67,6 +68,7 @@ public:
> > > >  	std::size_t size() const;
> > > >
> > > >  	Transform transform;
> > > > +	Orientation orientation;
> > > >
> > > >  protected:
> > > >  	CameraConfiguration();
> > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > > index 408b7acf152c..a24c50d66a82 100644
> > > > --- a/include/libcamera/meson.build
> > > > +++ b/include/libcamera/meson.build
> > > > @@ -12,6 +12,7 @@ libcamera_public_headers = files([
> > > >      'framebuffer_allocator.h',
> > > >      'geometry.h',
> > > >      'logging.h',
> > > > +    'orientation.h',
> > > >      'pixel_format.h',
> > > >      'request.h',
> > > >      'stream.h',
> > > > diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h
> > > > new file mode 100644
> > > > index 000000000000..63ac4aba07ce
> > > > --- /dev/null
> > > > +++ b/include/libcamera/orientation.h
> > > > @@ -0,0 +1,28 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2023, Ideas On Board Oy
> > > > + *
> > > > + * orientation.h - Image orientation
> > > > + */
> > > > +
> > > > +#pragma once
> > > > +
> > > > +#include <iostream>
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +enum class Orientation {
> > > > +	/* EXIF tag 274 starts from '1' */
> > > > +	rotate0 = 1,
> > >
> > > Enumerators should start with an uppercase letter.
> > >
> > > > +	rotate0Flip,
> > > > +	rotate180,
> > > > +	rotate180Flip,
> > > > +	rotate90Flip,
> > > > +	rotate270,
> > >
> > > I think this should be rotate90.
> >
> > Ah!
> >
> > The EXIF tag 6 is described as
> >
> > The 0th row is the visual right-hand side of the image, and the 0th
> > column is the visual top.
>
> Looks like I've misread it then, 270 is right.
>
> > > > +	rotate270Flip,
> > > > +	rotate90,
> > >
> > > And here, rotate270.
> >
> > EXIF tag 8
> >
> > The 0th row is the visual left-hand side of the image, and the 0th
> > column is the visual bottom.
> >
> > I'll correct both
>
> No need to :-)
>

I keep reading the EXIF descriptions wrong

The "Oth row" refers to what is displayed, not the first row of the
image...

At least it seems like I finally got them right two months ago


> > > > +};
> > > > +
> > > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);
> > > > +
> > > > +} /* namespace libcamera */
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index 0eecee766f00..d4ad4a553752 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)
> > > >   * \brief Create an empty camera configuration
> > > >   */
> > > >  CameraConfiguration::CameraConfiguration()
> > > > -	: transform(Transform::Identity), config_({})
> > > > +	: transform(Transform::Identity), orientation(Orientation::rotate0),
> > > > +	  config_({})
> > > >  {
> > > >  }
> > > >
> > > > @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> > > >   * may adjust this field at its discretion if the selection is not supported.
> > > >   */
> > > >
> > > > +/**
> > > > + * \var CameraConfiguration::orientation
> > > > + * \brief The desired orientation of the images produced by the camera
> > > > + *
> > > > + * The orientation field is a user-specified 2D plane transformation that
> > >
> > > s/field/member/
> > >
> > > The word "field" in the C++ specification is used for bit-field only.
> > > "fields" of a class or structure are named "members" (or "data member"
> > > if you want to differentiate them from member functions).
> > >
> > > > + * specifies how the application wants the camera images to be rotated in
> > > > + * the memory buffers.
> > >
> > > As the whole point of this series is to move from transform to
> > > orientation, can we try to avoid using "transformation" to define the
> > > orientation ? How about the following ?
> > >
> > >  * The orientation specifies how the application wants images to be oriented in
> > >  * memory buffers. The camera will rotate and flip images as appropriate to
> > >  * obtain the desired orientation.
> >
> > I think there's difference between the Transform we want to move away
> > from and a 2d plane transformation, which is a specific term, but if
> > just seeing "transformation" spelled out irritates you.. ok...
>
> Yuo're right that the word "transformation" has a more general meaning,
> I didn't think about it during the review. We could indeed keep using
> it, when it's clear we're not referring to the Transform enum.
>
> > > > + *
> > > > + * If the application requested orientation cannot be obtained the validate()
> > >
> > > s/obtained/obtained,/
> > >
> > > > + * function will Adjust this field to the actual orientation of the images
> > >
> > > s/this field/the orientation member/
> > >
> > > > + * as produced by the camera pipeline.
> > >
> > > I recall we discussed that, in this case, the camera shouldn't try to
> > > pick a value it considers the closest to the requested orientation, but
> > > don't transform the image at all. Is that right ? If so, let's document
> > > it explicitly:
> >
> > Yes, that's what the proposed documentation says
> >
> >  * If the application requested orientation cannot be obtained the validate()
> >  * function will Adjust this field to the actual orientation of the images
> >  * as produced by the camera pipeline.
>
> "as produced by the camera pipeline" means that the orientation field
> gets adjusted to reflect what the camera produces, but it doesn't tell
> if the camera can/should still apply a transformation.
>

Oh, I see why it was confusing now

> > >  * If the orientation requested by the application cannot be obtained, the
> > >  * camera will not rotate or flip the images, and the validate() function will
> > >  * Adjust this value to the native image orientation produced by the camera.
> >
> > The two says the same thing, but.. ok
> >
> > > > + *
> > > > + * By default the orientation field is set to Orientation::rotate0.
> > >
> > > s/field/member/
> > >
> > > > + */
> > > > +
> > > >  /**
> > > >   * \var CameraConfiguration::config_
> > > >   * \brief The vector of stream configurations
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index b24f82965764..d0e26f6b4141 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -34,6 +34,7 @@ libcamera_sources = files([
> > > >      'mapped_framebuffer.cpp',
> > > >      'media_device.cpp',
> > > >      'media_object.cpp',
> > > > +    'orientation.cpp',
> > > >      'pipeline_handler.cpp',
> > > >      'pixel_format.cpp',
> > > >      'process.cpp',
> > > > diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp
> > > > new file mode 100644
> > > > index 000000000000..f2ee14dd4182
> > > > --- /dev/null
> > > > +++ b/src/libcamera/orientation.cpp
> > > > @@ -0,0 +1,78 @@
> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > +/*
> > > > + * Copyright (C) 2023, Ideas On Board Oy
> > > > + *
> > > > + * orientation.cpp - Image orientation
> > > > + */
> > > > +
> > > > +#include <libcamera/orientation.h>
> > > > +
> > > > +#include <array>
> > > > +#include <string>
> > > > +
> > > > +/**
> > > > + * \file libcamera/orientation.h
> > > > + * \brief Image orientation definition
> > > > + */
> > > > +
> > > > +namespace libcamera {
> > > > +
> > > > +/**
> > > > + * \enum Orientation
> > > > + * \brief The image orientation in a memory buffer
> > > > + *
> > > > + * The Orientation enumeration describes the orientation of the images
> > > > + * produced by the camera pipeline as they get received by the application
> > > > + * inside memory buffers.
> > > > + *
> > >
> > > From here to ...
> > >
> > > > + * All the possible 2D plane transformations of an image are expressed as the
> > > > + * combination of two basic operations:
> > > > + *
> > > > + *   'a': rotate the image 90 degrees clockwise
> > > > + *   'b': flip the image horizontally (mirroring)
> > > > + *
> > > > + * plus an identity operator 'e'.
> > > > + *
> > > > + * The composition of operations 'a' and 'b' according to the canonical function
> > > > + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied
> > > > + * next) gives origin to a symmetric group of order 8, or dihedral group.
> > > > + *
> > > > + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg
> > > > + * as an example of the 2D plane transformation and how they originate from
> > > > + * the composition of the 'a' and 'b' operations.
> > > > + *
> > > > + * The image orientation expressed using the Orientation enumeration can be
> > > > + * then inferred by applying a multiple of a 90 degrees rotation from the origin
> > > > + * and then applying any horizontal mirroring to a base image assume to be
> > > > + * naturally oriented (no rotation and no mirroring applied).
> > >
> > > ... here, is this a leftover of a previous implementation, or copied
> > > from somewhere else ? It doesn't seem to match the enumeration.
> >
> > Why it doesn't match the enumeration ? The enumerated members are
> > named as "90 degrees rotation" + "optional horizontal mirroring".
> >
> > What is wrong here ?
>
> The text above is not incorrect, but reading it, it appears barely
> connected to the enumerators, and I think it's also overly complicated.
>

When it comes to being possibily too complicated, I agree :)

I tried to keep on par with David's documentation about Transform
which provided a bit more of a formal background.

Is it ok to remove the part about the the dihedral group but keep the
last paragraph ("The image orientation expressed ..") or should it be
removed as well ?

> > > > + *
> > > > + * The enumeration numerical values follow the ones defined by the EXIF
> > > > + * Specification version 2.32, Tag 274 "Orientation", while the names of the
> > > > + * enumerated values report the rotation and mirroring operations performed.
> > > > + *
> > > > + * In example Orientation::rotate90Flip describes the image transformation
> > >
> > > s/In example/For example,/
> > >
> > > > + * obtained by rotating 90 degrees clockwise first and then applying an
> > > > + * horizontal mirroring.
> > > > + */
> > > > +
> > > > +/**
> > > > + * \brief Prints human-friendly names for Orientation items
> > >
> > > Let's be consistent with the other operator<<() implementations:
> > >
> > >  * \brief Insert a text representation of an Orientation into an output stream
> > >
> > > > + * \param[in] out The output stream
> > > > + * \param[in] orientation The Orientation item
> > >
> > > s/ item//
> > >
> > > > + * \return The output stream \a out
> > > > + */
> > > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)
> > > > +{
> > > > +	constexpr std::array<const char *, 9> orientationNames = {
> > > > +		"", /* Orientation starts counting from 1. */
> > > > +		"rotate0", "rotate0Flip",
> > > > +		"rotate180", "rotate180Flip",
> > > > +		"rotate90Flip", "rotate270",
> > > > +		"rotate270Flip", "rotate90",
> > > > +	};
> > > > +
> > > > +	out << orientationNames[static_cast<unsigned int>(orientation)];
> > >
> > > You can write
> > >
> > > 	/* The Orientation enumeration starts counting from 1. */
> > > 	out << orientationNames[static_cast<unsigned int>(orientation) - 1];
> > >
> > > And drop the first entry from the array.
> > >
> > > > +	return out;
> > > > +}
> > > > +
> > > > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Thu, Oct 19, 2023 at 11:19:33AM +0200, Jacopo Mondi wrote:
> On Thu, Oct 19, 2023 at 11:37:52AM +0300, Laurent Pinchart wrote:
> > On Thu, Oct 19, 2023 at 10:19:11AM +0200, Jacopo Mondi wrote:
> > > On Wed, Oct 18, 2023 at 09:37:10PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > On Fri, Sep 01, 2023 at 05:02:05PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > > > Introduce the Orientation enumeration which describes the possible 2D
> > > > > transformations that can be applied to an image using two basic plane
> > > > > transformations.
> > > > >
> > > > > Add to the CameraConfiguration class a new member 'orientation' which is
> > > > > used to specify the image orientation in the memory buffers delivered to
> > > > > applications.
> > > > >
> > > > > The enumeration values follow the ones defined by the EXIF specification at
> > > > > revision 2.32, Tag 274 'orientation'.
> > > > >
> > > > > The newly introduced field is meant to replace
> > > > > CameraConfiguration::transform which is not removed yet not to break
> > > > > compilation.
> > > > >
> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > > Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
> > > > > ---
> > > > >  include/libcamera/camera.h      |  2 +
> > > > >  include/libcamera/meson.build   |  1 +
> > > > >  include/libcamera/orientation.h | 28 ++++++++++++
> > > > >  src/libcamera/camera.cpp        | 18 +++++++-
> > > > >  src/libcamera/meson.build       |  1 +
> > > > >  src/libcamera/orientation.cpp   | 78 +++++++++++++++++++++++++++++++++
> > > > >  6 files changed, 127 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 include/libcamera/orientation.h
> > > > >  create mode 100644 src/libcamera/orientation.cpp
> > > > >
> > > > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > > > index 004bc89455f5..6e9342773962 100644
> > > > > --- a/include/libcamera/camera.h
> > > > > +++ b/include/libcamera/camera.h
> > > > > @@ -19,6 +19,7 @@
> > > > >  #include <libcamera/base/signal.h>
> > > > >
> > > > >  #include <libcamera/controls.h>
> > > > > +#include <libcamera/orientation.h>
> > > > >  #include <libcamera/request.h>
> > > > >  #include <libcamera/stream.h>
> > > > >  #include <libcamera/transform.h>
> > > > > @@ -67,6 +68,7 @@ public:
> > > > >  	std::size_t size() const;
> > > > >
> > > > >  	Transform transform;
> > > > > +	Orientation orientation;
> > > > >
> > > > >  protected:
> > > > >  	CameraConfiguration();
> > > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > > > index 408b7acf152c..a24c50d66a82 100644
> > > > > --- a/include/libcamera/meson.build
> > > > > +++ b/include/libcamera/meson.build
> > > > > @@ -12,6 +12,7 @@ libcamera_public_headers = files([
> > > > >      'framebuffer_allocator.h',
> > > > >      'geometry.h',
> > > > >      'logging.h',
> > > > > +    'orientation.h',
> > > > >      'pixel_format.h',
> > > > >      'request.h',
> > > > >      'stream.h',
> > > > > diff --git a/include/libcamera/orientation.h b/include/libcamera/orientation.h
> > > > > new file mode 100644
> > > > > index 000000000000..63ac4aba07ce
> > > > > --- /dev/null
> > > > > +++ b/include/libcamera/orientation.h
> > > > > @@ -0,0 +1,28 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2023, Ideas On Board Oy
> > > > > + *
> > > > > + * orientation.h - Image orientation
> > > > > + */
> > > > > +
> > > > > +#pragma once
> > > > > +
> > > > > +#include <iostream>
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +enum class Orientation {
> > > > > +	/* EXIF tag 274 starts from '1' */
> > > > > +	rotate0 = 1,
> > > >
> > > > Enumerators should start with an uppercase letter.
> > > >
> > > > > +	rotate0Flip,
> > > > > +	rotate180,
> > > > > +	rotate180Flip,
> > > > > +	rotate90Flip,
> > > > > +	rotate270,
> > > >
> > > > I think this should be rotate90.
> > >
> > > Ah!
> > >
> > > The EXIF tag 6 is described as
> > >
> > > The 0th row is the visual right-hand side of the image, and the 0th
> > > column is the visual top.
> >
> > Looks like I've misread it then, 270 is right.
> >
> > > > > +	rotate270Flip,
> > > > > +	rotate90,
> > > >
> > > > And here, rotate270.
> > >
> > > EXIF tag 8
> > >
> > > The 0th row is the visual left-hand side of the image, and the 0th
> > > column is the visual bottom.
> > >
> > > I'll correct both
> >
> > No need to :-)
> >
> 
> I keep reading the EXIF descriptions wrong
> 
> The "Oth row" refers to what is displayed, not the first row of the
> image...
> 
> At least it seems like I finally got them right two months ago

:-)

> > > > > +};
> > > > > +
> > > > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation);
> > > > > +
> > > > > +} /* namespace libcamera */
> > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > > index 0eecee766f00..d4ad4a553752 100644
> > > > > --- a/src/libcamera/camera.cpp
> > > > > +++ b/src/libcamera/camera.cpp
> > > > > @@ -160,7 +160,8 @@ LOG_DECLARE_CATEGORY(Camera)
> > > > >   * \brief Create an empty camera configuration
> > > > >   */
> > > > >  CameraConfiguration::CameraConfiguration()
> > > > > -	: transform(Transform::Identity), config_({})
> > > > > +	: transform(Transform::Identity), orientation(Orientation::rotate0),
> > > > > +	  config_({})
> > > > >  {
> > > > >  }
> > > > >
> > > > > @@ -404,6 +405,21 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> > > > >   * may adjust this field at its discretion if the selection is not supported.
> > > > >   */
> > > > >
> > > > > +/**
> > > > > + * \var CameraConfiguration::orientation
> > > > > + * \brief The desired orientation of the images produced by the camera
> > > > > + *
> > > > > + * The orientation field is a user-specified 2D plane transformation that
> > > >
> > > > s/field/member/
> > > >
> > > > The word "field" in the C++ specification is used for bit-field only.
> > > > "fields" of a class or structure are named "members" (or "data member"
> > > > if you want to differentiate them from member functions).
> > > >
> > > > > + * specifies how the application wants the camera images to be rotated in
> > > > > + * the memory buffers.
> > > >
> > > > As the whole point of this series is to move from transform to
> > > > orientation, can we try to avoid using "transformation" to define the
> > > > orientation ? How about the following ?
> > > >
> > > >  * The orientation specifies how the application wants images to be oriented in
> > > >  * memory buffers. The camera will rotate and flip images as appropriate to
> > > >  * obtain the desired orientation.
> > >
> > > I think there's difference between the Transform we want to move away
> > > from and a 2d plane transformation, which is a specific term, but if
> > > just seeing "transformation" spelled out irritates you.. ok...
> >
> > Yuo're right that the word "transformation" has a more general meaning,
> > I didn't think about it during the review. We could indeed keep using
> > it, when it's clear we're not referring to the Transform enum.
> >
> > > > > + *
> > > > > + * If the application requested orientation cannot be obtained the validate()
> > > >
> > > > s/obtained/obtained,/
> > > >
> > > > > + * function will Adjust this field to the actual orientation of the images
> > > >
> > > > s/this field/the orientation member/
> > > >
> > > > > + * as produced by the camera pipeline.
> > > >
> > > > I recall we discussed that, in this case, the camera shouldn't try to
> > > > pick a value it considers the closest to the requested orientation, but
> > > > don't transform the image at all. Is that right ? If so, let's document
> > > > it explicitly:
> > >
> > > Yes, that's what the proposed documentation says
> > >
> > >  * If the application requested orientation cannot be obtained the validate()
> > >  * function will Adjust this field to the actual orientation of the images
> > >  * as produced by the camera pipeline.
> >
> > "as produced by the camera pipeline" means that the orientation field
> > gets adjusted to reflect what the camera produces, but it doesn't tell
> > if the camera can/should still apply a transformation.
> >
> 
> Oh, I see why it was confusing now
> 
> > > >  * If the orientation requested by the application cannot be obtained, the
> > > >  * camera will not rotate or flip the images, and the validate() function will
> > > >  * Adjust this value to the native image orientation produced by the camera.
> > >
> > > The two says the same thing, but.. ok
> > >
> > > > > + *
> > > > > + * By default the orientation field is set to Orientation::rotate0.
> > > >
> > > > s/field/member/
> > > >
> > > > > + */
> > > > > +
> > > > >  /**
> > > > >   * \var CameraConfiguration::config_
> > > > >   * \brief The vector of stream configurations
> > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > > index b24f82965764..d0e26f6b4141 100644
> > > > > --- a/src/libcamera/meson.build
> > > > > +++ b/src/libcamera/meson.build
> > > > > @@ -34,6 +34,7 @@ libcamera_sources = files([
> > > > >      'mapped_framebuffer.cpp',
> > > > >      'media_device.cpp',
> > > > >      'media_object.cpp',
> > > > > +    'orientation.cpp',
> > > > >      'pipeline_handler.cpp',
> > > > >      'pixel_format.cpp',
> > > > >      'process.cpp',
> > > > > diff --git a/src/libcamera/orientation.cpp b/src/libcamera/orientation.cpp
> > > > > new file mode 100644
> > > > > index 000000000000..f2ee14dd4182
> > > > > --- /dev/null
> > > > > +++ b/src/libcamera/orientation.cpp
> > > > > @@ -0,0 +1,78 @@
> > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > > > > +/*
> > > > > + * Copyright (C) 2023, Ideas On Board Oy
> > > > > + *
> > > > > + * orientation.cpp - Image orientation
> > > > > + */
> > > > > +
> > > > > +#include <libcamera/orientation.h>
> > > > > +
> > > > > +#include <array>
> > > > > +#include <string>
> > > > > +
> > > > > +/**
> > > > > + * \file libcamera/orientation.h
> > > > > + * \brief Image orientation definition
> > > > > + */
> > > > > +
> > > > > +namespace libcamera {
> > > > > +
> > > > > +/**
> > > > > + * \enum Orientation
> > > > > + * \brief The image orientation in a memory buffer
> > > > > + *
> > > > > + * The Orientation enumeration describes the orientation of the images
> > > > > + * produced by the camera pipeline as they get received by the application
> > > > > + * inside memory buffers.
> > > > > + *
> > > >
> > > > From here to ...
> > > >
> > > > > + * All the possible 2D plane transformations of an image are expressed as the
> > > > > + * combination of two basic operations:
> > > > > + *
> > > > > + *   'a': rotate the image 90 degrees clockwise
> > > > > + *   'b': flip the image horizontally (mirroring)
> > > > > + *
> > > > > + * plus an identity operator 'e'.
> > > > > + *
> > > > > + * The composition of operations 'a' and 'b' according to the canonical function
> > > > > + * composition notion 'b * a' (where 'a' is applied first, then 'b' is applied
> > > > > + * next) gives origin to a symmetric group of order 8, or dihedral group.
> > > > > + *
> > > > > + * See https://en.wikipedia.org/wiki/Dihedral_group#/media/File:Dih4_cycle_graph.svg
> > > > > + * as an example of the 2D plane transformation and how they originate from
> > > > > + * the composition of the 'a' and 'b' operations.
> > > > > + *
> > > > > + * The image orientation expressed using the Orientation enumeration can be
> > > > > + * then inferred by applying a multiple of a 90 degrees rotation from the origin
> > > > > + * and then applying any horizontal mirroring to a base image assume to be
> > > > > + * naturally oriented (no rotation and no mirroring applied).
> > > >
> > > > ... here, is this a leftover of a previous implementation, or copied
> > > > from somewhere else ? It doesn't seem to match the enumeration.
> > >
> > > Why it doesn't match the enumeration ? The enumerated members are
> > > named as "90 degrees rotation" + "optional horizontal mirroring".
> > >
> > > What is wrong here ?
> >
> > The text above is not incorrect, but reading it, it appears barely
> > connected to the enumerators, and I think it's also overly complicated.
> 
> When it comes to being possibily too complicated, I agree :)
> 
> I tried to keep on par with David's documentation about Transform
> which provided a bit more of a formal background.
> 
> Is it ok to remove the part about the the dihedral group but keep the
> last paragraph ("The image orientation expressed ..") or should it be
> removed as well ?

I think that's fine for v6. I'll experiment with a few changes on top as
discussed in other e-mails in this thread, and if I can find ways to
improve the documentation, I'll do so as well. You should be freed from
the hassle of a v7 :-)

> > > > > + *
> > > > > + * The enumeration numerical values follow the ones defined by the EXIF
> > > > > + * Specification version 2.32, Tag 274 "Orientation", while the names of the
> > > > > + * enumerated values report the rotation and mirroring operations performed.
> > > > > + *
> > > > > + * In example Orientation::rotate90Flip describes the image transformation
> > > >
> > > > s/In example/For example,/
> > > >
> > > > > + * obtained by rotating 90 degrees clockwise first and then applying an
> > > > > + * horizontal mirroring.
> > > > > + */
> > > > > +
> > > > > +/**
> > > > > + * \brief Prints human-friendly names for Orientation items
> > > >
> > > > Let's be consistent with the other operator<<() implementations:
> > > >
> > > >  * \brief Insert a text representation of an Orientation into an output stream
> > > >
> > > > > + * \param[in] out The output stream
> > > > > + * \param[in] orientation The Orientation item
> > > >
> > > > s/ item//
> > > >
> > > > > + * \return The output stream \a out
> > > > > + */
> > > > > +std::ostream &operator<<(std::ostream &out, const Orientation &orientation)
> > > > > +{
> > > > > +	constexpr std::array<const char *, 9> orientationNames = {
> > > > > +		"", /* Orientation starts counting from 1. */
> > > > > +		"rotate0", "rotate0Flip",
> > > > > +		"rotate180", "rotate180Flip",
> > > > > +		"rotate90Flip", "rotate270",
> > > > > +		"rotate270Flip", "rotate90",
> > > > > +	};
> > > > > +
> > > > > +	out << orientationNames[static_cast<unsigned int>(orientation)];
> > > >
> > > > You can write
> > > >
> > > > 	/* The Orientation enumeration starts counting from 1. */
> > > > 	out << orientationNames[static_cast<unsigned int>(orientation) - 1];
> > > >
> > > > And drop the first entry from the array.
> > > >
> > > > > +	return out;
> > > > > +}
> > > > > +
> > > > > +} /* namespace libcamera */