[libcamera-devel] [PATCH] libcamera: v4l2_subdevice: Add JPEG_1X8 and BGR888_1X24 mbus formats to formatInfoMap

[Jacopo Mondi]

Hi Umang

On Mon, Oct 10, 2022 at 10:42:23PM +0530, Umang Jain wrote:
> Hello
>
> On 10/10/22 3:07 PM, Jacopo Mondi via libcamera-devel wrote:
> > Hi Xavier
> >
> > On Mon, Oct 10, 2022 at 11:22:56AM +0200, Xavier Roumegue (OSS) via libcamera-devel wrote:
> > > From: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
> > >
> > > The warnings "Unknown subdev format 0x4001, defaulting to RGB encoding" and
> > > "Unknown subdev format 0x1013, defaulting to RGB encoding" are thrown while using
> > > simple pipeline handler with NXP ISI hardware.
> > > The JPEG_1X8 and BGR888_1X24 media bus formats, supported by the ISI driver, are
> > > missing in the V4L2SubdeviceFormatInfo structure storing the correspondence
> > > between a media bus format and a colour encoding. So populate the structure with
> > > the missing media bus formats.
> > >
> > > Signed-off-by: Xavier Roumegue <xavier.roumegue at oss.nxp.com>
> > Just to point out that for JPEG, the entries we have in the main
> > formats map, report YUV as color encoding
>
> Aren't we comparing two different things here. I remember asking Laurent
> that we do colorEncoding / PixelFormatInfo like structures for media-bus
> formats as well and we ended up with  [1]
>
> It's mentioned in the documentation [2] that V4L2 Pixel formats are
> different from media bus formats. So I guess the formats mentioned in
> formats.cpp do not apply / convey any kind of reference here

Sure the two identifier spaces represent different things, as
pixelformats describe how pixels are displaced in memory while media
bus formats describe how pixel are transported "on the wires", but as
MEDIA_BUS_FMT_YUYV_* and V4L2_PIX_FMT_YUYV (and NV16/61, for this matter)
convey the same information about the downsampling ratio between
the luminance and chrominances components (they're all 4:2:2
downsampled), it is legit to assume that JPEG-encoded formats share the
same encoding scheme.

However, being JPEG (in)famously under-specified as a term I cannot say with
full-confidence that ColourEncodingYUV is correct, but I would be
surprised if the color encoding scheme gets changed when transmitted
in JPEG_1X8 format are stored into memory.

>
> "The media bus pixel codes describe image formats as flowing over physical
> buses (both between separate physical components and inside SoC devices).
>  This should not be confused with the V4L2 pixel formats that describe,
> using four character codes, image formats as stored in memory."
>
> [1] https://git.libcamera.org/libcamera/libcamera.git/commit/?id=d5ad19b
> [2] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html#media-bus-pixel-codes
> >
> > src/libcamera/formats.cpp-      /* Compressed formats. */
> > src/libcamera/formats.cpp-      { formats::MJPEG, {
> > src/libcamera/formats.cpp-              .name = "MJPEG",
> > src/libcamera/formats.cpp-              .format = formats::MJPEG,
> > src/libcamera/formats.cpp-              .v4l2Formats = {
> > src/libcamera/formats.cpp-                      V4L2PixelFormat(V4L2_PIX_FMT_MJPEG),
> > src/libcamera/formats.cpp:                      V4L2PixelFormat(V4L2_PIX_FMT_JPEG),
> > src/libcamera/formats.cpp-              },
> > src/libcamera/formats.cpp-              .bitsPerPixel = 0,
> > src/libcamera/formats.cpp-              .colourEncoding = PixelFormatInfo::ColourEncodingYUV,
> > src/libcamera/formats.cpp-              .packed = false,
> > src/libcamera/formats.cpp-              .pixelsPerGroup = 1,
> > src/libcamera/formats.cpp-              .planes = {{ { 1, 1 }, { 0, 0 }, { 0, 0 } }},
> > src/libcamera/formats.cpp-      } },
> >
> > Unfortunately the kernel documentation for the format doesn't help
> > much:
> >
> > https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html?highlight=media_bus_fmt_jpeg_1x8
> > > ---
> > >   src/libcamera/v4l2_subdevice.cpp | 2 ++
> > >   1 file changed, 2 insertions(+)
> > >
> > > diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> > > index 9ef95963..f34eea24 100644
> > > --- a/src/libcamera/v4l2_subdevice.cpp
> > > +++ b/src/libcamera/v4l2_subdevice.cpp
> > > @@ -68,6 +68,7 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > >   	{ MEDIA_BUS_FMT_RGB565_2X8_LE, { 16, "RGB565_2X8_LE", PixelFormatInfo::ColourEncodingRGB } },
> > >   	{ MEDIA_BUS_FMT_RGB666_1X18, { 18, "RGB666_1X18", PixelFormatInfo::ColourEncodingRGB } },
> > >   	{ MEDIA_BUS_FMT_RGB888_1X24, { 24, "RGB888_1X24", PixelFormatInfo::ColourEncodingRGB } },
> > > +	{ MEDIA_BUS_FMT_BGR888_1X24, { 24, "BGR888_1X24", PixelFormatInfo::ColourEncodingRGB } },
>
> This looks good to me
> > >   	{ 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 } },
> > > @@ -133,6 +134,7 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
> > >   	{ 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::ColourEncodingRGB } },
>
> This needs some investigation to be sure...
> > >   };
> > >
> > >   } /* namespace */
> > > --
> > > 2.37.3
> > >
>