| Message ID | 20211118151933.15627-5-david.plowman@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi David, On 11/18/21 8:49 PM, David Plowman wrote: > The ColorSpace from the StreamConfiguration is now handled > appropriately in the V4L2VideoDevice. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/internal/v4l2_videodevice.h | 2 + > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- > 2 files changed, 51 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index a1c458e4..00bc50e7 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -20,6 +20,7 @@ > #include <libcamera/base/log.h> > #include <libcamera/base/signal.h> > > +#include <libcamera/color_space.h> > #include <libcamera/framebuffer.h> > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > @@ -167,6 +168,7 @@ public: > > V4L2PixelFormat fourcc; > Size size; > + std::optional<ColorSpace> colorSpace; > > std::array<Plane, 3> planes; > unsigned int planesCount = 0; > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 4f04212d..e03ff8b5 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > * \brief The plane line stride (in bytes) > */ > > +/** > + * \var V4L2DeviceFormat::fourcc > + * \brief The fourcc code describing the pixel encoding scheme > + * > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > + * that identifies the image format pixel encoding scheme. > + */ > + > /** > * \var V4L2DeviceFormat::size > * \brief The image size in pixels > */ > > /** > - * \var V4L2DeviceFormat::fourcc > - * \brief The fourcc code describing the pixel encoding scheme > + * \var V4L2DeviceFormat::colorSpace > + * \brief The color space of the pixels > * > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > - * that identifies the image format pixel encoding scheme. > + * The color space of the image. When setting the format this may be > + * unset, in which case the driver gets to use its default color space. > + * After being set, this value should contain the color space that the > + * was actually used. > */ > > /** > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > format->fourcc = V4L2PixelFormat(pix->pixelformat); > format->planesCount = pix->num_planes; > > + format->colorSpace = toColorSpace(*pix); > + if (!format->colorSpace) > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; You have marked this as an Error, should we be return here too? > + > for (unsigned int i = 0; i < format->planesCount; ++i) { > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > format->planes[i].size = pix->plane_fmt[i].sizeimage; > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; > int ret; > > + if (!format->colorSpace) > + LOG(V4L2, Warning) << "Setting undefined color space"; > + > v4l2Format.type = bufferType_; > pix->width = format->size.width; > pix->height = format->size.height; > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > pix->num_planes = format->planesCount; > pix->field = V4L2_FIELD_NONE; > > + ret = fromColorSpace(format->colorSpace, *pix); > + if (ret < 0) > + LOG(V4L2, Warning) > + << "Setting color space unrecognised by V4L2: " > + << ColorSpace::toString(format->colorSpace); > + > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > format->planes[i].size = pix->plane_fmt[i].sizeimage; > } > > + format->colorSpace = toColorSpace(*pix); > + if (!format->colorSpace) > + LOG(V4L2, Warning) << "Unrecognised color space has been set"; > + > return 0; > } > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > format->planes[0].bpl = pix->bytesperline; > format->planes[0].size = pix->sizeimage; > > + format->colorSpace = toColorSpace(*pix); > + if (!format->colorSpace) > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > + > return 0; > } > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; > int ret; > > + if (!format->colorSpace) > + LOG(V4L2, Error) << "Trying to set unrecognised color space"; > + > v4l2Format.type = bufferType_; > pix->width = format->size.width; > pix->height = format->size.height; > pix->pixelformat = format->fourcc; > pix->bytesperline = format->planes[0].bpl; > pix->field = V4L2_FIELD_NONE; > + > + ret = fromColorSpace(format->colorSpace, *pix); > + if (ret < 0) > + LOG(V4L2, Warning) > + << "Set color space unrecognised by V4L2: " > + << ColorSpace::toString(format->colorSpace); > + > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); > if (ret) { > LOG(V4L2, Error) > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > format->planes[0].bpl = pix->bytesperline; > format->planes[0].size = pix->sizeimage; > > + format->colorSpace = toColorSpace(*pix); > + if (!format->colorSpace) > + LOG(V4L2, Error) << "Undefined color space has been set"; > + Similar concern here. I am finding the Error/Warning strings to be a bit confusing. Set/Setting "undefined/unrecognised" color space = What does that mean actually? Is it about unsetting the color space so that the driver can use and set it's default one? > return 0; > } >
Hi Umang Thanks for looking at this! On Tue, 23 Nov 2021 at 08:01, Umang Jain <umang.jain@ideasonboard.com> wrote: > > Hi David, > > On 11/18/21 8:49 PM, David Plowman wrote: > > The ColorSpace from the StreamConfiguration is now handled > > appropriately in the V4L2VideoDevice. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/internal/v4l2_videodevice.h | 2 + > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > index a1c458e4..00bc50e7 100644 > > --- a/include/libcamera/internal/v4l2_videodevice.h > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > @@ -20,6 +20,7 @@ > > #include <libcamera/base/log.h> > > #include <libcamera/base/signal.h> > > > > +#include <libcamera/color_space.h> > > #include <libcamera/framebuffer.h> > > #include <libcamera/geometry.h> > > #include <libcamera/pixel_format.h> > > @@ -167,6 +168,7 @@ public: > > > > V4L2PixelFormat fourcc; > > Size size; > > + std::optional<ColorSpace> colorSpace; > > > > std::array<Plane, 3> planes; > > unsigned int planesCount = 0; > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index 4f04212d..e03ff8b5 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > * \brief The plane line stride (in bytes) > > */ > > > > +/** > > + * \var V4L2DeviceFormat::fourcc > > + * \brief The fourcc code describing the pixel encoding scheme > > + * > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > + * that identifies the image format pixel encoding scheme. > > + */ > > + > > /** > > * \var V4L2DeviceFormat::size > > * \brief The image size in pixels > > */ > > > > /** > > - * \var V4L2DeviceFormat::fourcc > > - * \brief The fourcc code describing the pixel encoding scheme > > + * \var V4L2DeviceFormat::colorSpace > > + * \brief The color space of the pixels > > * > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > - * that identifies the image format pixel encoding scheme. > > + * The color space of the image. When setting the format this may be > > + * unset, in which case the driver gets to use its default color space. > > + * After being set, this value should contain the color space that the > > + * was actually used. > > */ > > > > /** > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > > format->fourcc = V4L2PixelFormat(pix->pixelformat); > > format->planesCount = pix->num_planes; > > > > + format->colorSpace = toColorSpace(*pix); > > + if (!format->colorSpace) > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > You have marked this as an Error, should we be return here too? I think the general consensus was that the ColorSpace should list all the colour spaces that are ever used, and if we come across new ones then we will need to add them. For this reason I think "Error" is reasonable here to make the developer take note. They will probably need to add their colour space to the ColorSpace class. However, returning with a "hard error" is unhelpful, in my view, because the code will run on and "pretty much work", just with (probably) some confusion about whether the colour space is right. > > > + > > for (unsigned int i = 0; i < format->planesCount; ++i) { > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; > > int ret; > > > > + if (!format->colorSpace) > > + LOG(V4L2, Warning) << "Setting undefined color space"; > > + > > v4l2Format.type = bufferType_; > > pix->width = format->size.width; > > pix->height = format->size.height; > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > pix->num_planes = format->planesCount; > > pix->field = V4L2_FIELD_NONE; > > > > + ret = fromColorSpace(format->colorSpace, *pix); > > + if (ret < 0) > > + LOG(V4L2, Warning) > > + << "Setting color space unrecognised by V4L2: " > > + << ColorSpace::toString(format->colorSpace); > > + > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > } > > > > + format->colorSpace = toColorSpace(*pix); > > + if (!format->colorSpace) > > + LOG(V4L2, Warning) << "Unrecognised color space has been set"; Actually I think this should be "Error", not "Warning", for the same reasons as above. It means the developer needs to add a new colour space to the ColorSpace class. > > + > > return 0; > > } > > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > > format->planes[0].bpl = pix->bytesperline; > > format->planes[0].size = pix->sizeimage; > > > > + format->colorSpace = toColorSpace(*pix); > > + if (!format->colorSpace) > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > + > > return 0; > > } > > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; > > int ret; > > > > + if (!format->colorSpace) > > + LOG(V4L2, Error) << "Trying to set unrecognised color space"; > > + > > v4l2Format.type = bufferType_; > > pix->width = format->size.width; > > pix->height = format->size.height; > > pix->pixelformat = format->fourcc; > > pix->bytesperline = format->planes[0].bpl; > > pix->field = V4L2_FIELD_NONE; > > + > > + ret = fromColorSpace(format->colorSpace, *pix); > > + if (ret < 0) > > + LOG(V4L2, Warning) > > + << "Set color space unrecognised by V4L2: " > > + << ColorSpace::toString(format->colorSpace); > > + > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); > > if (ret) { > > LOG(V4L2, Error) > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > format->planes[0].bpl = pix->bytesperline; > > format->planes[0].size = pix->sizeimage; > > > > + format->colorSpace = toColorSpace(*pix); > > + if (!format->colorSpace) > > + LOG(V4L2, Error) << "Undefined color space has been set"; > > + > > > Similar concern here. This should be "Unrecognised" rather than "Undefined". By "unrecognised" I mean that the ColorSpace class does not have an entry for the colour space V4L2 is using. It's a perfectly good colour space, just our ColorSpace class does not recognise it. > > I am finding the Error/Warning strings to be a bit confusing. > Set/Setting "undefined/unrecognised" color space = What does that mean > actually? Is it about unsetting the color space so that the driver can > use and set it's default one? Yes, I'm sorry this is a bit confusing. Here are some of the things that can happen: 1. Application leaves the ColorSpace "unset" (it's actually a std::optional). This is OK, you'll get the driver's default choice, but I print out a "Warning" just to make it clear that you should check what colour space you are getting. I refer to this often as "setting an undefined colour space". Note that you cannot "set an unrecognised colour space" - by "unrecognised" I mean specifically that a colour space is missing from the ColorSpace class - see below. 2. Cannot turn ColorSpace into V4L2 enums Actually this can't happen currently because the ColorSpace class is a subset of V4L2 ones. But it might in the future. I print out a "Warning" because it would be something to investigate, but an "Error", especially a "hard failure" seems unhelpful. Again, things will "mostly work", and in any case, the problem may not actually be fixable. 3. Cannot turn V4L2 enums into a ColorSpace This would be bad and the consensus the other day was that a new entry should be added to the ColorSpace class to fix the problem. So I think "Error" is reasonable, though again, a "hard failure" seems unhelpful because, as before, everything will "mostly work". This is what I mean by "unrecognised". Does that help at all? Thanks again and best regards David > > > return 0; > > } > >
Quoting David Plowman (2021-11-24 11:55:15) > Hi Umang > > Thanks for looking at this! > > On Tue, 23 Nov 2021 at 08:01, Umang Jain <umang.jain@ideasonboard.com> wrote: > > > > Hi David, > > > > On 11/18/21 8:49 PM, David Plowman wrote: > > > The ColorSpace from the StreamConfiguration is now handled > > > appropriately in the V4L2VideoDevice. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/internal/v4l2_videodevice.h | 2 + > > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- > > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > index a1c458e4..00bc50e7 100644 > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > @@ -20,6 +20,7 @@ > > > #include <libcamera/base/log.h> > > > #include <libcamera/base/signal.h> > > > > > > +#include <libcamera/color_space.h> > > > #include <libcamera/framebuffer.h> > > > #include <libcamera/geometry.h> > > > #include <libcamera/pixel_format.h> > > > @@ -167,6 +168,7 @@ public: > > > > > > V4L2PixelFormat fourcc; > > > Size size; > > > + std::optional<ColorSpace> colorSpace; > > > > > > std::array<Plane, 3> planes; > > > unsigned int planesCount = 0; > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > index 4f04212d..e03ff8b5 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > * \brief The plane line stride (in bytes) > > > */ > > > > > > +/** > > > + * \var V4L2DeviceFormat::fourcc > > > + * \brief The fourcc code describing the pixel encoding scheme > > > + * > > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > + * that identifies the image format pixel encoding scheme. > > > + */ > > > + > > > /** > > > * \var V4L2DeviceFormat::size > > > * \brief The image size in pixels > > > */ > > > > > > /** > > > - * \var V4L2DeviceFormat::fourcc > > > - * \brief The fourcc code describing the pixel encoding scheme > > > + * \var V4L2DeviceFormat::colorSpace > > > + * \brief The color space of the pixels > > > * > > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > - * that identifies the image format pixel encoding scheme. > > > + * The color space of the image. When setting the format this may be > > > + * unset, in which case the driver gets to use its default color space. > > > + * After being set, this value should contain the color space that the > > > + * was actually used. > > > */ > > > > > > /** > > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > > > format->fourcc = V4L2PixelFormat(pix->pixelformat); > > > format->planesCount = pix->num_planes; > > > > > > + format->colorSpace = toColorSpace(*pix); > > > + if (!format->colorSpace) > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > > > > You have marked this as an Error, should we be return here too? > > I think the general consensus was that the ColorSpace should list all > the colour spaces that are ever used, and if we come across new ones > then we will need to add them. Yes, getting the format should still continue. We may still be able to 'set' an appropriate one later... so this I think is part of the negotiation phases. Delving further into the color-space issues, we'll need to consider gstreamer support too. They define their colorspace support at: https://github.com/GStreamer/gst-plugins-base/blob/master/gst-libs/gst/video/video-color.h Which may be useful reference. Our gstreamer element will need to convert between colorimetry and colorspace, and negotiate between the two frameworks, so it likely provides a really good test case for all of this. (Related bug, from a raspberry pi user) - https://bugs.libcamera.org/show_bug.cgi?id=75 > For this reason I think "Error" is reasonable here to make the > developer take note. They will probably need to add their colour space > to the ColorSpace class. > > However, returning with a "hard error" is unhelpful, in my view, > because the code will run on and "pretty much work", just with > (probably) some confusion about whether the colour space is right. > > > > > > + > > > for (unsigned int i = 0; i < format->planesCount; ++i) { > > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; > > > int ret; > > > > > > + if (!format->colorSpace) > > > + LOG(V4L2, Warning) << "Setting undefined color space"; > > > + > > > v4l2Format.type = bufferType_; > > > pix->width = format->size.width; > > > pix->height = format->size.height; > > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > pix->num_planes = format->planesCount; > > > pix->field = V4L2_FIELD_NONE; > > > > > > + ret = fromColorSpace(format->colorSpace, *pix); > > > + if (ret < 0) > > > + LOG(V4L2, Warning) I think that can be Error too. This shouldn't happen, and if it does - it should be fixed (in the kernel?) > > > + << "Setting color space unrecognised by V4L2: " > > > + << ColorSpace::toString(format->colorSpace); > > > + > > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > > > > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > } > > > > > > + format->colorSpace = toColorSpace(*pix); > > > + if (!format->colorSpace) > > > + LOG(V4L2, Warning) << "Unrecognised color space has been set"; > > Actually I think this should be "Error", not "Warning", for the same > reasons as above. It means the developer needs to add a new colour > space to the ColorSpace class. I agree here, I think Error makes sense. > > > > + > > > return 0; > > > } > > > > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > > > format->planes[0].bpl = pix->bytesperline; > > > format->planes[0].size = pix->sizeimage; > > > > > > + format->colorSpace = toColorSpace(*pix); > > > + if (!format->colorSpace) > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > + > > > return 0; > > > } > > > > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; > > > int ret; > > > > > > + if (!format->colorSpace) > > > + LOG(V4L2, Error) << "Trying to set unrecognised color space"; > > > + > > > v4l2Format.type = bufferType_; > > > pix->width = format->size.width; > > > pix->height = format->size.height; > > > pix->pixelformat = format->fourcc; > > > pix->bytesperline = format->planes[0].bpl; > > > pix->field = V4L2_FIELD_NONE; > > > + > > > + ret = fromColorSpace(format->colorSpace, *pix); > > > + if (ret < 0) > > > + LOG(V4L2, Warning) > > > + << "Set color space unrecognised by V4L2: " > > > + << ColorSpace::toString(format->colorSpace); > > > + > > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); > > > if (ret) { > > > LOG(V4L2, Error) > > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > format->planes[0].bpl = pix->bytesperline; > > > format->planes[0].size = pix->sizeimage; > > > > > > + format->colorSpace = toColorSpace(*pix); > > > + if (!format->colorSpace) > > > + LOG(V4L2, Error) << "Undefined color space has been set"; > > > + > > > > > > Similar concern here. > > This should be "Unrecognised" rather than "Undefined". By > "unrecognised" I mean that the ColorSpace class does not have an entry > for the colour space V4L2 is using. It's a perfectly good colour > space, just our ColorSpace class does not recognise it. > > > > > I am finding the Error/Warning strings to be a bit confusing. > > Set/Setting "undefined/unrecognised" color space = What does that mean > > actually? Is it about unsetting the color space so that the driver can > > use and set it's default one? > > Yes, I'm sorry this is a bit confusing. Here are some of the things > that can happen: > > 1. Application leaves the ColorSpace "unset" (it's actually a std::optional). > > This is OK, you'll get the driver's default choice, but I print out a > "Warning" just to make it clear that you should check what colour > space you are getting. I refer to this often as "setting an undefined > colour space". Note that you cannot "set an unrecognised colour space" > - by "unrecognised" I mean specifically that a colour space is missing > from the ColorSpace class - see below. > > 2. Cannot turn ColorSpace into V4L2 enums > > Actually this can't happen currently because the ColorSpace class is a > subset of V4L2 ones. But it might in the future. I print out a > "Warning" because it would be something to investigate, but an > "Error", especially a "hard failure" seems unhelpful. Again, things > will "mostly work", and in any case, the problem may not actually be > fixable. I think this should report as an Error - but not be a hard failure (i.e. continue). The fix would be required on the kernel side in that case... > 3. Cannot turn V4L2 enums into a ColorSpace > > This would be bad and the consensus the other day was that a new entry > should be added to the ColorSpace class to fix the problem. So I think > "Error" is reasonable, though again, a "hard failure" seems unhelpful > because, as before, everything will "mostly work". This is what I mean > by "unrecognised". > > Does that help at all? > > Thanks again and best regards > David > > > > > > return 0; > > > } > > >
Hi David, On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote: > The ColorSpace from the StreamConfiguration is now handled > appropriately in the V4L2VideoDevice. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > include/libcamera/internal/v4l2_videodevice.h | 2 + > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- > 2 files changed, 51 insertions(+), 4 deletions(-) > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > index a1c458e4..00bc50e7 100644 > --- a/include/libcamera/internal/v4l2_videodevice.h > +++ b/include/libcamera/internal/v4l2_videodevice.h > @@ -20,6 +20,7 @@ > #include <libcamera/base/log.h> > #include <libcamera/base/signal.h> > > +#include <libcamera/color_space.h> > #include <libcamera/framebuffer.h> > #include <libcamera/geometry.h> > #include <libcamera/pixel_format.h> > @@ -167,6 +168,7 @@ public: > > V4L2PixelFormat fourcc; > Size size; > + std::optional<ColorSpace> colorSpace; > > std::array<Plane, 3> planes; > unsigned int planesCount = 0; > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > index 4f04212d..e03ff8b5 100644 > --- a/src/libcamera/v4l2_videodevice.cpp > +++ b/src/libcamera/v4l2_videodevice.cpp > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > * \brief The plane line stride (in bytes) > */ > > +/** > + * \var V4L2DeviceFormat::fourcc > + * \brief The fourcc code describing the pixel encoding scheme > + * > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > + * that identifies the image format pixel encoding scheme. > + */ > + Nice that the uncomplete documentation about color spaces has been dropped, but why moving the documentation block above ? Please keep the same order as the declaration order in the header file. It will also make it easier to diff it during review. > /** > * \var V4L2DeviceFormat::size > * \brief The image size in pixels > */ > > /** > - * \var V4L2DeviceFormat::fourcc > - * \brief The fourcc code describing the pixel encoding scheme > + * \var V4L2DeviceFormat::colorSpace > + * \brief The color space of the pixels > * > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > - * that identifies the image format pixel encoding scheme. > + * The color space of the image. When setting the format this may be > + * unset, in which case the driver gets to use its default color space. > + * After being set, this value should contain the color space that the s/that the/that ? > + * was actually used. > */ > > /** > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > format->fourcc = V4L2PixelFormat(pix->pixelformat); > format->planesCount = pix->num_planes; > > + format->colorSpace = toColorSpace(*pix); > + if (!format->colorSpace) > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > + > for (unsigned int i = 0; i < format->planesCount; ++i) { > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > format->planes[i].size = pix->plane_fmt[i].sizeimage; > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; > int ret; > > + if (!format->colorSpace) > + LOG(V4L2, Warning) << "Setting undefined color space"; > + > v4l2Format.type = bufferType_; > pix->width = format->size.width; > pix->height = format->size.height; > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > pix->num_planes = format->planesCount; > pix->field = V4L2_FIELD_NONE; > > + ret = fromColorSpace(format->colorSpace, *pix); > + if (ret < 0) > + LOG(V4L2, Warning) > + << "Setting color space unrecognised by V4L2: " > + << ColorSpace::toString(format->colorSpace); > + > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > format->planes[i].size = pix->plane_fmt[i].sizeimage; > } > > + format->colorSpace = toColorSpace(*pix); > + if (!format->colorSpace) > + LOG(V4L2, Warning) << "Unrecognised color space has been set"; > + Does colorspace handling stricly have to be tied to format configuration ? This would lead to undesired side effects, like the metadata pad being poked for colorspace and complaining as it doesn't really have one. Should colorspace configuration be achieved with dedicated functions in the V4L2*Device classes so that pipeline handlers know what devices to apply/retrieve colorspace from, like they do with formats ? > return 0; > } > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > format->planes[0].bpl = pix->bytesperline; > format->planes[0].size = pix->sizeimage; > > + format->colorSpace = toColorSpace(*pix); > + if (!format->colorSpace) > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > + > return 0; > } > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; > int ret; > > + if (!format->colorSpace) > + LOG(V4L2, Error) << "Trying to set unrecognised color space"; > + > v4l2Format.type = bufferType_; > pix->width = format->size.width; > pix->height = format->size.height; > pix->pixelformat = format->fourcc; > pix->bytesperline = format->planes[0].bpl; > pix->field = V4L2_FIELD_NONE; > + > + ret = fromColorSpace(format->colorSpace, *pix); > + if (ret < 0) > + LOG(V4L2, Warning) > + << "Set color space unrecognised by V4L2: " > + << ColorSpace::toString(format->colorSpace); > + > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); > if (ret) { > LOG(V4L2, Error) > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > format->planes[0].bpl = pix->bytesperline; > format->planes[0].size = pix->sizeimage; > > + format->colorSpace = toColorSpace(*pix); > + if (!format->colorSpace) > + LOG(V4L2, Error) << "Undefined color space has been set"; > + > return 0; > } > > -- > 2.20.1 >
Hi Jacopo Thanks for looking at this! On Wed, 24 Nov 2021 at 23:42, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > > On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote: > > The ColorSpace from the StreamConfiguration is now handled > > appropriately in the V4L2VideoDevice. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > include/libcamera/internal/v4l2_videodevice.h | 2 + > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > index a1c458e4..00bc50e7 100644 > > --- a/include/libcamera/internal/v4l2_videodevice.h > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > @@ -20,6 +20,7 @@ > > #include <libcamera/base/log.h> > > #include <libcamera/base/signal.h> > > > > +#include <libcamera/color_space.h> > > #include <libcamera/framebuffer.h> > > #include <libcamera/geometry.h> > > #include <libcamera/pixel_format.h> > > @@ -167,6 +168,7 @@ public: > > > > V4L2PixelFormat fourcc; > > Size size; > > + std::optional<ColorSpace> colorSpace; > > > > std::array<Plane, 3> planes; > > unsigned int planesCount = 0; > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > index 4f04212d..e03ff8b5 100644 > > --- a/src/libcamera/v4l2_videodevice.cpp > > +++ b/src/libcamera/v4l2_videodevice.cpp > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > * \brief The plane line stride (in bytes) > > */ > > > > +/** > > + * \var V4L2DeviceFormat::fourcc > > + * \brief The fourcc code describing the pixel encoding scheme > > + * > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > + * that identifies the image format pixel encoding scheme. > > + */ > > + > > Nice that the uncomplete documentation about color spaces has been > dropped, but why moving the documentation block above ? Please keep > the same order as the declaration order in the header file. > > It will also make it easier to diff it during review. > > > /** > > * \var V4L2DeviceFormat::size > > * \brief The image size in pixels > > */ > > > > /** > > - * \var V4L2DeviceFormat::fourcc > > - * \brief The fourcc code describing the pixel encoding scheme > > + * \var V4L2DeviceFormat::colorSpace > > + * \brief The color space of the pixels > > * > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > - * that identifies the image format pixel encoding scheme. > > + * The color space of the image. When setting the format this may be > > + * unset, in which case the driver gets to use its default color space. > > + * After being set, this value should contain the color space that the > > s/that the/that ? > > > + * was actually used. > > */ > > > > /** > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > > format->fourcc = V4L2PixelFormat(pix->pixelformat); > > format->planesCount = pix->num_planes; > > > > + format->colorSpace = toColorSpace(*pix); > > + if (!format->colorSpace) > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > + > > for (unsigned int i = 0; i < format->planesCount; ++i) { > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; > > int ret; > > > > + if (!format->colorSpace) > > + LOG(V4L2, Warning) << "Setting undefined color space"; > > + > > v4l2Format.type = bufferType_; > > pix->width = format->size.width; > > pix->height = format->size.height; > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > pix->num_planes = format->planesCount; > > pix->field = V4L2_FIELD_NONE; > > > > + ret = fromColorSpace(format->colorSpace, *pix); > > + if (ret < 0) > > + LOG(V4L2, Warning) > > + << "Setting color space unrecognised by V4L2: " > > + << ColorSpace::toString(format->colorSpace); > > + > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > } > > > > + format->colorSpace = toColorSpace(*pix); > > + if (!format->colorSpace) > > + LOG(V4L2, Warning) << "Unrecognised color space has been set"; > > + > > Does colorspace handling stricly have to be tied to format > configuration ? This would lead to undesired side effects, like the metadata pad > being poked for colorspace and complaining as it doesn't really have > one. > > Should colorspace configuration be achieved with dedicated functions > in the V4L2*Device classes so that pipeline handlers know what devices > to apply/retrieve colorspace from, like they do with formats ? Yes, there is a problem here that I talked about in one of the earlier revisions. Actually it's only an issue for subdevices, so you might notice that there's a "todo" in v4l2_subdevice.cpp getFormat. The problem is that I end up trying to convert the colour space on a metadata pad - and of course that doesn't work. Nothing breaks, you just get a warning message. I'm open to suggestions on how to resolve this. An extra parameter to the get/setFormat ("I am not an image data pad"?). Separate functions get/setImageFormat and get/setMetadataFormat? Rely on the caller to check for a bad colour space? (but will they bother?) Thoughts, anyone? Thanks! David > > > return 0; > > } > > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > > format->planes[0].bpl = pix->bytesperline; > > format->planes[0].size = pix->sizeimage; > > > > + format->colorSpace = toColorSpace(*pix); > > + if (!format->colorSpace) > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > + > > return 0; > > } > > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; > > int ret; > > > > + if (!format->colorSpace) > > + LOG(V4L2, Error) << "Trying to set unrecognised color space"; > > + > > v4l2Format.type = bufferType_; > > pix->width = format->size.width; > > pix->height = format->size.height; > > pix->pixelformat = format->fourcc; > > pix->bytesperline = format->planes[0].bpl; > > pix->field = V4L2_FIELD_NONE; > > + > > + ret = fromColorSpace(format->colorSpace, *pix); > > + if (ret < 0) > > + LOG(V4L2, Warning) > > + << "Set color space unrecognised by V4L2: " > > + << ColorSpace::toString(format->colorSpace); > > + > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); > > if (ret) { > > LOG(V4L2, Error) > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > format->planes[0].bpl = pix->bytesperline; > > format->planes[0].size = pix->sizeimage; > > > > + format->colorSpace = toColorSpace(*pix); > > + if (!format->colorSpace) > > + LOG(V4L2, Error) << "Undefined color space has been set"; > > + > > return 0; > > } > > > > -- > > 2.20.1 > >
Hi David, one more comment and a reply On Thu, Nov 25, 2021 at 04:36:40PM +0000, David Plowman wrote: > Hi Jacopo > > Thanks for looking at this! > > On Wed, 24 Nov 2021 at 23:42, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi David, > > > > On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote: > > > The ColorSpace from the StreamConfiguration is now handled > > > appropriately in the V4L2VideoDevice. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > include/libcamera/internal/v4l2_videodevice.h | 2 + > > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- > > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > index a1c458e4..00bc50e7 100644 > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > @@ -20,6 +20,7 @@ > > > #include <libcamera/base/log.h> > > > #include <libcamera/base/signal.h> > > > > > > +#include <libcamera/color_space.h> > > > #include <libcamera/framebuffer.h> > > > #include <libcamera/geometry.h> > > > #include <libcamera/pixel_format.h> > > > @@ -167,6 +168,7 @@ public: > > > > > > V4L2PixelFormat fourcc; > > > Size size; > > > + std::optional<ColorSpace> colorSpace; > > > > > > std::array<Plane, 3> planes; > > > unsigned int planesCount = 0; > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > index 4f04212d..e03ff8b5 100644 > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > * \brief The plane line stride (in bytes) > > > */ > > > > > > +/** > > > + * \var V4L2DeviceFormat::fourcc > > > + * \brief The fourcc code describing the pixel encoding scheme > > > + * > > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > + * that identifies the image format pixel encoding scheme. > > > + */ > > > + > > > > Nice that the uncomplete documentation about color spaces has been > > dropped, but why moving the documentation block above ? Please keep > > the same order as the declaration order in the header file. > > > > It will also make it easier to diff it during review. > > > > > /** > > > * \var V4L2DeviceFormat::size > > > * \brief The image size in pixels > > > */ > > > > > > /** > > > - * \var V4L2DeviceFormat::fourcc > > > - * \brief The fourcc code describing the pixel encoding scheme > > > + * \var V4L2DeviceFormat::colorSpace > > > + * \brief The color space of the pixels > > > * > > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > - * that identifies the image format pixel encoding scheme. > > > + * The color space of the image. When setting the format this may be > > > + * unset, in which case the driver gets to use its default color space. > > > + * After being set, this value should contain the color space that the > > > > s/that the/that ? > > > > > + * was actually used. > > > */ > > > > > > /** > > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > > > format->fourcc = V4L2PixelFormat(pix->pixelformat); > > > format->planesCount = pix->num_planes; > > > > > > + format->colorSpace = toColorSpace(*pix); > > > + if (!format->colorSpace) > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > + > > > for (unsigned int i = 0; i < format->planesCount; ++i) { > > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; > > > int ret; > > > > > > + if (!format->colorSpace) > > > + LOG(V4L2, Warning) << "Setting undefined color space"; > > > + > > > v4l2Format.type = bufferType_; > > > pix->width = format->size.width; > > > pix->height = format->size.height; > > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > pix->num_planes = format->planesCount; > > > pix->field = V4L2_FIELD_NONE; > > > > > > + ret = fromColorSpace(format->colorSpace, *pix); Looking at how this is used it really looks like this could be called ret = fillV4L2ColorSpace(pix, format->colorSpace); > > > + if (ret < 0) > > > + LOG(V4L2, Warning) > > > + << "Setting color space unrecognised by V4L2: " > > > + << ColorSpace::toString(format->colorSpace); > > > + > > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > > > > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > } > > > > > > + format->colorSpace = toColorSpace(*pix); > > > + if (!format->colorSpace) > > > + LOG(V4L2, Warning) << "Unrecognised color space has been set"; > > > + > > > > Does colorspace handling stricly have to be tied to format > > configuration ? This would lead to undesired side effects, like the metadata pad > > being poked for colorspace and complaining as it doesn't really have > > one. > > > > Should colorspace configuration be achieved with dedicated functions > > in the V4L2*Device classes so that pipeline handlers know what devices > > to apply/retrieve colorspace from, like they do with formats ? > > Yes, there is a problem here that I talked about in one of the earlier > revisions. Actually it's only an issue for subdevices, so you might > notice that there's a "todo" in v4l2_subdevice.cpp getFormat. > > The problem is that I end up trying to convert the colour space on a > metadata pad - and of course that doesn't work. Nothing breaks, you I'm aware of the issue and it means to me the implemented API falls a bit short if it causes annoyances with a standard use case like this one. My proposal was to leave set/getFormat() in their current version in V4L2VideoDevice and V4L2Subdevice and add a new setColorSpace() or similar which the pipeline handlers would be in charge to call on the devices it consider appropriate. This would separate the ColorSpace configuration from the image format configuration. In V4L2 they go through the same IOCTL and the same structure, but if that doesn't work well for us we can deviate from it. Also looking at how colorspaces are validated in your last patch int ret = dev->tryFormat(&format); if (ret) return Invalid; + if (!format.colorSpace || cfg.colorSpace != format.colorSpace) { + status = Adjusted; + LOG(RPI, Warning) + << "Color space changed from " + << ColorSpace::toString(cfg.colorSpace) << " to " + << ColorSpace::toString(format.colorSpace); + } + cfg.colorSpace = format.colorSpace; This really seems like it could be int ret = dev->tryFormat(&format); if (ret) return Invalid; ret = dev->setColorSpace(cfg.colorSpace); if (cfg.colorSpace != dev->getColorSpace()) { status = Adjusted; cfg.colorSpace = dev->getColorSpace(); } True, setting an image format resets the colorspace maybe but I think it's fine as long as it is documented. If in the setColorSpace() function you need a v4l2_format you can use the currently applied format. Of course you would need to add the single planar/multiplanar V4L2 API behind the single setColorSpace() function. In my mental picture there's nothing that prevents this from being implemented (not that I'm saying it's a good idea, but it's doable at last), am I wrong ? > just get a warning message. I'm open to suggestions on how to resolve > this. An extra parameter to the get/setFormat ("I am not an image data > pad"?). Separate functions get/setImageFormat and > get/setMetadataFormat? Rely on the caller to check for a bad colour > space? (but will they bother?) > > Thoughts, anyone? > > Thanks! > David > > > > > > return 0; > > > } > > > > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > > > format->planes[0].bpl = pix->bytesperline; > > > format->planes[0].size = pix->sizeimage; > > > > > > + format->colorSpace = toColorSpace(*pix); > > > + if (!format->colorSpace) > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > + > > > return 0; > > > } > > > > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; > > > int ret; > > > > > > + if (!format->colorSpace) > > > + LOG(V4L2, Error) << "Trying to set unrecognised color space"; > > > + > > > v4l2Format.type = bufferType_; > > > pix->width = format->size.width; > > > pix->height = format->size.height; > > > pix->pixelformat = format->fourcc; > > > pix->bytesperline = format->planes[0].bpl; > > > pix->field = V4L2_FIELD_NONE; > > > + > > > + ret = fromColorSpace(format->colorSpace, *pix); > > > + if (ret < 0) > > > + LOG(V4L2, Warning) > > > + << "Set color space unrecognised by V4L2: " > > > + << ColorSpace::toString(format->colorSpace); > > > + > > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); > > > if (ret) { > > > LOG(V4L2, Error) > > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > format->planes[0].bpl = pix->bytesperline; > > > format->planes[0].size = pix->sizeimage; > > > > > > + format->colorSpace = toColorSpace(*pix); > > > + if (!format->colorSpace) > > > + LOG(V4L2, Error) << "Undefined color space has been set"; > > > + > > > return 0; > > > } > > > > > > -- > > > 2.20.1 > > >
Hi Jacopo Thanks for the suggestions. On Thu, 25 Nov 2021 at 17:18, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > one more comment and a reply > > On Thu, Nov 25, 2021 at 04:36:40PM +0000, David Plowman wrote: > > Hi Jacopo > > > > Thanks for looking at this! > > > > On Wed, 24 Nov 2021 at 23:42, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi David, > > > > > > On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote: > > > > The ColorSpace from the StreamConfiguration is now handled > > > > appropriately in the V4L2VideoDevice. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > include/libcamera/internal/v4l2_videodevice.h | 2 + > > > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- > > > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > > index a1c458e4..00bc50e7 100644 > > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > > @@ -20,6 +20,7 @@ > > > > #include <libcamera/base/log.h> > > > > #include <libcamera/base/signal.h> > > > > > > > > +#include <libcamera/color_space.h> > > > > #include <libcamera/framebuffer.h> > > > > #include <libcamera/geometry.h> > > > > #include <libcamera/pixel_format.h> > > > > @@ -167,6 +168,7 @@ public: > > > > > > > > V4L2PixelFormat fourcc; > > > > Size size; > > > > + std::optional<ColorSpace> colorSpace; > > > > > > > > std::array<Plane, 3> planes; > > > > unsigned int planesCount = 0; > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > index 4f04212d..e03ff8b5 100644 > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > * \brief The plane line stride (in bytes) > > > > */ > > > > > > > > +/** > > > > + * \var V4L2DeviceFormat::fourcc > > > > + * \brief The fourcc code describing the pixel encoding scheme > > > > + * > > > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > > + * that identifies the image format pixel encoding scheme. > > > > + */ > > > > + > > > > > > Nice that the uncomplete documentation about color spaces has been > > > dropped, but why moving the documentation block above ? Please keep > > > the same order as the declaration order in the header file. > > > > > > It will also make it easier to diff it during review. > > > > > > > /** > > > > * \var V4L2DeviceFormat::size > > > > * \brief The image size in pixels > > > > */ > > > > > > > > /** > > > > - * \var V4L2DeviceFormat::fourcc > > > > - * \brief The fourcc code describing the pixel encoding scheme > > > > + * \var V4L2DeviceFormat::colorSpace > > > > + * \brief The color space of the pixels > > > > * > > > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > > - * that identifies the image format pixel encoding scheme. > > > > + * The color space of the image. When setting the format this may be > > > > + * unset, in which case the driver gets to use its default color space. > > > > + * After being set, this value should contain the color space that the > > > > > > s/that the/that ? > > > > > > > + * was actually used. > > > > */ > > > > > > > > /** > > > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > > > > format->fourcc = V4L2PixelFormat(pix->pixelformat); > > > > format->planesCount = pix->num_planes; > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > + if (!format->colorSpace) > > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > > + > > > > for (unsigned int i = 0; i < format->planesCount; ++i) { > > > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; > > > > int ret; > > > > > > > > + if (!format->colorSpace) > > > > + LOG(V4L2, Warning) << "Setting undefined color space"; > > > > + > > > > v4l2Format.type = bufferType_; > > > > pix->width = format->size.width; > > > > pix->height = format->size.height; > > > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > > pix->num_planes = format->planesCount; > > > > pix->field = V4L2_FIELD_NONE; > > > > > > > > + ret = fromColorSpace(format->colorSpace, *pix); > > Looking at how this is used it really looks like this could be called > > ret = fillV4L2ColorSpace(pix, format->colorSpace); > > > > > + if (ret < 0) > > > > + LOG(V4L2, Warning) > > > > + << "Setting color space unrecognised by V4L2: " > > > > + << ColorSpace::toString(format->colorSpace); > > > > + > > > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > > > > > > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > > > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > > } > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > + if (!format->colorSpace) > > > > + LOG(V4L2, Warning) << "Unrecognised color space has been set"; > > > > + > > > > > > Does colorspace handling stricly have to be tied to format > > > configuration ? This would lead to undesired side effects, like the metadata pad > > > being poked for colorspace and complaining as it doesn't really have > > > one. > > > > > > Should colorspace configuration be achieved with dedicated functions > > > in the V4L2*Device classes so that pipeline handlers know what devices > > > to apply/retrieve colorspace from, like they do with formats ? > > > > Yes, there is a problem here that I talked about in one of the earlier > > revisions. Actually it's only an issue for subdevices, so you might > > notice that there's a "todo" in v4l2_subdevice.cpp getFormat. > > > > The problem is that I end up trying to convert the colour space on a > > metadata pad - and of course that doesn't work. Nothing breaks, you > > I'm aware of the issue and it means to me the implemented API falls a > bit short if it causes annoyances with a standard use case like this > one. > > My proposal was to leave set/getFormat() in their current version in > V4L2VideoDevice and V4L2Subdevice and add a new setColorSpace() or > similar which the pipeline handlers would be in charge to call on > the devices it consider appropriate. > > This would separate the ColorSpace configuration from the image format > configuration. In V4L2 they go through the same IOCTL and the same > structure, but if that doesn't work well for us we can deviate from > it. > > Also looking at how colorspaces are validated in your last patch > > int ret = dev->tryFormat(&format); > if (ret) > return Invalid; > + if (!format.colorSpace || cfg.colorSpace != format.colorSpace) { > + status = Adjusted; > + LOG(RPI, Warning) > + << "Color space changed from " > + << ColorSpace::toString(cfg.colorSpace) << " to " > + << ColorSpace::toString(format.colorSpace); > + } > + cfg.colorSpace = format.colorSpace; > > This really seems like it could be > > int ret = dev->tryFormat(&format); > if (ret) > return Invalid; > > ret = dev->setColorSpace(cfg.colorSpace); > if (cfg.colorSpace != dev->getColorSpace()) { > status = Adjusted; > cfg.colorSpace = dev->getColorSpace(); > } > > > True, setting an image format resets the colorspace maybe but I think > it's fine as long as it is documented. > > If in the setColorSpace() function you need a v4l2_format you can use > the currently applied format. Of course you would need to add the > single planar/multiplanar V4L2 API behind the single setColorSpace() > function. > > In my mental picture there's nothing that prevents this from being > implemented (not that I'm saying it's a good idea, but it's doable at > last), am I wrong ? So I agree that it could be done like this, but I guess the following things bother me a bit. Mainly it seems more complicated (you have to do 2 things all the time, not just one - source of bugs right there). It confounds existing expectations about V4L2 - why confuse people who thought they knew how V4L2 works? This is a "helpful C++ layer" on top of V4L2, right? And the actual mechanics of splitting this up - every "set" operation becomes a "query-first-then-set"- is not lovely. Actually I think there's a perfectly good solution which is simply to let the caller check the result. I know I get obsessed about colour spaces, but apparently not everyone does! This way everyone gets what they want. I can check. Others can check if they want. All problems solved. David > > > just get a warning message. I'm open to suggestions on how to resolve > > this. An extra parameter to the get/setFormat ("I am not an image data > > pad"?). Separate functions get/setImageFormat and > > get/setMetadataFormat? Rely on the caller to check for a bad colour > > space? (but will they bother?) > > > > Thoughts, anyone? > > > > Thanks! > > David > > > > > > > > > return 0; > > > > } > > > > > > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > > > > format->planes[0].bpl = pix->bytesperline; > > > > format->planes[0].size = pix->sizeimage; > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > + if (!format->colorSpace) > > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; > > > > int ret; > > > > > > > > + if (!format->colorSpace) > > > > + LOG(V4L2, Error) << "Trying to set unrecognised color space"; > > > > + > > > > v4l2Format.type = bufferType_; > > > > pix->width = format->size.width; > > > > pix->height = format->size.height; > > > > pix->pixelformat = format->fourcc; > > > > pix->bytesperline = format->planes[0].bpl; > > > > pix->field = V4L2_FIELD_NONE; > > > > + > > > > + ret = fromColorSpace(format->colorSpace, *pix); > > > > + if (ret < 0) > > > > + LOG(V4L2, Warning) > > > > + << "Set color space unrecognised by V4L2: " > > > > + << ColorSpace::toString(format->colorSpace); > > > > + > > > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); > > > > if (ret) { > > > > LOG(V4L2, Error) > > > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > > format->planes[0].bpl = pix->bytesperline; > > > > format->planes[0].size = pix->sizeimage; > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > + if (!format->colorSpace) > > > > + LOG(V4L2, Error) << "Undefined color space has been set"; > > > > + > > > > return 0; > > > > } > > > > > > > > -- > > > > 2.20.1 > > > >
Hi David, On Thu, Nov 25, 2021 at 08:38:25PM +0000, David Plowman wrote: > Hi Jacopo > > Thanks for the suggestions. > > On Thu, 25 Nov 2021 at 17:18, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > Hi David, > > one more comment and a reply > > > > On Thu, Nov 25, 2021 at 04:36:40PM +0000, David Plowman wrote: > > > Hi Jacopo > > > > > > Thanks for looking at this! > > > > > > On Wed, 24 Nov 2021 at 23:42, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > Hi David, > > > > > > > > On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote: > > > > > The ColorSpace from the StreamConfiguration is now handled > > > > > appropriately in the V4L2VideoDevice. > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > --- > > > > > include/libcamera/internal/v4l2_videodevice.h | 2 + > > > > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- > > > > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > > > index a1c458e4..00bc50e7 100644 > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > > > @@ -20,6 +20,7 @@ > > > > > #include <libcamera/base/log.h> > > > > > #include <libcamera/base/signal.h> > > > > > > > > > > +#include <libcamera/color_space.h> > > > > > #include <libcamera/framebuffer.h> > > > > > #include <libcamera/geometry.h> > > > > > #include <libcamera/pixel_format.h> > > > > > @@ -167,6 +168,7 @@ public: > > > > > > > > > > V4L2PixelFormat fourcc; > > > > > Size size; > > > > > + std::optional<ColorSpace> colorSpace; > > > > > > > > > > std::array<Plane, 3> planes; > > > > > unsigned int planesCount = 0; > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > > index 4f04212d..e03ff8b5 100644 > > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > > * \brief The plane line stride (in bytes) > > > > > */ > > > > > > > > > > +/** > > > > > + * \var V4L2DeviceFormat::fourcc > > > > > + * \brief The fourcc code describing the pixel encoding scheme > > > > > + * > > > > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > > > + * that identifies the image format pixel encoding scheme. > > > > > + */ > > > > > + > > > > > > > > Nice that the uncomplete documentation about color spaces has been > > > > dropped, but why moving the documentation block above ? Please keep > > > > the same order as the declaration order in the header file. > > > > > > > > It will also make it easier to diff it during review. > > > > > > > > > /** > > > > > * \var V4L2DeviceFormat::size > > > > > * \brief The image size in pixels > > > > > */ > > > > > > > > > > /** > > > > > - * \var V4L2DeviceFormat::fourcc > > > > > - * \brief The fourcc code describing the pixel encoding scheme > > > > > + * \var V4L2DeviceFormat::colorSpace > > > > > + * \brief The color space of the pixels > > > > > * > > > > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > > > - * that identifies the image format pixel encoding scheme. > > > > > + * The color space of the image. When setting the format this may be > > > > > + * unset, in which case the driver gets to use its default color space. > > > > > + * After being set, this value should contain the color space that the > > > > > > > > s/that the/that ? > > > > > > > > > + * was actually used. > > > > > */ > > > > > > > > > > /** > > > > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > > > > > format->fourcc = V4L2PixelFormat(pix->pixelformat); > > > > > format->planesCount = pix->num_planes; > > > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > > + if (!format->colorSpace) > > > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > > > + > > > > > for (unsigned int i = 0; i < format->planesCount; ++i) { > > > > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; > > > > > int ret; > > > > > > > > > > + if (!format->colorSpace) > > > > > + LOG(V4L2, Warning) << "Setting undefined color space"; > > > > > + > > > > > v4l2Format.type = bufferType_; > > > > > pix->width = format->size.width; > > > > > pix->height = format->size.height; > > > > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > > > pix->num_planes = format->planesCount; > > > > > pix->field = V4L2_FIELD_NONE; > > > > > > > > > > + ret = fromColorSpace(format->colorSpace, *pix); > > > > Looking at how this is used it really looks like this could be called > > > > ret = fillV4L2ColorSpace(pix, format->colorSpace); > > > > > > > + if (ret < 0) > > > > > + LOG(V4L2, Warning) > > > > > + << "Setting color space unrecognised by V4L2: " > > > > > + << ColorSpace::toString(format->colorSpace); > > > > > + > > > > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > > > > > > > > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > > > > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > > > } > > > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > > + if (!format->colorSpace) > > > > > + LOG(V4L2, Warning) << "Unrecognised color space has been set"; > > > > > + > > > > > > > > Does colorspace handling stricly have to be tied to format > > > > configuration ? This would lead to undesired side effects, like the metadata pad > > > > being poked for colorspace and complaining as it doesn't really have > > > > one. > > > > > > > > Should colorspace configuration be achieved with dedicated functions > > > > in the V4L2*Device classes so that pipeline handlers know what devices > > > > to apply/retrieve colorspace from, like they do with formats ? > > > > > > Yes, there is a problem here that I talked about in one of the earlier > > > revisions. Actually it's only an issue for subdevices, so you might > > > notice that there's a "todo" in v4l2_subdevice.cpp getFormat. > > > > > > The problem is that I end up trying to convert the colour space on a > > > metadata pad - and of course that doesn't work. Nothing breaks, you > > > > I'm aware of the issue and it means to me the implemented API falls a > > bit short if it causes annoyances with a standard use case like this > > one. > > > > My proposal was to leave set/getFormat() in their current version in > > V4L2VideoDevice and V4L2Subdevice and add a new setColorSpace() or > > similar which the pipeline handlers would be in charge to call on > > the devices it consider appropriate. > > > > This would separate the ColorSpace configuration from the image format > > configuration. In V4L2 they go through the same IOCTL and the same > > structure, but if that doesn't work well for us we can deviate from > > it. > > > > Also looking at how colorspaces are validated in your last patch > > > > int ret = dev->tryFormat(&format); > > if (ret) > > return Invalid; > > + if (!format.colorSpace || cfg.colorSpace != format.colorSpace) { > > + status = Adjusted; > > + LOG(RPI, Warning) > > + << "Color space changed from " > > + << ColorSpace::toString(cfg.colorSpace) << " to " > > + << ColorSpace::toString(format.colorSpace); > > + } > > + cfg.colorSpace = format.colorSpace; > > > > This really seems like it could be > > > > int ret = dev->tryFormat(&format); > > if (ret) > > return Invalid; > > > > ret = dev->setColorSpace(cfg.colorSpace); > > if (cfg.colorSpace != dev->getColorSpace()) { > > status = Adjusted; > > cfg.colorSpace = dev->getColorSpace(); > > } > > > > > > True, setting an image format resets the colorspace maybe but I think > > it's fine as long as it is documented. > > > > If in the setColorSpace() function you need a v4l2_format you can use > > the currently applied format. Of course you would need to add the > > single planar/multiplanar V4L2 API behind the single setColorSpace() > > function. > > > > In my mental picture there's nothing that prevents this from being > > implemented (not that I'm saying it's a good idea, but it's doable at > > last), am I wrong ? > > So I agree that it could be done like this, but I guess the following > things bother me a bit. > > Mainly it seems more complicated (you have to do 2 things all the > time, not just one - source of bugs right there). It confounds I don't think it will be source of bugs, but rather something not all pipeline handlers might do. But we'll have compliance test suites, and this will be caught "early" > existing expectations about V4L2 - why confuse people who thought they > knew how V4L2 works? This is a "helpful C++ layer" on top of V4L2, Well, no :) libcamera might theoretically exists on top of other abstractions and when it makes sense to deviate from v4l2 we're free to do so (again, if it makes sense) > right? And the actual mechanics of splitting this up - every "set" > operation becomes a "query-first-then-set"- is not lovely. The "query-first-then-set" happens in the theoretical V4L2*Device::setColorSpace() function or in the pipeline handler code ? > > Actually I think there's a perfectly good solution which is simply to > let the caller check the result. I know I get obsessed about colour > spaces, but apparently not everyone does! This way everyone gets what > they want. I can check. Others can check if they want. All problems > solved. Not really, as setting a format on a metadata pad creates an error message, and it doesn't anyway make much sense to configure a colorspace there. As soon as we have devices producing data in non-pixel formats (depth maps etc etc) we'll hit the same issue. Thanks j > > David > > > > > > just get a warning message. I'm open to suggestions on how to resolve > > > this. An extra parameter to the get/setFormat ("I am not an image data > > > pad"?). Separate functions get/setImageFormat and > > > get/setMetadataFormat? Rely on the caller to check for a bad colour > > > space? (but will they bother?) > > > > > > Thoughts, anyone? > > > > > > Thanks! > > > David > > > > > > > > > > > > return 0; > > > > > } > > > > > > > > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > > > > > format->planes[0].bpl = pix->bytesperline; > > > > > format->planes[0].size = pix->sizeimage; > > > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > > + if (!format->colorSpace) > > > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; > > > > > int ret; > > > > > > > > > > + if (!format->colorSpace) > > > > > + LOG(V4L2, Error) << "Trying to set unrecognised color space"; > > > > > + > > > > > v4l2Format.type = bufferType_; > > > > > pix->width = format->size.width; > > > > > pix->height = format->size.height; > > > > > pix->pixelformat = format->fourcc; > > > > > pix->bytesperline = format->planes[0].bpl; > > > > > pix->field = V4L2_FIELD_NONE; > > > > > + > > > > > + ret = fromColorSpace(format->colorSpace, *pix); > > > > > + if (ret < 0) > > > > > + LOG(V4L2, Warning) > > > > > + << "Set color space unrecognised by V4L2: " > > > > > + << ColorSpace::toString(format->colorSpace); > > > > > + > > > > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); > > > > > if (ret) { > > > > > LOG(V4L2, Error) > > > > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > > > format->planes[0].bpl = pix->bytesperline; > > > > > format->planes[0].size = pix->sizeimage; > > > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > > + if (!format->colorSpace) > > > > > + LOG(V4L2, Error) << "Undefined color space has been set"; > > > > > + > > > > > return 0; > > > > > } > > > > > > > > > > -- > > > > > 2.20.1 > > > > >
Hi Jacopo On Fri, 26 Nov 2021 at 08:20, Jacopo Mondi <jacopo@jmondi.org> wrote: > > Hi David, > > On Thu, Nov 25, 2021 at 08:38:25PM +0000, David Plowman wrote: > > Hi Jacopo > > > > Thanks for the suggestions. > > > > On Thu, 25 Nov 2021 at 17:18, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > Hi David, > > > one more comment and a reply > > > > > > On Thu, Nov 25, 2021 at 04:36:40PM +0000, David Plowman wrote: > > > > Hi Jacopo > > > > > > > > Thanks for looking at this! > > > > > > > > On Wed, 24 Nov 2021 at 23:42, Jacopo Mondi <jacopo@jmondi.org> wrote: > > > > > > > > > > Hi David, > > > > > > > > > > On Thu, Nov 18, 2021 at 03:19:30PM +0000, David Plowman wrote: > > > > > > The ColorSpace from the StreamConfiguration is now handled > > > > > > appropriately in the V4L2VideoDevice. > > > > > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > > > --- > > > > > > include/libcamera/internal/v4l2_videodevice.h | 2 + > > > > > > src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- > > > > > > 2 files changed, 51 insertions(+), 4 deletions(-) > > > > > > > > > > > > diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h > > > > > > index a1c458e4..00bc50e7 100644 > > > > > > --- a/include/libcamera/internal/v4l2_videodevice.h > > > > > > +++ b/include/libcamera/internal/v4l2_videodevice.h > > > > > > @@ -20,6 +20,7 @@ > > > > > > #include <libcamera/base/log.h> > > > > > > #include <libcamera/base/signal.h> > > > > > > > > > > > > +#include <libcamera/color_space.h> > > > > > > #include <libcamera/framebuffer.h> > > > > > > #include <libcamera/geometry.h> > > > > > > #include <libcamera/pixel_format.h> > > > > > > @@ -167,6 +168,7 @@ public: > > > > > > > > > > > > V4L2PixelFormat fourcc; > > > > > > Size size; > > > > > > + std::optional<ColorSpace> colorSpace; > > > > > > > > > > > > std::array<Plane, 3> planes; > > > > > > unsigned int planesCount = 0; > > > > > > diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp > > > > > > index 4f04212d..e03ff8b5 100644 > > > > > > --- a/src/libcamera/v4l2_videodevice.cpp > > > > > > +++ b/src/libcamera/v4l2_videodevice.cpp > > > > > > @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const > > > > > > * \brief The plane line stride (in bytes) > > > > > > */ > > > > > > > > > > > > +/** > > > > > > + * \var V4L2DeviceFormat::fourcc > > > > > > + * \brief The fourcc code describing the pixel encoding scheme > > > > > > + * > > > > > > + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > > > > + * that identifies the image format pixel encoding scheme. > > > > > > + */ > > > > > > + > > > > > > > > > > Nice that the uncomplete documentation about color spaces has been > > > > > dropped, but why moving the documentation block above ? Please keep > > > > > the same order as the declaration order in the header file. > > > > > > > > > > It will also make it easier to diff it during review. > > > > > > > > > > > /** > > > > > > * \var V4L2DeviceFormat::size > > > > > > * \brief The image size in pixels > > > > > > */ > > > > > > > > > > > > /** > > > > > > - * \var V4L2DeviceFormat::fourcc > > > > > > - * \brief The fourcc code describing the pixel encoding scheme > > > > > > + * \var V4L2DeviceFormat::colorSpace > > > > > > + * \brief The color space of the pixels > > > > > > * > > > > > > - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, > > > > > > - * that identifies the image format pixel encoding scheme. > > > > > > + * The color space of the image. When setting the format this may be > > > > > > + * unset, in which case the driver gets to use its default color space. > > > > > > + * After being set, this value should contain the color space that the > > > > > > > > > > s/that the/that ? > > > > > > > > > > > + * was actually used. > > > > > > */ > > > > > > > > > > > > /** > > > > > > @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) > > > > > > format->fourcc = V4L2PixelFormat(pix->pixelformat); > > > > > > format->planesCount = pix->num_planes; > > > > > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > > > + if (!format->colorSpace) > > > > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > > > > + > > > > > > for (unsigned int i = 0; i < format->planesCount; ++i) { > > > > > > format->planes[i].bpl = pix->plane_fmt[i].bytesperline; > > > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > > > > @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > > > > struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; > > > > > > int ret; > > > > > > > > > > > > + if (!format->colorSpace) > > > > > > + LOG(V4L2, Warning) << "Setting undefined color space"; > > > > > > + > > > > > > v4l2Format.type = bufferType_; > > > > > > pix->width = format->size.width; > > > > > > pix->height = format->size.height; > > > > > > @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > > > > pix->num_planes = format->planesCount; > > > > > > pix->field = V4L2_FIELD_NONE; > > > > > > > > > > > > + ret = fromColorSpace(format->colorSpace, *pix); > > > > > > Looking at how this is used it really looks like this could be called > > > > > > ret = fillV4L2ColorSpace(pix, format->colorSpace); > > > > > > > > > + if (ret < 0) > > > > > > + LOG(V4L2, Warning) > > > > > > + << "Setting color space unrecognised by V4L2: " > > > > > > + << ColorSpace::toString(format->colorSpace); > > > > > > + > > > > > > ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); > > > > > > > > > > > > for (unsigned int i = 0; i < pix->num_planes; ++i) { > > > > > > @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) > > > > > > format->planes[i].size = pix->plane_fmt[i].sizeimage; > > > > > > } > > > > > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > > > + if (!format->colorSpace) > > > > > > + LOG(V4L2, Warning) << "Unrecognised color space has been set"; > > > > > > + > > > > > > > > > > Does colorspace handling stricly have to be tied to format > > > > > configuration ? This would lead to undesired side effects, like the metadata pad > > > > > being poked for colorspace and complaining as it doesn't really have > > > > > one. > > > > > > > > > > Should colorspace configuration be achieved with dedicated functions > > > > > in the V4L2*Device classes so that pipeline handlers know what devices > > > > > to apply/retrieve colorspace from, like they do with formats ? > > > > > > > > Yes, there is a problem here that I talked about in one of the earlier > > > > revisions. Actually it's only an issue for subdevices, so you might > > > > notice that there's a "todo" in v4l2_subdevice.cpp getFormat. > > > > > > > > The problem is that I end up trying to convert the colour space on a > > > > metadata pad - and of course that doesn't work. Nothing breaks, you > > > > > > I'm aware of the issue and it means to me the implemented API falls a > > > bit short if it causes annoyances with a standard use case like this > > > one. > > > > > > My proposal was to leave set/getFormat() in their current version in > > > V4L2VideoDevice and V4L2Subdevice and add a new setColorSpace() or > > > similar which the pipeline handlers would be in charge to call on > > > the devices it consider appropriate. > > > > > > This would separate the ColorSpace configuration from the image format > > > configuration. In V4L2 they go through the same IOCTL and the same > > > structure, but if that doesn't work well for us we can deviate from > > > it. > > > > > > Also looking at how colorspaces are validated in your last patch > > > > > > int ret = dev->tryFormat(&format); > > > if (ret) > > > return Invalid; > > > + if (!format.colorSpace || cfg.colorSpace != format.colorSpace) { > > > + status = Adjusted; > > > + LOG(RPI, Warning) > > > + << "Color space changed from " > > > + << ColorSpace::toString(cfg.colorSpace) << " to " > > > + << ColorSpace::toString(format.colorSpace); > > > + } > > > + cfg.colorSpace = format.colorSpace; > > > > > > This really seems like it could be > > > > > > int ret = dev->tryFormat(&format); > > > if (ret) > > > return Invalid; > > > > > > ret = dev->setColorSpace(cfg.colorSpace); > > > if (cfg.colorSpace != dev->getColorSpace()) { > > > status = Adjusted; > > > cfg.colorSpace = dev->getColorSpace(); > > > } > > > > > > > > > True, setting an image format resets the colorspace maybe but I think > > > it's fine as long as it is documented. > > > > > > If in the setColorSpace() function you need a v4l2_format you can use > > > the currently applied format. Of course you would need to add the > > > single planar/multiplanar V4L2 API behind the single setColorSpace() > > > function. > > > > > > In my mental picture there's nothing that prevents this from being > > > implemented (not that I'm saying it's a good idea, but it's doable at > > > last), am I wrong ? > > > > So I agree that it could be done like this, but I guess the following > > things bother me a bit. > > > > Mainly it seems more complicated (you have to do 2 things all the > > time, not just one - source of bugs right there). It confounds > > I don't think it will be source of bugs, but rather something not all > pipeline handlers might do. But we'll have compliance test suites, and > this will be caught "early" > > > existing expectations about V4L2 - why confuse people who thought they > > knew how V4L2 works? This is a "helpful C++ layer" on top of V4L2, > > Well, no :) > > libcamera might theoretically exists on top of other abstractions and > when it makes sense to deviate from v4l2 we're free to do so (again, > if it makes sense) > > > right? And the actual mechanics of splitting this up - every "set" > > operation becomes a "query-first-then-set"- is not lovely. > > The "query-first-then-set" happens in the theoretical > V4L2*Device::setColorSpace() function or in the pipeline handler code > ? > > > > > Actually I think there's a perfectly good solution which is simply to > > let the caller check the result. I know I get obsessed about colour > > spaces, but apparently not everyone does! This way everyone gets what > > they want. I can check. Others can check if they want. All problems > > solved. > > Not really, as setting a format on a metadata pad creates an error > message, and it doesn't anyway make much sense to configure a > colorspace there. As soon as we have devices producing data in > non-pixel formats (depth maps etc etc) we'll hit the same issue. Yes, I think I agree with this - and that's why it's best to leave the check to the caller. If it's image data, and they care about colour spaces, they can check. Otherwise they can leave it alone and all will be fine. I'll prepare another version with these and other changes and post it shortly! Thanks David > > Thanks > j > > > > > David > > > > > > > > > just get a warning message. I'm open to suggestions on how to resolve > > > > this. An extra parameter to the get/setFormat ("I am not an image data > > > > pad"?). Separate functions get/setImageFormat and > > > > get/setMetadataFormat? Rely on the caller to check for a bad colour > > > > space? (but will they bother?) > > > > > > > > Thoughts, anyone? > > > > > > > > Thanks! > > > > David > > > > > > > > > > > > > > > return 0; > > > > > > } > > > > > > > > > > > > @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) > > > > > > format->planes[0].bpl = pix->bytesperline; > > > > > > format->planes[0].size = pix->sizeimage; > > > > > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > > > + if (!format->colorSpace) > > > > > > + LOG(V4L2, Error) << "Retrieved unrecognised color space"; > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > > > > struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; > > > > > > int ret; > > > > > > > > > > > > + if (!format->colorSpace) > > > > > > + LOG(V4L2, Error) << "Trying to set unrecognised color space"; > > > > > > + > > > > > > v4l2Format.type = bufferType_; > > > > > > pix->width = format->size.width; > > > > > > pix->height = format->size.height; > > > > > > pix->pixelformat = format->fourcc; > > > > > > pix->bytesperline = format->planes[0].bpl; > > > > > > pix->field = V4L2_FIELD_NONE; > > > > > > + > > > > > > + ret = fromColorSpace(format->colorSpace, *pix); > > > > > > + if (ret < 0) > > > > > > + LOG(V4L2, Warning) > > > > > > + << "Set color space unrecognised by V4L2: " > > > > > > + << ColorSpace::toString(format->colorSpace); > > > > > > + > > > > > > ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); > > > > > > if (ret) { > > > > > > LOG(V4L2, Error) > > > > > > @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) > > > > > > format->planes[0].bpl = pix->bytesperline; > > > > > > format->planes[0].size = pix->sizeimage; > > > > > > > > > > > > + format->colorSpace = toColorSpace(*pix); > > > > > > + if (!format->colorSpace) > > > > > > + LOG(V4L2, Error) << "Undefined color space has been set"; > > > > > > + > > > > > > return 0; > > > > > > } > > > > > > > > > > > > -- > > > > > > 2.20.1 > > > > > >
diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h index a1c458e4..00bc50e7 100644 --- a/include/libcamera/internal/v4l2_videodevice.h +++ b/include/libcamera/internal/v4l2_videodevice.h @@ -20,6 +20,7 @@ #include <libcamera/base/log.h> #include <libcamera/base/signal.h> +#include <libcamera/color_space.h> #include <libcamera/framebuffer.h> #include <libcamera/geometry.h> #include <libcamera/pixel_format.h> @@ -167,6 +168,7 @@ public: V4L2PixelFormat fourcc; Size size; + std::optional<ColorSpace> colorSpace; std::array<Plane, 3> planes; unsigned int planesCount = 0; diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp index 4f04212d..e03ff8b5 100644 --- a/src/libcamera/v4l2_videodevice.cpp +++ b/src/libcamera/v4l2_videodevice.cpp @@ -370,17 +370,27 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const * \brief The plane line stride (in bytes) */ +/** + * \var V4L2DeviceFormat::fourcc + * \brief The fourcc code describing the pixel encoding scheme + * + * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, + * that identifies the image format pixel encoding scheme. + */ + /** * \var V4L2DeviceFormat::size * \brief The image size in pixels */ /** - * \var V4L2DeviceFormat::fourcc - * \brief The fourcc code describing the pixel encoding scheme + * \var V4L2DeviceFormat::colorSpace + * \brief The color space of the pixels * - * The fourcc code, as defined by the V4L2 API with the V4L2_PIX_FMT_* macros, - * that identifies the image format pixel encoding scheme. + * The color space of the image. When setting the format this may be + * unset, in which case the driver gets to use its default color space. + * After being set, this value should contain the color space that the + * was actually used. */ /** @@ -879,6 +889,10 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format) format->fourcc = V4L2PixelFormat(pix->pixelformat); format->planesCount = pix->num_planes; + format->colorSpace = toColorSpace(*pix); + if (!format->colorSpace) + LOG(V4L2, Error) << "Retrieved unrecognised color space"; + for (unsigned int i = 0; i < format->planesCount; ++i) { format->planes[i].bpl = pix->plane_fmt[i].bytesperline; format->planes[i].size = pix->plane_fmt[i].sizeimage; @@ -893,6 +907,9 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp; int ret; + if (!format->colorSpace) + LOG(V4L2, Warning) << "Setting undefined color space"; + v4l2Format.type = bufferType_; pix->width = format->size.width; pix->height = format->size.height; @@ -900,6 +917,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) pix->num_planes = format->planesCount; pix->field = V4L2_FIELD_NONE; + ret = fromColorSpace(format->colorSpace, *pix); + if (ret < 0) + LOG(V4L2, Warning) + << "Setting color space unrecognised by V4L2: " + << ColorSpace::toString(format->colorSpace); + ASSERT(pix->num_planes <= std::size(pix->plane_fmt)); for (unsigned int i = 0; i < pix->num_planes; ++i) { @@ -928,6 +951,10 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set) format->planes[i].size = pix->plane_fmt[i].sizeimage; } + format->colorSpace = toColorSpace(*pix); + if (!format->colorSpace) + LOG(V4L2, Warning) << "Unrecognised color space has been set"; + return 0; } @@ -951,6 +978,10 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format) format->planes[0].bpl = pix->bytesperline; format->planes[0].size = pix->sizeimage; + format->colorSpace = toColorSpace(*pix); + if (!format->colorSpace) + LOG(V4L2, Error) << "Retrieved unrecognised color space"; + return 0; } @@ -960,12 +991,22 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) struct v4l2_pix_format *pix = &v4l2Format.fmt.pix; int ret; + if (!format->colorSpace) + LOG(V4L2, Error) << "Trying to set unrecognised color space"; + v4l2Format.type = bufferType_; pix->width = format->size.width; pix->height = format->size.height; pix->pixelformat = format->fourcc; pix->bytesperline = format->planes[0].bpl; pix->field = V4L2_FIELD_NONE; + + ret = fromColorSpace(format->colorSpace, *pix); + if (ret < 0) + LOG(V4L2, Warning) + << "Set color space unrecognised by V4L2: " + << ColorSpace::toString(format->colorSpace); + ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format); if (ret) { LOG(V4L2, Error) @@ -985,6 +1026,10 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set) format->planes[0].bpl = pix->bytesperline; format->planes[0].size = pix->sizeimage; + format->colorSpace = toColorSpace(*pix); + if (!format->colorSpace) + LOG(V4L2, Error) << "Undefined color space has been set"; + return 0; }
The ColorSpace from the StreamConfiguration is now handled appropriately in the V4L2VideoDevice. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- include/libcamera/internal/v4l2_videodevice.h | 2 + src/libcamera/v4l2_videodevice.cpp | 53 +++++++++++++++++-- 2 files changed, 51 insertions(+), 4 deletions(-)