Message ID  20230901150215.115857jacopo.mondi@ideasonboard.com 

State  Superseded 
Headers  show 
Series 

Related  show 
Hi Jacopo, Thank you for the patch. On Fri, Sep 01, 2023 at 05:02:09PM +0200, Jacopo Mondi via libcameradevel 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 ? > Signedoffby: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Reviewedby: 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. > ~~~ > AB BA BD > @@ 206,7 +209,7 @@ Input image   > HFLip >   > Transpose >   = Rot270 > * Note that composition is generally noncommutative for Transforms, > * and not the same as XORing 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 libcameradevel wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Sep 01, 2023 at 05:02:09PM +0200, Jacopo Mondi via libcameradevel 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. > > Signedoffby: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Reviewedby: 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. > > ~~~ > > AB BA BD > > @@ 206,7 +209,7 @@ Input image   > HFLip >   > Transpose >   = Rot270 > > * Note that composition is generally noncommutative for Transforms, > > * and not the same as XORing 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 libcameradevel wrote: > > On Fri, Sep 01, 2023 at 05:02:09PM +0200, Jacopo Mondi via libcameradevel 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 UTF8 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 ? > > > Signedoffby: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > Reviewedby: 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. > > > ~~~ > > > AB BA BD > > > @@ 206,7 +209,7 @@ Input image   > HFLip >   > Transpose >   = Rot270 > > > * Note that composition is generally noncommutative for Transforms, > > > * and not the same as XORing 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 libcameradevel wrote: > > > On Fri, Sep 01, 2023 at 05:02:09PM +0200, Jacopo Mondi via libcameradevel 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 UTF8 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 lefttoright 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 bugprone for the majority is probably the best answer. David > > > > > Signedoffby: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Reviewedby: 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. > > > > ~~~ > > > > AB BA BD > > > > @@ 206,7 +209,7 @@ Input image   > HFLip >   > Transpose >   = Rot270 > > > > * Note that composition is generally noncommutative for Transforms, > > > > * and not the same as XORing 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
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. ~~~ AB BA BD @@ 206,7 +209,7 @@ Input image   > HFLip >   > Transpose >   = Rot270 * Note that composition is generally noncommutative for Transforms, * and not the same as XORing 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