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

[Jacopo Mondi]

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

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