[libcamera-devel] [PATCH 6/7] qcam: viewfinder_gl: Store textures in an array

[Andrey Konovalov]

Hi Laurent,

Thank you for the patch!

On 03.11.2020 18:50, Laurent Pinchart wrote:
> In preparation for RGB formats support, store the three Y, U and V
> textures in an array. This makes the code more generic, and will avoid
> referring to an RGB texture as textureY_.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>   src/qcam/viewfinder_gl.cpp | 37 +++++++++++++++++--------------------
>   src/qcam/viewfinder_gl.h   |  5 ++---
>   2 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> index e6625cac9795..cbc1365500f5 100644
> --- a/src/qcam/viewfinder_gl.cpp
> +++ b/src/qcam/viewfinder_gl.cpp
> @@ -33,10 +33,7 @@ static const QList<libcamera::PixelFormat> supportedFormats{
>   
>   ViewFinderGL::ViewFinderGL(QWidget *parent)
>   	: QOpenGLWidget(parent), buffer_(nullptr), data_(nullptr),
> -	  vertexBuffer_(QOpenGLBuffer::VertexBuffer),
> -	  textureU_(QOpenGLTexture::Target2D),
> -	  textureV_(QOpenGLTexture::Target2D),
> -	  textureY_(QOpenGLTexture::Target2D)
> +	  vertexBuffer_(QOpenGLBuffer::VertexBuffer)
>   {
>   }
>   
> @@ -263,14 +260,14 @@ bool ViewFinderGL::createFragmentShader()
>   	textureUniformV_ = shaderProgram_.uniformLocation("tex_v");
>   	textureUniformStepX_ = shaderProgram_.uniformLocation("tex_stepx");
>   
> -	if (!textureY_.isCreated())
> -		textureY_.create();
> +	/* Create the textures. */
> +	for (std::unique_ptr<QOpenGLTexture> &texture : textures_) {
> +		if (texture)
> +			continue;
>   
> -	if (!textureU_.isCreated())
> -		textureU_.create();
> -
> -	if (!textureV_.isCreated())
> -		textureV_.create();
> +		texture = std::make_unique<QOpenGLTexture>(QOpenGLTexture::Target2D);
> +		texture->create();
> +	}
>   
>   	return true;
>   }
> @@ -337,7 +334,7 @@ void ViewFinderGL::doRender()
>   	case libcamera::formats::NV42:
>   		/* Activate texture Y */
>   		glActiveTexture(GL_TEXTURE0);
> -		configureTexture(textureY_);
> +		configureTexture(*textures_[0]);
>   		glTexImage2D(GL_TEXTURE_2D,
>   			     0,
>   			     GL_RED,
> @@ -351,7 +348,7 @@ void ViewFinderGL::doRender()
>   
>   		/* Activate texture UV/VU */
>   		glActiveTexture(GL_TEXTURE1);
> -		configureTexture(textureU_);
> +		configureTexture(*textures_[1]);
>   		glTexImage2D(GL_TEXTURE_2D,
>   			     0,
>   			     GL_RG,
> @@ -367,7 +364,7 @@ void ViewFinderGL::doRender()
>   	case libcamera::formats::YUV420:
>   		/* Activate texture Y */
>   		glActiveTexture(GL_TEXTURE0);
> -		configureTexture(textureY_);
> +		configureTexture(*textures_[0]);
>   		glTexImage2D(GL_TEXTURE_2D,
>   			     0,
>   			     GL_RED,
> @@ -381,7 +378,7 @@ void ViewFinderGL::doRender()
>   
>   		/* Activate texture U */
>   		glActiveTexture(GL_TEXTURE1);
> -		configureTexture(textureU_);
> +		configureTexture(*textures_[1]);
>   		glTexImage2D(GL_TEXTURE_2D,
>   			     0,
>   			     GL_RED,
> @@ -395,7 +392,7 @@ void ViewFinderGL::doRender()
>   
>   		/* Activate texture V */
>   		glActiveTexture(GL_TEXTURE2);
> -		configureTexture(textureV_);
> +		configureTexture(*textures_[2]);
>   		glTexImage2D(GL_TEXTURE_2D,
>   			     0,
>   			     GL_RED,
> @@ -411,7 +408,7 @@ void ViewFinderGL::doRender()
>   	case libcamera::formats::YVU420:
>   		/* Activate texture Y */
>   		glActiveTexture(GL_TEXTURE0);
> -		configureTexture(textureY_);
> +		configureTexture(*textures_[0]);
>   		glTexImage2D(GL_TEXTURE_2D,
>   			     0,
>   			     GL_RED,
> @@ -425,7 +422,7 @@ void ViewFinderGL::doRender()
>   
>   		/* Activate texture V */
>   		glActiveTexture(GL_TEXTURE2);
> -		configureTexture(textureV_);
> +		configureTexture(*textures_[2]);
>   		glTexImage2D(GL_TEXTURE_2D,
>   			     0,
>   			     GL_RED,
> @@ -439,7 +436,7 @@ void ViewFinderGL::doRender()
>   
>   		/* Activate texture U */
>   		glActiveTexture(GL_TEXTURE1);
> -		configureTexture(textureU_);
> +		configureTexture(*textures_[1]);
>   		glTexImage2D(GL_TEXTURE_2D,
>   			     0,
>   			     GL_RED,
> @@ -462,7 +459,7 @@ void ViewFinderGL::doRender()
>   		 * The texture width is thus half of the image with.
>   		 */
>   		glActiveTexture(GL_TEXTURE0);
> -		configureTexture(textureY_);
> +		configureTexture(*textures_[0]);
>   		glTexImage2D(GL_TEXTURE_2D,
>   			     0,
>   			     GL_RGBA,
> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h
> index b3e36514d3d4..17c824a69e39 100644
> --- a/src/qcam/viewfinder_gl.h
> +++ b/src/qcam/viewfinder_gl.h
> @@ -8,6 +8,7 @@
>   #ifndef __VIEWFINDER_GL_H__
>   #define __VIEWFINDER_GL_H__
>   
> +#include <array>
>   #include <memory>
>   
>   #include <QImage>
> @@ -82,9 +83,7 @@ private:

These two lines are not visible in the diff:

         /* YUV texture planars and parameters */
         GLuint textureUniformU_;
>   	GLuint textureUniformV_;
>   	GLuint textureUniformY_;
>   	GLuint textureUniformStepX_;
> -	QOpenGLTexture textureU_;
> -	QOpenGLTexture textureV_;
> -	QOpenGLTexture textureY_;
> +	std::array<std::unique_ptr<QOpenGLTexture>, 3> textures_;

- now the textures_ should be better moved outside the /* YUV texture planars and parameters */ block

Other than that

Reviewed-by: Andrey Konovalov <andrey.konovalov at linaro.org>

Thanks,
Andrey

>   	unsigned int horzSubSample_;
>   	unsigned int vertSubSample_;
>   
>