[libcamera-devel] [PATCH v7 6/7] libcamera: Add validateColorSpaces to CameraConfiguration class

[Jacopo Mondi]

Hi David
On Fri, Nov 26, 2021 at 10:40:44AM +0000, David Plowman wrote:
> This method forces raw streams to have the "raw" color space, and also
> optionally makes all output streams to share the same color space as
> some platforms may require this.
>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/camera.h |  2 ++
>  src/libcamera/camera.cpp   | 38 ++++++++++++++++++++++++++++++++++++++

Helpers of this type doesn't really have a place in the current
codebase.

Ideally this could go in a CameraConfiguration::Private which we don't
have at the moment ?

I don't have better places to suggest I'm afraid, also the
PipelineHandler base class should be considered. Otherwise it's fine
here..

>  2 files changed, 40 insertions(+)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index a7759ccb..32a7f812 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -69,6 +69,8 @@ public:
>  protected:
>  	CameraConfiguration();
>
> +	Status validateColorSpaces(bool sharedColorSpace);
> +
>  	std::vector<StreamConfiguration> config_;
>  };
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 400a7cf0..dd06f600 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -20,6 +20,7 @@
>
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_controls.h"
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/pipeline_handler.h"

You should include color_space.h

>
>  /**
> @@ -314,6 +315,43 @@ std::size_t CameraConfiguration::size() const
>  	return config_.size();
>  }
>
> +static bool isRaw(const PixelFormat &pixFmt)
> +{
> +	const PixelFormatInfo &info = PixelFormatInfo::info(pixFmt);
> +	return info.isValid() &&
> +	       info.colourEncoding == PixelFormatInfo::ColourEncodingRAW;
> +}
> +
> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool shareOuputColorSpaces)
> +{
> +	Status status = Valid;
> +
> +	/*
> +	 * Set all raw streams to the Raw color space, and make a note of the largest
> +	 * non-raw stream with a defined color space (if there is one).
> +	 */
> +	int index = -1;
> +	for (auto [i, cfg] : utils::enumerate(config_)) {
> +		if (isRaw(cfg.pixelFormat))
> +			cfg.colorSpace = ColorSpace::Raw;

This might actually be an adjustment

			if (cfg.colorSpace != ColorSpace::Raw) {
				cfg.colorSpace = ColorSpace::Raw;
				status = Adjusted;
			}

> +		else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size))
> +			index = i;
> +	}
> +
> +	/* Make all output color spaces the same, if requested. */
> +	if (index >= 0 && shareOuputColorSpaces) {

I would
        if (index < 0 || !shareOuputColorSpaces)
                return status;

to reduce one indentation level and return earlier. up to you.

> +		for (auto &cfg : config_) {
> +			if (!isRaw(cfg.pixelFormat) &&

Same here
                        if (isRaw(cfg.pixelFormat)
                                continue;

up to you

> +			    cfg.colorSpace != config_[index].colorSpace) {
> +				cfg.colorSpace = config_[index].colorSpace;
> +				status = Adjusted;
> +			}
> +		}
> +	}
> +
> +	return status;
> +}
> +
>  /**
>   * \var CameraConfiguration::transform
>   * \brief User-specified transform to be applied to the image
> --
> 2.30.2
>