[libcamera-devel,1/7] qcam: viewfinder_gl: Fix fragment shader rebuild when setting format
diff mbox series

Message ID 20201103155025.5948-2-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 
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@ideasonboard.com>
---
 src/qcam/viewfinder_gl.cpp | 19 ++++++-------------
 src/qcam/viewfinder_gl.h   |  6 ++++--
 2 files changed, 10 insertions(+), 15 deletions(-)

Comments

Kieran Bingham Nov. 3, 2020, 10:49 p.m. UTC | #1 
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@ideasonboard.com>

Reviewed-by: Kieran Bingham <kieran.bingham@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_;
>  
>
Niklas Söderlund Nov. 6, 2020, 12:24 a.m. UTC | #2 
Hi Laurent,

Thanks for your patch.

On 2020-11-03 17:50:19 +0200, 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@ideasonboard.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
>  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,
> 
> Laurent Pinchart
> 
> _______________________________________________
> 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 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_;