[PATCH v2 4/4] libcamera: v4l2subdev: Organise the formatInfoMap

Tue Jan 23 16:13:09 CET 2024

Quoting Laurent Pinchart (2024-01-23 15:06:55)
> On Tue, Jan 23, 2024 at 03:00:37PM +0000, Kieran Bingham wrote:
> > Quoting Laurent Pinchart (2024-01-23 14:49:00)
> > > On Tue, Jan 23, 2024 at 02:35:35PM +0000, Kieran Bingham wrote:
> > > > The formatInfoMap is large, cluttered and not sorted.
> > > 
> > > It's sorted in the same order as the formats in media-bus-format.h. We
> > > can depart from that given a good reason.
> > 
> > Aha, that wasn't clear.
> > 
> > I can either drop this patch or replace it with a comment stating to
> > match the sort order in media-bus-format.h if you prefer?
> > 
> > Personally I like the groups so that the mono/greyscale formats are
> > clearer ... but I don't really care either way.
> 
> A comment would be good indeed if we keep the current order. I think I
> have a slight preference for that, as it should make it easier to
> compile this map and the media-bus-format.h header to find missing
> formats, but I'm OK if you prefer changing the order. I think ordering
> bus formats is difficult as the names are a bit cryptic sometimes,
> neither alphabetical nor numerical ordering makes real sense, and then
> we get into endless bikeshedding to find a good order :-) The one in the

Well the only good order is one that helps a human find out if something
is missing. The only better order is one that a computer can do that for
us and prevent us needing to in the first place. So for now I'll just
add a comment saying we match the sort order of media-bus-format.h.

> kernel header file may or may not be good, but it has the advantage of
> freeing us from thinking about the issue.
> 
> > > > Organise the table into groups, and move the mono formats to their own
> > > > grouping, as they are currently added in arbitrary points in the table.
> > > > 
> > > > Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> > > > ---
> > > > v2: new patch
> > > > 
> > > >  src/libcamera/v4l2_subdevice.cpp | 14 +++++++++++---
> > > >  1 file changed, 11 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > > index 265240dbd405..d5605c7c8ac5 100644
> > > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > > @@ -57,6 +57,7 @@ struct V4L2SubdeviceFormatInfo {
> > > >   * bus codes
> > > >   */
> > > >  const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > > +     /* RGB */
> > > >       { MEDIA_BUS_FMT_RGB444_2X8_PADHI_BE, { 16, "RGB444_2X8_PADHI_BE", PixelFormatInfo::ColourEncodingRGB } },
> > > >       { MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE, { 16, "RGB444_2X8_PADHI_LE", PixelFormatInfo::ColourEncodingRGB } },
> > > >       { MEDIA_BUS_FMT_RGB555_2X8_PADHI_BE, { 16, "RGB555_2X8_PADHI_BE", PixelFormatInfo::ColourEncodingRGB } },
> > > > @@ -72,7 +73,14 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > >       { MEDIA_BUS_FMT_RGB888_2X12_BE, { 24, "RGB888_2X12_BE", PixelFormatInfo::ColourEncodingRGB } },
> > > >       { MEDIA_BUS_FMT_RGB888_2X12_LE, { 24, "RGB888_2X12_LE", PixelFormatInfo::ColourEncodingRGB } },
> > > >       { MEDIA_BUS_FMT_ARGB8888_1X32, { 32, "ARGB8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
> > > > +
> > > > +     /* Mono */
> > > >       { MEDIA_BUS_FMT_Y8_1X8, { 8, "Y8_1X8", PixelFormatInfo::ColourEncodingYUV } },
> > > > +     { MEDIA_BUS_FMT_Y10_1X10, { 10, "Y10_1X10", PixelFormatInfo::ColourEncodingYUV } },
> > > > +     { MEDIA_BUS_FMT_Y12_1X12, { 12, "Y12_1X12", PixelFormatInfo::ColourEncodingYUV } },
> > > > +     { MEDIA_BUS_FMT_Y16_1X16, { 16, "Y16_1X16", PixelFormatInfo::ColourEncodingYUV } },
> > > > +
> > > > +     /* YUV */
> > > >       { MEDIA_BUS_FMT_UV8_1X8, { 8, "UV8_1X8", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_UYVY8_1_5X8, { 12, "UYVY8_1_5X8", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_VYUY8_1_5X8, { 12, "VYUY8_1_5X8", PixelFormatInfo::ColourEncodingYUV } },
> > > > @@ -82,13 +90,10 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > >       { MEDIA_BUS_FMT_VYUY8_2X8, { 16, "VYUY8_2X8", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_YUYV8_2X8, { 16, "YUYV8_2X8", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_YVYU8_2X8, { 16, "YVYU8_2X8", PixelFormatInfo::ColourEncodingYUV } },
> > > > -     { MEDIA_BUS_FMT_Y10_1X10, { 10, "Y10_1X10", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_UYVY10_2X10, { 20, "UYVY10_2X10", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_VYUY10_2X10, { 20, "VYUY10_2X10", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_YUYV10_2X10, { 20, "YUYV10_2X10", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_YVYU10_2X10, { 20, "YVYU10_2X10", PixelFormatInfo::ColourEncodingYUV } },
> > > > -     { MEDIA_BUS_FMT_Y12_1X12, { 12, "Y12_1X12", PixelFormatInfo::ColourEncodingYUV } },
> > > > -     { MEDIA_BUS_FMT_Y16_1X16, { 16, "Y16_1X16", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_UYVY8_1X16, { 16, "UYVY8_1X16", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_VYUY8_1X16, { 16, "VYUY8_1X16", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_YUYV8_1X16, { 16, "YUYV8_1X16", PixelFormatInfo::ColourEncodingYUV } },
> > > > @@ -109,6 +114,8 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > >       { MEDIA_BUS_FMT_VYUY12_1X24, { 24, "VYUY12_1X24", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_YUYV12_1X24, { 24, "YUYV12_1X24", PixelFormatInfo::ColourEncodingYUV } },
> > > >       { MEDIA_BUS_FMT_YVYU12_1X24, { 24, "YVYU12_1X24", PixelFormatInfo::ColourEncodingYUV } },
> > > > +
> > > > +     /* Raw Bayer */
> > > >       { MEDIA_BUS_FMT_SBGGR8_1X8, { 8, "SBGGR8_1X8", PixelFormatInfo::ColourEncodingRAW } },
> > > >       { MEDIA_BUS_FMT_SGBRG8_1X8, { 8, "SGBRG8_1X8", PixelFormatInfo::ColourEncodingRAW } },
> > > >       { MEDIA_BUS_FMT_SGRBG8_1X8, { 8, "SGRBG8_1X8", PixelFormatInfo::ColourEncodingRAW } },
> > > > @@ -133,6 +140,7 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > > >       { MEDIA_BUS_FMT_SGBRG12_1X12, { 12, "SGBRG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > >       { MEDIA_BUS_FMT_SGRBG12_1X12, { 12, "SGRBG12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > >       { MEDIA_BUS_FMT_SRGGB12_1X12, { 12, "SRGGB12_1X12", PixelFormatInfo::ColourEncodingRAW } },
> > > > +
> > > >       /* \todo Clarify colour encoding for HSV formats */
> > > >       { MEDIA_BUS_FMT_AHSV8888_1X32, { 32, "AHSV8888_1X32", PixelFormatInfo::ColourEncodingRGB } },
> > > >       { MEDIA_BUS_FMT_JPEG_1X8, { 8, "JPEG_1X8", PixelFormatInfo::ColourEncodingYUV } },
> 
> -- 
> Regards,
> 
> Laurent Pinchart