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

[Umang Jain]

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

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