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

[Laurent Pinchart]

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
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