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

[Laurent Pinchart]

Hi Jacopo,

CC'ing David for a question below.

On Thu, Oct 19, 2023 at 10:31:24AM +0200, Jacopo Mondi wrote:
> On Wed, Oct 18, 2023 at 10:51:13PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > 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.

What I mean is that we've followed the mathematical concepts of
composing functions. Transforms are functions, they're composed with the
∘ operator, which we replaced with operator*() as C++ doesn't allow
custom UTF-8 operators (I could image this being useful, but also
soooooo probe to abuse). If we follow this logic, using the call
operator to apply transforms would be quite natural:

	Orientation o = Transform::HFlip(Orientation::Rot180);

Sure, we can use our own conventions instead, but I think that's more
error prone than using established convensions, as users will have to
learn and change their frame of mind.

I don't recall how this was decided with David in August (I may not have
followed the discussion, or just forgotten about it, in either case,
sorry about that). David, was there or is there a particular reason why
departing from the usual function composition order would work better
for you ?

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