[libcamera-devel] [PATCH v2 6/9] libcamera: transform: Add operations with Orientation

[David Plowman]

Hi Jacopo

On Mon, 17 Jul 2023 at 16:03, Jacopo Mondi
<jacopo.mondi at ideasonboard.com> wrote:
>
> Hi David
>
> On Mon, Jul 17, 2023 at 03:01:52PM +0100, David Plowman via libcamera-devel wrote:
> > Hi
>
> [snip]
>
> > > > On a semi-related note, I find myself second-guessing the order of
> > > > transform composition. The documentation is pretty clear that we're
> > > > using traditional mathematical composition rules, but does that still
> > > > make sense?
> > > >
> > > > We defined operator*(orientation, transform) so that we have:
> > > >
> > > > o1 * t = o2
> > > >
> > > > but if t = t1 * t2 then (according to the current rules)
> > > >
> > > > o2 * (t1 * t2) = o2 and therefore (o2 * t2) * t1 = o2 so in fact o2 *
> > > > t2 * t1 = o2. Or in other words, these two flavours of * aren't
> > > > associative. It all feels wrong.
> > >
> > > I got the same doubt to be honest. I just found more natural to have
> > >
> > > Orientation = Orientation * transform
> > >
> > > which reads "The Transform you have to apply to an Orientation o1 to
> > > get Orientation o2" as an API, but I agree the fact I then need to
> > > swap operands in the implementation to apply the orientation first
> > > then the transform is misleading, even more so with multiple
> > > compositions as per your example
> > >
> > > > Having a prefix version, namely operator*(transform, orientation)
> > > > would fix this nastiness, but I do wonder whether it would be more
> > > > natural to change the transform order when we compose them. Opinions
> > > > sought!!
> > >
> > > I honestly find the composition notion implemented by
> > > operator*(Transform t1, Transform t2) a little bit confusing, but it
> > > is indeed more formally correct.
> > >
> > > I guess we have to chose between
> > >
> > > 1)
> > >
> > > operator*(Transform t1, Transform t2) = Apply t2 then t1
> > > operator*(Transform t, Orientation o) = Apply o then t
> > >
> > > 2)
> > >
> > > operator*(Transform t1, Transform t2) = Apply t1 then t2
> > > operator*(Orientation o, Transform t) = Apply o then t
> > >
> > > The only argument I have is the one I expressed above:
> > >
> > > "Orientation * transform" as "the Transform t applied to Orientation o"
> > >
> > > reads more naturally than
> > >
> > > "Transform * Orientation" as "the Transform t applied to Orientation o"
> > >
> > > But it's not a strong opinion. What is your preference ?
> >
> > Yes, this is exactly it, we need to choose either 1 or 2 to be
> > consistent. The mere fact that, the other week, I automatically went
> > for operator*(Orientation, Transform) probably underlines that 2 is
> > more natural, and I would therefore prefer to change to this view of
> > the world. But maybe we need to see if others are OK for us to do
> > this!
>
> Assuming nobody else has different ideas, would you like me to change
> the operation semantic as part of this series or do you prefer to do
> it yourself and update your consumers (libcamera-apps and picamera2)
> at the same time ?

Thanks for the offer, and I'd be very happy for you to do it!

libcamera-apps and Picamera2 both just pass the requested transform
through and don't do anything with it - that's because they're not
really "end-user applications" in the conventional sense, they're more
"tools for building the end-user application". So it would only be
user code that calls out to these that might be affected, and I doubt
there's very much.

Thanks!
David

>
> >
> > David
> >
> > >
> > > >
> > > > Thanks
> > > > David
> > > >
> > > > > +}
> > > > > +
> > > > >  /**
> > > > >   * \brief Return a character string describing the transform
> > > > >   * \param[in] t The transform to be described.
> > > > > --
> > > > > 2.40.1
> > > > >