[libcamera-devel] libcamera: v4l2_subdevice: Add JPEG_1X8 and BGR888_1X24 mbus formats to formatInfoMap
diff mbox series

Message ID 20221010092256.400870-1-xavier.roumegue@oss.nxp.com
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: v4l2_subdevice: Add JPEG_1X8 and BGR888_1X24 mbus formats to formatInfoMap
Related show

Commit Message

Xavier Roumegue Oct. 10, 2022, 9:22 a.m. UTC 
From: Xavier Roumegue <xavier.roumegue@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@oss.nxp.com>
---
 src/libcamera/v4l2_subdevice.cpp | 2 ++
 1 file changed, 2 insertions(+)

Comments

Jacopo Mondi Oct. 10, 2022, 9:37 a.m. UTC | #1 
Hi Xavier

On Mon, Oct 10, 2022 at 11:22:56AM +0200, Xavier Roumegue (OSS) via libcamera-devel wrote:
> From: Xavier Roumegue <xavier.roumegue@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@oss.nxp.com>

Just to point out that for JPEG, the entries we have in the main
formats map, report YUV as color encoding

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 } },
>  	{ 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 } },
>  };
>
>  } /* namespace */
> --
> 2.37.3
>
Xavier Roumegue Oct. 10, 2022, 10:40 a.m. UTC | #2 
Hi Jacopo,

On 10/10/22 11:37, Jacopo Mondi 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@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@oss.nxp.com>
> 
> Just to point out that for JPEG, the entries we have in the main
> formats map, report YUV as color encoding
> 
> 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
My poor colorspace knowledge does not give me a valuable opinion on that. It 
seems that jpeg is not really bound to a colorspace.
I posted a v2 version with YUV as color encoding for JPEG so that the encoding 
remains consistant in libcamera.

Xavier

>> ---
>>   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 } },
>>   	{ 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 } },
>>   };
>>
>>   } /* namespace */
>> --
>> 2.37.3
>>
Jacopo Mondi Oct. 10, 2022, 11:34 a.m. UTC | #3 
Hi Xavier

On Mon, Oct 10, 2022 at 12:40:36PM +0200, Xavier Roumegue (OSS) wrote:
> Hi Jacopo,
>
> On 10/10/22 11:37, Jacopo Mondi 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@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@oss.nxp.com>
> >
> > Just to point out that for JPEG, the entries we have in the main
> > formats map, report YUV as color encoding
> >
> > 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
> My poor colorspace knowledge does not give me a valuable opinion on that. It
> seems that jpeg is not really bound to a colorspace.
> I posted a v2 version with YUV as color encoding for JPEG so that the
> encoding remains consistant in libcamera.

Same for me, I cannot confirm which one is "right", but given that
colorspaces have been investigated in detail recently, I would use the
main formats table as a reference.

Thanks
  j

>
> Xavier
>
> > > ---
> > >   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 } },
> > >   	{ 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 } },
> > >   };
> > >
> > >   } /* namespace */
> > > --
> > > 2.37.3
> > >
Jacopo Mondi Oct. 10, 2022, 12:36 p.m. UTC | #4 
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@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@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
> > >
>
Umang Jain Oct. 10, 2022, 5:12 p.m. UTC | #5 
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@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@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

"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
>>
Umang Jain Oct. 14, 2022, 2:16 p.m. UTC | #6 
Hi Jacopo , Xavier

Following up on the discussion

On 10/10/22 6:06 PM, Jacopo Mondi wrote:
> 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@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@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.

That's correct, I guess we can't say will full confidence after all.

Hence I would adopt a pragmatic approach and say ColourEncodingYUV for 
JPEG media-bus format  as well.

There is v2 attached to this thread separately, so I'll give a R-B tag 
there.
>
>> "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
>>>>

Patch
diff mbox series 

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 } },
 	{ 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 } },
 };
 
 } /* namespace */