[libcamera-devel] [RFC PATCH] qcam: format_converter: add 10 and 12 bit packed raw Bayer formats

[Kieran Bingham]

Hi Andrey,

On 31/08/2020 09:46, Andrey Konovalov wrote:
> No interpolation is used to get more speed by the price of lower image
> quality. In qcam format_converter is used for viewfinder only, and in
> this case lower lag is more important than the image quality.
> 

Great, I think this will be useful for early bring up of a new platform
/ sensor when we just want to 'see' the image as quickly as possible.

We should really get more 'information' presented in Qcam regarding what
processing is going on - so that it's clear to a viewer this is doing
software debayering or such - but that's way out of scope for this patch.



> Signed-off-by: Andrey Konovalov <andrey.konovalov at linaro.org>
> ---
>  Only SRGGB10P and SRGGB12P formats were tested (the ones I can get from
>  the camera sensor I am currently using)
> 
>  src/qcam/format_converter.cpp | 118 ++++++++++++++++++++++++++++++++++
>  src/qcam/format_converter.h   |  14 ++++
>  2 files changed, 132 insertions(+)
> 
> diff --git a/src/qcam/format_converter.cpp b/src/qcam/format_converter.cpp
> index 4b9722d..c9f94d3 100644
> --- a/src/qcam/format_converter.cpp
> +++ b/src/qcam/format_converter.cpp
> @@ -136,6 +136,62 @@ int FormatConverter::configure(const libcamera::PixelFormat &format,
>  		formatFamily_ = MJPEG;
>  		break;
>  
> +	case libcamera::formats::SRGGB10_CSI2P:
> +		formatFamily_ = RAW_CSI2P;
> +		r_pos_ = 0;
> +		bpp_numer_ = 4;
> +		bpp_denom_ = 5;
> +		break;
> +
> +	case libcamera::formats::SGRBG10_CSI2P:
> +		formatFamily_ = RAW_CSI2P;
> +		r_pos_ = 1;
> +		bpp_numer_ = 4;
> +		bpp_denom_ = 5;
> +		break;
> +
> +	case libcamera::formats::SGBRG10_CSI2P:
> +		formatFamily_ = RAW_CSI2P;
> +		r_pos_ = 2;
> +		bpp_numer_ = 4;
> +		bpp_denom_ = 5;
> +		break;
> +
> +	case libcamera::formats::SBGGR10_CSI2P:
> +		formatFamily_ = RAW_CSI2P;
> +		r_pos_ = 3;
> +		bpp_numer_ = 4;
> +		bpp_denom_ = 5;
> +		break;
> +
> +	case libcamera::formats::SRGGB12_CSI2P:
> +		formatFamily_ = RAW_CSI2P;
> +		r_pos_ = 0;
> +		bpp_numer_ = 2;
> +		bpp_denom_ = 3;
> +		break;
> +
> +	case libcamera::formats::SGRBG12_CSI2P:
> +		formatFamily_ = RAW_CSI2P;
> +		r_pos_ = 1;
> +		bpp_numer_ = 2;
> +		bpp_denom_ = 3;
> +		break;
> +
> +	case libcamera::formats::SGBRG12_CSI2P:
> +		formatFamily_ = RAW_CSI2P;
> +		r_pos_ = 2;
> +		bpp_numer_ = 2;
> +		bpp_denom_ = 3;
> +		break;
> +
> +	case libcamera::formats::SBGGR12_CSI2P:
> +		formatFamily_ = RAW_CSI2P;
> +		r_pos_ = 3;
> +		bpp_numer_ = 2;
> +		bpp_denom_ = 3;
> +		break;
> +
>  	default:
>  		return -EINVAL;
>  	};
> @@ -163,9 +219,71 @@ void FormatConverter::convert(const unsigned char *src, size_t size,
>  	case NV:
>  		convertNV(src, dst->bits());
>  		break;
> +	case RAW_CSI2P:
> +		convertRAW_CSI2P(src, dst->bits());
> +		break;
>  	};
>  }
>  
> +/*
> + * The pixels are processed in groups of 4 (2 by 2 squares), and the
> + * assumption is that height_ and width_ are even numbers.

Do we need an assertion, or round-down in here to make sure we don't
overflow if the assumption is even numbers?

(I can't imagine having an odd number on a bayer pattern, but with
cropping or such perhaps it could happen - I don't know)


Also - I wonder how cache-efficient it is processing like that. I'm not
sure if there's a way to make it more efficient, but even if there was,
that could be handled later and out of scope of this patch as this just
gets new functionality all the same.


> + */
> +void FormatConverter::convertRAW_CSI2P(const unsigned char *src,
> +				       unsigned char *dst)
> +{
> +	unsigned int r_pos, b_pos, g1_pos, g2_pos;
> +	unsigned char r, g1, g2, b;
> +	unsigned int s_linelen = width_ * bpp_denom_ / bpp_numer_;
> +	unsigned int d_linelen = width_ * 4;
> +
> +	/*
> +	 * Calculate the offsets of the color values in the src buffer.
> +	 * g1 is green value from the even (upper) line, g2 is the green
> +	 * value from the odd (lower) line.
> +	 */
> +	if ( r_pos_ > 1) {
> +		r_pos = r_pos_ - 2 + s_linelen;
> +		b_pos = 3 - r_pos_;
> +	} else {
> +		r_pos = r_pos_;
> +		b_pos = 1 - r_pos_ + s_linelen;
> +	}
> +	g1_pos = (r_pos == 0 || b_pos == 0) ? 1 : 0;
> +	g2_pos = 1 - g1_pos + s_linelen;

Could those calculations be done at configure time rather than per-frame?

> +
> +	for (unsigned int y = 0; y < height_; y += 2) {
> +		for (unsigned x = 0; x < width_; x += bpp_numer_) {
> +			for (unsigned int i = 0; i < bpp_numer_ ; i += 2) {
> +				/* read the colors for the current 2x2 group: */
> +				r = src[r_pos];
> +				g1 = src[g1_pos];
> +				g2 = src[g2_pos];
> +				b = src[b_pos];
> +				src += 2;
> +				/* two left pixels of the four: */
> +				dst[0] = dst[0 + d_linelen] = b;
> +				dst[1] = g1;
> +				dst[1 + d_linelen] = g2;
> +				dst[2] = dst[2 + d_linelen] = r;
> +				dst[3] = dst[3 + d_linelen] = 0xff;
> +				dst += 4;
> +				/* two right pixels of the four: */
> +				dst[0] = dst[0 + d_linelen] = b;
> +				dst[1] = g1;
> +				dst[1 + d_linelen] = g2;
> +				dst[2] = dst[2 + d_linelen] = r;
> +				dst[3] = dst[3 + d_linelen] = 0xff;
> +				dst += 4;
> +			}
> +			src += bpp_denom_ - bpp_numer_;
> +		}
> +		/* move to the next even line: */
> +		src += s_linelen;
> +		dst += d_linelen;
> +	}
> +}
> +
>  static void yuv_to_rgb(int y, int u, int v, int *r, int *g, int *b)
>  {
>  	int c = y - 16;
> diff --git a/src/qcam/format_converter.h b/src/qcam/format_converter.h
> index e389b24..5d4f31f 100644
> --- a/src/qcam/format_converter.h
> +++ b/src/qcam/format_converter.h
> @@ -26,11 +26,13 @@ private:
>  	enum FormatFamily {
>  		MJPEG,
>  		NV,
> +		RAW_CSI2P,
>  		RGB,
>  		YUV,
>  	};
>  
>  	void convertNV(const unsigned char *src, unsigned char *dst);
> +	void convertRAW_CSI2P(const unsigned char *src, unsigned char *dst);
>  	void convertRGB(const unsigned char *src, unsigned char *dst);
>  	void convertYUV(const unsigned char *src, unsigned char *dst);
>  
> @@ -45,6 +47,18 @@ private:
>  	unsigned int vertSubSample_;
>  	bool nvSwap_;
>  
> +	/* RAW Bayer CSI2P parameters */
> +	/*
> +	 * Bytes per pixel is a fractional number, and is represented by
> +	 * integer numerator and denominator.
> +	 */
> +	unsigned int bpp_numer_;
> +	unsigned int bpp_denom_;
> +	/*
> +	 * Unsigned int r_pos_ from RGB parameters is reused; blue
> +	 * and green positions are deduced from the red one.
> +	 */

Hrm ... I suspect it might be nicer to put the different configurations
into a union, rather than using a parameters of another configuration type.

That would also make it neater to pre-compute and store the Bayer
specific offsets too...

--
Kieran


> +
>  	/* RGB parameters */
>  	unsigned int bpp_;
>  	unsigned int r_pos_;
> 

-- 
Regards
--
Kieran