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

[David Plowman]

Hi Jacopo

Thanks for doing this!

On Tue, 18 Jul 2023 at 11:52, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> 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
> Transform t3 = o * t

Strictly, o * t gives us an orientation and not a transform. But
otherwise I agree!

Reviewed-by: David Plowman <david.plowman at raspberrypi.com>

Thanks!
David

>              = o * (t1 * t2) = (o * t1) * t2 = o * t1 * t2
>
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.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
> --
> 2.40.1
>