[libcamera-devel,6/7] qcam: viewfinder_gl: Store textures in an array
diff mbox series

Message ID 20201103155025.5948-7-laurent.pinchart@ideasonboard.com
State Accepted
Headers show
Series
  • qcam: Miscellaneous shaders cleanups and RGB support
Related show

Commit Message

Laurent Pinchart Nov. 3, 2020, 3:50 p.m. UTC 
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@ideasonboard.com>
---
 src/qcam/viewfinder_gl.cpp | 37 +++++++++++++++++--------------------
 src/qcam/viewfinder_gl.h   |  5 ++---
 2 files changed, 19 insertions(+), 23 deletions(-)

Comments

Andrey Konovalov Nov. 3, 2020, 7:23 p.m. UTC | #1 
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@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@linaro.org>

Thanks,
Andrey

>   	unsigned int horzSubSample_;
>   	unsigned int vertSubSample_;
>   
>
Kieran Bingham Nov. 4, 2020, 10:39 a.m. UTC | #2 
Hi Laurent,

On 03/11/2020 19:23, Andrey Konovalov wrote:
> 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_.

Definitely helpful ;-)


>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@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@linaro.org>

Yup, organised however best fits - storing TEXTURE0 in textures_[0]
makes me quite happy ;-)

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


> 
> Thanks,
> Andrey
> 
>>       unsigned int horzSubSample_;
>>       unsigned int vertSubSample_;
>>  
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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:
 	GLuint textureUniformV_;
 	GLuint textureUniformY_;
 	GLuint textureUniformStepX_;
-	QOpenGLTexture textureU_;
-	QOpenGLTexture textureV_;
-	QOpenGLTexture textureY_;
+	std::array<std::unique_ptr<QOpenGLTexture>, 3> textures_;
 	unsigned int horzSubSample_;
 	unsigned int vertSubSample_;