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

[Umang Jain]

Hi Jacopo,

Thank you for the patch

On 3/21/23 10:50 PM, 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
>
> 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>

Reviewed-by: Umang Jain <umang.jain 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)
>   {
>   	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);