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

[Laurent Pinchart]

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 at ideasonboard.com>
> Reviewed-by: David Plowman <david.plowman at 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 */

-- 
Regards,

Laurent Pinchart