[libcamera-devel,1/5] libcamera: Add Transform class
diff mbox series

Message ID 20200729093154.12296-2-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Transform implementation
Related show

Commit Message

David Plowman July 29, 2020, 9:31 a.m. UTC 
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

Comments

Laurent Pinchart July 29, 2020, 3:12 p.m. UTC | #1 
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.

> 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 :-)

> +	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.

> +
> +	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 ?

> +
> +	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.
David Plowman July 29, 2020, 4:36 p.m. UTC | #2 
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@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
Laurent Pinchart July 29, 2020, 4:49 p.m. UTC | #3 
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.

Patch
diff mbox series 

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();
 
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.
+ *
+ * transform.h - Implementation of 2d plane transforms
+ */
+
+#ifndef __LIBCAMERA_TRANSFORM_H__
+#define __LIBCAMERA_TRANSFORM_H__
+
+#include <string>
+
+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();
+	}
+
+	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;
+	}
+
+	constexpr bool contains(Transform xfm) const
+	{
+		return (type_ & xfm.type_) == xfm.type_;
+	}
+
+	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 };
+
+		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_));
+	}
+
+	bool operator==(Transform xfm) const
+	{
+		return type_ == xfm.type_;
+	}
+
+	bool operator!=(Transform xfm) const
+	{
+		return !(*this == xfm);
+	}
+
+	std::string toString() const
+	{
+		char const *strings[] = {
+			"identity",
+			"hflip",
+			"vflip",
+			"hvflip",
+			"transpose",
+			"rot270",
+			"rot90",
+			"rot180transpose"
+		};
+		return strings[type_];
+	}
+
+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__ */
+