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

Message ID 20220801082420.68120-1-umang.jain@ideasonboard.com
State Accepted
Delegated to: Umang Jain
Headers show
Series
  • [libcamera-devel,v2] libcamera: v4l2: Set colorspace flags
Related show

Commit Message

Umang Jain Aug. 1, 2022, 8:24 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>
---
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(-)

Comments

Laurent Pinchart Aug. 4, 2022, 5:39 p.m. UTC | #1 
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@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) {
Umang Jain Aug. 5, 2022, 4:52 a.m. UTC | #2 
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@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) {

Patch
diff mbox series 

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