[libcamera-devel] [PATCH 1/5] libcamera: Add Transform class
David Plowman
david.plowman at raspberrypi.com
Wed Jul 29 18:36:16 CEST 2020
Hi Laurent
Thanks for the feedback. That all looks good, let me just clarify one
or two points.
On Wed, 29 Jul 2020 at 16:12, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi David,
>
> Thank you for the patch.
>
> On Wed, Jul 29, 2020 at 10:31:50AM +0100, David Plowman wrote:
> > Add implementation of 2d plane transforms, and include a Transform
> > field in the CameraConfiguration to record the application's choice of
> > image transform for this session.
> > ---
> > include/libcamera/camera.h | 3 +
> > include/libcamera/meson.build | 1 +
> > include/libcamera/transform.h | 193 ++++++++++++++++++++++++++++++++++
> > 3 files changed, 197 insertions(+)
> > create mode 100644 include/libcamera/transform.h
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 4d1a4a9..fd9355e 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -16,6 +16,7 @@
> > #include <libcamera/request.h>
> > #include <libcamera/signal.h>
> > #include <libcamera/stream.h>
> > +#include <libcamera/transform.h>
> >
> > namespace libcamera {
> >
> > @@ -60,6 +61,8 @@ public:
> > bool empty() const;
> > std::size_t size() const;
> >
> > + Transform userTransform;
> > +
> > protected:
> > CameraConfiguration();
> >
>
> This needs to be documented in the CameraConfiguration class. Didn't
> doxygen warn you ? I would also split this to a separate patch, and
> update the existing pipeline handlers to always set userTransform to
> Identity in their validate() function.
I wasn't sure I'm qualified to touch existing pipeline handlers, but I
guess I can do so if necessary! I'm still uncertain whether
"adjusting" the transform is reasonable or whether failing is better.
In the Raspberry Pi pipeline handler I felt that adjusting a transform
to a completely different one isn't a "reasonable" change, and the
application should know and not carry on with the thing it didn't
want. It's not like changing an image size "a bit", after all. But I'm
open to persuasion...
>
> > 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..cfd5026
> > --- /dev/null
> > +++ b/include/libcamera/transform.h
> > @@ -0,0 +1,193 @@
> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> > +/*
> > + * Copyright (C) 2020, Google Inc.
>
> As mentioned in the reply to the cover letter, this should have your
> copyright.
>
> > + *
> > + * transform.h - Implementation of 2d plane transforms
>
> '2d' or '2D' ?
>
> > + */
> > +
> > +#ifndef __LIBCAMERA_TRANSFORM_H__
> > +#define __LIBCAMERA_TRANSFORM_H__
> > +
> > +#include <string>
>
> You should also include stdint.h for int32_t, although I think you could
> use int instead of int32_t.
>
> > +
> > +namespace libcamera {
> > +
> > +class Transform
> > +{
> > +public:
> > + constexpr Transform()
> > + : Transform(Identity)
> > + {
> > + }
> > +
> > + constexpr static Transform identity()
> > + {
> > + return Transform(Identity);
> > + }
> > +
> > + constexpr static Transform rot0()
> > + {
> > + return identity();
> > + }
> > +
> > + constexpr static Transform hflip()
> > + {
> > + return Transform(HFlip);
> > + }
> > +
> > + constexpr static Transform vflip()
> > + {
> > + return Transform(VFlip);
> > + }
> > +
> > + constexpr static Transform hvflip()
> > + {
> > + return Transform(HVFlip);
> > + }
> > +
> > + constexpr static Transform rot180()
> > + {
> > + return hvflip();
> > + }
> > +
> > + constexpr static Transform transpose()
> > + {
> > + return Transform(Transpose);
> > + }
> > +
> > + constexpr static Transform rot270()
> > + {
> > + return Transform(Rot270);
> > + }
> > +
> > + constexpr static Transform rot90()
> > + {
> > + return Transform(Rot90);
> > + }
> > +
> > + constexpr static Transform rot180Transpose()
> > + {
> > + return Transform(Rot180Transpose);
> > + }
> > +
> > + constexpr static Transform hvflipTranspose()
> > + {
> > + return rot180Transpose();
> > + }
> > +
>
> Wouldn't it be simpler to make the Type enum public, as well as the
> constructor that takes a Type ? I will also be way less code to type :-)
I did wonder about this. I didn't like making the Type public because
either you use enum class and end up writing stuff like
Transform(Transform::Type::Identity) instead of Transform::identity(),
or you could use a straight enum and write
Transform(Transform::Identity) which I like, but you have to worry
about what this means:
Transform transform = Transform::Identity * Transform::HFlip;
So I thought typing more in the header file was better than putting
the burden on the client code. But I don't feel very strongly...
>
> > + constexpr static bool rotation(int32_t angle, Transform &xfm)
> > + {
> > + bool success = true;
> > + angle = angle % 360;
> > + if (angle < 0)
> > + angle += 360;
> > +
> > + if (angle == 0)
> > + xfm = identity();
> > + else if (angle == 90)
> > + xfm = rot90();
> > + else if (angle == 180)
> > + xfm = rot180();
> > + else if (angle == 270)
> > + xfm = rot270();
> > + else
> > + success = false;
> > +
> > + return success;
> > + }
> > +
> > + constexpr bool rotation(int32_t &angle) const
> > + {
> > + bool success = true;
> > +
> > + if (type_ == Identity)
> > + angle = 0;
> > + else if (type_ == Rot90)
> > + angle = 90;
> > + else if (type_ == HVFlip)
> > + angle = 180;
> > + else if (type_ == Rot270)
> > + angle = 270;
> > + else
> > + success = false;
> > +
> > + return success;
> > + }
>
> That's two "rotation" functions, one applying a rotation, the other one
> returning the rotation. I think it's a bit confusing. I would turn the
> former into
>
> bool rotate(int angle);
>
> and the later into
>
> constexpr unsigned int rotationAngle(bool *success = nullptr) const;
>
> I also think the two functions should be implemented in the .cpp file.
Yes, two functions with the same name was always nasty. The first one
is actually creating a Transform from a rotation angle, not applying a
rotation (so obviously documentation will have to be clear on that).
If we prefer the success code as the optional parameter it could look like:
constexpr static Transform rotation(int angle, bool *success = nullptr);
For the second one I like rotationAngle.
>
> > +
> > + constexpr bool contains(Transform xfm) const
> > + {
> > + return (type_ & xfm.type_) == xfm.type_;
> > + }
>
> This function is used in the rest of the series to check if a
> transformation contains hflip, vflip or transpose. For that use case
> it's fine, but it could also be used with
>
> Transform::rot180Transpose().contains(Transform::rot270())
>
> and return true, which is quite confusing to read. I'm not sure how to
> improve that though. Maybe defining three functions, to test for hflip,
> vflip and transpose separately ? isHorizontallyFlipped(),
> isVerticallyFlipped, isTransposed ? Or isHFlipped(), isVFlipped() for
> the first two ?
Yep, I could go with isHFlipped(), isVFlipped(), isTransposed().
Thanks!
David
>
> > +
> > + constexpr Transform inverse() const
> > + {
> > + /* They're all self-inverses, except for Rot90 and Rot270. */
> > + const Type inverses[] = { Identity, HFlip, VFlip, HVFlip,
> > + Transpose, Rot90, Rot270, Rot180Transpose };
>
> Shouldn't this be static const ?
>
> > +
> > + return Transform(inverses[type_]);
> > + }
> > +
> > + constexpr Transform operator*(Transform xfm) const
> > + {
> > + /*
> > + * Reorder the operations so that we imagine doing xfm's transpose
> > + * (if any) after our flips. The effect is to swap our hflips for
> > + * vflips and vice versa, after which we can just xor all the bits.
> > + */
> > + int reorderedType = type_;
> > + if (xfm.type_ & Transpose)
> > + reorderedType = (type_ & 4) | ((type_ & 1) << 1) | ((type_ & 2) >> 1);
> > +
> > + return Transform((Type)(reorderedType ^ xfm.type_));
> > + }
>
> I would move the implementation of these two functions to the .cpp file
> too.
>
> > +
> > + bool operator==(Transform xfm) const
> > + {
> > + return type_ == xfm.type_;
> > + }
> > +
> > + bool operator!=(Transform xfm) const
> > + {
> > + return !(*this == xfm);
> > + }
> > +
> > + std::string toString() const
> > + {
> > + char const *strings[] = {
>
> static const char ?
>
> > + "identity",
> > + "hflip",
> > + "vflip",
> > + "hvflip",
> > + "transpose",
> > + "rot270",
> > + "rot90",
> > + "rot180transpose"
> > + };
> > + return strings[type_];
> > + }
>
> Of course all this needs documentation :-) A good introduction (in the
> \class Transform block) would be very useful to explain the concepts I
> think.
>
> > +
> > +private:
> > + enum Type : int {
> > + Identity = 0,
> > + HFlip = 1,
> > + VFlip = 2,
> > + HVFlip = HFlip | VFlip,
> > + Transpose = 4,
> > + Rot270 = HFlip | Transpose,
> > + Rot90 = VFlip | Transpose,
> > + Rot180Transpose = HFlip | VFlip | Transpose
> > + };
> > +
> > + constexpr Transform(Type type)
> > + : type_(type)
> > + {
> > + }
> > +
> > + Type type_;
> > +};
> > +
> > +} /* namespace libcamera */
> > +
> > +#endif /* __LIBCAMERA_TRANSFORM_H__ */
> > +
>
> Extra blank line.
>
> --
> Regards,
>
> Laurent Pinchart
More information about the libcamera-devel
mailing list