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

14978 diff mbox series

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@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(-)

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@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
>