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

[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 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            | 37 ++++++++++++++++++++-
>  3 files changed, 39 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..8538e66c 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,12 @@ int V4L2Subdevice::getFormat(unsigned int pad,
> V4L2SubdeviceFormat *format,
>         format->size.height = subdevFmt.format.height;
>         format->mbus_code = subdevFmt.format.code;
>
> +       format->colorSpace = v4l2ToColorSpace(subdevFmt.format);
> +       if (!format->colorSpace.isFullyDefined())
> +               LOG(V4L2, Warning)
> +                       << "Retrieved undefined color space: "
> +                       << format->colorSpace.toString();
> +
>         return 0;
>  }
>
> @@ -418,6 +436,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();
> +
>

Like the previous patch, should we return with an error code here? Other
than that:

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


>         struct v4l2_subdev_format subdevFmt = {};
>         subdevFmt.which = whence == ActiveFormat ?
> V4L2_SUBDEV_FORMAT_ACTIVE
>                         : V4L2_SUBDEV_FORMAT_TRY;
> @@ -427,7 +450,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 = colorSpaceToV4l2(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 +468,12 @@ int V4L2Subdevice::setFormat(unsigned int pad,
> V4L2SubdeviceFormat *format,
>         format->size.height = subdevFmt.format.height;
>         format->mbus_code = subdevFmt.format.code;
>
> +       format->colorSpace = v4l2ToColorSpace(subdevFmt.format);
> +       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/5d4b46f4/attachment-0001.htm>