[libcamera-devel,v5,5/7] libcamera: Support passing ColorSpaces to V4L2 subdevices
diff mbox series

Message ID 20211104135805.5269-6-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 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            | 42 ++++++++++++++++++++-
 3 files changed, 44 insertions(+), 1 deletion(-)

Comments

Kieran Bingham Nov. 5, 2021, 12:46 p.m. UTC | #1 
Quoting David Plowman (2021-11-04 13:58:03)
> 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            | 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;
>  

This generates the following compile failures:

[19/131] Compiling C++ object src/libcamera/libcamera.so.0.0.0.p/pipeline_ipu3_cio2.cpp.o
FAILED: src/libcamera/libcamera.so.0.0.0.p/pipeline_ipu3_cio2.cpp.o 
g++ -Isrc/libcamera/libcamera.so.0.0.0.p -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -Iinclude/libcamera -Iinclude/libcamera/ipa -Iinclude/libcamera/internal -Isrc/libcamera/proxy -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wshadow -include config.h -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/libcamera/libcamera.so.0.0.0.p/pipeline_ipu3_cio2.cpp.o -MF src/libcamera/libcamera.so.0.0.0.p/pipeline_ipu3_cio2.cpp.o.d -o src/libcamera/libcamera.so.0.0.0.p/pipeline_ipu3_cio2.cpp.o -c ../../src/libcamera/pipeline/ipu3/cio2.cpp
../../src/libcamera/pipeline/ipu3/cio2.cpp: In member function ‘libcamera::V4L2SubdeviceFormat libcamera::CIO2Device::getSensorFormat(const std::vector<unsigned int>&, const libcamera::Size&) const’:
../../src/libcamera/pipeline/ipu3/cio2.cpp:328:2: error: missing initializer for member ‘libcamera::V4L2SubdeviceFormat::colorSpace’ [-Werror=missing-field-initializers]
  328 |  };
      |  ^
cc1plus: all warnings being treated as errors
[41/131] Compiling C++ object src/libcamera/libcamera.so.0.0.0.p/pipeline_simple_simple.cpp.o
FAILED: src/libcamera/libcamera.so.0.0.0.p/pipeline_simple_simple.cpp.o 
g++ -Isrc/libcamera/libcamera.so.0.0.0.p -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -Iinclude/libcamera -Iinclude/libcamera/ipa -Iinclude/libcamera/internal -Isrc/libcamera/proxy -fdiagnostics-color=always -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++17 -g -Wshadow -include config.h -fPIC -DLIBCAMERA_BASE_PRIVATE -MD -MQ src/libcamera/libcamera.so.0.0.0.p/pipeline_simple_simple.cpp.o -MF src/libcamera/libcamera.so.0.0.0.p/pipeline_simple_simple.cpp.o.d -o src/libcamera/libcamera.so.0.0.0.p/pipeline_simple_simple.cpp.o -c ../../src/libcamera/pipeline/simple/simple.cpp
../../src/libcamera/pipeline/simple/simple.cpp: In member function ‘int libcamera::SimpleCameraData::init()’:
../../src/libcamera/pipeline/simple/simple.cpp:460:59: error: missing initializer for member ‘libcamera::V4L2SubdeviceFormat::colorSpace’ [-Werror=missing-field-initializers]
  460 |   V4L2SubdeviceFormat format{ code, sensor_->resolution() };
      |                                                           ^
../../src/libcamera/pipeline/simple/simple.cpp: In member function ‘virtual int libcamera::SimplePipelineHandler::configure(libcamera::Camera*, libcamera::CameraConfiguration*)’:
../../src/libcamera/pipeline/simple/simple.cpp:911:76: error: missing initializer for member ‘libcamera::V4L2SubdeviceFormat::colorSpace’ [-Werror=missing-field-initializers]
  911 |  V4L2SubdeviceFormat format{ pipeConfig->code, data->sensor_->resolution() };
      |                                                                            ^
cc1plus: all warnings being treated as errors

I guess we don't want to add a default constructor. Are there obvious
choices to initialise the color space on those formats?




>         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();
> +
>         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
>
Jacopo Mondi Nov. 6, 2021, 4:35 p.m. UTC | #2 
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@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
>

Patch
diff mbox series 

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();
+
 	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;
 }