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

18969 diff mbox series

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

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: David Plowman <david.plowman@raspberrypi.com>
---
 src/libcamera/camera_sensor.cpp |  4 ++--
 src/libcamera/transform.cpp     | 19 +++++++++++--------
 2 files changed, 13 insertions(+), 10 deletions(-)

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

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 ?

> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: David Plowman <david.plowman@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

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@ideasonboard.com>
> > Reviewed-by: David Plowman <david.plowman@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

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@ideasonboard.com>
> > > Reviewed-by: David Plowman <david.plowman@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

Hi everyone

Yes, this seems like a long time ago!

On Thu, 19 Oct 2023 at 14:29, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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 ?

My recollection here was simply that it all felt less confusing when
we used the left-to-right convention. You can add, for example, " * t1
" to the end of an existing expression and t1 gets applied last.
Having to remember to put it at the front, when it's happening last,
just seemed to involve more cognitive effort than I would have
expected. When we switched, I could easily write stuff down and have
the instant "that's obviously correct" feeling.

But I agree... that's just how I felt. Whatever is less bug-prone for
the majority is probably the best answer.

David

>
> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > > Reviewed-by: David Plowman <david.plowman@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