[libcamera-devel] [PATCH v5 5/7] libcamera: Support passing ColorSpaces to V4L2 subdevices

[Jacopo Mondi]

Hi David,

On Thu, Nov 04, 2021 at 01:58:03PM +0000, David Plowman wrote:
> The ColorSpace from the StreamConfiguration is now handled
> appropriately in the V4L2Subdevice.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_subdevice.h |  2 +
>  src/libcamera/camera_sensor.cpp             |  1 +
>  src/libcamera/v4l2_subdevice.cpp            | 42 ++++++++++++++++++++-
>  3 files changed, 44 insertions(+), 1 deletion(-)
>
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 97b89fb9..f3ab8454 100644
> --- a/include/libcamera/internal/v4l2_subdevice.h
> +++ b/include/libcamera/internal/v4l2_subdevice.h
> @@ -14,6 +14,7 @@
>  #include <libcamera/base/class.h>
>  #include <libcamera/base/log.h>
>
> +#include <libcamera/color_space.h>
>  #include <libcamera/geometry.h>
>
>  #include "libcamera/internal/formats.h"
> @@ -27,6 +28,7 @@ class MediaDevice;
>  struct V4L2SubdeviceFormat {
>  	uint32_t mbus_code;
>  	Size size;
> +	ColorSpace colorSpace;
>
>  	const std::string toString() const;
>  	uint8_t bitsPerPixel() const;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 9fdb8c09..6fcd1c1d 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -613,6 +613,7 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
>  	V4L2SubdeviceFormat format{
>  		.mbus_code = bestCode,
>  		.size = *bestSize,
> +		.colorSpace = ColorSpace::Raw,
>  	};
>
>  	return format;
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 023e2328..b348170f 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -168,6 +168,18 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
>   * \brief The image size in pixels
>   */
>
> +/**
> + * \var V4L2SubdeviceFormat::colorSpace
> + * \brief The color space of the pixels
> + *
> + * When setting or trying a format, passing in "Undefined" fields in the
> + * ColorSpace is not recommended because the driver will then make an
> + * arbitrary choice of its own. Choices made by the driver will be
> + * passed back in the normal way, though note that "Undefined" values can
> + * be returned if the device has chosen something that the ColorSpace
> + * cannot represent.
> + */
> +
>  /**
>   * \brief Assemble and return a string describing the format
>   * \return A string describing the V4L2SubdeviceFormat
> @@ -400,6 +412,17 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  	format->size.height = subdevFmt.format.height;
>  	format->mbus_code = subdevFmt.format.code;
>
> +	format->colorSpace = toColorSpace(subdevFmt.format);
> +	/*
> +	 * This warning can be ignored on metadata pads. These are normally
> +	 * pads other than zero.
> +	 * \todo find a way to detect metadata pads and ignore them
> +	 */
> +	if (!format->colorSpace.isFullyDefined())
> +		LOG(V4L2, Warning)
> +			<< "Retrieved undefined color space on pad " << pad
> +			<< ": " << format->colorSpace.toString();
> +

Argh. Think makes me think that if we want to forbid applications from
passing in an Undefined color space, pipeline handler should be
capable of saying that they don't care, as they know that, in example,
setting a color space for embedded data makes no sense (is my
interpreation that it makes no sense right ?)

Thanks
   j

>  	return 0;
>  }
>
> @@ -418,6 +441,11 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  			     Whence whence)
>  {
> +	if (!format->colorSpace.isFullyDefined())
> +		LOG(V4L2, Error)
> +			<< "Trying to set undefined color space: "
> +			<< format->colorSpace.toString();
> +
>  	struct v4l2_subdev_format subdevFmt = {};
>  	subdevFmt.which = whence == ActiveFormat ? V4L2_SUBDEV_FORMAT_ACTIVE
>  			: V4L2_SUBDEV_FORMAT_TRY;
> @@ -427,7 +455,13 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  	subdevFmt.format.code = format->mbus_code;
>  	subdevFmt.format.field = V4L2_FIELD_NONE;
>
> -	int ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
> +	int ret = fromColorSpace(format->colorSpace, subdevFmt.format);
> +	if (ret < 0)
> +		LOG(V4L2, Warning)
> +			<< "Setting color space unrecognised by V4L2: "
> +			<< format->colorSpace.toString();
> +
> +	ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
>  	if (ret) {
>  		LOG(V4L2, Error)
>  			<< "Unable to set format on pad " << pad
> @@ -439,6 +473,12 @@ int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  	format->size.height = subdevFmt.format.height;
>  	format->mbus_code = subdevFmt.format.code;
>
> +	format->colorSpace = toColorSpace(subdevFmt.format);
> +	if (!format->colorSpace.isFullyDefined())
> +		LOG(V4L2, Warning)
> +			<< "Undefined color space has been set: "
> +			<< format->colorSpace.toString();
> +
>  	return 0;
>  }
>
> --
> 2.20.1
>