[libcamera-devel,v2,2/5] qcam: viewfinder_gl: Add shader to render packed RAW10 formats

12449 diff mbox series

The shader supports all 4 packed RAW10 variants.
Simple bi-linear filtering is implemented.
The 2 LS bits of the 10-bit colour values are dropped as the RGBA
format we convert into has only 8 bits per colour.

The texture coordinates passed to the fragment shader are ajusted
to point to the nearest pixel in the image. This prevents artifacts
when the image is scaled from the frame resolution to the window size.

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 src/qcam/assets/shader/bayer_1x_packed.frag | 174 ++++++++++++++++++++
 src/qcam/assets/shader/shaders.qrc          |   1 +
 src/qcam/viewfinder_gl.cpp                  |  77 ++++++++-
 src/qcam/viewfinder_gl.h                    |   6 +
 4 files changed, 256 insertions(+), 2 deletions(-)
 create mode 100644 src/qcam/assets/shader/bayer_1x_packed.frag

Hi Andrey,

Thank you for the patch.

On Thu, May 27, 2021 at 01:55:08PM +0300, Andrey Konovalov wrote:
> The shader supports all 4 packed RAW10 variants.
> Simple bi-linear filtering is implemented.

I had initially interpreted this as bi-linear filtering for the scaling
(when the window size doesn't match the texture size), but it's for the
Bayer interpolation. Can we make it clear by writing

Simple bi-linear Bayer interpolation of nearest pixels is implemented.

?

> The 2 LS bits of the 10-bit colour values are dropped as the RGBA
> format we convert into has only 8 bits per colour.
> 
> The texture coordinates passed to the fragment shader are ajusted
> to point to the nearest pixel in the image. This prevents artifacts
> when the image is scaled from the frame resolution to the window size.
>
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  src/qcam/assets/shader/bayer_1x_packed.frag | 174 ++++++++++++++++++++
>  src/qcam/assets/shader/shaders.qrc          |   1 +
>  src/qcam/viewfinder_gl.cpp                  |  77 ++++++++-
>  src/qcam/viewfinder_gl.h                    |   6 +
>  4 files changed, 256 insertions(+), 2 deletions(-)
>  create mode 100644 src/qcam/assets/shader/bayer_1x_packed.frag
> 
> diff --git a/src/qcam/assets/shader/bayer_1x_packed.frag b/src/qcam/assets/shader/bayer_1x_packed.frag
> new file mode 100644
> index 00000000..0a87c6db
> --- /dev/null
> +++ b/src/qcam/assets/shader/bayer_1x_packed.frag
> @@ -0,0 +1,174 @@
> +/* SPDX-License-Identifier: BSD-2-Clause */
> +/*
> + * Based on the code from http://jgt.akpeters.com/papers/McGuire08/
> + *
> + * Efficient, High-Quality Bayer Demosaic Filtering on GPUs
> + *
> + * Morgan McGuire
> + *
> + * This paper appears in issue Volume 13, Number 4.
> + * ---------------------------------------------------------
> + * Copyright (c) 2008, Morgan McGuire. All rights reserved.
> + *
> + *
> + * Modified by Linaro Ltd for 10/12-bit packed vs 8-bit raw Bayer format,
> + * and for simpler demosaic algorithm.
> + * Copyright (C) 2020, Linaro
> + *
> + * bayer_1x_packed.frag - Fragment shader code for raw Bayer 10-bit and 12-bit
> + * packed formats
> + */
> +
> +#ifdef GL_ES
> +precision mediump float;
> +#endif
> +
> +varying vec2 textureOut;
> +
> +/* the texture size: tex_size.xy is in bytes, tex_size.zw is in pixels */
> +uniform vec4 tex_size;

tex_size.xy isn't used anywhere as far as I can see. Could it be turned
into a vec2 and only store the size in pixels, or do you expect the size
in bytes to be needed later ?

> +uniform vec2 tex_step;
> +uniform vec2 tex_bayer_first_red;
> +
> +uniform sampler2D tex_raw;
> +
> +void main(void)
> +{
> +	vec3 rgb;
> +
> +	/*
> +	 * center.xy holds the coordinates of the pixel being sampled
> +	 * on the [0, 1] range.

Looking at the code, this doesn't seem to be correct. I think it holds
the value in bytes, while .zw is in pixels.

Would it be more readable to create two vec2 variables named
center_pixel and center_bytes ? Or would this have an impact on
performance ?

> +	 * center.zw holds the coordinates of the pixel being sampled
> +	 * on the [0, width/height-1] range.
> +	 */
> +	vec4 center;

Could you please add blank lines before every comment block ?

> +	/*
> +	 * x-positions of the adjacent pixels on the [0, 1] range.
> +	 */
> +	vec2 xcoords;
> +	/*
> +	 * y-positions of the adjacent pixels on the [0, 1] range.
> +	 */
> +	vec2 ycoords;
> +
> +	/*
> +	 * The coordinates passed to the shader in textureOut may point
> +	 * to a place in between the pixels if the viewfinder window is scaled
> +	 * from the original captured frame size. Align them to the nearest
> +	 * pixel.
> +	 */
> +	center.zw = floor(textureOut * tex_size.zw);

Unless I'm mistaken, this doesn't align to the nearest pixel, but to the
previous one. We could use round() instead, but that would require GLSL
1.30.

Can textureOut really point to between pixels, given that
textureMinMagFilters_ is set to GL_NEAREST ?

> +	center.y = center.w;
> +	/*
> +	 * Add a small number (a few mantissa's LSBs) to avoid float
> +	 * representation issues. Maybe paranoic.
> +	 */
> +	center.x = BPP_X * center.z + 0.02;
> +
> +	const float threshold_l = 0.127 /* fract(BPP_X) * 0.5 + 0.02 */;
> +	const float threshold_h = 0.625 /* 1.0 - fract(BPP_X) * 1.5 */;
> +
> +	float fract_x = fract(center.x);
> +	/*
> +	 * The below floor() call ensures that center.x points
> +	 * at one of the bytes representing the 8 higher bits of
> +	 * the pixel value, not at the byte containing the LS bits
> +	 * of the group of the pixels.
> +	 */
> +	center.x = floor(center.x);
> +	center.xy *= tex_step;
> +
> +	xcoords = center.x + vec2(-tex_step.x, tex_step.x);
> +	ycoords = center.y + vec2(-tex_step.y, tex_step.y);
> +	/*
> +	 * If xcoords[0] points at the byte containing the LS bits
> +         * of the previous group of the pixels, move xcoords[0] one

Incorrect indentation here (and same below).

> +	 * byte back.
> +	 */
> +	xcoords[0] += (fract_x < threshold_l) ? -tex_step.x : 0.0;
> +	/*
> +	 * If xcoords[1] points at the byte containing the LS bits
> +         * of the current group of the pixels, move xcoords[1] one
> +	 * byte forward.
> +	 */
> +	xcoords[1] += (fract_x > threshold_h) ? tex_step.x : 0.0;
> +
> +	vec2 alternate = mod(center.zw + tex_bayer_first_red, 2.0);
> +	bool even_col = alternate.x < 1.0;
> +	bool even_raw = alternate.y < 1.0;

s/raw/row/

> +
> +	/*
> +	 * We need to sample the central pixel and the ones with offset
> +	 * of -1 to +1 pixel in both X and Y directions. Let's name these
> +	 * pixels as below, where C is the central pixel:

I'd add a blank line here and a few below too.

> +	 *   +----+----+----+----+
> +	 *   | \ x|    |    |    |
> +	 *   |y \ | -1 |  0 | +1 | 
> +	 *   +----+----+----+----+
> +	 *   | +1 | D2 | A1 | D3 |
> +	 *   +----+----+----+----+
> +	 *   |  0 | B0 |  C | B1 |
> +	 *   +----+----+----+----+
> +	 *   | -1 | D0 | A0 | D1 |
> +	 *   +----+----+----+----+
> +	 * In the below equations (0,-1).r means "r component of the texel
> +	 * shifted by -tex_step.y from the center.xy one" etc.
> +	 * In the even raw / even column (EE) case the colour values are:

s/raw/row/ (same below).

> +	 *   R = C = (0,0).r,
> +	 *   G = (A0 + A1 + B0 + B1) / 4.0 =
> +	 *       ( (0,-1).r + (0,1).r + (-1,0).r + (1,0).r ) / 4.0,
> +	 *   B = (D0 + D1 + D2 + D3) / 4.0 =
> +	 *       ( (-1,-1).r + (1,-1).r + (-1,1).r + (1,1).r ) / 4.0
> +	 * For even raw / odd column (EO):
> +	 *   R = (B0 + B1) / 2.0 = ( (-1,0).r + (1,0).r ) / 2.0,
> +	 *   G = C = (0,0).r,
> +	 *   B = (A0 + A1) / 2.0 = ( (0,-1).r + (0,1).r ) / 2.0
> +	 * For odd raw / even column (OE):
> +	 *   R = (A0 + A1) / 2.0 = ( (0,-1).r + (0,1).r ) / 2.0,
> +	 *   G = C = (0,0).r,
> +	 *   B = (B0 + B1) / 2.0 = ( (-1,0).r + (1,0).r ) / 2.0
> +	 * For odd raw / odd column (OO):
> +	 *   R = (D0 + D1 + D2 + D3) / 4.0 =
> +	 *       ( (-1,-1).r + (1,-1).r + (-1,1).r + (1,1).r ) / 4.0,
> +	 *   G = (A0 + A1 + B0 + B1) / 4.0 =
> +	 *       ( (0,-1).r + (0,1).r + (-1,0).r + (1,0).r ) / 4.0,
> +	 *   B = C = (0,0).r
> +	 */
> +
> +	/*
> +	 * Fetch the values and precalculate the terms:
> +	 *   patterns.x = (A0 + A1) / 2.0
> +	 *   patterns.y = (B0 + B1) / 2.0
> +	 *   patterns.z = (A0 + A1 + B0 + B1) / 4.0
> +	 *   patterns.w = (D0 + D1 + D2 + D3) / 4.0
> +	 */
> +	#define fetch(x, y) texture2D(tex_raw, vec2(x, y)).r
> +
> +	float C = texture2D(tex_raw, center.xy).r;
> +	vec4 patterns = vec4(
> +		fetch(center.x, ycoords[0]),	/* A0: (0,-1) */
> +		fetch(xcoords[0], center.y),	/* B0: (-1,0) */
> +		fetch(xcoords[0], ycoords[0]),	/* D0: (-1,-1) */
> +		fetch(xcoords[1], ycoords[0]));	/* D1: (1,-1) */
> +	vec4 temp = vec4(
> +		fetch(center.x, ycoords[1]),	/* A1: (0,1) */
> +		fetch(xcoords[1], center.y),	/* B1: (1,0) */
> +		fetch(xcoords[1], ycoords[1]),	/* D3: (1,1) */
> +		fetch(xcoords[0], ycoords[1]));	/* D2: (-1,1) */
> +	patterns = (patterns + temp) * 0.5;
> +		/* .x = (A0 + A1) / 2.0, .y = (B0 + B1) / 2.0 */
> +		/* .z = (D0 + D3) / 2.0, .w = (D1 + D2) / 2.0 */
> +	patterns.w = (patterns.z + patterns.w) * 0.5;
> +	patterns.z = (patterns.x + patterns.y) * 0.5;
> +
> +	rgb = (even_col) ?
> +		((even_raw) ?
> +			vec3(C, patterns.zw) :
> +			vec3(patterns.x, C, patterns.y)) :
> +		((even_raw) ?
> +			vec3(patterns.y, C, patterns.x) :
> +			vec3(patterns.wz, C));

No need for parentheses around even_col and even_raw.

> +
> +	gl_FragColor = vec4(rgb, 1.0);
> +}
> diff --git a/src/qcam/assets/shader/shaders.qrc b/src/qcam/assets/shader/shaders.qrc
> index 8a8f9de1..d76d65c5 100644
> --- a/src/qcam/assets/shader/shaders.qrc
> +++ b/src/qcam/assets/shader/shaders.qrc
> @@ -5,6 +5,7 @@
>  	<file>YUV_2_planes.frag</file>
>  	<file>YUV_3_planes.frag</file>
>  	<file>YUV_packed.frag</file>
> +	<file>bayer_1x_packed.frag</file>
>  	<file>identity.vert</file>
>  </qresource>
>  </RCC>
> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> index ff719418..b324d77f 100644
> --- a/src/qcam/viewfinder_gl.cpp
> +++ b/src/qcam/viewfinder_gl.cpp
> @@ -36,6 +36,11 @@ static const QList<libcamera::PixelFormat> supportedFormats{
>  	libcamera::formats::RGBA8888,
>  	libcamera::formats::BGR888,
>  	libcamera::formats::RGB888,
> +	/* Raw Bayer 10-bit packed */
> +	libcamera::formats::SBGGR10_CSI2P,
> +	libcamera::formats::SGBRG10_CSI2P,
> +	libcamera::formats::SGRBG10_CSI2P,
> +	libcamera::formats::SRGGB10_CSI2P,
>  };
>  
>  ViewFinderGL::ViewFinderGL(QWidget *parent)
> @@ -114,6 +119,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)
>  {
>  	bool ret = true;
>  
> +	/* Set min/mag filters to GL_LINEAR by default. */
> +	textureMinMagFilters_ = GL_LINEAR;
> +
>  	fragmentShaderDefines_.clear();
>  
>  	switch (format) {
> @@ -203,6 +211,34 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)
>  		fragmentShaderDefines_.append("#define RGB_PATTERN bgr");
>  		fragmentShaderFile_ = ":RGB.frag";
>  		break;
> +	case libcamera::formats::SBGGR10_CSI2P:
> +		firstRed_.setX(1.0);
> +		firstRed_.setY(1.0);
> +		fragmentShaderDefines_.append("#define BPP_X 1.25");
> +		fragmentShaderFile_ = ":bayer_1x_packed.frag";
> +		textureMinMagFilters_ = GL_NEAREST;
> +		break;
> +	case libcamera::formats::SGBRG10_CSI2P:
> +		firstRed_.setX(0.0);
> +		firstRed_.setY(1.0);
> +		fragmentShaderDefines_.append("#define BPP_X 1.25");
> +		fragmentShaderFile_ = ":bayer_1x_packed.frag";
> +		textureMinMagFilters_ = GL_NEAREST;
> +		break;
> +	case libcamera::formats::SGRBG10_CSI2P:
> +		firstRed_.setX(1.0);
> +		firstRed_.setY(0.0);
> +		fragmentShaderDefines_.append("#define BPP_X 1.25");
> +		fragmentShaderFile_ = ":bayer_1x_packed.frag";
> +		textureMinMagFilters_ = GL_NEAREST;
> +		break;
> +	case libcamera::formats::SRGGB10_CSI2P:
> +		firstRed_.setX(0.0);
> +		firstRed_.setY(0.0);
> +		fragmentShaderDefines_.append("#define BPP_X 1.25");
> +		fragmentShaderFile_ = ":bayer_1x_packed.frag";
> +		textureMinMagFilters_ = GL_NEAREST;
> +		break;
>  	default:
>  		ret = false;
>  		qWarning() << "[ViewFinderGL]:"
> @@ -290,6 +326,8 @@ bool ViewFinderGL::createFragmentShader()
>  	textureUniformU_ = shaderProgram_.uniformLocation("tex_u");
>  	textureUniformV_ = shaderProgram_.uniformLocation("tex_v");
>  	textureUniformStep_ = shaderProgram_.uniformLocation("tex_step");
> +	textureUniformSize_ = shaderProgram_.uniformLocation("tex_size");
> +	textureUniformBayerFirstRed_ = shaderProgram_.uniformLocation("tex_bayer_first_red");
>  
>  	/* Create the textures. */
>  	for (std::unique_ptr<QOpenGLTexture> &texture : textures_) {
> @@ -306,8 +344,10 @@ bool ViewFinderGL::createFragmentShader()
>  void ViewFinderGL::configureTexture(QOpenGLTexture &texture)
>  {
>  	glBindTexture(GL_TEXTURE_2D, texture.textureId());
> -	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
> -	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
> +	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
> +			textureMinMagFilters_);
> +	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
> +			textureMinMagFilters_);
>  	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
>  	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
>  }
> @@ -547,6 +587,39 @@ void ViewFinderGL::doRender()
>  		shaderProgram_.setUniformValue(textureUniformY_, 0);
>  		break;
>  
> +	case libcamera::formats::SBGGR10_CSI2P:
> +	case libcamera::formats::SGBRG10_CSI2P:
> +	case libcamera::formats::SGRBG10_CSI2P:
> +	case libcamera::formats::SRGGB10_CSI2P:
> +		/*
> +		 * Packed raw Bayer 10-bit formats are stored in GL_RED texture.
> +		 * The texture width is 10/8 of the image width.
> +		 * TODO: account for padding bytes at the end of the line.

s/TODO:/\todo/

But it's fixed in 3/5, which seems to have forgotten to drop this line.

> +		 */
> +		glActiveTexture(GL_TEXTURE0);
> +		configureTexture(*textures_[0]);
> +		glTexImage2D(GL_TEXTURE_2D,
> +			     0,
> +			     GL_RED,
> +			     size_.width() * 5 / 4,
> +			     size_.height(),
> +			     0,
> +			     GL_RED,
> +			     GL_UNSIGNED_BYTE,
> +			     data_);
> +		shaderProgram_.setUniformValue(textureUniformY_, 0);
> +		shaderProgram_.setUniformValue(textureUniformBayerFirstRed_,
> +					       firstRed_);
> +		shaderProgram_.setUniformValue(textureUniformSize_,
> +					       size_.width() * 5 / 4,
> +					       size_.height(),
> +					       size_.width(),
> +					       size_.height());
> +		shaderProgram_.setUniformValue(textureUniformStep_,
> +					       1.0f / (size_.width() * 5 / 4 - 1),
> +					       1.0f / (size_.height() - 1));
> +		break;
> +
>  	default:
>  		break;
>  	};
> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h
> index 1b1faa91..337718e3 100644
> --- a/src/qcam/viewfinder_gl.h
> +++ b/src/qcam/viewfinder_gl.h
> @@ -81,13 +81,19 @@ private:
>  	/* Textures */
>  	std::array<std::unique_ptr<QOpenGLTexture>, 3> textures_;
>  
> +	/* Common texture parameters */
> +	GLuint textureMinMagFilters_;

A blank line would be nice here.

>  	/* YUV texture parameters */
>  	GLuint textureUniformU_;
>  	GLuint textureUniformV_;
>  	GLuint textureUniformY_;
> +	GLuint textureUniformSize_;

This is for Bayer textures, so it should be moved below.

>  	GLuint textureUniformStep_;
>  	unsigned int horzSubSample_;
>  	unsigned int vertSubSample_;

And here too.

> +	/* Raw Bayer texture parameters */
> +	GLuint textureUniformBayerFirstRed_;
> +	QPointF firstRed_;
>  
>  	QMutex mutex_; /* Prevent concurrent access to image_ */
>  };

Hi Laurent,

Thank you for reviewing these patches!

On 10.06.2021 00:56, Laurent Pinchart wrote:
> Hi Andrey,
> 
> Thank you for the patch.
> 
> On Thu, May 27, 2021 at 01:55:08PM +0300, Andrey Konovalov wrote:
>> The shader supports all 4 packed RAW10 variants.
>> Simple bi-linear filtering is implemented.
> 
> I had initially interpreted this as bi-linear filtering for the scaling
> (when the window size doesn't match the texture size), but it's for the
> Bayer interpolation. Can we make it clear by writing
> 
> Simple bi-linear Bayer interpolation of nearest pixels is implemented.
> 
> ?

This will be in v4.

>> The 2 LS bits of the 10-bit colour values are dropped as the RGBA
>> format we convert into has only 8 bits per colour.
>>
>> The texture coordinates passed to the fragment shader are ajusted
>> to point to the nearest pixel in the image. This prevents artifacts
>> when the image is scaled from the frame resolution to the window size.
>>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
>> ---
>>   src/qcam/assets/shader/bayer_1x_packed.frag | 174 ++++++++++++++++++++
>>   src/qcam/assets/shader/shaders.qrc          |   1 +
>>   src/qcam/viewfinder_gl.cpp                  |  77 ++++++++-
>>   src/qcam/viewfinder_gl.h                    |   6 +
>>   4 files changed, 256 insertions(+), 2 deletions(-)
>>   create mode 100644 src/qcam/assets/shader/bayer_1x_packed.frag
>>
>> diff --git a/src/qcam/assets/shader/bayer_1x_packed.frag b/src/qcam/assets/shader/bayer_1x_packed.frag
>> new file mode 100644
>> index 00000000..0a87c6db
>> --- /dev/null
>> +++ b/src/qcam/assets/shader/bayer_1x_packed.frag
>> @@ -0,0 +1,174 @@
>> +/* SPDX-License-Identifier: BSD-2-Clause */
>> +/*
>> + * Based on the code from http://jgt.akpeters.com/papers/McGuire08/
>> + *
>> + * Efficient, High-Quality Bayer Demosaic Filtering on GPUs
>> + *
>> + * Morgan McGuire
>> + *
>> + * This paper appears in issue Volume 13, Number 4.
>> + * ---------------------------------------------------------
>> + * Copyright (c) 2008, Morgan McGuire. All rights reserved.
>> + *
>> + *
>> + * Modified by Linaro Ltd for 10/12-bit packed vs 8-bit raw Bayer format,
>> + * and for simpler demosaic algorithm.
>> + * Copyright (C) 2020, Linaro
>> + *
>> + * bayer_1x_packed.frag - Fragment shader code for raw Bayer 10-bit and 12-bit
>> + * packed formats
>> + */
>> +
>> +#ifdef GL_ES
>> +precision mediump float;
>> +#endif
>> +
>> +varying vec2 textureOut;
>> +
>> +/* the texture size: tex_size.xy is in bytes, tex_size.zw is in pixels */
>> +uniform vec4 tex_size;
> 
> tex_size.xy isn't used anywhere as far as I can see. Could it be turned
> into a vec2 and only store the size in pixels, or do you expect the size
> in bytes to be needed later ?

You are right. Will make this change in v4.

>> +uniform vec2 tex_step;
>> +uniform vec2 tex_bayer_first_red;
>> +
>> +uniform sampler2D tex_raw;
>> +
>> +void main(void)
>> +{
>> +	vec3 rgb;
>> +
>> +	/*
>> +	 * center.xy holds the coordinates of the pixel being sampled
>> +	 * on the [0, 1] range.
> 
> Looking at the code, this doesn't seem to be correct. I think it holds
> the value in bytes, while .zw is in pixels.

Correct. I've fixed the comment for v4.

> Would it be more readable to create two vec2 variables named
> center_pixel and center_bytes ? Or would this have an impact on
> performance ?

I'll split vec4 center into two vec2 for v4 of the patchset. It shouldn't
impact the performance, just will use two GPU registers instead of one
(not a big deal here).

>> +	 * center.zw holds the coordinates of the pixel being sampled
>> +	 * on the [0, width/height-1] range.
>> +	 */
>> +	vec4 center;
> 
> Could you please add blank lines before every comment block ?

OK.

>> +	/*
>> +	 * x-positions of the adjacent pixels on the [0, 1] range.
>> +	 */
>> +	vec2 xcoords;
>> +	/*
>> +	 * y-positions of the adjacent pixels on the [0, 1] range.
>> +	 */
>> +	vec2 ycoords;
>> +
>> +	/*
>> +	 * The coordinates passed to the shader in textureOut may point
>> +	 * to a place in between the pixels if the viewfinder window is scaled
>> +	 * from the original captured frame size. Align them to the nearest
>> +	 * pixel.
>> +	 */
>> +	center.zw = floor(textureOut * tex_size.zw);
> 
> Unless I'm mistaken, this doesn't align to the nearest pixel, but to the
> previous one.

To my understanding, if textureOut points in between the pixels, floor() will
set center.zw (center_pixel) to the nearest pixel to the left (speaking of the
X axis).

> We could use round() instead, but that would require GLSL
> 1.30.

Not sure if using round() has benefits over using floor().
(Shifting the whole image by half a pixel could hardly be noticed by a naked eye
anyway)

> Can textureOut really point to between pixels, given that
> textureMinMagFilters_ is set to GL_NEAREST ?

The comment in the shader code says that it is possible if the window is
not of the same size as the sensor resolution. And re-reading it now, this
doesn't seem to be the real reason.
But the problem with MIPI packed bayer formats (the 10- and 12-bit formats
supported by this and the 4/5 patches) is that we use 8-bit (GL_RED) texture
to store 10- or 12-bit pixels (as there is no texture format with 10/12-bit
texels). And in this case GL_NEAREST wouldn't work correctly. Hence the need
for manual tricks.

What I am not currently sure of, is if the 8-bit raw Bayer format would
just work then. This is not a packed format, and it fits GL_RED texture just fine.
Though I did tried using the original vertex and fragment shaders by Morgan,
and did see the same defects as the ones fixed by aligning the pixel coordinates
manually.

I'll check the 8-bit raw Bayer case again to make sure I didn't messed
something up back in February when v1 of this patchset was created.

And if the above explanation for the packed formats sounds OK for you,
I could move the last patch which adds 8-bit raw Bayer support out of this
patchset, and post the 8-bit one separately to speed the things up.

>> +	center.y = center.w;
>> +	/*
>> +	 * Add a small number (a few mantissa's LSBs) to avoid float
>> +	 * representation issues. Maybe paranoic.
>> +	 */
>> +	center.x = BPP_X * center.z + 0.02;
>> +
>> +	const float threshold_l = 0.127 /* fract(BPP_X) * 0.5 + 0.02 */;
>> +	const float threshold_h = 0.625 /* 1.0 - fract(BPP_X) * 1.5 */;
>> +
>> +	float fract_x = fract(center.x);
>> +	/*
>> +	 * The below floor() call ensures that center.x points
>> +	 * at one of the bytes representing the 8 higher bits of
>> +	 * the pixel value, not at the byte containing the LS bits
>> +	 * of the group of the pixels.
>> +	 */
>> +	center.x = floor(center.x);
>> +	center.xy *= tex_step;
>> +
>> +	xcoords = center.x + vec2(-tex_step.x, tex_step.x);
>> +	ycoords = center.y + vec2(-tex_step.y, tex_step.y);
>> +	/*
>> +	 * If xcoords[0] points at the byte containing the LS bits
>> +         * of the previous group of the pixels, move xcoords[0] one
> 
> Incorrect indentation here (and same below).

Ack, will fix in v4.

>> +	 * byte back.
>> +	 */
>> +	xcoords[0] += (fract_x < threshold_l) ? -tex_step.x : 0.0;
>> +	/*
>> +	 * If xcoords[1] points at the byte containing the LS bits
>> +         * of the current group of the pixels, move xcoords[1] one
>> +	 * byte forward.
>> +	 */
>> +	xcoords[1] += (fract_x > threshold_h) ? tex_step.x : 0.0;
>> +
>> +	vec2 alternate = mod(center.zw + tex_bayer_first_red, 2.0);
>> +	bool even_col = alternate.x < 1.0;
>> +	bool even_raw = alternate.y < 1.0;
> 
> s/raw/row/

Will be fixed in v4.

>> +
>> +	/*
>> +	 * We need to sample the central pixel and the ones with offset
>> +	 * of -1 to +1 pixel in both X and Y directions. Let's name these
>> +	 * pixels as below, where C is the central pixel:
> 
> I'd add a blank line here and a few below too.

Will be fixed in v4.

>> +	 *   +----+----+----+----+
>> +	 *   | \ x|    |    |    |
>> +	 *   |y \ | -1 |  0 | +1 |
>> +	 *   +----+----+----+----+
>> +	 *   | +1 | D2 | A1 | D3 |
>> +	 *   +----+----+----+----+
>> +	 *   |  0 | B0 |  C | B1 |
>> +	 *   +----+----+----+----+
>> +	 *   | -1 | D0 | A0 | D1 |
>> +	 *   +----+----+----+----+
>> +	 * In the below equations (0,-1).r means "r component of the texel
>> +	 * shifted by -tex_step.y from the center.xy one" etc.
>> +	 * In the even raw / even column (EE) case the colour values are:
> 
> s/raw/row/ (same below).

Will be fixed in v4.

>> +	 *   R = C = (0,0).r,
>> +	 *   G = (A0 + A1 + B0 + B1) / 4.0 =
>> +	 *       ( (0,-1).r + (0,1).r + (-1,0).r + (1,0).r ) / 4.0,
>> +	 *   B = (D0 + D1 + D2 + D3) / 4.0 =
>> +	 *       ( (-1,-1).r + (1,-1).r + (-1,1).r + (1,1).r ) / 4.0
>> +	 * For even raw / odd column (EO):
>> +	 *   R = (B0 + B1) / 2.0 = ( (-1,0).r + (1,0).r ) / 2.0,
>> +	 *   G = C = (0,0).r,
>> +	 *   B = (A0 + A1) / 2.0 = ( (0,-1).r + (0,1).r ) / 2.0
>> +	 * For odd raw / even column (OE):
>> +	 *   R = (A0 + A1) / 2.0 = ( (0,-1).r + (0,1).r ) / 2.0,
>> +	 *   G = C = (0,0).r,
>> +	 *   B = (B0 + B1) / 2.0 = ( (-1,0).r + (1,0).r ) / 2.0
>> +	 * For odd raw / odd column (OO):
>> +	 *   R = (D0 + D1 + D2 + D3) / 4.0 =
>> +	 *       ( (-1,-1).r + (1,-1).r + (-1,1).r + (1,1).r ) / 4.0,
>> +	 *   G = (A0 + A1 + B0 + B1) / 4.0 =
>> +	 *       ( (0,-1).r + (0,1).r + (-1,0).r + (1,0).r ) / 4.0,
>> +	 *   B = C = (0,0).r
>> +	 */
>> +
>> +	/*
>> +	 * Fetch the values and precalculate the terms:
>> +	 *   patterns.x = (A0 + A1) / 2.0
>> +	 *   patterns.y = (B0 + B1) / 2.0
>> +	 *   patterns.z = (A0 + A1 + B0 + B1) / 4.0
>> +	 *   patterns.w = (D0 + D1 + D2 + D3) / 4.0
>> +	 */
>> +	#define fetch(x, y) texture2D(tex_raw, vec2(x, y)).r
>> +
>> +	float C = texture2D(tex_raw, center.xy).r;
>> +	vec4 patterns = vec4(
>> +		fetch(center.x, ycoords[0]),	/* A0: (0,-1) */
>> +		fetch(xcoords[0], center.y),	/* B0: (-1,0) */
>> +		fetch(xcoords[0], ycoords[0]),	/* D0: (-1,-1) */
>> +		fetch(xcoords[1], ycoords[0]));	/* D1: (1,-1) */
>> +	vec4 temp = vec4(
>> +		fetch(center.x, ycoords[1]),	/* A1: (0,1) */
>> +		fetch(xcoords[1], center.y),	/* B1: (1,0) */
>> +		fetch(xcoords[1], ycoords[1]),	/* D3: (1,1) */
>> +		fetch(xcoords[0], ycoords[1]));	/* D2: (-1,1) */
>> +	patterns = (patterns + temp) * 0.5;
>> +		/* .x = (A0 + A1) / 2.0, .y = (B0 + B1) / 2.0 */
>> +		/* .z = (D0 + D3) / 2.0, .w = (D1 + D2) / 2.0 */
>> +	patterns.w = (patterns.z + patterns.w) * 0.5;
>> +	patterns.z = (patterns.x + patterns.y) * 0.5;
>> +
>> +	rgb = (even_col) ?
>> +		((even_raw) ?
>> +			vec3(C, patterns.zw) :
>> +			vec3(patterns.x, C, patterns.y)) :
>> +		((even_raw) ?
>> +			vec3(patterns.y, C, patterns.x) :
>> +			vec3(patterns.wz, C));
> 
> No need for parentheses around even_col and even_raw.

Will be fixed in v4.

>> +
>> +	gl_FragColor = vec4(rgb, 1.0);
>> +}
>> diff --git a/src/qcam/assets/shader/shaders.qrc b/src/qcam/assets/shader/shaders.qrc
>> index 8a8f9de1..d76d65c5 100644
>> --- a/src/qcam/assets/shader/shaders.qrc
>> +++ b/src/qcam/assets/shader/shaders.qrc
>> @@ -5,6 +5,7 @@
>>   	<file>YUV_2_planes.frag</file>
>>   	<file>YUV_3_planes.frag</file>
>>   	<file>YUV_packed.frag</file>
>> +	<file>bayer_1x_packed.frag</file>
>>   	<file>identity.vert</file>
>>   </qresource>
>>   </RCC>
>> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
>> index ff719418..b324d77f 100644
>> --- a/src/qcam/viewfinder_gl.cpp
>> +++ b/src/qcam/viewfinder_gl.cpp
>> @@ -36,6 +36,11 @@ static const QList<libcamera::PixelFormat> supportedFormats{
>>   	libcamera::formats::RGBA8888,
>>   	libcamera::formats::BGR888,
>>   	libcamera::formats::RGB888,
>> +	/* Raw Bayer 10-bit packed */
>> +	libcamera::formats::SBGGR10_CSI2P,
>> +	libcamera::formats::SGBRG10_CSI2P,
>> +	libcamera::formats::SGRBG10_CSI2P,
>> +	libcamera::formats::SRGGB10_CSI2P,
>>   };
>>   
>>   ViewFinderGL::ViewFinderGL(QWidget *parent)
>> @@ -114,6 +119,9 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)
>>   {
>>   	bool ret = true;
>>   
>> +	/* Set min/mag filters to GL_LINEAR by default. */
>> +	textureMinMagFilters_ = GL_LINEAR;
>> +
>>   	fragmentShaderDefines_.clear();
>>   
>>   	switch (format) {
>> @@ -203,6 +211,34 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)
>>   		fragmentShaderDefines_.append("#define RGB_PATTERN bgr");
>>   		fragmentShaderFile_ = ":RGB.frag";
>>   		break;
>> +	case libcamera::formats::SBGGR10_CSI2P:
>> +		firstRed_.setX(1.0);
>> +		firstRed_.setY(1.0);
>> +		fragmentShaderDefines_.append("#define BPP_X 1.25");
>> +		fragmentShaderFile_ = ":bayer_1x_packed.frag";
>> +		textureMinMagFilters_ = GL_NEAREST;
>> +		break;
>> +	case libcamera::formats::SGBRG10_CSI2P:
>> +		firstRed_.setX(0.0);
>> +		firstRed_.setY(1.0);
>> +		fragmentShaderDefines_.append("#define BPP_X 1.25");
>> +		fragmentShaderFile_ = ":bayer_1x_packed.frag";
>> +		textureMinMagFilters_ = GL_NEAREST;
>> +		break;
>> +	case libcamera::formats::SGRBG10_CSI2P:
>> +		firstRed_.setX(1.0);
>> +		firstRed_.setY(0.0);
>> +		fragmentShaderDefines_.append("#define BPP_X 1.25");
>> +		fragmentShaderFile_ = ":bayer_1x_packed.frag";
>> +		textureMinMagFilters_ = GL_NEAREST;
>> +		break;
>> +	case libcamera::formats::SRGGB10_CSI2P:
>> +		firstRed_.setX(0.0);
>> +		firstRed_.setY(0.0);
>> +		fragmentShaderDefines_.append("#define BPP_X 1.25");
>> +		fragmentShaderFile_ = ":bayer_1x_packed.frag";
>> +		textureMinMagFilters_ = GL_NEAREST;
>> +		break;
>>   	default:
>>   		ret = false;
>>   		qWarning() << "[ViewFinderGL]:"
>> @@ -290,6 +326,8 @@ bool ViewFinderGL::createFragmentShader()
>>   	textureUniformU_ = shaderProgram_.uniformLocation("tex_u");
>>   	textureUniformV_ = shaderProgram_.uniformLocation("tex_v");
>>   	textureUniformStep_ = shaderProgram_.uniformLocation("tex_step");
>> +	textureUniformSize_ = shaderProgram_.uniformLocation("tex_size");
>> +	textureUniformBayerFirstRed_ = shaderProgram_.uniformLocation("tex_bayer_first_red");
>>   
>>   	/* Create the textures. */
>>   	for (std::unique_ptr<QOpenGLTexture> &texture : textures_) {
>> @@ -306,8 +344,10 @@ bool ViewFinderGL::createFragmentShader()
>>   void ViewFinderGL::configureTexture(QOpenGLTexture &texture)
>>   {
>>   	glBindTexture(GL_TEXTURE_2D, texture.textureId());
>> -	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
>> -	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
>> +	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER,
>> +			textureMinMagFilters_);
>> +	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
>> +			textureMinMagFilters_);
>>   	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
>>   	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
>>   }
>> @@ -547,6 +587,39 @@ void ViewFinderGL::doRender()
>>   		shaderProgram_.setUniformValue(textureUniformY_, 0);
>>   		break;
>>   
>> +	case libcamera::formats::SBGGR10_CSI2P:
>> +	case libcamera::formats::SGBRG10_CSI2P:
>> +	case libcamera::formats::SGRBG10_CSI2P:
>> +	case libcamera::formats::SRGGB10_CSI2P:
>> +		/*
>> +		 * Packed raw Bayer 10-bit formats are stored in GL_RED texture.
>> +		 * The texture width is 10/8 of the image width.
>> +		 * TODO: account for padding bytes at the end of the line.
> 
> s/TODO:/\todo/
> 
> But it's fixed in 3/5, which seems to have forgotten to drop this line.

Will be fixed in v4.

>> +		 */
>> +		glActiveTexture(GL_TEXTURE0);
>> +		configureTexture(*textures_[0]);
>> +		glTexImage2D(GL_TEXTURE_2D,
>> +			     0,
>> +			     GL_RED,
>> +			     size_.width() * 5 / 4,
>> +			     size_.height(),
>> +			     0,
>> +			     GL_RED,
>> +			     GL_UNSIGNED_BYTE,
>> +			     data_);
>> +		shaderProgram_.setUniformValue(textureUniformY_, 0);
>> +		shaderProgram_.setUniformValue(textureUniformBayerFirstRed_,
>> +					       firstRed_);
>> +		shaderProgram_.setUniformValue(textureUniformSize_,
>> +					       size_.width() * 5 / 4,
>> +					       size_.height(),
>> +					       size_.width(),
>> +					       size_.height());
>> +		shaderProgram_.setUniformValue(textureUniformStep_,
>> +					       1.0f / (size_.width() * 5 / 4 - 1),
>> +					       1.0f / (size_.height() - 1));
>> +		break;
>> +
>>   	default:
>>   		break;
>>   	};
>> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h
>> index 1b1faa91..337718e3 100644
>> --- a/src/qcam/viewfinder_gl.h
>> +++ b/src/qcam/viewfinder_gl.h
>> @@ -81,13 +81,19 @@ private:
>>   	/* Textures */
>>   	std::array<std::unique_ptr<QOpenGLTexture>, 3> textures_;
>>   
>> +	/* Common texture parameters */
>> +	GLuint textureMinMagFilters_;
> 
> A blank line would be nice here.

Will be fixed in v4.

>>   	/* YUV texture parameters */
>>   	GLuint textureUniformU_;
>>   	GLuint textureUniformV_;
>>   	GLuint textureUniformY_;
>> +	GLuint textureUniformSize_;
> 
> This is for Bayer textures, so it should be moved below.

Will be fixed in v4.

>>   	GLuint textureUniformStep_;
>>   	unsigned int horzSubSample_;
>>   	unsigned int vertSubSample_;
> 
> And here too.

Will be fixed in v4.

Thanks,
Andrey

>> +	/* Raw Bayer texture parameters */
>> +	GLuint textureUniformBayerFirstRed_;
>> +	QPointF firstRed_;
>>   
>>   	QMutex mutex_; /* Prevent concurrent access to image_ */
>>   };
>