| Message ID | 20210126184854.46156-5-sebastian.fricke@posteo.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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. > 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@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@ideasonboard.com> 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; > } >
Hi David, Would you have a bit of time to review this ? On Wed, Jan 27, 2021 at 01:07:38AM +0200, Laurent Pinchart 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. > > 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@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@ideasonboard.com> > > 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; > > } > >
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@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@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@ideasonboard.com> Yes, I agree. So with this change, also: Reviewed-by: David Plowman <david.plowman@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
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@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@ideasonboard.com> > > Yes, I agree. So with this change, also: > > Reviewed-by: David Plowman <david.plowman@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; > > > } > > >
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. - * * \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; }
To transpose a BayerFormat means to flip it over its main-diagonal. For example: G B G R -> R G B G The main-diagonal goes from the top left to the bottom right. 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. 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@posteo.net> --- src/libcamera/bayer_format.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)