Re: [raw2rgbpnm,PATCH] Add support for all RAW memory layouts

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

I know this has been merged already, but it only came to my attention
now.

On Sun, May 11, 2025 at 05:46:59PM +0200, Niklas Söderlund wrote:
> Convert all supported RAW input images to GRBG memory layout before
> feeding it to the RAW to RGB conversion algorithms. This way all layouts
> can produced good colors in the output image.

I don't think the result will be accurate. It does produce better
colours, but all liens and columns in the output image end up being
swapped in groups of two. I would have kept printing a warning message.

> Limit the conversion to input images that have an even width and height
> to make the conversion easier.
> 
> Images with and uneven width or hight can sill be processed, but will
> be processed without first being converted to GRBG layout. Such input
> images will result, just as before this change, in output images with
> bad colors. The warning message about this have been preserved.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  raw2rgbpnm.c | 98 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 95 insertions(+), 3 deletions(-)
> 
> diff --git a/raw2rgbpnm.c b/raw2rgbpnm.c
> index 5838bc7347e9..b834add4ea6c 100644
> --- a/raw2rgbpnm.c
> +++ b/raw2rgbpnm.c
> @@ -186,6 +186,97 @@ static unsigned char *read_raw_data(char *filename, int width, int height,
>  	return b;
>  }
>  
> +static int raw_layout_to_grbg(const struct format_info *info, unsigned char *src,
> +			      int src_width, int src_height, unsigned int src_stride)
> +{
> +	int swap_line = 0;
> +	int swap_gbrg = 0;
> +	int dst_x, dst_y;
> +
> +	switch (info->fmt) {
> +	case V4L2_PIX_FMT_SBGGR16:
> +	case V4L2_PIX_FMT_SBGGR14:
> +	case V4L2_PIX_FMT_SBGGR12:
> +	case V4L2_PIX_FMT_SBGGR10:
> +	case V4L2_PIX_FMT_SBGGR8:
> +		swap_line = 1; /* BGGR -> GBRG */
> +		swap_gbrg = 1; /* GBRG -> GRBG */
> +		break;
> +	case V4L2_PIX_FMT_SGBRG16:
> +	case V4L2_PIX_FMT_SGBRG14:
> +	case V4L2_PIX_FMT_SGBRG12:
> +	case V4L2_PIX_FMT_SGBRG10:
> +	case V4L2_PIX_FMT_SGBRG8:
> +		swap_gbrg = 1; /* GBRG -> GRBG */
> +		break;
> +	case V4L2_PIX_FMT_SRGGB16:
> +	case V4L2_PIX_FMT_SRGGB14:
> +	case V4L2_PIX_FMT_SRGGB12:
> +	case V4L2_PIX_FMT_SRGGB10:
> +	case V4L2_PIX_FMT_SRGGB8:
> +		swap_line = 1; /* RGGB -> GRBG */
> +		break;
> +	case V4L2_PIX_FMT_SGRBG16:
> +	case V4L2_PIX_FMT_SGRBG14:
> +	case V4L2_PIX_FMT_SGRBG12:
> +	case V4L2_PIX_FMT_SGRBG10:
> +	case V4L2_PIX_FMT_SGRBG8:
> +		/* No conversion needed. */
> +		break;
> +	default:
> +		return -1;
> +	}
> +
> +	if (src_width % 2 != 0 || src_height % 2 != 0) {
> +		printf("WARNING: bayer layout conversion not supported for uneven sized images -> expect bad colors\n");
> +		return 0;
> +	}
> +
> +	if (swap_line) {
> +		/* Swap pixel pairs on each line AB -> BA. */
> +		for (dst_y=0; dst_y<src_height; dst_y++) {
> +			for (dst_x=0; dst_x<src_width; dst_x += 2) {
> +				if (info->bpp == 8) {
> +					unsigned char *p1 = &(src[src_stride*dst_y+(dst_x+0)]);
> +					unsigned char *p2 = &(src[src_stride*dst_y+(dst_x+1)]);
> +					unsigned char tmp = *p1;
> +					*p1 = *p2;
> +					*p2 = tmp;
> +				} else {
> +					unsigned short *p1 = (unsigned short *)&(src[src_stride*dst_y+(dst_x+0)*2]);
> +					unsigned short *p2 = (unsigned short *)&(src[src_stride*dst_y+(dst_x+1)*2]);
> +					unsigned short tmp = *p1;
> +					*p1 = *p2;
> +					*p2 = tmp;
> +				}
> +			}
> +		}
> +	}
> +
> +	if (swap_gbrg) {
> +		/* Swap R and B components, format is always GBRG at this point. */
> +		for (dst_y=0; dst_y<src_height; dst_y += 2) {
> +			for (dst_x=0; dst_x<src_width; dst_x += 2) {
> +				if (info->bpp == 8) {
> +					unsigned char *p1 = &(src[src_stride*(dst_y+0)+(dst_x+1)]);
> +					unsigned char *p2 = &(src[src_stride*(dst_y+1)+(dst_x+0)]);
> +					unsigned char tmp = *p1;
> +					*p1 = *p2;
> +					*p2 = tmp;
> +				} else {
> +					unsigned short *p1 = (unsigned short *)&(src[src_stride*(dst_y+0)+(dst_x+1)*2]);
> +					unsigned short *p2 = (unsigned short *)&(src[src_stride*(dst_y+1)+(dst_x+0)*2]);
> +					unsigned short tmp = *p1;
> +					*p1 = *p2;
> +					*p2 = tmp;
> +				}
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static void raw_to_rgb(const struct format_info *info,
>  		       unsigned char *src, int src_width, int src_height, unsigned char *rgb)
>  {
> @@ -480,7 +571,8 @@ static void raw_to_rgb(const struct format_info *info,
>  	case V4L2_PIX_FMT_SBGGR10:
>  	case V4L2_PIX_FMT_SGBRG10:
>  	case V4L2_PIX_FMT_SRGGB10:
> -		printf("WARNING: bayer phase not supported -> expect bad colors\n");
> +		if (raw_layout_to_grbg(info, src, src_width, src_height, src_stride))
> +			error("Can't convert RAW layout to GRBG");
>  		/* fallthrough */
>  	case V4L2_PIX_FMT_SGRBG16:
>  	case V4L2_PIX_FMT_SGRBG14:
> @@ -523,10 +615,10 @@ static void raw_to_rgb(const struct format_info *info,
>  	case V4L2_PIX_FMT_SBGGR8:
>  	case V4L2_PIX_FMT_SGBRG8:
>  	case V4L2_PIX_FMT_SRGGB8:
> -		printf("WARNING: bayer phase not supported -> expect bad colors\n");
> +		if (raw_layout_to_grbg(info, src, src_width, src_height, src_stride))
> +			error("Can't convert RAW layout to GRBG");
>  		/* fallthrough */
>  	case V4L2_PIX_FMT_SGRBG8:
> -		/* FIXME: only SGRBG8 handled properly: color phase is ignored. */
>  		buf = malloc(src_width * src_height * 3);
>  		if (buf==NULL) error("out of memory");
>  		qc_imag_bay2rgb8(src, src_stride, buf, src_width*3, src_width, src_height, 3);

-- 
Regards,

Laurent Pinchart