[libcamera-devel,RFC,2/2] qcam: viewfinder_gl: Add shader to render packed RAW12 formats

10399 diff mbox series

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

Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
---
 src/qcam/assets/shader/bayer.vert           | 28 ++++++++
 src/qcam/assets/shader/bayer_12_packed.frag | 68 +++++++++++++++++++
 src/qcam/assets/shader/shaders.qrc          |  2 +
 src/qcam/viewfinder_gl.cpp                  | 74 ++++++++++++++++++++-
 src/qcam/viewfinder_gl.h                    |  7 ++
 5 files changed, 177 insertions(+), 2 deletions(-)
 create mode 100644 src/qcam/assets/shader/bayer.vert
 create mode 100644 src/qcam/assets/shader/bayer_12_packed.frag

Hi Andrey,

Thank you for the patch.

On Tue, Nov 10, 2020 at 12:34:11AM +0300, Andrey Konovalov wrote:
> The shader supports all 4 packed RAW12 variants.
> Simple bi-linear filtering is implemented.
> The 4 LS bits of the 12-bit colour values are dropped as the RGBA
> format we convert into has only 8 bits per colour.
> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  src/qcam/assets/shader/bayer.vert           | 28 ++++++++
>  src/qcam/assets/shader/bayer_12_packed.frag | 68 +++++++++++++++++++
>  src/qcam/assets/shader/shaders.qrc          |  2 +
>  src/qcam/viewfinder_gl.cpp                  | 74 ++++++++++++++++++++-
>  src/qcam/viewfinder_gl.h                    |  7 ++
>  5 files changed, 177 insertions(+), 2 deletions(-)
>  create mode 100644 src/qcam/assets/shader/bayer.vert
>  create mode 100644 src/qcam/assets/shader/bayer_12_packed.frag
> 
> diff --git a/src/qcam/assets/shader/bayer.vert b/src/qcam/assets/shader/bayer.vert
> new file mode 100644
> index 00000000..375e0b60
> --- /dev/null
> +++ b/src/qcam/assets/shader/bayer.vert
> @@ -0,0 +1,28 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Linaro
> + *
> + * bayer.vert - Vertex shader for raw Bayer to RGB conversion
> + */
> +
> +attribute vec4 vertexIn;
> +attribute vec2 textureIn;
> +
> +uniform vec4 srcSize;
> +uniform vec2 firstRed;
> +
> +varying vec4 center;
> +varying vec2 xcoords;
> +varying vec2 ycoords;
> +
> +void main(void)
> +{
> +	center.xy = textureIn;
> +	center.zw = textureIn * srcSize.xy + firstRed;

center.xy and center.zw store quite different data. Wouldn't the code be
more readable if we used two different variables ?

> +
> +	vec2 invSize = srcSize.zw;
> +	xcoords = center.x + vec2(-invSize.x, invSize.x);
> +	ycoords = center.y + vec2(-invSize.y, invSize.y);
> +
> +	gl_Position = vertexIn;
> +}
> diff --git a/src/qcam/assets/shader/bayer_12_packed.frag b/src/qcam/assets/shader/bayer_12_packed.frag
> new file mode 100644
> index 00000000..eb822b85
> --- /dev/null
> +++ b/src/qcam/assets/shader/bayer_12_packed.frag
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +/*
> + * Copyright (C) 2020, Linaro
> + *
> + * bayer_12_packed.frag - Fragment shader code for raw Bayer 12-bit packed
> + * format
> + */
> +
> +#ifdef GL_ES
> +precision mediump float;
> +#endif
> +
> +varying vec4 center;
> +varying vec2 xcoords;
> +varying vec2 ycoords;

If I understand correctly, center.xy stores the texture coordinates of
the pixel, while xcoords (ycoords) stores the horizontal (vertical)
coordinates of the previous and next pixels. Is that correct ? If so, I
wonder if we shouldn't instead declare xcoords and ycoords as vec3, to
store the coordinates of the previous, current and next pixels. The code
below could become, for instance,

	vec4 vals_BCD = vec4(
		texture2D(tex_raw, vec2(xcoords[1], ycoords[1]).rg,
		texture2D(tex_raw, vec2(xcoords[0], ycoords[1])).g,
		texture2D(tex_raw, vec2(xcoords[2], ycoords[1])).r);

> +
> +uniform sampler2D tex_raw;
> +
> +void main(void)
> +{
> +	vec3 rgb;
> +
> +	vec2 alternate = mod(center.zw, 2.0);
> +
> +	bool even_col = alternate.x < 1.0;
> +	bool even_raw = alternate.y < 1.0;
> +
> +	/* .xy = (0,-1).rg, zw = (0, 1).rg */
> +	vec4 vals_AD = vec4(
> +		texture2D(tex_raw, vec2(center.x, ycoords[0])).rg,
> +		texture2D(tex_raw, vec2(center.x, ycoords[1])).rg);
> +	/* .xy = (0,0).rg, .z = (-1,0).g, .w = (1,0).r */
> +	vec4 vals_BCD = vec4(
> +		texture2D(tex_raw, center.xy).rg,
> +		texture2D(tex_raw, vec2(xcoords[0], center.y)).g,
> +		texture2D(tex_raw, vec2(xcoords[1], center.y)).r);
> +	/* .x = (-1,-1).g, .y = (-1,1).g, .z = (1,-1).r, .w = (1,1).r */
> +	vec4 vals_D = vec4(
> +		texture2D(tex_raw, vec2(xcoords[0], ycoords[0])).g,
> +		texture2D(tex_raw, vec2(xcoords[0], ycoords[1])).g,
> +		texture2D(tex_raw, vec2(xcoords[1], ycoords[0])).r,
> +		texture2D(tex_raw, vec2(xcoords[1], ycoords[1])).r);
> +
> +	vec4 EFGH = vec4(
> +		vals_AD.x + vals_AD.z,		/* 2*E = (0,-1).r + (0, 1).r */
> +		vals_AD.y + vals_AD.w, 		/* 2*F = (0,-1).g + (0, 1).g */
> +		vals_BCD.y + vals_BCD.z,	/* 2*G = (0,0).g + (-1,0).g */
> +		vals_BCD.x + vals_BCD.w		/* 2*H = (0,0).r + (1,0).r */
> +		) / 2.0;
> +	vec2 JK = vec2(
> +		vals_D.x + vals_D.y,		/* 2*J = (-1,-1).g + (-1,1).g */
> +		vals_D.z + vals_D.w		/* 2*K = (1,-1).r + (1,1).r */
> +		) / 2.0;
> +
> +	if (even_col) {
> +		rgb = (even_raw) ?
> +			vec3(vals_BCD.x, (EFGH.x + EFGH.z) / 2.0,
> +			     (EFGH.y + JK.x) / 2.0) :
> +			vec3(EFGH.x, vals_BCD.x, EFGH.z);
> +	} else {
> +		rgb = (even_raw) ?
> +			vec3(EFGH.w, vals_BCD.y, EFGH.y) :
> +			vec3((EFGH.x + JK.y) / 2.0,
> +			     (EFGH.y + EFGH.w) / 2.0,
> +			     vals_BCD.y);
> +	}
> +	gl_FragColor = vec4(rgb, 1.0);

I'm having a very hard time reviewing this as ABCDEFGHJK are not very
descriptive names... Could this be improved, both with better variable
names, and a comment to explain the logic ?

> +}
> diff --git a/src/qcam/assets/shader/shaders.qrc b/src/qcam/assets/shader/shaders.qrc
> index 8a8f9de1..7bb18033 100644
> --- a/src/qcam/assets/shader/shaders.qrc
> +++ b/src/qcam/assets/shader/shaders.qrc
> @@ -5,6 +5,8 @@
>  	<file>YUV_2_planes.frag</file>
>  	<file>YUV_3_planes.frag</file>
>  	<file>YUV_packed.frag</file>
> +	<file>bayer_12_packed.frag</file>
> +	<file>bayer.vert</file>
>  	<file>identity.vert</file>
>  </qresource>
>  </RCC>
> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> index c74ce77b..e504a6c2 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 12-bit packed */
> +	libcamera::formats::SBGGR12_CSI2P,
> +	libcamera::formats::SGBRG12_CSI2P,
> +	libcamera::formats::SGRBG12_CSI2P,
> +	libcamera::formats::SRGGB12_CSI2P,
>  };
>  
>  ViewFinderGL::ViewFinderGL(QWidget *parent)
> @@ -115,6 +120,7 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)
>  	bool ret = true;
>  
>  	vertexShaderFile_ = ":identity.vert";
> +	textureMinMaxFilters_ = GL_LINEAR;
>  
>  	fragmentShaderDefines_.clear();
>  
> @@ -205,6 +211,34 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)
>  		fragmentShaderDefines_.append("#define RGB_PATTERN bgr");
>  		fragmentShaderFile_ = ":RGB.frag";
>  		break;
> +	case libcamera::formats::SBGGR12_CSI2P:
> +		firstRed_[0] = 1.0;
> +		firstRed_[1] = 1.0;
> +		fragmentShaderFile_ = ":bayer_12_packed.frag";
> +		vertexShaderFile_ = ":bayer.vert";
> +		textureMinMaxFilters_ = GL_NEAREST;
> +		break;
> +	case libcamera::formats::SGBRG12_CSI2P:
> +		firstRed_[0] = 0.0;
> +		firstRed_[1] = 1.0;
> +		fragmentShaderFile_ = ":bayer_12_packed.frag";
> +		vertexShaderFile_ = ":bayer.vert";
> +		textureMinMaxFilters_ = GL_NEAREST;
> +		break;
> +	case libcamera::formats::SGRBG12_CSI2P:
> +		firstRed_[0] = 1.0;
> +		firstRed_[1] = 0.0;
> +		fragmentShaderFile_ = ":bayer_12_packed.frag";
> +		vertexShaderFile_ = ":bayer.vert";
> +		textureMinMaxFilters_ = GL_NEAREST;
> +		break;
> +	case libcamera::formats::SRGGB12_CSI2P:
> +		firstRed_[0] = 0.0;
> +		firstRed_[1] = 0.0;
> +		fragmentShaderFile_ = ":bayer_12_packed.frag";
> +		vertexShaderFile_ = ":bayer.vert";
> +		textureMinMaxFilters_ = GL_NEAREST;

At some point it may make sense to store all this in an array of format
info structures, but that's not a candidate for this patch.

> +		break;
>  	default:
>  		ret = false;
>  		qWarning() << "[ViewFinderGL]:"
> @@ -292,6 +326,9 @@ bool ViewFinderGL::createFragmentShader()
>  	textureUniformU_ = shaderProgram_.uniformLocation("tex_u");
>  	textureUniformV_ = shaderProgram_.uniformLocation("tex_v");
>  	textureUniformStepX_ = shaderProgram_.uniformLocation("tex_stepx");
> +	textureUniformRaw_ = shaderProgram_.uniformLocation("tex_raw");

Could we reuse textureUniformY_ as we do for RGB formats ?

> +	textureUniformSrcSize_ = shaderProgram_.uniformLocation("srcSize");
> +	textureUniformFirstRed_ = shaderProgram_.uniformLocation("firstRed");

How about naming these "tex_bayer_size" and "tex_bayer_first_red" to
match the naming scheme of other uniforms ?

>  
>  	/* Create the textures. */
>  	for (std::unique_ptr<QOpenGLTexture> &texture : textures_) {
> @@ -308,8 +345,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,
> +			textureMinMaxFilters_);

Should this be called textureMinMagFilters_ (min stands for minifying
and mag for magnification according to
https://www.khronos.org/registry/OpenGL-Refpages/es2.0/xhtml/glTexParameter.xml)
? Or just textureFilter_ as both are identical in this implementation ?

> +	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER,
> +			textureMinMaxFilters_);
>  	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
>  	glTexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
>  }
> @@ -548,6 +587,37 @@ void ViewFinderGL::doRender()
>  		shaderProgram_.setUniformValue(textureUniformY_, 0);
>  		break;
>  
> +	case libcamera::formats::SBGGR12_CSI2P:
> +	case libcamera::formats::SGBRG12_CSI2P:
> +	case libcamera::formats::SGRBG12_CSI2P:
> +	case libcamera::formats::SRGGB12_CSI2P:
> +		/*
> +		 * Packed raw Bayer 12-bit foramts are stored in RGB texture

s/foramts/formats/

> +		 * to match the OpenGL texel size with the 3 bytes repeating
> +		 * pattern in RAW12P.
> +		 * The texture width is thus half of the image with.

I dread to think how we'll handle packed RAW10, as we'll need 5 bytes
for 4 pixels. I suppose we'll need to use GL_R with an even more
complicated fragment shader :-S

> +		 */
> +		glActiveTexture(GL_TEXTURE0);
> +		configureTexture(*textures_[0]);
> +		glTexImage2D(GL_TEXTURE_2D,
> +			     0,
> +			     GL_RGB,
> +			     size_.width() / 2,
> +			     size_.height(),
> +			     0,
> +			     GL_RGB,
> +			     GL_UNSIGNED_BYTE,
> +			     data_);
> +		shaderProgram_.setUniformValue(textureUniformRaw_, 0);
> +		shaderProgram_.setUniformValue(textureUniformFirstRed_,
> +					       firstRed_[0], firstRed_[1]);
> +		shaderProgram_.setUniformValue(textureUniformSrcSize_,
> +					       size_.width(),
> +					       size_.height(),
> +					       1.0f / (size_.width() / 2 - 1),
> +					       1.0f / (size_.height() - 1));

Instead of bundling the two together, how about turning the float
tex_stepx into a vec2 tex_step, and using it to convey the step in all
cases ? The size could be stored in a separate uniform called tex_size.
This would make the different shaders a bit more consistent, which
should improve readability and maintainability.

> +		break;
> +
>  	default:
>  		break;
>  	};
> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h
> index 6cf8f347..186492f3 100644
> --- a/src/qcam/viewfinder_gl.h
> +++ b/src/qcam/viewfinder_gl.h
> @@ -82,6 +82,8 @@ private:
>  	/* Textures */
>  	std::array<std::unique_ptr<QOpenGLTexture>, 3> textures_;
>  
> +	/* Common texture parameters */
> +	GLuint textureMinMaxFilters_;
>  	/* YUV texture parameters */
>  	GLuint textureUniformU_;
>  	GLuint textureUniformV_;
> @@ -89,6 +91,11 @@ private:
>  	GLuint textureUniformStepX_;
>  	unsigned int horzSubSample_;
>  	unsigned int vertSubSample_;
> +	/* Raw Bayer texture parameters */
> +	GLuint textureUniformRaw_;
> +	GLuint textureUniformSrcSize_;
> +	GLuint textureUniformFirstRed_;
> +	GLfloat firstRed_[2];

Would the code look a bit easier to read if we stored this as

	QPointF firstRed_;

or

? QOpenGLShaderProgram::setUniformValue() has an overload that takes a
QPointF.

>  	QMutex mutex_; /* Prevent concurrent access to image_ */
>  };