[libcamera-devel] [PATCH v5 02/12] libcamera: camera: Introduce Orientation
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Oct 19 10:37:52 CEST 2023
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 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.
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 */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list