Message ID | 20220812085324.266351-1-umang.jain@ideasonboard.com |
---|---|
State | Accepted |
Commit | f094c2922ad5708c876796f0ae5e7732b51e9c66 |
Headers | show |
Series |
|
Related | show |
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) {
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 >
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) {
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(-)