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