[libcamera-devel,v9,7/8] libcamera: Add validateColorSpaces to CameraConfiguration class

15048 diff mbox series

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@raspberrypi.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/camera.h |  2 ++
 src/libcamera/camera.cpp   | 59 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

Hi David

On Mon, Dec 06, 2021 at 10:50:30AM +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@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/camera.h |  2 ++
>  src/libcamera/camera.cpp   | 59 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
>
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index a7759ccb..fdcb66ab 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -69,6 +69,8 @@ public:
>  protected:
>  	CameraConfiguration();
>
> +	Status validateColorSpaces(bool shareOutputColorSpaces);
> +
>  	std::vector<StreamConfiguration> config_;
>  };
>
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 400a7cf0..388af4f7 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -14,12 +14,14 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/thread.h>
>
> +#include <libcamera/color_space.h>
>  #include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_controls.h"
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/pipeline_handler.h"
>
>  /**
> @@ -314,6 +316,63 @@ 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;
> +}
> +
> +/**
> + * \brief Check and update the color spaces requested for each stream
> + * \param[in] shareOutputColorSpaces Force all non-raw output streams to share
> + * the same color space
> + *
> + * This function performs certain consistency checks on the color spaces of
> + * the streams and may adjust them so that:
> + *
> + * - Any raw streams have the Raw color space
> + * - If shareOutputColorSpaces is set, all output streams are forced to share
> + * the same color space (this may be a constraint on some platforms).
> + *
> + * It is optional for a pipeline handler to use this method.
> + *
> + * \return A CameraConfiguration::Status value that describes the validation
> + * status (same as CameraConfiguration::validate).
> + */
> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool shareOutputColorSpaces)
> +{
> +	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) {
> +			cfg.colorSpace = ColorSpace::Raw;
> +			status = Adjusted;
> +		}
> +		else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size))

Note for when applying:

                if () {

                } else if {

                }
> +			index = i;
> +	}
> +
> +	if (index < 0 || !shareOutputColorSpaces)
> +		return status;
> +
> +	/* Make all output color spaces the same, if requested. */
> +	for (auto &cfg : config_) {
> +		if (!isRaw(cfg.pixelFormat) &&
> +		    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.20.1
>

Hi David,

Thank you for the patch.

On Mon, Dec 06, 2021 at 10:50:30AM +0000, David Plowman wrote:
> This method forces raw streams to have the "raw" color space, and also

s/method/function/ (same below)

> optionally makes all output streams to share the same color space as
> some platforms may require this.
> 
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/camera.h |  2 ++
>  src/libcamera/camera.cpp   | 59 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index a7759ccb..fdcb66ab 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -69,6 +69,8 @@ public:
>  protected:
>  	CameraConfiguration();
>  
> +	Status validateColorSpaces(bool shareOutputColorSpaces);
> +

I'm not a big fan of boolean parameters for this kind of use case, as
it's not clear what they mean when you look at the callers. Explicit
flags are better:

	enum class ColorSpaceFlag {
		None,
		StreamsShareColorSpace,	-- or a better name
	};

	using ColorSpaceFlags = Flags<ColorSpaceFlag>;

	Status validateColorSpaces(ColorSpaceFlags flags = ColorSpaceFlag::None);

It may be overkill to use the Flags class for a single flag, the enum
alone would be enough.

>  	std::vector<StreamConfiguration> config_;
>  };
>  
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 400a7cf0..388af4f7 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -14,12 +14,14 @@
>  #include <libcamera/base/log.h>
>  #include <libcamera/base/thread.h>
>  
> +#include <libcamera/color_space.h>
>  #include <libcamera/framebuffer_allocator.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  
>  #include "libcamera/internal/camera.h"
>  #include "libcamera/internal/camera_controls.h"
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  
>  /**
> @@ -314,6 +316,63 @@ 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;
> +}

namespace {

bool isRaw(const PixelFormat &pixFmt)
{
	...
}

} /* namespace */

would be the C++ way.

> +
> +/**
> + * \brief Check and update the color spaces requested for each stream
> + * \param[in] shareOutputColorSpaces Force all non-raw output streams to share
> + * the same color space
> + *
> + * This function performs certain consistency checks on the color spaces of
> + * the streams and may adjust them so that:
> + *
> + * - Any raw streams have the Raw color space
> + * - If shareOutputColorSpaces is set, all output streams are forced to share
> + * the same color space (this may be a constraint on some platforms).
> + *
> + * It is optional for a pipeline handler to use this method.
> + *
> + * \return A CameraConfiguration::Status value that describes the validation
> + * status (same as CameraConfiguration::validate).
> + */
> +CameraConfiguration::Status CameraConfiguration::validateColorSpaces(bool shareOutputColorSpaces)
> +{
> +	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) {
> +			cfg.colorSpace = ColorSpace::Raw;
> +			status = Adjusted;
> +		}
> +		else if (cfg.colorSpace && (index == -1 || cfg.size > config_[i].size))

As Jacopo said :-)

Any particular reason to go for the largest stream ?

> +			index = i;
> +	}
> +
> +	if (index < 0 || !shareOutputColorSpaces)
> +		return status;
> +
> +	/* Make all output color spaces the same, if requested. */
> +	for (auto &cfg : config_) {
> +		if (!isRaw(cfg.pixelFormat) &&
> +		    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