[libcamera-devel,v5,4/4] libcamera: rkisp1: Fix enumeration of RAW formats

18712 diff mbox series

The current implementation enumerates a single RAW format (the sensor's
resolution) and does that regardless of what role the
CameraConfiguration has been generated for.

Fix this by:
- Enumerate RAW StreamFormats only when the requested role is
  StreamRole::Raw
- Add all the sensor's provided resolutions that fit the video device
  output maximum size

Before this patch, a single RAW size was enumerated in stream formats

 * Pixelformat: SRGGB10 (4208x3120)-(4208x3120)/(+1,+1)
  - 4208x3120

With this patch applied all sensor's supported resolutions are
enumerated but only when the stream role RAW is explicitly requested

 * Pixelformat: SRGGB10 (1048x780)-(4208x3120)/(+0,+0)
  - 1048x780
  - 2104x1560
  - 4032x3024
  - 4208x3120

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 20 +++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

Hi Jacopo,

Thank you for the patch.

On Tue, Jun 06, 2023 at 12:33:36PM +0200, Jacopo Mondi via libcamera-devel wrote:
> The current implementation enumerates a single RAW format (the sensor's
> resolution) and does that regardless of what role the
> CameraConfiguration has been generated for.
> 
> Fix this by:
> - Enumerate RAW StreamFormats only when the requested role is
>   StreamRole::Raw
> - Add all the sensor's provided resolutions that fit the video device
>   output maximum size
> 
> Before this patch, a single RAW size was enumerated in stream formats
> 
>  * Pixelformat: SRGGB10 (4208x3120)-(4208x3120)/(+1,+1)
>   - 4208x3120
> 
> With this patch applied all sensor's supported resolutions are
> enumerated but only when the stream role RAW is explicitly requested
> 
>  * Pixelformat: SRGGB10 (1048x780)-(4208x3120)/(+0,+0)
>   - 1048x780
>   - 2104x1560
>   - 4032x3024
>   - 4208x3120
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>
> Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

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

> ---
>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 20 +++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index 89fb6596b0be..b62b645cae24 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -149,18 +149,34 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,
>  	for (const auto &format : streamFormats_) {
>  		const PixelFormatInfo &info = PixelFormatInfo::info(format);
>  
> +		/* Populate stream formats for non-RAW configurations. */
>  		if (info.colourEncoding != PixelFormatInfo::ColourEncodingRAW) {
> +			if (role == StreamRole::Raw)
> +				continue;
> +
>  			streamFormats[format] = { { minResolution, maxResolution } };
>  			continue;
>  		}
>  
> -		/* Skip raw formats not supported by the sensor. */
> +		/* Skip RAW formats for non-raw roles. */
> +		if (role != StreamRole::Raw)
> +			continue;
> +
> +		/* Populate stream formats for RAW configurations. */
>  		uint32_t mbusCode = formatToMediaBus.at(format);
>  		if (std::find(mbusCodes.begin(), mbusCodes.end(), mbusCode) ==
>  		    mbusCodes.end())
> +			/* Skip formats not supported by sensor. */
>  			continue;
>  
> -		streamFormats[format] = { { resolution, resolution } };
> +		/* Add all the RAW sizes the sensor can produce for this code. */
> +		for (const auto &rawSize : sensor->sizes(mbusCode)) {
> +			if (rawSize.width > maxResolution_.width ||
> +			    rawSize.height > maxResolution_.height)
> +				continue;
> +
> +			streamFormats[format].push_back({ rawSize, rawSize });
> +		}
>  
>  		/*
>  		 * Store the raw format with the highest bits per pixel for