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

[Andrey Konovalov]

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 at 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_ */
>>   };
>