[libcamera-devel] [PATCH v3 4/5] libcamera: Add the transpose transformation

[David Plowman]

Hi Sebastian, Laurent,

Yes, when I first saw this patch - back when we didn't handle
transpose - I thought "why don't we do transpose?", rather forgetting
that it was me who chickened out of doing it in the first place!

But I think it's actually good to handle it in the same manner as we
do elsewhere, first the flips and then the transpose after. It would
seem like an omission if we didn't, and you never know, someone might
want to apply an arbitrary transform to a Bayer pattern one day.

Of course, users have to be aware of the order, but this is
implemented the same as everywhere else and so is consistent. I
suppose if you really wanted the other order, it's not difficult to
accomplish.

On Tue, 26 Jan 2021 at 23:07, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> Hi Sebastian,
>
> Thank you for the patch.
>
> On Tue, Jan 26, 2021 at 07:48:53PM +0100, Sebastian Fricke wrote:
> > To transpose a BayerFormat means to flip it over its main-diagonal.
>
> s/main-diagonal/main diagonal/
>
> >
> > For example:
> > G B    G R
> >     ->
> > R G    B G
> >
> > The main-diagonal goes from the top left to the bottom right.
>
> Same here.
>
> > This means, that the only two orders affected by a transpose
> > are GBRG & GRBG. When a transpose is used in combination with horizontal
> > and/or vertical flips it is performed with the lowest priority.

Just a tiny nit-pick here. "with the lowest priority" sounds a bit
vague to me, I think "after the flips" is clearer.

> > Therefore add the functionality by switching GBRG (index 1) with GRBG
> > (index 2), after the flips have been applied.
> >
> > Signed-off-by: Sebastian Fricke <sebastian.fricke at posteo.net>
> > ---
> >  src/libcamera/bayer_format.cpp | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/bayer_format.cpp b/src/libcamera/bayer_format.cpp
> > index d4e7f142..e06cd6e8 100644
> > --- a/src/libcamera/bayer_format.cpp
> > +++ b/src/libcamera/bayer_format.cpp
> > @@ -272,10 +272,6 @@ BayerFormat BayerFormat::fromV4L2PixelFormat(V4L2PixelFormat v4l2Format)
> >   * The transformed image would have a GRBG order. The bit depth and modifiers
> >   * are not affected.
> >   *
> > - * Note that transpositions are ignored as the order of a transpose with
> > - * respect to the flips would have to be defined, and sensors are not expected
> > - * to support transposition.
> > - *
>
> This needs to now document the order in which the flips and transpose
> are applied. Maybe something along the lines of
>
>   * Horizontal and vertical flips are applied before transpose.
>
> With this change,
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

Yes, I agree. So with this change, also:

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

Thanks very much for all this work!
David

>
> but I'd like to get David's feedback on this.
>
> >   * \return The transformed Bayer format
> >   */
> >  BayerFormat BayerFormat::transform(Transform t) const
> > @@ -292,6 +288,11 @@ BayerFormat BayerFormat::transform(Transform t) const
> >       if (!!(t & Transform::VFlip))
> >               result.order = static_cast<Order>(result.order ^ 2);
> >
> > +     if (!!(t & Transform::Transpose) && result.order == 1)
> > +             result.order = static_cast<Order>(2);
> > +     else if (!!(t & Transform::Transpose) && result.order == 2)
> > +             result.order = static_cast<Order>(1);
> > +
> >       return result;
> >  }
> >
>
> --
> Regards,
>
> Laurent Pinchart