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

9264 diff mbox series

We implement 2d transforms as an enum class with 8 elements,
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.
---
 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

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 */

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@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