[libcamera-devel] [PATCH 5/6] libcamera: pipeline: rkisp1: Implement color space support

[paul.elder at ideasonboard.com]

Hi Laurent,

On Tue, Aug 23, 2022 at 08:43:13PM +0300, Laurent Pinchart via libcamera-devel wrote:

I think some changelog would be nice to have :)


Paul

> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 38 ++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 03bbe6b467ae..25fbf9f1a0a9 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -18,6 +18,7 @@
>  #include <libcamera/base/utils.h>
>  
>  #include <libcamera/camera.h>
> +#include <libcamera/color_space.h>
>  #include <libcamera/control_ids.h>
>  #include <libcamera/formats.h>
>  #include <libcamera/framebuffer.h>
> @@ -416,11 +417,13 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()
>  {
>  	const CameraSensor *sensor = data_->sensor_.get();
>  	unsigned int pathCount = data_->selfPath_ ? 2 : 1;
> -	Status status = Valid;
> +	Status status;
>  
>  	if (config_.empty())
>  		return Invalid;
>  
> +	status = validateColorSpaces(ColorSpaceFlag::StreamsShareColorSpace);
> +
>  	if (transform != Transform::Identity) {
>  		transform = Transform::Identity;
>  		status = Adjusted;
> @@ -547,21 +550,44 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  	if (roles.empty())
>  		return config;
>  
> +	/*
> +	 * As the ISP can't output different color spaces for the main and self
> +	 * path, pick a sensible default color space based on the role of the
> +	 * first stream and use it for all streams.
> +	 */
> +	std::optional<ColorSpace> colorSpace;
> +
>  	bool mainPathAvailable = true;
>  	bool selfPathAvailable = data->selfPath_;
> +
>  	for (const StreamRole role : roles) {
>  		bool useMainPath;
>  
>  		switch (role) {
> -		case StreamRole::StillCapture: {
> +		case StreamRole::StillCapture:
>  			useMainPath = mainPathAvailable;
> +			/* JPEG encoders typically expect sYCC. */
> +			if (!colorSpace)
> +				colorSpace = ColorSpace::Sycc;
>  			break;
> -		}
> +
>  		case StreamRole::Viewfinder:
> -		case StreamRole::VideoRecording: {
>  			useMainPath = !selfPathAvailable;
> +			/*
> +			 * sYCC is the YCbCr encoding of sRGB, which is commonly
> +			 * used by displays.
> +			 */
> +			if (!colorSpace)
> +				colorSpace = ColorSpace::Sycc;
>  			break;
> -		}
> +
> +		case StreamRole::VideoRecording:
> +			useMainPath = !selfPathAvailable;
> +			/* Rec. 709 is a good default for HD video recording. */
> +			if (!colorSpace)
> +				colorSpace = ColorSpace::Rec709;
> +			break;
> +
>  		default:
>  			LOG(RkISP1, Warning)
>  				<< "Requested stream role not supported: " << role;
> @@ -580,6 +606,7 @@ CameraConfiguration *PipelineHandlerRkISP1::generateConfiguration(Camera *camera
>  			selfPathAvailable = false;
>  		}
>  
> +		cfg.colorSpace = colorSpace;
>  		config->addConfiguration(cfg);
>  	}
>  
> @@ -642,6 +669,7 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>  	if (ret < 0)
>  		return ret;
>  
> +	format.colorSpace = config->at(0).colorSpace;
>  	ret = isp_->setFormat(2, &format);
>  	if (ret < 0)
>  		return ret;
> -- 
> Regards,
> 
> Laurent Pinchart
>