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

[Umang Jain]

Hi Laurent,

On 8/1/22 01:47, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> 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).

>
>> +		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) {
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20220801/19d3f53a/attachment.htm>