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

[Jacopo Mondi]

Hi again

On Thu, Oct 19, 2023 at 12:34:08PM +0200, Jacopo Mondi via libcamera-devel wrote:
> 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 ?
>

Found it in 10/12

I think we can remove these two functions by:

- Introduce a orientationFromRotation() to replace

        rotationTransform_ = transformFromRotation(propertyValue, &success);

  in CameraSensor::initProperties()

  so that we can remove transformToOrientation() in CameraSensor::computeTransform()

- Move the RPi pipeline and IPA to use Orientation and remove

	params.transform =
		static_cast<unsigned int>(transformFromOrientation(config->orientation));

 so we can remove transformFromOrientation()

- Replace py_transform which uses Transform

This is all doable, but I would like to sync with RPi as I presume
they have code in their apps which is based on these interfaces

CC-ed David and Naush


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