[RFC,v2] libcamera: software_isp: Assign colour spaces in configurations

23067 diff mbox series

StreamConfiguration's should have colorSpace set.  This is not the case
in the simple pipeline.  Let's set it there.  This also fixes a crash in
`cam' due to accessing an unset colorSpace.

The colour space is assigned as follows:

- If raw role is requested, then the colour space must be raw.
- Otherwise, if software ISP is used, the default colour space is
  set to Srgb (YcbcrEncoding::None, Range::Full).
- Otherwise, if a converter is used, the default colour space is left
  unset.
- Then in configuration validation, if the pixel format is changed, the
  colour space is set according to the new pixel format.
- If the colour space is still unset, it is set according to the
  resulting pixel format.

Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 25 +++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

I moved this patch to the front of the raw stream patches series, since
the series somewhat depends on it.

Milan Zamazal <mzamazal@redhat.com> writes:

> StreamConfiguration's should have colorSpace set.  This is not the case
> in the simple pipeline.  Let's set it there.  This also fixes a crash in
> `cam' due to accessing an unset colorSpace.
>
> The colour space is assigned as follows:
>
> - If raw role is requested, then the colour space must be raw.
> - Otherwise, if software ISP is used, the default colour space is
>   set to Srgb (YcbcrEncoding::None, Range::Full).
> - Otherwise, if a converter is used, the default colour space is left
>   unset.
> - Then in configuration validation, if the pixel format is changed, the
>   colour space is set according to the new pixel format.
> - If the colour space is still unset, it is set according to the
>   resulting pixel format.
>
> Signed-off-by: Milan Zamazal <mzamazal@redhat.com>
> ---
>  src/libcamera/pipeline/simple/simple.cpp | 25 +++++++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 6e039bf3..07cf7c11 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -25,6 +25,7 @@
>  #include <libcamera/base/log.h>
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/color_space.h>
>  #include <libcamera/control_ids.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
> @@ -35,6 +36,7 @@
>  #include "libcamera/internal/converter.h"
>  #include "libcamera/internal/delayed_controls.h"
>  #include "libcamera/internal/device_enumerator.h"
> +#include "libcamera/internal/formats.h"
>  #include "libcamera/internal/media_device.h"
>  #include "libcamera/internal/pipeline_handler.h"
>  #include "libcamera/internal/software_isp/software_isp.h"
> @@ -1088,8 +1090,24 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  		if (cfg.pixelFormat != pixelFormat) {
>  			LOG(SimplePipeline, Debug) << "Adjusting pixel format";
>  			cfg.pixelFormat = pixelFormat;
> +			if (cfg.colorSpace && cfg.colorSpace != ColorSpace::Raw)
> +				cfg.colorSpace->adjust(pixelFormat);
>  			status = Adjusted;
>  		}
> +		if (!cfg.colorSpace) {
> +			const PixelFormatInfo &info = PixelFormatInfo::info(pixelFormat);
> +			switch (info.colourEncoding) {
> +			case PixelFormatInfo::ColourEncodingRGB:
> +				cfg.colorSpace = ColorSpace::Srgb;
> +				break;
> +			case libcamera::PixelFormatInfo::ColourEncodingYUV:
> +				cfg.colorSpace = ColorSpace::Sycc;
> +				break;
> +			default:
> +				cfg.colorSpace = ColorSpace::Raw;
> +			}
> +			cfg.colorSpace->adjust(pixelFormat);
> +		}
>  
>  		if (!pipeConfig_->outputSizes.contains(cfg.size)) {
>  			Size adjustedSize = pipeConfig_->captureSize;
> @@ -1196,11 +1214,16 @@ SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRo
>  	 *
>  	 * \todo Implement a better way to pick the default format
>  	 */
> -	for ([[maybe_unused]] StreamRole role : roles) {
> +	for (StreamRole role : roles) {
>  		StreamConfiguration cfg{ StreamFormats{ formats } };
>  		cfg.pixelFormat = formats.begin()->first;
>  		cfg.size = formats.begin()->second[0].max;
>  
> +		if (role == StreamRole::Raw)
> +			cfg.colorSpace = ColorSpace::Raw;
> +		else if (data->swIsp_)
> +			cfg.colorSpace = ColorSpace::Srgb;
> +
>  		config->addConfiguration(cfg);
>  	}