[libcamera-devel] [PATCH v5 05/12] libcamera: transform: Add functions to convert Orientation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Sep 01, 2023 at 05:02:08PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Add two helper functions to the transform.cpp file that allows to
> convert to and from an Orientation.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/transform.h |  4 +++
>  src/libcamera/transform.cpp   | 60 +++++++++++++++++++++++++++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h
> index 2a6838c78369..14f1db0e8572 100644
> --- a/include/libcamera/transform.h
> +++ b/include/libcamera/transform.h
> @@ -11,6 +11,8 @@
>  
>  namespace libcamera {
>  
> +enum class Orientation;
> +
>  enum class Transform : int {
>  	Identity = 0,
>  	Rot0 = Identity,
> @@ -69,6 +71,8 @@ constexpr Transform operator~(Transform t)
>  }
>  
>  Transform transformFromRotation(int angle, bool *success = nullptr);
> +Transform transformFromOrientation(const Orientation &orientation);
> +Orientation transformToOrientation(const Transform &transform);
>  
>  const char *transformToString(Transform t);
>  
> diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp
> index 4668303d0676..c70cac3f14ee 100644
> --- a/src/libcamera/transform.cpp
> +++ b/src/libcamera/transform.cpp
> @@ -7,6 +7,8 @@
>  
>  #include <libcamera/transform.h>
>  
> +#include <libcamera/orientation.h>
> +
>  /**
>   * \file transform.h
>   * \brief Enum to represent and manipulate 2D plane transforms
> @@ -299,6 +301,64 @@ Transform transformFromRotation(int angle, bool *success)
>  	return Transform::Identity;
>  }
>  
> +/**
> + * \brief Return the transform representing \a orientation
> + * \param[in] orientation The orientation to convert
> + * \return The transform corresponding to \a orientation
> + */

I don't like these functions much, as orientations and transforms are
not intrinsincly convertible. There's no cardinal concept such as "a
transform representing an orientation".

Fortunately, transformToOrientation() can easily be dropped on top of this
series, as one of its callers can go away (see reviews of other patches
in the series), and the code can then be folded in the other caller,
`Orientation operator*(const Orientation &o, const Transform &t)`.

Similarly, I would like to drop the call to transformFromOrientation()
in RPi::CameraData::configureIPA() and replace it there with usage of
`Transform operator/(const Orientation &o1, const Orientation &o2)`.

This can be done on top, as avoiding the introduction of these two
functions in the series may cause annoying conflicts when rebasing and
refactoring.

> +Transform transformFromOrientation(const Orientation &orientation)
> +{
> +	switch (orientation) {
> +	case Orientation::rotate0:
> +		return Transform::Identity;
> +	case Orientation::rotate0Flip:
> +		return Transform::HFlip;
> +	case Orientation::rotate180:
> +		return Transform::Rot180;
> +	case Orientation::rotate180Flip:
> +		return Transform::VFlip;
> +	case Orientation::rotate90Flip:
> +		return Transform::Transpose;
> +	case Orientation::rotate90:
> +		return Transform::Rot90;
> +	case Orientation::rotate270Flip:
> +		return Transform::Rot180Transpose;
> +	case Orientation::rotate270:
> +		return Transform::Rot270;
> +	}
> +
> +	return Transform::Identity;
> +}
> +
> +/**
> + * \brief Return the Orientation representing \a transform
> + * \param[in] transform The transform to convert
> + * \return The Orientation corresponding to \a transform
> + */
> +Orientation transformToOrientation(const Transform &transform)
> +{
> +	switch (transform) {
> +	case Transform::Identity:
> +		return Orientation::rotate0;
> +	case Transform::HFlip:
> +		return Orientation::rotate0Flip;
> +	case Transform::VFlip:
> +		return Orientation::rotate180Flip;
> +	case Transform::Rot180:
> +		return Orientation::rotate180;
> +	case Transform::Transpose:
> +		return Orientation::rotate90Flip;
> +	case Transform::Rot270:
> +		return Orientation::rotate270;
> +	case Transform::Rot90:
> +		return Orientation::rotate90;
> +	case Transform::Rot180Transpose:
> +		return Orientation::rotate270Flip;
> +	}
> +
> +	return Orientation::rotate0;
> +}
> +
>  /**
>   * \brief Return a character string describing the transform
>   * \param[in] t The transform to be described.

-- 
Regards,

Laurent Pinchart