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

[Jacopo Mondi]

Hi David,

On Thu, Nov 18, 2021 at 03:19:32PM +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.

I feel like it's a bit early, we don't have much platforms supporting
colorspaces and it might be the requirements are different ?

>
> Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> ---
>  include/libcamera/camera.h |  2 ++
>  src/libcamera/camera.cpp   | 51 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 53 insertions(+)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 601ee46e..fdab4410 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..c1541685 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"
>
>  /**
> @@ -314,6 +315,56 @@ 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;
> +}

I've seen this pattern so many times I'm surprised we don't have an
helper

> +
> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool sharedColorSpace)

Shouldn't this be documented ?

> +{
> +	Status status = Valid;
> +
> +	/* Find the largest non-raw stream (if any). */
> +	int index = -1;
> +	for (unsigned int i = 0; i < config_.size(); i++) {

for (auto &[i, cfg] utils::enumerate(config_)) is nicer

> +		const StreamConfiguration &cfg = config_[i];
> +		if (!isRaw(cfg.pixelFormat) && (index < 0 || cfg.size > config_[i].size))
> +			index = i;
> +	}

If you make sure the selected index has a colorspace the rest of the
function can be simplified

        StreamConfiguration largerYUV;
        for (auto const &cfg : configs_) {
                if (isRaw(cfg.pixelformat))
                        continue;

                if (cfg.size > largerYUV.size && cfg.colorSpace)
                        largerYUV = cfg;
        }

        if (!largerYUV.colorSpace)
                ...

And you could catch the case where there are no colorspaces at all
earlier (what happens in that case ?)


> +
> +	/*
> +	 * Here we force raw streams to the correct color space. We handle
> +	 * the case where all output streams are to share a color space,
> +	 * which will match the color space of the largest (non-raw) stream.
> +	 */
> +	for (auto &cfg : config_) {
> +		std::optional<ColorSpace> initialColorSpace = cfg.colorSpace;
> +
> +		if (isRaw(cfg.pixelFormat))
> +			cfg.colorSpace = ColorSpace::Raw;

		if (isRaw(cfg.pixelFormat)) {
                        if (cfg.colorSpace == ColorSpace::Raw)
                                continue;

			cfg.colorSpace = ColorSpace::Raw;
                        status = Adjusted;
                        continue;
                }

> +		else if (sharedColorSpace) {

the rest of the function can then be

                if (!sharedColorSpace)
                        continue;

                if (cfg->colorSpace == largerYUV.colorSpace)
                        continue;

                cfg.colorspace = largerYUV.colorspace
                status = Adjusted;
        }

        return status;

Again not tested, I might have missed something indeed

I would also call the "sharedColorSpace" flag "forceColorSpace" ?

> +			/*
> +			 * When all outputs share a color space, use the biggest
> +			 * output if that was set. If that was blank, but this
> +			 * stream's is not, then use this stream's color space for
> +			 * largest stream.
> +			 * (index must be != -1 if there are any non-raw streams.)
> +			 */
> +			if (config_[index].colorSpace)

Can index be -1 here ?

> +				cfg.colorSpace = config_[index].colorSpace;
> +			else if (cfg.colorSpace)

Thanks
   j

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