[libcamera-devel] [PATCH v4 1/4] libcamera: rkisp1: Generate config using main path

[Laurent Pinchart]

Hi Jacopo,

On Tue, Mar 21, 2023 at 06:20:01PM +0100, Jacopo Mondi wrote:
> The generateConfiguration() implementation in the Rockchip RkISP1
> pipeline handler uses by default the self path (if available) for
> the Viewfinder and VideoRecording StreamRoles.
> 
> The validate() implementation, at the contrary, prefers using the main
> path, when available, for all streams.
> 
> As the self-path is limited in output resolution to 1920x1920,
> generating a configuration using the self path limits the maximum
> stream size to 1920x1920, while higher resolutions can be obtained by
> using the main path.
> 
> Align the generateConfiguration() implementation to the validate() one
> by using the main path by default if available.
> 
> Bug: https://bugs.libcamera.org/show_bug.cgi?id=180
> Reported-by: libcamera at luigi311.com
> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8a30fe061d04..fd331168af80 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -630,23 +630,18 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  	 * 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:
> -			useMainPath = mainPathAvailable;
>  			/* JPEG encoders typically expect sYCC. */
>  			if (!colorSpace)
>  				colorSpace = ColorSpace::Sycc;
>  			break;
>  
>  		case StreamRole::Viewfinder:
> -			useMainPath = !selfPathAvailable;
>  			/*
>  			 * sYCC is the YCbCr encoding of sRGB, which is commonly
>  			 * used by displays.
> @@ -656,7 +651,6 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  			break;
>  
>  		case StreamRole::VideoRecording:
> -			useMainPath = !selfPathAvailable;
>  			/* Rec. 709 is a good default for HD video recording. */
>  			if (!colorSpace)
>  				colorSpace = ColorSpace::Rec709;
> @@ -669,7 +663,6 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  				return nullptr;
>  			}
>  
> -			useMainPath = true;
>  			colorSpace = ColorSpace::Raw;
>  			break;
>  
> @@ -681,12 +674,11 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  
>  		RkISP1Path *path;
>  

Could you add a comment to record the problem I raised in the review of
v3 ? Otherwise we'll forget about it.

		/*
		 * Prefer the main path if available, as it supports higher
		 * resolutions.
		 *
		 * \todo Using the main path unconditionally hides support for
		 * RGB (only available on the self path) in the streams formats
		 * exposed to applications. This likely calls for a better API
		 * to expose streams capabilities.
		 */

> -		if (useMainPath) {
> +		if (mainPathAvailable) {
>  			path = data->mainPath_;
>  			mainPathAvailable = false;
>  		} else {
>  			path = data->selfPath_;
> -			selfPathAvailable = false;
>  		}
>  
>  		StreamConfiguration cfg =

-- 
Regards,

Laurent Pinchart