[libcamera-devel] [PATCH 1/5] libcamera: Add Transform class

[Laurent Pinchart]

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.

-- 
Regards,

Laurent Pinchart