[libcamera-devel] [PATCH v6 4/7] libcamera: Support passing ColorSpaces to V4L2 video devices
Jacopo Mondi
jacopo at jmondi.org
Thu Nov 25 18:19:02 CET 2021
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 ?
> 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