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

[Laurent Pinchart]

Hi Paul,

On Thu, Aug 25, 2022 at 03:08:13PM -0500, paul.elder at ideasonboard.com wrote:
> 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 :)

Oops :-) I'll add

Implement color space support in the rkisp1 pipeline handler, in the
configuration generation, configuration validation and camera
configuration. As all the processing related to the color space is
performed in the part of the pipeline shared by all streams, a single
color space must cover all stream configurations. This is enforced
manually when generating the configuration, and with the
validateColorSpaces() helper when validating it.

Only the Y'CbCr encoding and quantization range are currently taken into
account, and they are programmed through the V4L2 color space API. The
primary colors chromaticities and the transfer function need to be
configured in the ISP parameters buffer, and thus conveyed to the IPA,
but the rkisp1 driver does not currently support tone mapping, so this
will be done later.

> > 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