[libcamera-devel,v2,2/3] libcamera: Support passing ColorSpaces to V4L2 drivers

13952 diff mbox series

The ColorSpace class is added to the StreamConfiguration, and is now
passed to V4L2 devices where it is handled appropriately. Note how
this means that the colour space is configured per-stream (though
platforms may restrict this).

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/internal/v4l2_videodevice.h |   2 +
 include/libcamera/stream.h                    |   3 +
 src/libcamera/v4l2_videodevice.cpp            | 119 ++++++++++++++++++
 3 files changed, 124 insertions(+)

Hi David,

(CC'ing Hans)

Thank you for the patch.

On Mon, Sep 27, 2021 at 01:33:26PM +0100, David Plowman wrote:
> The ColorSpace class is added to the StreamConfiguration, and is now
> passed to V4L2 devices where it is handled appropriately. Note how
> this means that the colour space is configured per-stream (though
> platforms may restrict this).
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |   2 +
>  include/libcamera/stream.h                    |   3 +
>  src/libcamera/v4l2_videodevice.cpp            | 119 ++++++++++++++++++

I'd split this patch in two if you post a v3, as the change to
StreamConfiguration is not directly related to the colour space support
in V4L2VideoDevice.

>  3 files changed, 124 insertions(+)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index efe34d47..34cc9cdd 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>
> @@ -163,6 +164,7 @@ public:
>  
>  	V4L2PixelFormat fourcc;
>  	Size size;
> +	ColorSpace colorSpace;

We'll need to same change in V4L2SubdeviceFormat, right ?

>  
>  	std::array<Plane, 3> planes;
>  	unsigned int planesCount = 0;
> diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> index 0c55e716..131f7733 100644
> --- a/include/libcamera/stream.h
> +++ b/include/libcamera/stream.h
> @@ -12,6 +12,7 @@
>  #include <string>
>  #include <vector>
>  
> +#include <libcamera/color_space.h>
>  #include <libcamera/framebuffer.h>
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
> @@ -47,6 +48,8 @@ struct StreamConfiguration {
>  
>  	unsigned int bufferCount;
>  
> +	ColorSpace colorSpace;

This is missing documentation. It's probably the most crucial part as we
need to define the behaviour (such as how pipeline handlers should
adjust unsupported values, but also how to handle inter-stream
constraints for instance).

> +
>  	Stream *stream() const { return stream_; }
>  	void setStream(Stream *stream) { stream_ = stream; }
>  	const StreamFormats &formats() const { return formats_; }
> diff --git a/src/libcamera/v4l2_videodevice.cpp b/src/libcamera/v4l2_videodevice.cpp
> index ba5f88cd..3e6c8b87 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -369,6 +369,11 @@ bool V4L2BufferCache::Entry::operator==(const FrameBuffer &buffer) const
>   * \brief The image size in pixels
>   */
>  
> +/**
> + * \var V4L2DeviceFormat::colorSpace
> + * \brief The color space of the pixels

More documentation would be useful here too. In particular, the VIDEO
encoding is not valid in V4L2DeviceFormat as it's meant to be handled by
pipeline handler.

> + */
> +
>  /**
>   * \var V4L2DeviceFormat::fourcc
>   * \brief The fourcc code describing the pixel encoding scheme

While at it, could you move the documentation of fourcc before size ?

> @@ -752,6 +757,114 @@ int V4L2VideoDevice::getFormat(V4L2DeviceFormat *format)
>  		return getFormatSingleplane(format);
>  }
>  
> +static const std::vector<std::pair<ColorSpace, v4l2_colorspace>> colorSpaceToV4l2 = {

As mentioned separately, I think you could also use a
std::unordered_map<> to have better lookup performance without having to
define an order for colour spaces.

On the other hand, given that you also define maps in the other
direction, maybe we could just use vectors of pairs and always look up
using the vectors (possibly with a small helper function template to
make the syntax less cumbersome). O(n) may be more efficient than
average O(1) for small values of n, and there will then be less risks of
the two maps getting out of sync. Up to you.

> +	{ ColorSpace::RAW, V4L2_COLORSPACE_RAW },
> +	{ ColorSpace::JFIF, V4L2_COLORSPACE_JPEG },
> +	{ ColorSpace::SMPTE170M, V4L2_COLORSPACE_SMPTE170M },
> +	{ ColorSpace::REC709, V4L2_COLORSPACE_REC709 },
> +	{ ColorSpace::REC2020, V4L2_COLORSPACE_BT2020 },
> +};
> +
> +static const std::map<ColorSpace::Encoding, v4l2_ycbcr_encoding> encodingToV4l2 = {
> +	{ ColorSpace::Encoding::REC601, V4L2_YCBCR_ENC_601 },
> +	{ ColorSpace::Encoding::REC709, V4L2_YCBCR_ENC_709 },
> +	{ ColorSpace::Encoding::REC2020, V4L2_YCBCR_ENC_BT2020 },
> +};
> +
> +static const std::map<ColorSpace::TransferFunction, v4l2_xfer_func> transferFunctionToV4l2 = {
> +	{ ColorSpace::TransferFunction::IDENTITY, V4L2_XFER_FUNC_NONE },
> +	{ ColorSpace::TransferFunction::SRGB, V4L2_XFER_FUNC_SRGB },
> +	{ ColorSpace::TransferFunction::REC709, V4L2_XFER_FUNC_709 },
> +};
> +
> +static const std::map<ColorSpace::Range, v4l2_quantization> rangeToV4l2 = {
> +	{ ColorSpace::Range::FULL, V4L2_QUANTIZATION_FULL_RANGE },
> +	{ ColorSpace::Range::LIMITED, V4L2_QUANTIZATION_LIM_RANGE },
> +};
> +
> +static const std::map<uint32_t, ColorSpace> v4l2ToColorSpace = {
> +	{ V4L2_COLORSPACE_RAW, ColorSpace::RAW },
> +	{ V4L2_COLORSPACE_JPEG, ColorSpace::JFIF },
> +	{ V4L2_COLORSPACE_SRGB, ColorSpace::JFIF },
> +	{ V4L2_COLORSPACE_SMPTE170M, ColorSpace::SMPTE170M },
> +	{ V4L2_COLORSPACE_REC709, ColorSpace::REC709 },
> +	{ V4L2_COLORSPACE_BT2020, ColorSpace::REC2020 },
> +};
> +
> +static const std::map<uint32_t, ColorSpace::Encoding> v4l2ToEncoding = {
> +	{ V4L2_YCBCR_ENC_601, ColorSpace::Encoding::REC601 },
> +	{ V4L2_YCBCR_ENC_709, ColorSpace::Encoding::REC709 },
> +	{ V4L2_YCBCR_ENC_BT2020, ColorSpace::Encoding::REC2020 },
> +};
> +
> +static const std::map<uint32_t, ColorSpace::TransferFunction> v4l2ToTransferFunction = {
> +	{ V4L2_XFER_FUNC_NONE, ColorSpace::TransferFunction::IDENTITY },
> +	{ V4L2_XFER_FUNC_SRGB, ColorSpace::TransferFunction::SRGB },
> +	{ V4L2_XFER_FUNC_709, ColorSpace::TransferFunction::REC709 },
> +};
> +
> +static const std::map<uint32_t, ColorSpace::Range> v4l2ToRange = {
> +	{ V4L2_QUANTIZATION_FULL_RANGE, ColorSpace::Range::FULL },
> +	{ V4L2_QUANTIZATION_LIM_RANGE, ColorSpace::Range::LIMITED },
> +};
> +
> +template<typename T>
> +static void setColorSpace(const ColorSpace &colorSpace, T &v4l2PixFormat)
> +{
> +	if (!colorSpace.isFullyDefined())
> +		LOG(V4L2, Warning) << "Setting non-fully defined colour space"
> +				   << colorSpace.toString();
> +
> +	v4l2PixFormat.colorspace = V4L2_COLORSPACE_DEFAULT;
> +	v4l2PixFormat.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +	v4l2PixFormat.xfer_func = V4L2_XFER_FUNC_DEFAULT;
> +	v4l2PixFormat.quantization = V4L2_QUANTIZATION_DEFAULT;
> +
> +	auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),
> +				     [&colorSpace](const std::pair<ColorSpace, v4l2_colorspace> &item) { return colorSpace == item.first; });

Let's shorten this a bit:

	auto itColor = std::find_if(colorSpaceToV4l2.begin(), colorSpaceToV4l2.end(),
				    [&colorSpace](const std::pair<ColorSpace, v4l2_colorspace> &item) {
					    return colorSpace == item.first;
				    });

> +	if (itColor != colorSpaceToV4l2.end())
> +		v4l2PixFormat.colorspace = itColor->second;
> +
> +	auto itEncoding = encodingToV4l2.find(colorSpace.encoding);
> +	if (itEncoding != encodingToV4l2.end())
> +		v4l2PixFormat.ycbcr_enc = itEncoding->second;
> +
> +	auto itTransfer = transferFunctionToV4l2.find(colorSpace.transferFunction);
> +	if (itTransfer != transferFunctionToV4l2.end())
> +		v4l2PixFormat.xfer_func = itTransfer->second;
> +
> +	auto itRange = rangeToV4l2.find(colorSpace.range);
> +	if (itRange != rangeToV4l2.end())
> +		v4l2PixFormat.quantization = itRange->second;
> +}
> +
> +template<typename T>
> +static ColorSpace getColorSpace(const T &v4l2PixFormat)
> +{
> +	ColorSpace colorSpace;
> +
> +	auto itColor = v4l2ToColorSpace.find(v4l2PixFormat.colorspace);
> +	if (itColor != v4l2ToColorSpace.end())
> +		colorSpace = itColor->second;
> +
> +	auto itEncoding = v4l2ToEncoding.find(v4l2PixFormat.ycbcr_enc);
> +	if (itEncoding != v4l2ToEncoding.end())
> +		colorSpace.encoding = itEncoding->second;
> +
> +	auto itTransfer = v4l2ToTransferFunction.find(v4l2PixFormat.xfer_func);
> +	if (itTransfer != v4l2ToTransferFunction.end())
> +		colorSpace.transferFunction = itTransfer->second;
> +
> +	auto itRange = v4l2ToRange.find(v4l2PixFormat.quantization);
> +	if (itRange != v4l2ToRange.end())
> +		colorSpace.range = itRange->second;
> +
> +	if (!colorSpace.isFullyDefined())
> +		LOG(V4L2, Warning) << "Returning non-fully defined colour space"
> +				   << colorSpace.toString();
> +	return colorSpace;
> +}
> +
>  /**
>   * \brief Try an image format on the V4L2 video device
>   * \param[inout] format The image format to test applicability to the video device
> @@ -871,6 +984,7 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
>  	format->size.width = pix->width;
>  	format->size.height = pix->height;
>  	format->fourcc = V4L2PixelFormat(pix->pixelformat);
> +	format->colorSpace = getColorSpace(*pix);
>  	format->planesCount = pix->num_planes;
>  
>  	for (unsigned int i = 0; i < format->planesCount; ++i) {
> @@ -893,6 +1007,7 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>  	pix->pixelformat = format->fourcc;
>  	pix->num_planes = format->planesCount;
>  	pix->field = V4L2_FIELD_NONE;
> +	setColorSpace(format->colorSpace, *pix);
>  
>  	ASSERT(pix->num_planes <= std::size(pix->plane_fmt));
>  
> @@ -921,6 +1036,7 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>  		format->planes[i].bpl = pix->plane_fmt[i].bytesperline;
>  		format->planes[i].size = pix->plane_fmt[i].sizeimage;
>  	}
> +	format->colorSpace = getColorSpace(*pix);
>  
>  	return 0;
>  }
> @@ -941,6 +1057,7 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
>  	format->size.width = pix->width;
>  	format->size.height = pix->height;
>  	format->fourcc = V4L2PixelFormat(pix->pixelformat);
> +	format->colorSpace = getColorSpace(*pix);
>  	format->planesCount = 1;
>  	format->planes[0].bpl = pix->bytesperline;
>  	format->planes[0].size = pix->sizeimage;
> @@ -960,6 +1077,7 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
>  	pix->pixelformat = format->fourcc;
>  	pix->bytesperline = format->planes[0].bpl;
>  	pix->field = V4L2_FIELD_NONE;
> +	setColorSpace(format->colorSpace, *pix);
>  	ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);
>  	if (ret) {
>  		LOG(V4L2, Error)
> @@ -978,6 +1096,7 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
>  	format->planesCount = 1;
>  	format->planes[0].bpl = pix->bytesperline;
>  	format->planes[0].size = pix->sizeimage;
> +	format->colorSpace = getColorSpace(*pix);
>  
>  	return 0;
>  }