[libcamera-devel,v2,1/4] libcamera: rkisp1: Generate config using main path
diff mbox series

Message ID 20230222151917.669526-2-jacopo.mondi@ideasonboard.com
State Superseded
Headers show
Series
  • libcamera: rkisp1: Fix generateConfiguration
Related show

Commit Message

Jacopo Mondi Feb. 22, 2023, 3:19 p.m. UTC
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@luigi311.com
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

Comments

Paul Elder March 3, 2023, 8:31 a.m. UTC | #1
On Wed, Feb 22, 2023 at 04:19:14PM +0100, Jacopo Mondi via libcamera-devel 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@luigi311.com
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

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

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8a30fe061d04..569fb8ecb629 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -630,23 +630,19 @@ 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;
> +		bool useMainPath = mainPathAvailable;
>  
>  		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 +652,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 +664,6 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  				return nullptr;
>  			}
>  
> -			useMainPath = true;
>  			colorSpace = ColorSpace::Raw;
>  			break;
>  
> @@ -686,7 +680,6 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>  			mainPathAvailable = false;
>  		} else {
>  			path = data->selfPath_;
> -			selfPathAvailable = false;
>  		}
>  
>  		StreamConfiguration cfg =
> -- 
> 2.39.0
>

Patch
diff mbox series

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 8a30fe061d04..569fb8ecb629 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -630,23 +630,19 @@  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;
+		bool useMainPath = mainPathAvailable;
 
 		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 +652,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 +664,6 @@  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 				return nullptr;
 			}
 
-			useMainPath = true;
 			colorSpace = ColorSpace::Raw;
 			break;
 
@@ -686,7 +680,6 @@  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
 			mainPathAvailable = false;
 		} else {
 			path = data->selfPath_;
-			selfPathAvailable = false;
 		}
 
 		StreamConfiguration cfg =