[libcamera-devel,v5,4/7] libcamera: Support passing ColorSpaces to V4L2 video devices
diff mbox series

Message ID 20211104135805.5269-5-david.plowman@raspberrypi.com
State Superseded
Headers show
Series
  • Colour spaces
Related show

Commit Message

David Plowman Nov. 4, 2021, 1:58 p.m. UTC 
The ColorSpace from the StreamConfiguration is now handled
appropriately in the V4L2VideoDevice.

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

Comments

Kieran Bingham Nov. 5, 2021, 12:22 p.m. UTC | #1 
Quoting David Plowman (2021-11-04 13:58:02)
> The ColorSpace from the StreamConfiguration is now handled
> appropriately in the V4L2VideoDevice.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  2 +
>  src/libcamera/v4l2_videodevice.cpp            | 67 +++++++++++++++++--
>  2 files changed, 65 insertions(+), 4 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index a1c458e4..6ca749c1 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;
> +       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..ccf3d28c 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -370,17 +370,29 @@ 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.
> + */
> +

Aha, these were incorrectly ordered. This could be a separate patch, but
it's probably easier to just explictily state in the commit message:

  "Re-sort the documentation correctly while adding the colorSpace"

>  /**
>   * \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.
> + * When setting or trying a format, passing in "Undefined" fields in the
> + * ColorSpace is not permitted 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
> + * class cannot represent.
>   */
>  
>  /**
> @@ -879,6 +891,12 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
>         format->fourcc = V4L2PixelFormat(pix->pixelformat);
>         format->planesCount = pix->num_planes;
>  
> +       format->colorSpace = toColorSpace(*pix);
> +       if (!format->colorSpace.isFullyDefined())
> +               LOG(V4L2, Warning)
> +                       << "Retrieved undefined color space: "
> +                       << format->colorSpace.toString();
> +
>         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 +911,11 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>         struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;
>         int ret;
>  
> +       if (!format->colorSpace.isFullyDefined())
> +               LOG(V4L2, Error)
> +                       << "Trying to set undefined color space: "
> +                       << format->colorSpace.toString();
> +
>         v4l2Format.type = bufferType_;
>         pix->width = format->size.width;
>         pix->height = format->size.height;
> @@ -900,6 +923,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: "
> +                       << format->colorSpace.toString();
> +
>         ASSERT(pix->num_planes <= std::size(pix->plane_fmt));
>  
>         for (unsigned int i = 0; i < pix->num_planes; ++i) {
> @@ -928,6 +957,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>                 format->planes[i].size = pix->plane_fmt[i].sizeimage;
>         }
>  
> +       format->colorSpace = toColorSpace(*pix);
> +       if (!format->colorSpace.isFullyDefined())
> +               LOG(V4L2, Warning)
> +                       << "Undefined color space has been set: "
> +                       << format->colorSpace.toString();
> +
>         return 0;
>  }
>  
> @@ -951,6 +986,12 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
>         format->planes[0].bpl = pix->bytesperline;
>         format->planes[0].size = pix->sizeimage;
>  
> +       format->colorSpace = toColorSpace(*pix);
> +       if (!format->colorSpace.isFullyDefined())
> +               LOG(V4L2, Warning)
> +                       << "Retrieved undefined color space: "
> +                       << format->colorSpace.toString();
> +
>         return 0;
>  }
>  
> @@ -960,12 +1001,24 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
>         struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;
>         int ret;
>  
> +       if (!format->colorSpace.isFullyDefined())
> +               LOG(V4L2, Error)
> +                       << "Trying to set undefined color space: "
> +                       << format->colorSpace.toString();

I wonder if these might get a bit verbose - but I think that's the
point, to highlight that the colorspaces are not being handled correctly
so I'm fine with that for now.

If we find it's overly verbose in conditions that it doesn't need to be
we can handle those accordingly. Better to be loud to start with I
think.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> +
>         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: "
> +                       << format->colorSpace.toString();
> +
>         ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);
>         if (ret) {
>                 LOG(V4L2, Error)
> @@ -985,6 +1038,12 @@ 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.isFullyDefined())
> +               LOG(V4L2, Warning)
> +                       << "Undefined color space has been set: "
> +                       << format->colorSpace.toString();
> +
>         return 0;
>  }
>  
> -- 
> 2.20.1
>
Jacopo Mondi Nov. 6, 2021, 4:32 p.m. UTC | #2 
Hi David,

On Thu, Nov 04, 2021 at 01:58:02PM +0000, David Plowman wrote:
> The ColorSpace from the StreamConfiguration is now handled
> appropriately in the V4L2VideoDevice.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_videodevice.h |  2 +
>  src/libcamera/v4l2_videodevice.cpp            | 67 +++++++++++++++++--
>  2 files changed, 65 insertions(+), 4 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
> index a1c458e4..6ca749c1 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;
> +	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..ccf3d28c 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -370,17 +370,29 @@ 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.
> + */
> +
>  /**
>   * \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.
> + * When setting or trying a format, passing in "Undefined" fields in the
> + * ColorSpace is not permitted 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
> + * class cannot represent.
>   */
>
>  /**
> @@ -879,6 +891,12 @@ int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
>  	format->fourcc = V4L2PixelFormat(pix->pixelformat);
>  	format->planesCount = pix->num_planes;
>
> +	format->colorSpace = toColorSpace(*pix);
> +	if (!format->colorSpace.isFullyDefined())
> +		LOG(V4L2, Warning)
> +			<< "Retrieved undefined color space: "
> +			<< format->colorSpace.toString();
> +

If this gets in in the current form, all other platforms will have
their log polluted by these messages. For most pipeline handlers I
assume with the appropriate knowledge if applications are not
instrumented to pass in a valid color space, it can be adjusted to
something meaningul. But what about the simple pipeline handler which
works with devices that simply transfer to application streams in
YUV/RGB formats produced by the sensor ? What colorspace should
pipeline handler adjust to ?

>  	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 +911,11 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>  	struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;
>  	int ret;
>
> +	if (!format->colorSpace.isFullyDefined())
> +		LOG(V4L2, Error)
> +			<< "Trying to set undefined color space: "
> +			<< format->colorSpace.toString();
> +
>  	v4l2Format.type = bufferType_;
>  	pix->width = format->size.width;
>  	pix->height = format->size.height;
> @@ -900,6 +923,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: "
> +			<< format->colorSpace.toString();
> +
>  	ASSERT(pix->num_planes <= std::size(pix->plane_fmt));
>
>  	for (unsigned int i = 0; i < pix->num_planes; ++i) {
> @@ -928,6 +957,12 @@ int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>  		format->planes[i].size = pix->plane_fmt[i].sizeimage;
>  	}
>
> +	format->colorSpace = toColorSpace(*pix);
> +	if (!format->colorSpace.isFullyDefined())
> +		LOG(V4L2, Warning)
> +			<< "Undefined color space has been set: "
> +			<< format->colorSpace.toString();
> +
>  	return 0;
>  }
>
> @@ -951,6 +986,12 @@ int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
>  	format->planes[0].bpl = pix->bytesperline;
>  	format->planes[0].size = pix->sizeimage;
>
> +	format->colorSpace = toColorSpace(*pix);
> +	if (!format->colorSpace.isFullyDefined())
> +		LOG(V4L2, Warning)
> +			<< "Retrieved undefined color space: "
> +			<< format->colorSpace.toString();
> +
>  	return 0;
>  }
>
> @@ -960,12 +1001,24 @@ int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
>  	struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;
>  	int ret;
>
> +	if (!format->colorSpace.isFullyDefined())
> +		LOG(V4L2, Error)
> +			<< "Trying to set undefined color space: "
> +			<< format->colorSpace.toString();
> +
>  	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: "
> +			<< format->colorSpace.toString();
> +
>  	ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);
>  	if (ret) {
>  		LOG(V4L2, Error)
> @@ -985,6 +1038,12 @@ 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.isFullyDefined())
> +		LOG(V4L2, Warning)
> +			<< "Undefined color space has been set: "
> +			<< format->colorSpace.toString();
> +
>  	return 0;
>  }
>
> --
> 2.20.1
>

Patch
diff mbox series 

diff --git a/include/libcamera/internal/v4l2_videodevice.h b/include/libcamera/internal/v4l2_videodevice.h
index a1c458e4..6ca749c1 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;
+	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..ccf3d28c 100644
--- a/src/libcamera/v4l2_videodevice.cpp
+++ b/src/libcamera/v4l2_videodevice.cpp
@@ -370,17 +370,29 @@  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.
+ */
+
 /**
  * \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.
+ * When setting or trying a format, passing in "Undefined" fields in the
+ * ColorSpace is not permitted 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
+ * class cannot represent.
  */
 
 /**
@@ -879,6 +891,12 @@  int V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
 	format->fourcc = V4L2PixelFormat(pix->pixelformat);
 	format->planesCount = pix->num_planes;
 
+	format->colorSpace = toColorSpace(*pix);
+	if (!format->colorSpace.isFullyDefined())
+		LOG(V4L2, Warning)
+			<< "Retrieved undefined color space: "
+			<< format->colorSpace.toString();
+
 	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 +911,11 @@  int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
 	struct v4l2_pix_format_mplane *pix = &v4l2Format.fmt.pix_mp;
 	int ret;
 
+	if (!format->colorSpace.isFullyDefined())
+		LOG(V4L2, Error)
+			<< "Trying to set undefined color space: "
+			<< format->colorSpace.toString();
+
 	v4l2Format.type = bufferType_;
 	pix->width = format->size.width;
 	pix->height = format->size.height;
@@ -900,6 +923,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: "
+			<< format->colorSpace.toString();
+
 	ASSERT(pix->num_planes <= std::size(pix->plane_fmt));
 
 	for (unsigned int i = 0; i < pix->num_planes; ++i) {
@@ -928,6 +957,12 @@  int V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
 		format->planes[i].size = pix->plane_fmt[i].sizeimage;
 	}
 
+	format->colorSpace = toColorSpace(*pix);
+	if (!format->colorSpace.isFullyDefined())
+		LOG(V4L2, Warning)
+			<< "Undefined color space has been set: "
+			<< format->colorSpace.toString();
+
 	return 0;
 }
 
@@ -951,6 +986,12 @@  int V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
 	format->planes[0].bpl = pix->bytesperline;
 	format->planes[0].size = pix->sizeimage;
 
+	format->colorSpace = toColorSpace(*pix);
+	if (!format->colorSpace.isFullyDefined())
+		LOG(V4L2, Warning)
+			<< "Retrieved undefined color space: "
+			<< format->colorSpace.toString();
+
 	return 0;
 }
 
@@ -960,12 +1001,24 @@  int V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
 	struct v4l2_pix_format *pix = &v4l2Format.fmt.pix;
 	int ret;
 
+	if (!format->colorSpace.isFullyDefined())
+		LOG(V4L2, Error)
+			<< "Trying to set undefined color space: "
+			<< format->colorSpace.toString();
+
 	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: "
+			<< format->colorSpace.toString();
+
 	ret = ioctl(set ? VIDIOC_S_FMT : VIDIOC_TRY_FMT, &v4l2Format);
 	if (ret) {
 		LOG(V4L2, Error)
@@ -985,6 +1038,12 @@  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.isFullyDefined())
+		LOG(V4L2, Warning)
+			<< "Undefined color space has been set: "
+			<< format->colorSpace.toString();
+
 	return 0;
 }