[libcamera-devel,v3] libcamera: v4l2: Set colorspace flags
diff mbox series

Message ID 20220812085324.266351-1-umang.jain@ideasonboard.com
State Accepted
Commit f094c2922ad5708c876796f0ae5e7732b51e9c66
Headers show
Series
  • [libcamera-devel,v3] libcamera: v4l2: Set colorspace flags
Related show

Commit Message

Umang Jain Aug. 12, 2022, 8:53 a.m. UTC 
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@ideasonboard.com>
---
 src/libcamera/v4l2_subdevice.cpp   |  8 +++++++-
 src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++++--
 2 files changed, 19 insertions(+), 3 deletions(-)

Comments

Laurent Pinchart Aug. 15, 2022, 2:52 p.m. UTC | #1 
Hi Umang,

Thank you for the patch.

On Fri, Aug 12, 2022 at 02:23:24PM +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@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  src/libcamera/v4l2_subdevice.cpp   |  8 +++++++-
>  src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index e5d20f2c..d8fd92e3 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -526,7 +526,13 @@ 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);
> +	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;
> +	}
>  
>  	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 b80ee1cd..5a2d0e5b 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -953,7 +953,12 @@ 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);
> +	if (format->colorSpace) {
> +		fromColorSpace(format->colorSpace, *pix);
> +
> +		if (caps_.isVideoCapture())
> +			pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
> +	}
>  
>  	ASSERT(pix->num_planes <= std::size(pix->plane_fmt));
>  
> @@ -1023,7 +1028,12 @@ 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);
> +	if (format->colorSpace) {
> +		fromColorSpace(format->colorSpace, *pix);
> +
> +		if (caps_.isVideoCapture())
> +			pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
> +	}
>  
>  	ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);
>  	if (ret) {
Kieran Bingham Aug. 16, 2022, 8:57 a.m. UTC | #2 
Quoting Umang Jain via libcamera-devel (2022-08-12 09:53:24)
> 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.

That describes the problem, but not what the patch does. I'd append:

"""
Set the flags accordingly within the V4L2SubDevice and V4L2Device when a
colorspace is being set by the application.
"""

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  src/libcamera/v4l2_subdevice.cpp   |  8 +++++++-
>  src/libcamera/v4l2_videodevice.cpp | 14 ++++++++++++--
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index e5d20f2c..d8fd92e3 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -526,7 +526,13 @@ 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);
> +       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;
> +       }
>  
>         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 b80ee1cd..5a2d0e5b 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -953,7 +953,12 @@ 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);
> +       if (format->colorSpace) {
> +               fromColorSpace(format->colorSpace, *pix);
> +
> +               if (caps_.isVideoCapture())
> +                       pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
> +       }
>  
>         ASSERT(pix->num_planes <= std::size(pix->plane_fmt));
>  
> @@ -1023,7 +1028,12 @@ 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);
> +       if (format->colorSpace) {
> +               fromColorSpace(format->colorSpace, *pix);
> +
> +               if (caps_.isVideoCapture())
> +                       pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
> +       }
>  
>         ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);
>         if (ret) {
> -- 
> 2.31.1
>

Patch
diff mbox series 

diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
index e5d20f2c..d8fd92e3 100644
--- a/src/libcamera/v4l2_subdevice.cpp
+++ b/src/libcamera/v4l2_subdevice.cpp
@@ -526,7 +526,13 @@  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);
+	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;
+	}
 
 	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 b80ee1cd..5a2d0e5b 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -953,7 +953,12 @@  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);
+	if (format->colorSpace) {
+		fromColorSpace(format->colorSpace, *pix);
+
+		if (caps_.isVideoCapture())
+			pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
+	}
 
 	ASSERT(pix->num_planes <= std::size(pix->plane_fmt));
 
@@ -1023,7 +1028,12 @@  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);
+	if (format->colorSpace) {
+		fromColorSpace(format->colorSpace, *pix);
+
+		if (caps_.isVideoCapture())
+			pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC;
+	}
 
 	ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);
 	if (ret) {