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

[Jacopo Mondi]

Hi David,

On Wed, Dec 01, 2021 at 03:44:31PM +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             |  1 +
>  src/libcamera/pipeline/ipu3/cio2.cpp        |  7 +++----
>  src/libcamera/pipeline/simple/simple.cpp    |  8 ++++++--
>  src/libcamera/v4l2_subdevice.cpp            | 15 +++++++++++++++
>  5 files changed, 27 insertions(+), 6 deletions(-)
>
> diff --git a/include/libcamera/internal/v4l2_subdevice.h b/include/libcamera/internal/v4l2_subdevice.h
> index 484fcfdd..e6fa451b 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

Does camera_sensor include color_space.h

> @@ -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/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;
>
>  	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 023e2328..66e08333 100644
> --- a/src/libcamera/v4l2_subdevice.cpp
> +++ b/src/libcamera/v4l2_subdevice.cpp
> @@ -168,6 +168,21 @@ 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

What do you mean "After being set" ?
Does this mean "after validate()" ? I would drop this, none of the
other StreamConfiguration fields documents that, it is implicit that
the pipeline handler update fields to what it can produce, this is no
special

> + * was actually used. If this value is unset, then the color space chosen
> + * by the driver could not be represented by the ColorSpace class (and
> + * should probably be added).

This (the fact it can be 'unset') is special instead and should be documented
like you're doing

> + *
> + * It is up to the pipeline handler or application to check if the
> + * resulting color space is acceptable.

The same happens for, say, the image format. If application get an
adjusted configuration they should check if the returned config is
correct for them. I would drop this last paragraph.

> + */
> +
>  /**
>   * \brief Assemble and return a string describing the format
>   * \return A string describing the V4L2SubdeviceFormat
> --
> 2.30.2
>