[v2] pipeline: simple: Consider output sizes when choosing pipe config

24836 diff mbox series

When a converter or the software ISP is used, output sizes do not equal
input sizes - they notably can be smaller.

Previous to this patch only capture sizes were considered, in some cases
resulting in configs with too small maximum output sizes being selected,
such as 1912x1080 for stream sizes of 1920x1080.

Check that the maximum output sizes are big enough instead, while continuing
to minimize capture sizes.

Closes https://gitlab.freedesktop.org/camera/libcamera/-/issues/236

Signed-off-by: Robert Mader <robert.mader@collabora.com>
---
 src/libcamera/pipeline/simple/simple.cpp | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Hi Robert,

thank you for the update.

Robert Mader <robert.mader@collabora.com> writes:

> When a converter or the software ISP is used, output sizes do not equal
> input sizes - they notably can be smaller.
>
> Previous to this patch only capture sizes were considered, in some cases
> resulting in configs with too small maximum output sizes being selected,
> such as 1912x1080 for stream sizes of 1920x1080.
>
> Check that the maximum output sizes are big enough instead, while continuing
> to minimize capture sizes.
>
> Closes https://gitlab.freedesktop.org/camera/libcamera/-/issues/236
>
> Signed-off-by: Robert Mader <robert.mader@collabora.com>

Looks reasonable to me now.

Reviewed-by: Milan Zamazal <mzamazal@redhat.com>

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 7b0783cdb..91715b7f8 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -1164,15 +1164,16 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  	pipeConfig_ = nullptr;
>  
>  	for (const SimpleCameraData::Configuration *pipeConfig : *configs) {
> -		const Size &size = pipeConfig->captureSize;
> +		const Size &captureSize = pipeConfig->captureSize;
> +		const Size &maxOutputSize = pipeConfig->outputSizes.max;
>  
> -		if (size.width >= maxStreamSize.width &&
> -		    size.height >= maxStreamSize.height) {
> -			if (!pipeConfig_ || size < pipeConfig_->captureSize)
> +		if (maxOutputSize.width >= maxStreamSize.width &&
> +		    maxOutputSize.height >= maxStreamSize.height) {
> +			if (!pipeConfig_ || captureSize < pipeConfig_->captureSize)
>  				pipeConfig_ = pipeConfig;
>  		}
>  
> -		if (!maxPipeConfig || maxPipeConfig->captureSize < size)
> +		if (!maxPipeConfig || maxPipeConfig->captureSize < captureSize)
>  			maxPipeConfig = pipeConfig;
>  	}

Hi Robert,

On Tue, Oct 28, 2025 at 12:20:47AM +0100, Robert Mader wrote:
> When a converter or the software ISP is used, output sizes do not equal
> input sizes - they notably can be smaller.
> 
> Previous to this patch only capture sizes were considered, in some cases
> resulting in configs with too small maximum output sizes being selected,
> such as 1912x1080 for stream sizes of 1920x1080.
> 
> Check that the maximum output sizes are big enough instead, while continuing
> to minimize capture sizes.
> 
> Closes https://gitlab.freedesktop.org/camera/libcamera/-/issues/236
> 

As per the recent discussion on the issue, this probably closes the
specific issue for your platform but doesn't address the underlying issue of
maxStreamSize being adjusted after the pipeConfig is selected, which I
believe should be fixed to ensure this doesn't happen on other platforms
resolutions combinations.

> Signed-off-by: Robert Mader <robert.mader@collabora.com>

Anyway, I think the patch is reasonable for now, I will try to look at
validate() logic to address the underlying issue,

Acked-by: Umang Jain <uajain@igalia.com>

> ---
>  src/libcamera/pipeline/simple/simple.cpp | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 7b0783cdb..91715b7f8 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -1164,15 +1164,16 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()
>  	pipeConfig_ = nullptr;
>  
>  	for (const SimpleCameraData::Configuration *pipeConfig : *configs) {
> -		const Size &size = pipeConfig->captureSize;
> +		const Size &captureSize = pipeConfig->captureSize;
> +		const Size &maxOutputSize = pipeConfig->outputSizes.max;
>  
> -		if (size.width >= maxStreamSize.width &&
> -		    size.height >= maxStreamSize.height) {
> -			if (!pipeConfig_ || size < pipeConfig_->captureSize)
> +		if (maxOutputSize.width >= maxStreamSize.width &&
> +		    maxOutputSize.height >= maxStreamSize.height) {
> +			if (!pipeConfig_ || captureSize < pipeConfig_->captureSize)
>  				pipeConfig_ = pipeConfig;
>  		}
>  
> -		if (!maxPipeConfig || maxPipeConfig->captureSize < size)
> +		if (!maxPipeConfig || maxPipeConfig->captureSize < captureSize)
>  			maxPipeConfig = pipeConfig;
>  	}
>  
> -- 
> 2.51.1
>