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

[paul.elder at ideasonboard.com]

Hi Laurent,

On Thu, Aug 25, 2022 at 11:17:11PM +0300, Laurent Pinchart wrote:
> 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.

Looks good.

Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>

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