[libcamera-devel] [PATCH v9 5/8] libcamera: Add colorSpace field to V4L2SubdeviceFormat

[Laurent Pinchart]

Hi David,

Thank you for the patch.

On Mon, Dec 06, 2021 at 10:50:28AM +0000, David Plowman wrote:
> This adds a ColorSpace field to the V4L2SubdeviceFormat so that we can
> set and request particular color spaces from V4L2.
> 
> This commit simply adds the field and fixes some occurrences of brace
> initializers that would otherwise be broken. A subsequent commit will
> pass and retrieve the value correctly to/from V4l2 itself.
> 
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/internal/v4l2_subdevice.h |  2 ++
>  src/libcamera/camera_sensor.cpp             |  2 ++
>  src/libcamera/pipeline/ipu3/cio2.cpp        |  7 +++----
>  src/libcamera/pipeline/simple/simple.cpp    |  8 ++++++--
>  src/libcamera/v4l2_subdevice.cpp            | 11 +++++++++++
>  5 files changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index a6873b67..358bf2b6 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;

#include <optional>

>  
>  	const std::string toString() const;
>  	uint8_t bitsPerPixel() const;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 4c142a58..14358333 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -15,6 +15,7 @@
>  #include <math.h>
>  #include <string.h>
>  
> +#include <libcamera/color_space.h>
>  #include <libcamera/property_ids.h>
>  
>  #include <libcamera/base/utils.h>
> @@ -586,6 +587,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/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 59dda56b..f4e8c663 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -322,10 +322,9 @@ V4L2SubdeviceFormat CIO2Device::getSensorFormat(const std::vector<unsigned int>
>  		return {};
>  	}
>  
> -	V4L2SubdeviceFormat format{
> -		.mbus_code = bestCode,
> -		.size = bestSize,
> -	};
> +	V4L2SubdeviceFormat format{};
> +	format.mbus_code = bestCode;
> +	format.size = bestSize;

Nitpicking, I would have used the same code construct in both
camera_sensor.cpp and cio2.cpp.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  
>  	return format;
>  }
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 701fb4be..a3108fc0 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -457,7 +457,9 @@ int SimpleCameraData::init()
>  	 * formats on the video node.
>  	 */
>  	for (unsigned int code : sensor_->mbusCodes()) {
> -		V4L2SubdeviceFormat format{ code, sensor_->resolution() };
> +		V4L2SubdeviceFormat format{};
> +		format.mbus_code = code;
> +		format.size = sensor_->resolution();
>  
>  		ret = setupFormats(&format, V4L2Subdevice::TryFormat);
>  		if (ret < 0) {
> @@ -908,7 +910,9 @@ int SimplePipelineHandler::configure(Camera *camera, CameraConfiguration *c)
>  		return ret;
>  
>  	const SimpleCameraData::Configuration *pipeConfig = config->pipeConfig();
> -	V4L2SubdeviceFormat format{ pipeConfig->code, data->sensor_->resolution() };
> +	V4L2SubdeviceFormat format{};
> +	format.mbus_code = pipeConfig->code;
> +	format.size = data->sensor_->resolution();
>  
>  	ret = data->setupFormats(&format, V4L2Subdevice::ActiveFormat);
>  	if (ret < 0)
> diff --git a/src/libcamera/v4l2_subdevice.cpp b/src/libcamera/v4l2_subdevice.cpp
> index 61e15b69..981645e0 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -169,6 +169,17 @@ 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.
> + * If this value is unset after a call to validate(), then the color space
> + * chosen by the driver could not be represented by the ColorSpace class
> + * (and should probably be added).
> + */
> +
>  /**
>   * \brief Assemble and return a string describing the format
>   * \return A string describing the V4L2SubdeviceFormat

-- 
Regards,

Laurent Pinchart