[libcamera-devel] [PATCH] libcamera: v4l2: Pass set colorspace flags

[Umang Jain]

Hi,

On 8/1/22 12:59, Laurent Pinchart wrote:
> Hi Umang,
>
> On Mon, Aug 01, 2022 at 12:15:33PM +0530, Umang Jain wrote:
>> On 8/1/22 01:47, Laurent Pinchart wrote:
>>> On Fri, Jul 29, 2022 at 08:20:42PM +0530, Umang Jain via libcamera-devel wrote:
>>>> The colorspace fields as read-only from an application point of view,
>>> s/as read-only/are read-only/
>>>
>>>> both on video devices and on subdevs, unless the
>>>> V4L2_PIX_FMT_FLAG_SET_CSC or V4L2_MBUS_FRAMEFMT_SET_CSC flags
>>>> (respectively) are set when calling the S_FMT ioctl.
>>>>
>>>> Signed-off-by: Umang Jain<umang.jain at ideasonboard.com>
>>>> ---
>>>>    src/libcamera/v4l2_subdevice.cpp   | 3 ++-
>>>>    src/libcamera/v4l2_videodevice.cpp | 6 ++++--
>>>>    2 files changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
>>>> index 98a3911a..815c676e 100644
>>>> --- a/src/libcamera/v4l2_subdevice.cpp
>>>> +++ b/src/libcamera/v4l2_subdevice.cpp
>>>> @@ -459,7 +459,8 @@ intV4L2Subdevice::setFormat(unsigned  int pad, V4L2SubdeviceFormat *format,
>>>>    	subdevFmt.format.height = format->size.height;
>>>>    	subdevFmt.format.code = format->mbus_code;
>>>>    	subdevFmt.format.field = V4L2_FIELD_NONE;
>>>> -	fromColorSpace(format->colorSpace, subdevFmt.format);
>>>> +	if (fromColorSpace(format->colorSpace, subdevFmt.format) == 0)
>>> If format->colorSpace is nullopt, should be set the CSC flag ?
>> Good question, I remember I was wondering that as well...
>>
>> The documentation mentions:
>>
>> '''
>>
>> |V4L2_MBUS_FRAMEFMT_SET_CSC| then the application can set this field on
>> the source pad to request a specific colorspace for the media bus data
>>
>> '''
>>
>> So it seems flag is used to request a "specific" colorspace. So
>> *_DEFAULT are specific, I don't think so.
>>
>> So I think if format->colorSpace is nullopt, we should avoid setting
>> this flag, unless, we start mapping to *_DEFAULT to some kind of
>> specifics in the future (in libcamera).
> It's probably harmless to set the CSC flag with all the colorspace
> fields to *_DEFAULT, as it means the driver should then not modify what
> it does by default. I just feel a bit uncomfortable doing so, probably
> because there are only two drivers in the kernel that honour the CSC
> flag that it's hard to know what the right thing to do is.

Indeed, hard to infer as the flag is not currently widely used.

Looking from libcamera perspective, it doesn't make sense to set the 
flag with colorspace is nullopt.

To someone oblivious to the kernel in general, it might be confusing to 
look setting the flags, when colorspace is nullopt.

>
>>>> +		subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
>>> The flag is meant for source pads only, can you avoid setting it on sink
>>> pads ?
>> ack.
>>
>>>>    
>>>>    	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
>>>>    	if (ret) {
>>>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>>>> index 63911339..a969f7fa 100644
>>>> --- a/src/libcamera/v4l2_videodevice.cpp
>>>> +++ b/src/libcamera/v4l2_videodevice.cpp
>>>> @@ -940,7 +940,8 @@ intV4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat  *format, bool set)
>>>>    	pix->pixelformat = format->fourcc;
>>>>    	pix->num_planes = format->planesCount;
>>>>    	pix->field = V4L2_FIELD_NONE;
>>>> -	fromColorSpace(format->colorSpace, *pix);
>>>> +	if (fromColorSpace(format->colorSpace, *pix) == 0)
>>>> +		pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
>>> Similarly, the CSC flag is meant for capture devices only.
>> ack
>>
>>>>    
>>>>    	ASSERT(pix->num_planes <=std::size(pix->plane_fmt));
>>>>    
>>>> @@ -1010,7 +1011,8 @@ intV4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat  *format, bool set)
>>>>    	pix->pixelformat = format->fourcc;
>>>>    	pix->bytesperline = format->planes[0].bpl;
>>>>    	pix->field = V4L2_FIELD_NONE;
>>>> -	fromColorSpace(format->colorSpace, *pix);
>>>> +	if (fromColorSpace(format->colorSpace, *pix) == 0)
>>>> +		pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
>>>>    
>>>>    	ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);
>>>>    	if (ret) {