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

[Kieran Bingham]

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 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>

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

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


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

-- 
Regards
--
Kieran