[libcamera-devel] [PATCH v6 4/7] libcamera: Support passing ColorSpaces to V4L2 video devices
David Plowman
david.plowman at raspberrypi.com
Fri Nov 26 10:34:04 CET 2021
Hi Jacopo
On Fri, 26 Nov 2021 at 08:20, Jacopo Mondi <jacopo at 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 at 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 at 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 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.
> > > > > > + */
> > > > > > +
> > > > >
> > > > > 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
> > > > > >
More information about the libcamera-devel
mailing list