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

[Laurent Pinchart]

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

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

-- 
Regards,

Laurent Pinchart