[libcamera-devel] [PATCH 1/7] qcam: viewfinder_gl: Fix fragment shader rebuild when setting format

[Kieran Bingham]

On 03/11/2020 15:50, Laurent Pinchart wrote:
> When setting a new format, the existing fragment shader is deleted and a
> new shader should be created. However, the shader pointer isn't set to
> nullptr after deleting it, resulting in the deleter shader being reused.
> Fix it by managing shader pointers with std::unique_ptr<> to prevent
> similar bugs from happening in the future.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

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


> ---
>  src/qcam/viewfinder_gl.cpp | 19 ++++++-------------
>  src/qcam/viewfinder_gl.h   |  6 ++++--
>  2 files changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/src/qcam/viewfinder_gl.cpp b/src/qcam/viewfinder_gl.cpp
> index 0b5c942658cd..03a576ba89d2 100644
> --- a/src/qcam/viewfinder_gl.cpp
> +++ b/src/qcam/viewfinder_gl.cpp
> @@ -33,7 +33,6 @@ static const QList<libcamera::PixelFormat> supportedFormats{
>  
>  ViewFinderGL::ViewFinderGL(QWidget *parent)
>  	: QOpenGLWidget(parent), buffer_(nullptr), yuvData_(nullptr),
> -	  vertexShader_(nullptr), fragmentShader_(nullptr),
>  	  vertexBuffer_(QOpenGLBuffer::VertexBuffer),
>  	  textureU_(QOpenGLTexture::Target2D),
>  	  textureV_(QOpenGLTexture::Target2D),
> @@ -58,8 +57,8 @@ int ViewFinderGL::setFormat(const libcamera::PixelFormat &format,
>  	if (fragmentShader_) {
>  		if (shaderProgram_.isLinked()) {
>  			shaderProgram_.release();
> -			shaderProgram_.removeShader(fragmentShader_);
> -			delete fragmentShader_;
> +			shaderProgram_.removeShader(fragmentShader_.get());
> +			fragmentShader_.reset();
>  		}
>  	}
>  
> @@ -185,7 +184,7 @@ bool ViewFinderGL::selectFormat(const libcamera::PixelFormat &format)
>  bool ViewFinderGL::createVertexShader()
>  {
>  	/* Create Vertex Shader */
> -	vertexShader_ = new QOpenGLShader(QOpenGLShader::Vertex, this);
> +	vertexShader_ = std::make_unique<QOpenGLShader>(QOpenGLShader::Vertex, this);
>  
>  	/* Compile the vertex shader */
>  	if (!vertexShader_->compileSourceFile(":YUV.vert")) {
> @@ -193,7 +192,7 @@ bool ViewFinderGL::createVertexShader()
>  		return false;
>  	}
>  
> -	shaderProgram_.addShader(vertexShader_);
> +	shaderProgram_.addShader(vertexShader_.get());
>  	return true;
>  }
>  
> @@ -207,7 +206,7 @@ bool ViewFinderGL::createFragmentShader()
>  	 * program. The #define macros stored in fragmentShaderDefines_, if
>  	 * any, are prepended to the source code.
>  	 */
> -	fragmentShader_ = new QOpenGLShader(QOpenGLShader::Fragment, this);
> +	fragmentShader_ = std::make_unique<QOpenGLShader>(QOpenGLShader::Fragment, this);
>  
>  	QFile file(fragmentShaderFile_);
>  	if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) {
> @@ -224,7 +223,7 @@ bool ViewFinderGL::createFragmentShader()
>  		return false;
>  	}
>  
> -	shaderProgram_.addShader(fragmentShader_);
> +	shaderProgram_.addShader(fragmentShader_.get());
>  
>  	/* Link shader pipeline */
>  	if (!shaderProgram_.link()) {
> @@ -287,12 +286,6 @@ void ViewFinderGL::removeShader()
>  		shaderProgram_.release();
>  		shaderProgram_.removeAllShaders();
>  	}
> -
> -	if (fragmentShader_)
> -		delete fragmentShader_;
> -
> -	if (vertexShader_)
> -		delete vertexShader_;
>  }
>  
>  void ViewFinderGL::initializeGL()
> diff --git a/src/qcam/viewfinder_gl.h b/src/qcam/viewfinder_gl.h
> index ad1e195e45c7..40c04dc5f877 100644
> --- a/src/qcam/viewfinder_gl.h
> +++ b/src/qcam/viewfinder_gl.h
> @@ -8,6 +8,8 @@
>  #ifndef __VIEWFINDER_GL_H__
>  #define __VIEWFINDER_GL_H__
>  
> +#include <memory>
> +
>  #include <QImage>
>  #include <QMutex>
>  #include <QOpenGLBuffer>
> @@ -67,8 +69,8 @@ private:
>  
>  	/* Shaders */
>  	QOpenGLShaderProgram shaderProgram_;
> -	QOpenGLShader *vertexShader_;
> -	QOpenGLShader *fragmentShader_;
> +	std::unique_ptr<QOpenGLShader> vertexShader_;
> +	std::unique_ptr<QOpenGLShader> fragmentShader_;
>  	QString fragmentShaderFile_;
>  	QStringList fragmentShaderDefines_;
>  
> 

-- 
Regards
--
Kieran