[libcamera-devel,v6,5/7] libcamera: Support passing ColorSpaces to V4L2 subdevices

14630 diff mbox series

The ColorSpace from the StreamConfiguration is now handled
appropriately in the V4L2Subdevice.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
---
 include/libcamera/internal/v4l2_subdevice.h |  2 ++
 src/libcamera/camera_sensor.cpp             |  1 +
 src/libcamera/v4l2_subdevice.cpp            | 35 ++++++++++++++++++++-
 3 files changed, 37 insertions(+), 1 deletion(-)

Quoting David Plowman (2021-11-18 15:19:31)
> The ColorSpace from the StreamConfiguration is now handled
> appropriately in the V4L2Subdevice.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_subdevice.h |  2 ++
>  src/libcamera/camera_sensor.cpp             |  1 +
>  src/libcamera/v4l2_subdevice.cpp            | 35 ++++++++++++++++++++-
>  3 files changed, 37 insertions(+), 1 deletion(-)
> 
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 97b89fb9..a6a0a870 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;
> +       std::optional<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..b60ab69a 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -168,6 +168,16 @@ const std::map<uint32_t, V4L2SubdeviceFormatInfo> formatInfoMap = {
>   * \brief The image size in pixels
>   */
>  
> +/**
> + * \var V4L2SubdeviceFormat::colorSpace
> + * \brief The color space of the pixels
> + *
> + * The color space of the image. When setting the format this may be
> + * unset, in which case the driver gets to use its default color space.
> + * After being set, this value should contain the color space that the

should ? or will? (or ... shall?)

/that the was/that was/


> + * was actually used.

I'd put something in about how it is represented when invalid too:

Pinch or adapt from below if there's anything useful there...

"""
The color space of the image. When setting the format this may be left
unset, which will request the default color space from the driver. When
a format is returned from a device, this will contain the color space
that is used or become unset if an invalid or unsupported color space
was provided by the driver.
"""


> + */
> +
>  /**
>   * \brief Assemble and return a string describing the format
>   * \return A string describing the V4L2SubdeviceFormat
> @@ -400,6 +410,16 @@ 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)
> +               LOG(V4L2, Warning)
> +                       << "Retrieved unrecognised color space on pad " << pad;
> +
>         return 0;
>  }
>  
> @@ -418,6 +438,9 @@ int V4L2Subdevice::getFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>  int V4L2Subdevice::setFormat(unsigned int pad, V4L2SubdeviceFormat *format,
>                              Whence whence)
>  {
> +       if (!format->colorSpace)
> +               LOG(V4L2, Error) << "Setting an undefined color space";
> +

From my understanding now, this isn't an error as it will request the
color space from the driver?

>         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 = fromColorSpace(format->colorSpace, subdevFmt.format);
> +       if (ret < 0)
> +               LOG(V4L2, Warning)
> +                       << "Setting color space unrecognised by V4L2: "

But this should be ... as this is where we need to know what was/is to
be used...?

> +                       << ColorSpace::toString(format->colorSpace);
> +
> +       ret = ioctl(VIDIOC_SUBDEV_S_FMT, &subdevFmt);
>         if (ret) {
>                 LOG(V4L2, Error)
>                         << "Unable to set format on pad " << pad
> @@ -439,6 +468,10 @@ 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)
> +               LOG(V4L2, Warning) << "Unrecognised color space has been set";

Here too should be Error?

> +
>         return 0;
>  }
>  
> -- 
> 2.20.1
>