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

[Naushir Patuck]

Hi David,

Thank you for your work.

On Wed, 20 Oct 2021 at 12:08, David Plowman <david.plowman at raspberrypi.com>
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            | 69 +++++++++++++++++--
>  2 files changed, 66 insertions(+), 5 deletions(-)
>
> 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;
>
>         std::array<Plane, 3> planes;
>         unsigned int planesCount = 0;
> diff --git a/src/libcamera/v4l2_videodevice.cpp
> b/src/libcamera/v4l2_videodevice.cpp
> index ba5f88cd..9e57e257 100644
> --- a/src/libcamera/v4l2_videodevice.cpp
> +++ b/src/libcamera/v4l2_videodevice.cpp
> @@ -364,11 +364,6 @@ bool V4L2BufferCache::Entry::operator==(const
> FrameBuffer &buffer) const
>   * \brief The plane line stride (in bytes)
>   */
>
> -/**
> - * \var V4L2DeviceFormat::size
> - * \brief The image size in pixels
> - */
> -
>  /**
>   * \var V4L2DeviceFormat::fourcc
>   * \brief The fourcc code describing the pixel encoding scheme
> @@ -377,6 +372,23 @@ bool V4L2BufferCache::Entry::operator==(const
> FrameBuffer &buffer) const
>   * that identifies the image format pixel encoding scheme.
>   */
>
> +/**
> + * \var V4L2DeviceFormat::size
> + * \brief The image size in pixels
> + */
> +
> +/**
> + * \var V4L2DeviceFormat::colorSpace
> + * \brief The color space of the pixels
> + *
> + * 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.
> + */
> +
>  /**
>   * \var V4L2DeviceFormat::planes
>   * \brief The per-plane memory size information
> @@ -873,6 +885,12 @@ int
> V4L2VideoDevice::getFormatMultiplane(V4L2DeviceFormat *format)
>         format->fourcc = V4L2PixelFormat(pix->pixelformat);
>         format->planesCount = pix->num_planes;
>
> +       format->colorSpace = v4l2ToColorSpace(*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;
> @@ -887,6 +905,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();
> +
>

Should we return with an error code in the above if clause?  Similar for
the other setFormat() calls.
Other than that:

Reviewed-by: Naushir Patuck <naush at raspberrypi.com>



>         v4l2Format.type = bufferType_;
>         pix->width = format->size.width;
>         pix->height = format->size.height;
> @@ -894,6 +917,12 @@ int
> V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>         pix->num_planes = format->planesCount;
>         pix->field = V4L2_FIELD_NONE;
>
> +       ret = colorSpaceToV4l2(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) {
> @@ -922,6 +951,12 @@ int
> V4L2VideoDevice::trySetFormatMultiplane(V4L2DeviceFormat *format, bool set)
>                 format->planes[i].size = pix->plane_fmt[i].sizeimage;
>         }
>
> +       format->colorSpace = v4l2ToColorSpace(*pix);
> +       if (!format->colorSpace.isFullyDefined())
> +               LOG(V4L2, Warning)
> +                       << "Undefined color space has been set: "
> +                       << format->colorSpace.toString();
> +
>         return 0;
>  }
>
> @@ -945,6 +980,12 @@ int
> V4L2VideoDevice::getFormatSingleplane(V4L2DeviceFormat *format)
>         format->planes[0].bpl = pix->bytesperline;
>         format->planes[0].size = pix->sizeimage;
>
> +       format->colorSpace = v4l2ToColorSpace(*pix);
> +       if (!format->colorSpace.isFullyDefined())
> +               LOG(V4L2, Warning)
> +                       << "Retrieved undefined color space: "
> +                       << format->colorSpace.toString();
> +
>         return 0;
>  }
>
> @@ -954,12 +995,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 = colorSpaceToV4l2(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)
> @@ -979,6 +1032,12 @@ int
> V4L2VideoDevice::trySetFormatSingleplane(V4L2DeviceFormat *format, bool set)
>         format->planes[0].bpl = pix->bytesperline;
>         format->planes[0].size = pix->sizeimage;
>
> +       format->colorSpace = v4l2ToColorSpace(*pix);
> +       if (!format->colorSpace.isFullyDefined())
> +               LOG(V4L2, Warning)
> +                       << "Undefined color space has been set: "
> +                       << format->colorSpace.toString();
> +
>         return 0;
>  }
>
> --
> 2.20.1
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20211025/05502352/attachment.htm>