[libcamera-devel] [PATCH v6 4/7] libcamera: Support passing ColorSpaces to V4L2 video devices
David Plowman
david.plowman at raspberrypi.com
Wed Nov 24 12:55:15 CET 2021
Hi Umang
Thanks for looking at this!
On Tue, 23 Nov 2021 at 08:01, Umang Jain <umang.jain at 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 at 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;
> > }
> >
More information about the libcamera-devel
mailing list