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

[Jacopo Mondi]

Hi Laurent

On Thu, Oct 19, 2023 at 01:22:34AM +0300, Laurent Pinchart via libcamera-devel wrote:
> 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".
>

Are we sure ? Orientation and Transform describe exactly the same set
of 2d plane transformations, isn't there a 1-to-1 relationship ?

> 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)`.

You know, I can't find any comment related to this. Can you provide a
reference ?

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