[libcamera-devel] [PATCH v5 06/12] libcamera: transform: Invert operator*() operands

[Jacopo Mondi]

On Wed, Oct 18, 2023 at 10:51:13PM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Sep 01, 2023 at 05:02:09PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > The current definition of operator*(Transform t1, Transform t0) follows
> > the function composition notion, where t0 is applied first then t1 is
> > applied last.
> >
> > In order to introduce operator*(Orientation, Transform) where a
> > Transform is applied on top of an Orientation, invert the operand order
> > of operator*(Transform, Transform) so that usage of operator* with both
> > Orientation and Transform can be made associative.
> >
> > For example:
> >
> > Orientation o;
> > Transform t = t1 * t2
> > Orientation o1 = o * t
> > 	       = o * (t1 * t2) = (o * t1) * t2 = o * t1 * t2
>
> So, if we want to follow mathematical conventions, we shouldn't write
>
> 	Orientation o1 = o * t;
>
> but
>
> 	Orientation o1 = t(o);
>

Why ?

We're defining operators, the idea to follow the function composition
notion was in Transform but was an arbitrary pick.

If we assign to operator*(t1, t2) the semantic "apply t1 and then t2
on top" I think it's equally valid

> which can be done by implementing
>
> 	Orientation Transform::operator(const Orientation *orientation);
>
> This would lead to
>
> 	t2(t1(o)) == (t2 * t1)(o)
>
> Would that be a better alternative ?
>

This has been agreed with David in August, which has implemented
support in libcamera apps for this notion. If we both have to change
this I would like a more detailed reason.

> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  src/libcamera/camera_sensor.cpp |  4 ++--
> >  src/libcamera/transform.cpp     | 19 +++++++++++--------
> >  2 files changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 3ba364c44a40..038d8b959072 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -1051,7 +1051,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const
> >  	 * Combine the requested transform to compensate the sensor mounting
> >  	 * rotation.
> >  	 */
> > -	Transform combined = *transform * rotationTransform_;
> > +	Transform combined = rotationTransform_ * *transform;
> >
> >  	/*
> >  	 * We combine the platform and user transform, but must "adjust away"
> > @@ -1080,7 +1080,7 @@ Transform CameraSensor::validateTransform(Transform *transform) const
> >  		 * If the sensor can do no transforms, then combined must be
> >  		 * changed to the identity. The only user transform that gives
> >  		 * rise to this is the inverse of the rotation. (Recall that
> > -		 * combined = transform * rotationTransform.)
> > +		 * combined = rotationTransform * transform.)
> >  		 */
> >  		*transform = -rotationTransform_;
> >  		combined = Transform::Identity;
> > diff --git a/src/libcamera/transform.cpp b/src/libcamera/transform.cpp
> > index c70cac3f14ee..d192d63d9290 100644
> > --- a/src/libcamera/transform.cpp
> > +++ b/src/libcamera/transform.cpp
> > @@ -189,14 +189,17 @@ Input image   | |   goes to output image   | |
> >   */
> >
> >  /**
> > - * \brief Compose two transforms together
> > - * \param[in] t1 The second transform
> > - * \param[in] t0 The first transform
> > + * \brief Compose two transforms by applying \a t0 first then \a t1
> > + * \param[in] t0 The first transform to apply
> > + * \param[in] t1 The second transform to apply
> > + *
> > + * Compose two transforms by applying \a t1 after \a t0. The operation
> > + * is conceptually equivalent to the canonical notion of function composition,
> > + * with inverse order of operands. If in the canonical function composition
> > + * notation "f * g" equals to "f(g())", the notation for Transforms composition
> > + * "t0 * t1" equals to "t1(t0()))" where \a t0 is applied first, then \a t1.
> >   *
> > - * Composing transforms follows the usual mathematical convention for
> > - * composing functions. That is, when performing `t1 * t0`, \a t0 is applied
> > - * first, and then \a t1.
> > - * For example, `Transpose * HFlip` performs `HFlip` first and then the
> > + * For example, `HFlip * Transpose` performs `HFlip` first and then the
> >   * `Transpose` yielding `Rot270`, as shown below.
> >  ~~~
> >               A-B                 B-A                     B-D
> > @@ -206,7 +209,7 @@ Input image  | |   -> HFLip ->   | |   -> Transpose ->   | |   = Rot270
> >   * Note that composition is generally non-commutative for Transforms,
> >   * and not the same as XOR-ing the underlying bit representations.
> >   */
> > -Transform operator*(Transform t1, Transform t0)
> > +Transform operator*(Transform t0, Transform t1)
> >  {
> >  	/*
> >  	 * Reorder the operations so that we imagine doing t0's transpose
>
> --
> Regards,
>
> Laurent Pinchart