[libcamera-devel] [PATCH v2 1/5] libcamera: Add Transform enum to represet 2d plane transforms.

[David Plowman]

Hi Laurent

Thanks for the review, much appreciated!. Those various suggestions
all seem good to me so I'll prepare a v3 set that includes them. I'll
also add the missing documentation this time round and then hopefully
we'll be close!

Best regards
David

On Wed, 19 Aug 2020 at 02:56, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> s/represet/represent/ in the subject line.
>
> On Thu, Aug 06, 2020 at 05:36:35PM +0100, David Plowman wrote:
> > We implement 2d transforms as an enum class with 8 elements,
>
> 2d or 2D ?
>
> > consisting of the usual 2d plane transformations (flips, rotations
> > etc.).
> >
> > The transform is made up of 3 bits, indicating whether the transform
> > includes: a transpose, a horizontal flip (mirror) and a vertical flip.
>
> Missing Signed-off-by line.
>
> Overall this looks good to me, please see below for a handful of small
> comments.
>
> > ---
> >  include/libcamera/meson.build |  1 +
> >  include/libcamera/transform.h | 58 ++++++++++++++++++++++++
> >  src/libcamera/meson.build     |  1 +
> >  src/libcamera/transform.cpp   | 83 +++++++++++++++++++++++++++++++++++
> >  4 files changed, 143 insertions(+)
> >  create mode 100644 include/libcamera/transform.h
> >  create mode 100644 src/libcamera/transform.cpp
> >
> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > index cdb8e03..7fae5e5 100644
> > --- a/include/libcamera/meson.build
> > +++ b/include/libcamera/meson.build
> > @@ -19,6 +19,7 @@ libcamera_public_headers = files([
> >      'span.h',
> >      'stream.h',
> >      'timer.h',
> > +    'transform.h',
> >  ])
> >
> >  include_dir = join_paths(libcamera_include_dir, 'libcamera')
> > diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h
> > new file mode 100644
> > index 0000000..658beb9
> > --- /dev/null
> > +++ b/include/libcamera/transform.h
> > @@ -0,0 +1,58 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited
> > + *
> > + * transform.h - Implementation of 2d plane transforms
>
> s/Implementation of //
>
> (same comment for the .cpp file)
>
> > + */
> > +
> > +#ifndef __LIBCAMERA_TRANSFORM_H__
> > +#define __LIBCAMERA_TRANSFORM_H__
> > +
> > +#include <string>
> > +
> > +namespace libcamera {
> > +
> > +enum class Transform : int {
> > +     Identity = 0,
> > +     Rot0 = Identity,
> > +     HFlip = 1,
> > +     VFlip = 2,
> > +     HVFlip = HFlip | VFlip,
> > +     Rot180 = HVFlip,
> > +     Transpose = 4,
> > +     Rot270 = HFlip | Transpose,
> > +     Rot90 = VFlip | Transpose,
> > +     Rot180Transpose = HFlip | VFlip | Transpose
> > +};
> > +
> > +constexpr Transform operator&(Transform t0, Transform t1)
> > +{
> > +     return static_cast<Transform>(static_cast<int>(t0) & static_cast<int>(t1));
> > +}
> > +
> > +constexpr Transform operator|(Transform t0, Transform t1)
> > +{
> > +     return static_cast<Transform>(static_cast<int>(t0) | static_cast<int>(t1));
> > +}
> > +
> > +constexpr Transform operator^(Transform t0, Transform t1)
> > +{
> > +     return static_cast<Transform>(static_cast<int>(t0) ^ static_cast<int>(t1));
> > +}
> > +
> > +Transform operator*(Transform t0, Transform t1);
> > +
> > +Transform operator-(Transform t);
> > +
> > +constexpr bool operator!(Transform t)
> > +{
> > +     return t == Transform::Identity;
> > +}
> > +
> > +Transform transformFromRotation(int angle, bool *success = nullptr);
> > +
> > +std::string transformToString(Transform t);
>
> A class would have allowed these to be member functions, but I think I
> agree that it's not worth the churn.
>
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_TRANSFORM_H__ */
> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > index bada45b..b46247d 100644
> > --- a/src/libcamera/meson.build
> > +++ b/src/libcamera/meson.build
> > @@ -44,6 +44,7 @@ libcamera_sources = files([
> >      'sysfs.cpp',
> >      'thread.cpp',
> >      'timer.cpp',
> > +    'transform.cpp',
> >      'utils.cpp',
> >      'v4l2_controls.cpp',
> >      'v4l2_device.cpp',
> > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp
> > new file mode 100644
> > index 0000000..5f00a5c
> > --- /dev/null
> > +++ b/src/libcamera/transform.cpp
> > @@ -0,0 +1,83 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Raspberry Pi (Trading) Limited
> > + *
> > + * transform.cpp - implementation of 2d plane transforms.
> > + */
> > +
> > +#include <libcamera/transform.h>
> > +
> > +/**
> > + * \file transform.h
> > + * \brief Enum to represent a 2d plane transforms.
> > + */
> > +
> > +namespace libcamera {
> > +
> > +Transform operator*(Transform t0, Transform t1)
> > +{
> > +     /*
> > +      * Reorder the operations so that we imagine doing t1's transpose
> > +      * (if any) after t0's flips. The effect is to swap t0's hflips for
> > +      * vflips and vice versa, after which we can just xor all the bits.
> > +      */
>
> I need to wrap my head around this. Documentation would help, as nowhere
> do we explicitly say in which direction rotations operate (clockwise or
> counterclockwise) and whether transpose is applied before or after flip
> for the transformations that have both set.
>
> > +     Transform reordered = t0;
> > +     if (!!(t1 & Transform::Transpose))
> > +             reordered = (t0 & Transform::Transpose) |
> > +                         (!!(t0 & Transform::HFlip) ? Transform::VFlip : Transform::Identity) |
> > +                         (!!(t0 & Transform::VFlip) ? Transform::HFlip : Transform::Identity);
> > +
> > +     return reordered ^ t1;
> > +}
> > +
> > +Transform operator-(Transform t)
> > +{
> > +     /* All are self-inverses, except for Rot270 and Rot90. */
> > +     static const Transform inverses[] = {
> > +             Transform::Identity, Transform::HFlip, Transform::VFlip, Transform::HVFlip,
> > +             Transform::Transpose, Transform::Rot90, Transform::Rot270, Transform::Rot180Transpose
>
> Maybe a few more line breaks to keep lines within the 80 columns limit ?
>
> > +     };
> > +
> > +     return inverses[static_cast<int>(t)];
> > +}
> > +
> > +Transform transformFromRotation(int angle, bool *success)
> > +{
> > +     angle = angle % 360;
> > +     if (angle < 0)
> > +             angle += 360;
> > +
> > +     if (success != nullptr)
> > +             *success = true;
> > +
> > +     if (angle == 0)
> > +             return Transform::Identity;
> > +     else if (angle == 90)
> > +             return Transform::Rot90;
> > +     else if (angle == 180)
> > +             return Transform::Rot180;
> > +     else if (angle == 270)
> > +             return Transform::Rot270;
> > +     else if (success != nullptr)
> > +             *success = false;
>
> Maybe a switch-case ?
>
>         switch (angle) {
>         case 0:
>                 return Transform::Identity;
>         case 90:
>                 return Transform::Rot90;
>         case 180:
>                 return Transform::Rot180;
>         case 270:
>                 return Transform::Rot270;
>         }
>
>         if (success != nullptr)
>                 *success = false;
>
> > +
> > +     return Transform::Identity;
> > +}
> > +
> > +std::string transformToString(Transform t)
> > +{
> > +     static const char *strings[] = {
> > +             "identity",
> > +             "hflip",
> > +             "vflip",
> > +             "hvflip",
> > +             "transpose",
> > +             "rot270",
> > +             "rot90",
> > +             "rot180transpose"
> > +     };
> > +
> > +     return strings[static_cast<int>(t)];
>
> How about returning a const char * ? If that's what the caller needs it
> will be cheaper, otherwise the caller can create an std::string()
> (mostly implicitly).
>
> > +}
> > +
> > +} /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart