[libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add operations with Orientation

[David Plowman]

Hi Jacopo

Thanks for the patch!

On Fri, 14 Jul 2023 at 15:16, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Add two operations that allows to combine Transform with Orientation.

I see that the English version is a bit awkward here. Maybe s/allows/allow us/?

>
> - Transform operator/(Orientation1, Orientation2)
>   allows to easily get back the Transform that needs to be applied to
>   Orientation2 to get Orientation1
>
> - Orientation operator*(Orientation1, Transform)
>   allows to apply a Transform to an Orientation and obtain back the

s/back//? The "back" seems marginally unnecessary to me.

>   combination of the two
>
> The two operations allow application that are willing to use Transform
> to operate with the Orientation part of CameraConfiguration.

Not sure about the wording, "are willing" sounds a bit like we're
trying to force them, or something. Maybe

"These two operations allow applications to use Transforms to
manipulate the Orientation inside the CameraConfiguration, if they
wish."

But please adjust it to something you're comfortable with!

>
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> ---
>  include/libcamera/transform.h |  5 ++
>  src/libcamera/transform.cpp   | 98 +++++++++++++++++++++++++++++++++++
>  2 files changed, 103 insertions(+)
>
> diff --git a/include/libcamera/transform.h b/include/libcamera/transform.h
> index 573adf18715d..a7470c70a755 100644
> --- a/include/libcamera/transform.h
> +++ b/include/libcamera/transform.h
> @@ -74,6 +74,11 @@ Transform transformFromRotation(int angle, bool *success = nullptr);
>  Transform transformFromOrientation(const CameraConfiguration::Orientation &orientation);
>  CameraConfiguration::Orientation transformToOrientation(const Transform &transform);
>
> +Transform operator/(CameraConfiguration::Orientation o1,
> +                   CameraConfiguration::Orientation o2);
> +CameraConfiguration::Orientation operator*(CameraConfiguration::Orientation o1,
> +                                          Transform t);
> +
>  const char *transformToString(Transform t);
>
>  } /* namespace libcamera */
> diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp
> index 03d2b52e7f38..61dbb1ff1dd5 100644
> --- a/src/libcamera/transform.cpp
> +++ b/src/libcamera/transform.cpp
> @@ -357,6 +357,104 @@ CameraConfiguration::Orientation transformToOrientation(const Transform &transfo
>         return CameraConfiguration::rotate0;
>  }
>
> +/**
> + * \brief Return the Transform that applied to \a o2 gives \a o1
> + * \param o1 The Orientation to obtain
> + * \param o2 The base Orientation
> + *
> + * This operation can be used to easily compute the Transform to apply to a
> + * base orientation \a o2 to get the desired orientation \a o1.
> + *
> + * The CameraSensor class uses this function to compute what Transform to apply
> + * to a camera with mounting rotation \a o2 (the base) to obtain the user
> + * requested orientation \a o1.
> + *
> + * \return A Transform that applied to \a o2 gives \a o1
> + */
> +Transform operator/(CameraConfiguration::Orientation o1,
> +                   CameraConfiguration::Orientation o2)
> +{
> +       Transform t1 = transformFromOrientation(o1);
> +       Transform t2 = transformFromOrientation(o2);
> +
> +       /* Return immediately if the two are identical. */
> +       if (t1 == t2)
> +               return Transform::Identity;
> +
> +       /* If t1 is identity we need to apply -t2 to get it. */
> +       if (t1 == Transform::Identity)
> +               return -t2;
> +
> +       /* If t2 is identity, to get t1 we still need to do... t1. */
> +       if (t2 == Transform::Identity)
> +               return t1;
> +
> +       Transform combined = t1 ^ t2;
> +       Transform result = combined;
> +
> +       /*
> +        * If the base orientation is transposed we need to invert the flips on
> +        * the combined result.
> +        *
> +        * Using Rot90 as an example:
> +        *
> +        * Rot180 / Rot90 = Rot90
> +        *  = (H|V) ^ (T|V) = (T|H)
> +        *  = invert_flips(T|H) = (T|V) = Rot90
> +        *
> +        * Rot0 / Rot90 = Rot270
> +        *  = () ^ (T|V) = (T|V)
> +        *  = invert_flips(T|V) = (T|H) = Rot270
> +        *
> +        * Rot270 / Rot90 = Rot180
> +        *  = (T|H) ^ (T|V) = (H|V)
> +        *  = invert_flips(H|V) = (V|H) = Rot180
> +        *
> +        * Rot180Transpose / Rot90 = V
> +        *  = (T|H|V) ^ (T|V) = (H)
> +        *  = invert_flips(H) = (V)
> +        *
> +        * Rot270 / Transpose = V
> +        *  = (T|H) ^ T = H
> +        *  = invert_flips(H) = (V)
> +        */
> +       if (!!(t2 & Transform::Transpose)) {
> +               result = combined & Transform::Transpose;
> +               if (!!(combined & Transform::HFlip))
> +                       result |= Transform::VFlip;
> +               if (!!(combined & Transform::VFlip))
> +                       result |= Transform::HFlip;
> +       }
> +
> +       return result;
> +}

I was expecting something a bit simpler here, maybe

       Transform t1 = transformFromOrientation(o1);
       Transform t2 = transformFromOrientation(o2);

        return transformToOrientation(t1 * -t2);

And it does have the benefit that you can glance at it and think
"yeah, it does what I expect". And if it isn't as simple as that, then
there's probably something wrong!!

> +
> +/**
> + * \brief Apply the Transform \a t on the base orientation \a o
> + * \param o The base orientation
> + * \param t The transform to apply on \a o
> + * \return The Orientation resulting from applying \a t on \a o
> + */
> +CameraConfiguration::Orientation operator*(CameraConfiguration::Orientation o,
> +                                          Transform t)
> +{
> +       /* Apply an Indentity always gives us back the other operand. */
> +       if (t == Transform::Identity)
> +               return o;
> +
> +       /*
> +        * As operator*(Transform t2, Transform t1) composes two Tranform
> +        * by applying t1 first then t2 according to the canonical function
> +        * composition notion, we here first apply a Transform corresponding to
> +        * the base orientation and the apply \a t to it.
> +        */
> +       Transform t1 = transformFromOrientation(o);
> +       if (t1 == Transform::Identity)
> +               return transformToOrientation(t);
> +
> +       return transformToOrientation(t * t1);

I would probably have gone straight to the final "return" statement,
without those other tests. I think it makes the whole function easier
to read and understand, and I don't think there's a meaningful gain
from the special cases.

On a semi-related note, I find myself second-guessing the order of
transform composition. The documentation is pretty clear that we're
using traditional mathematical composition rules, but does that still
make sense?

We defined operator*(orientation, transform) so that we have:

o1 * t = o2

but if t = t1 * t2 then (according to the current rules)

o2 * (t1 * t2) = o2 and therefore (o2 * t2) * t1 = o2 so in fact o2 *
t2 * t1 = o2. Or in other words, these two flavours of * aren't
associative. It all feels wrong.

Having a prefix version, namely operator*(transform, orientation)
would fix this nastiness, but I do wonder whether it would be more
natural to change the transform order when we compose them. Opinions
sought!!

Thanks
David

> +}
> +
>  /**
>   * \brief Return a character string describing the transform
>   * \param[in] t The transform to be described.
> --
> 2.40.1
>