[libcamera-devel] [PATCH 1/5] libcamera: Add Transform class
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 29 18:49:47 CEST 2020
Hi David,
On Wed, Jul 29, 2020 at 05:36:16PM +0100, David Plowman wrote:
> 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 wrote:
> > 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...
In that case we would need a mechanism to report the supported
transformations, otherwise an application wouldn't know what do to if
validate() fails. And speaking of that, wouldn't such a mechanism make
sense in any case ?
> > > 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...
Exposing the enum would avoid the nasty reinterpret_cast in the next
patch. I think an enum class would make sense, and if we really want to
avoid using Transform::Type::Identity, we could add
static const Transform Identity = Transform(Transform::Type::Identity);
(same for the other ones) to allow usage of Transform::Identity. It may
be a bit overkill though. In any case, I think
static const Transform Identity = Transform(Transform::Type::Identity);
would be better than
constexpr static Transform identity()
{
return Transform(Identity);
}
as there's no need for functions.
Exposing the enum would also allow using it in a bitfield to expose the
supported values if needed, for instance to report the supported values.
See https://lists.libcamera.org/pipermail/libcamera-devel/2020-July/011279.html.
> > > + 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);
Wouldn't it make sense to apply a rotation to an existing Transform ?
That's what I envisioned with
bool rotate(int angle);
and one could do
Transform t = Transform::Identity;
t.rotate(90);
to achieve the same result. Or if we want to support
Transform t = Transform::Identity.rotate(90);
then the function could be defined as
Transform rotate(int angle, bool *success = nullptr) const;
It may make sense to call it rotated() in that case ("rotate" modifies
the instance by applying a rotation, "rotated" returns a new instance
that results from applying the rotation). This is the naming scheme we
went for in geometry.h, see Size::alignDownTo() vs.
Size::alignedDownTo(). Both rotate() and rotated() could be provided if
needed.
> 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().
>
> > > +
> > > + 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