[libcamera-devel] [PATCH v2] libcamera: v4l2: Set colorspace flags

[Umang Jain]

Hi Laurent,

On 8/4/22 23:09, Laurent Pinchart wrote:
> Hi Umang,
>
> Thank you for the patch.
>
> On Mon, Aug 01, 2022 at 01:54:20PM +0530, Umang Jain via libcamera-devel wrote:
>> The colorspace fields are read-only from an application point of view,
>> 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>
>> ---
>> changes in v2:
>> - Set V4L2_MBUS_FRAMEFMT_SET_CSC flag only on source pad of
>>    media entity
>> - Set V4L2_PIX_FMT_FLAG_SET_CSC on video capture devices only
>> ---
>>   src/libcamera/v4l2_subdevice.cpp   | 6 ++++--
>>   src/libcamera/v4l2_videodevice.cpp | 8 ++++++--
>>   2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
>> index 98a3911a..8f6b327f 100644
>> --- a/src/libcamera/v4l2_subdevice.cpp
>> +++ b/src/libcamera/v4l2_subdevice.cpp
>> @@ -459,9 +459,11 @@ int V4L2Subdevice::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);
>> +	int ret = fromColorSpace(format->colorSpace, subdevFmt.format);
>> +	if (ret == 0 && (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE))
>> +		subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> I don't think that's the right condition, as from ColorSpace returns 0
> if colorSpace is nullopt. I'd write it as


When colorSpace is nullopt, v4l2Format colorspace identifiers set to 
*_DEFAULT. Doesn't that count?

/me *grins*

>
> 	if (format->colorSpace) {
> 		fromColorSpace(format->colorSpace, subdevFmt.format);
>
> 		/* The CSC flag is only applicable to source pads. */
> 		if (entity_->pads()[pad]->flags() & MEDIA_PAD_FL_SOURCE)
> 			subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC;
> 	}
>
> You could possibly move the fromColorSpace() call outside of the if (),
> as colorSpace is checked in the function itself, but given that an if ()
> is needed anyway, I'd leave it in there.
>
> Same below.
>
>>   
>> -	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
>> +	ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
>>   	if (ret) {
>>   		LOG(V4L2, Error)
>>   			<< "Unable to set format on pad " << pad
>> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
>> index 63911339..f3651740 100644
>> --- a/src/libcamera/v4l2_videodevice.cpp
>> +++ b/src/libcamera/v4l2_videodevice.cpp
>> @@ -940,7 +940,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>>   	pix->pixelformat = format->fourcc;
>>   	pix->num_planes = format->planesCount;
>>   	pix->field = V4L2_FIELD_NONE;
>> -	fromColorSpace(format->colorSpace, *pix);
>> +	ret = fromColorSpace(format->colorSpace, *pix);
>> +	if (ret == 0 && caps_.isVideoCapture())
>> +		pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
>>   
>>   	ASSERT(pix->num_planes <= std::size(pix->plane_fmt));
>>   
>> @@ -1010,7 +1012,9 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
>>   	pix->pixelformat = format->fourcc;
>>   	pix->bytesperline = format->planes[0].bpl;
>>   	pix->field = V4L2_FIELD_NONE;
>> -	fromColorSpace(format->colorSpace, *pix);
>> +	ret = fromColorSpace(format->colorSpace, *pix);
>> +	if (ret == 0 && caps_.isVideoCapture())
>> +		pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
>>   
>>   	ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);
>>   	if (ret) {