| Message ID | 20220729145042.531757-1-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
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@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 @@ 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 (fromColorSpace(format->colorSpace, subdevFmt.format) == 0) If format->colorSpace is nullopt, should be set the CSC flag ? > + subdevFmt.format.flags |= V4L2_MBUS_FRAMEFMT_SET_CSC; The flag is meant for source pads only, can you avoid setting it on sink pads ? > > 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 @@ 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 (fromColorSpace(format->colorSpace, *pix) == 0) > + pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC; Similarly, the CSC flag is meant for capture devices only. > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > @@ -1010,7 +1011,8 @@ 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 (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) {
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@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) {
Hi Umang, On Mon, Aug 01, 2022 at 12:15:33PM +0530, Umang Jain wrote: > On 8/1/22 01:47, Laurent Pinchart wrote: > > 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@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). It's probably harmless to set the CSC flag with all the colorspace fields to *_DEFAULT, as it means the driver should then not modify what it does by default. I just feel a bit uncomfortable doing so, probably because there are only two drivers in the kernel that honour the CSC flag that it's hard to know what the right thing to do is. > >> + 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) {
Hi, On 8/1/22 12:59, Laurent Pinchart wrote: > Hi Umang, > > On Mon, Aug 01, 2022 at 12:15:33PM +0530, Umang Jain wrote: >> On 8/1/22 01:47, Laurent Pinchart wrote: >>> 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@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). > It's probably harmless to set the CSC flag with all the colorspace > fields to *_DEFAULT, as it means the driver should then not modify what > it does by default. I just feel a bit uncomfortable doing so, probably > because there are only two drivers in the kernel that honour the CSC > flag that it's hard to know what the right thing to do is. Indeed, hard to infer as the flag is not currently widely used. Looking from libcamera perspective, it doesn't make sense to set the flag with colorspace is nullopt. To someone oblivious to the kernel in general, it might be confusing to look setting the flags, when colorspace is nullopt. > >>>> + 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) {
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 @@ 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 (fromColorSpace(format->colorSpace, subdevFmt.format) == 0) + 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 63911339..a969f7fa 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -940,7 +940,8 @@ 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 (fromColorSpace(format->colorSpace, *pix) == 0) + pix->flags |= V4L2_PIX_FMT_FLAG_SET_CSC; ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); @@ -1010,7 +1011,8 @@ 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 (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) {
The colorspace fields as 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 | 3 ++- src/libcamera/v4l2_videodevice.cpp | 6 ++++-- 2 files changed, 6 insertions(+), 3 deletions(-)