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

[Laurent Pinchart]

Hello,

On Thu, Feb 04, 2021 at 06:15:28PM +0000, David Plowman wrote:
> 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.

Thanks for the review David. I've pushed the series after applying the
small fixes listed below.

> On Tue, 26 Jan 2021 at 23:07, Laurent Pinchart wrote:
> > 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