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

[Laurent Pinchart]

Hello,

On Thu, Oct 19, 2023 at 02:05:38PM +0100, David Plowman wrote:
> On Thu, 19 Oct 2023 at 13:34, Jacopo Mondi wrote:
> > On Thu, Oct 19, 2023 at 12:34:08PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > On Thu, Oct 19, 2023 at 01:22:34AM +0300, Laurent Pinchart via libcamera-devel wrote:
> > > > 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 ?

Orientation describes how an image is stored in a buffer, while
Transform describes how it is processed. I think saying

	Orientation::Rot0 * Transform::HVFlip == Orientation::Rot180

makes sense, but saying

	Transform::HVFlip == Orientation::Rot180

doesn't.

> > > > 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
> 
> I don't think we care too much in our C++ apps. When these patches go
> in, we might need to convert between Transforms and Orientations here
> and there but that's easily accomplished using the * operator that we
> defined, or / (in the other direction).

That's what I had in mind. I'll experiment with removing these two
functions and replacing them with the * and / operators.

> I don't recall if we thought about the Python world - it's easy to
> overlook.

Indeed. Fortunately we have you and Tomi to remind us about Python when
needed :-)

> Here we have an API that's in use by users and exposes
> Transforms. We'd have the option either to change those over to
> Orientations and break things, or we could leave the Transforms alone
> and convert them internally. In the worst case one could just convert
> with a lookup table, but it would be nice to compose orientations and
> transforms as we can in the C++ world.

Ideally we should also standardize on Orientation in Python, but if
there are clear needs to be able to transform orientations in
application code, then exposing a Transform class would also make sense
as a convenience helper (it's *so* easy to get these things wrong). It
could be coded entirely in Python, no need to involve the Python
bindings there.

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