[libcamera-devel] [PATCH v4 4/4] libcamera: rkisp1: Fix enumeration of RAW formats

[Umang Jain]

Hi Jacopo,

Thank you for the patch

On 3/21/23 10:50 PM, 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 at ideasonboard.com>
> Reviewed-by: Paul Elder <paul.elder at ideasonboard.com>
> ---
>   src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 19 +++++++++++++++++--
>   1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> index cca593b84260..8d606de7880f 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp
> @@ -150,18 +150,33 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor,
>   	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;
>   		}


                 /* Populate stream formats for non-RAW configurations. */
                 if (role != StreamRole::Raw &&
                     info.colourEncoding != 
PixelFormatInfo::ColourEncodingRAW) {
                         streamFormats[format] = { { minResolution, 
maxResolution } };
                         continue;
                 }
Possibly?

Rest looks good to me

Reviewed-by: Umang Jain <umang.jain at ideasonboard.com>

>   
> -		/* 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 > maxResolution_)
> +				continue;
> +
> +			streamFormats[format].push_back({ rawSize, rawSize });
> +		}
>   
>   		/*
>   		 * Store the raw format with the highest bits per pixel for