[libcamera-devel] [PATCH v4 3/4] libcamera: rkisp1: Assign sizes to roles

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Mar 21, 2023 at 06:20:03PM +0100, Jacopo Mondi via libcamera-devel wrote:
> Currently each RkISP1 path (main and self) generate a configuration
> by bounding the sensor's resolution to their respective maximum output
> aspect ratio and size.
> 
> 	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
> 					   .boundedTo(resolution);
> 
> In the case of self path, whose maximum size is 1920x1920, the generated
> configuration could get assigned unusual sizes, as the result of the
> above operation

s/$/./

> 
> As an example, with the imx258 sensor whose resolution is 4208x3118, the
> resulting size for the Viewfinder use case is an unusual 1920x1423.
> 
> Fix this by assigning to each role a desired output size:
> - Use the sensor's resolution for StillCapture and Raw
> - Use 1080p for Viewfinder and VideoRecording
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 12 +++++++++++-
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 10 ++++++++--
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  1 +
>  3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index fd331168af80..5ae25a244408 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -162,6 +162,8 @@ public:
>  	bool match(DeviceEnumerator *enumerator) override;
>  
>  private:
> +	static constexpr Size kRkISP1PreviewSize = { 1920, 1080 };
> +
>  	RkISP1CameraData *cameraData(Camera *camera)
>  	{
>  		return static_cast<RkISP1CameraData *>(camera->_d());
> @@ -633,12 +635,15 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  	bool mainPathAvailable = true;
>  
>  	for (const StreamRole role : roles) {
> +		Size size;
>  
>  		switch (role) {
>  		case StreamRole::StillCapture:
>  			/* JPEG encoders typically expect sYCC. */
>  			if (!colorSpace)
>  				colorSpace = ColorSpace::Sycc;
> +
> +			size = data->sensor_->resolution();
>  			break;
>  
>  		case StreamRole::Viewfinder:
> @@ -648,12 +653,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  			 */
>  			if (!colorSpace)
>  				colorSpace = ColorSpace::Sycc;
> +
> +			size = kRkISP1PreviewSize;
>  			break;
>  
>  		case StreamRole::VideoRecording:
>  			/* Rec. 709 is a good default for HD video recording. */
>  			if (!colorSpace)
>  				colorSpace = ColorSpace::Rec709;
> +
> +			size = kRkISP1PreviewSize;
>  			break;
>  
>  		case StreamRole::Raw:
> @@ -664,6 +673,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  			}
>  
>  			colorSpace = ColorSpace::Raw;
> +			size = data->sensor_->resolution();
>  			break;
>  
>  		default:
> @@ -682,7 +692,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  		}
>  
>  		StreamConfiguration cfg =
> -			path->generateConfiguration(data->sensor_.get(), role);
> +			path->generateConfiguration(data->sensor_.get(), size, role);
>  		if (!cfg.pixelFormat.isValid())
>  			return nullptr;
>  
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 5547cc32b6ca..cca593b84260 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -127,15 +127,21 @@ void RkISP1Path::populateFormats()
>  }
>  
>  StreamConfiguration
> -RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
> +RkISP1Path::generateConfiguration(const CameraSensor *sensor,
> +				  const Size &size,
> +				  StreamRole role)

RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
				  StreamRole role)

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

>  {
>  	const std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();
>  	const Size &resolution = sensor->resolution();
>  
> +	/* Min and max resolutions to populate the available stream formats. */
>  	Size maxResolution = maxResolution_.boundedToAspectRatio(resolution)
>  					   .boundedTo(resolution);
>  	Size minResolution = minResolution_.expandedToAspectRatio(resolution);
>  
> +	/* The desired stream size, bound to the max resolution. */
> +	Size streamSize = size.boundedTo(maxResolution);
> +
>  	/* Create the list of supported stream formats. */
>  	std::map<PixelFormat, std::vector<SizeRange>> streamFormats;
>  	unsigned int rawBitsPerPixel = 0;
> @@ -189,7 +195,7 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)
>  	StreamFormats formats(streamFormats);
>  	StreamConfiguration cfg(formats);
>  	cfg.pixelFormat = format;
> -	cfg.size = maxResolution;
> +	cfg.size = streamSize;
>  	cfg.bufferCount = RKISP1_BUFFER_COUNT;
>  
>  	return cfg;
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index bdf3f95b95e1..cd77957ee4a6 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -41,6 +41,7 @@ public:
>  	bool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }
>  
>  	StreamConfiguration generateConfiguration(const CameraSensor *sensor,
> +						  const Size &resolution,
>  						  StreamRole role);
>  	CameraConfiguration::Status validate(const CameraSensor *sensor,
>  					     StreamConfiguration *cfg);

-- 
Regards,

Laurent Pinchart