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

[David Plowman]

Hi Jacopo

On Thu, 19 Oct 2023 at 13:34, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> 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

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

I don't recall if we thought about the Python world - it's easy to
overlook. 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.

David

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