[libcamera-devel] [PATCH v6 4/7] libcamera: Support passing ColorSpaces to V4L2 video devices

[Jacopo Mondi]

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);
> +	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
>