| Message ID | 20260630125214.3327516-6-laurent.pinchart@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
On 30/06/2026 13:52, Laurent Pinchart wrote: > The compileVertexShader() and compileFragmentShader() functions take a > pointer and length to the shader source code as separate arguments. This > is an error-prone practice. Replace them with a Span<>. > > Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- bod
2026. 06. 30. 14:52 keltezéssel, Laurent Pinchart írta: > The compileVertexShader() and compileFragmentShader() functions take a > pointer and length to the shader source code as separate arguments. This > is an error-prone practice. Replace them with a Span<>. > > Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- Looks ok to me. Reviewed-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > include/libcamera/internal/egl.h | 9 +++---- > src/libcamera/egl.cpp | 29 ++++++++++------------ > src/libcamera/software_isp/debayer_egl.cpp | 8 +++--- > 3 files changed, 20 insertions(+), 26 deletions(-) > > diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h > index f7bfb28d4e9a..523a62aa89d5 100644 > --- a/include/libcamera/internal/egl.h > +++ b/include/libcamera/internal/egl.h > @@ -112,11 +112,9 @@ public: > void pushEnv(std::vector<std::string> &shaderEnv, const char *str); > void makeCurrent(); > > - int compileVertexShader(GLuint &shaderId, const unsigned char *shaderData, > - unsigned int shaderDataLen, > + int compileVertexShader(GLuint &shaderId, Span<const unsigned char> shaderData, > Span<const std::string> shaderEnv); > - int compileFragmentShader(GLuint &shaderId, const unsigned char *shaderData, > - unsigned int shaderDataLen, > + int compileFragmentShader(GLuint &shaderId, Span<const unsigned char> shaderData, > Span<const std::string> shaderEnv); > int linkProgram(GLuint &programId, GLuint fragmentshaderId, GLuint vertexshaderId); > void dumpShaderSource(GLuint shaderId); > @@ -135,8 +133,7 @@ private: > EGLSurface surface_ = EGL_NO_SURFACE; > > static EGLDisplay probeDisplay(); > - int compileShader(int shaderType, GLuint &shaderId, const unsigned char *shaderData, > - unsigned int shaderDataLen, > + int compileShader(int shaderType, GLuint &shaderId, Span<const unsigned char> shaderData, > Span<const std::string> shaderEnv); > > int createDMABufTexture2D(eGLImage &eglImage, int fd, bool output); > diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp > index f03abb8ae07d..b5a5dc60b110 100644 > --- a/src/libcamera/egl.cpp > +++ b/src/libcamera/egl.cpp > @@ -463,8 +463,7 @@ void eGL::pushEnv(std::vector<std::string> &shaderEnv, const char *str) > /** > * \brief Compile a vertex shader > * \param[out] shaderId OpenGL shader object ID > - * \param[in] shaderData Pointer to shader source code > - * \param[in] shaderDataLen Length of shader source in bytes > + * \param[in] shaderData Shader source code > * \param[in] shaderEnv Span of preprocessor definitions to prepend > * > * Compiles a vertex shader from source code with optional preprocessor > @@ -472,18 +471,17 @@ void eGL::pushEnv(std::vector<std::string> &shaderEnv, const char *str) > * > * \return 0 on success, or -EINVAL on compilation failure > */ > -int eGL::compileVertexShader(GLuint &shaderId, const unsigned char *shaderData, > - unsigned int shaderDataLen, > +int eGL::compileVertexShader(GLuint &shaderId, > + Span<const unsigned char> shaderData, > Span<const std::string> shaderEnv) > { > - return compileShader(GL_VERTEX_SHADER, shaderId, shaderData, shaderDataLen, shaderEnv); > + return compileShader(GL_VERTEX_SHADER, shaderId, shaderData, shaderEnv); > } > > /** > * \brief Compile a fragment shader > * \param[out] shaderId OpenGL shader object ID > - * \param[in] shaderData Pointer to shader source code > - * \param[in] shaderDataLen Length of shader source in bytes > + * \param[in] shaderData Shader source code > * \param[in] shaderEnv Span of preprocessor definitions to prepend > * > * Compiles a fragment shader from source code with optional preprocessor > @@ -491,19 +489,18 @@ int eGL::compileVertexShader(GLuint &shaderId, const unsigned char *shaderData, > * > * \return 0 on success, or -EINVAL on compilation failure > */ > -int eGL::compileFragmentShader(GLuint &shaderId, const unsigned char *shaderData, > - unsigned int shaderDataLen, > +int eGL::compileFragmentShader(GLuint &shaderId, > + Span<const unsigned char> shaderData, > Span<const std::string> shaderEnv) > { > - return compileShader(GL_FRAGMENT_SHADER, shaderId, shaderData, shaderDataLen, shaderEnv); > + return compileShader(GL_FRAGMENT_SHADER, shaderId, shaderData, shaderEnv); > } > > /** > * \brief Compile a shader of specified type > * \param[in] shaderType GL_VERTEX_SHADER or GL_FRAGMENT_SHADER > * \param[out] shaderId OpenGL shader object ID > - * \param[in] shaderData Pointer to shader source code > - * \param[in] shaderDataLen Length of shader source in bytes > + * \param[in] shaderData Shader source code > * \param[in] shaderEnv Span of preprocessor definitions to prepend > * > * Internal helper function for shader compilation. Prepends environment > @@ -511,8 +508,8 @@ int eGL::compileFragmentShader(GLuint &shaderId, const unsigned char *shaderData > * > * \return 0 on success, or -EINVAL on compilation failure > */ > -int eGL::compileShader(int shaderType, GLuint &shaderId, const unsigned char *shaderData, > - unsigned int shaderDataLen, > +int eGL::compileShader(int shaderType, GLuint &shaderId, > + Span<const unsigned char> shaderData, > Span<const std::string> shaderEnv) > { > GLint success; > @@ -531,8 +528,8 @@ int eGL::compileShader(int shaderType, GLuint &shaderId, const unsigned char *sh > } > > // Now the main body of the shader program > - shaderSourceData[i] = reinterpret_cast<const GLchar *>(shaderData); > - shaderDataLengths[i] = shaderDataLen; > + shaderSourceData[i] = reinterpret_cast<const GLchar *>(shaderData.data()); > + shaderDataLengths[i] = shaderData.size(); > > // And create the shader > shaderId = glCreateShader(shaderType); > diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp > index 1f5fc6a4466d..56545816ead5 100644 > --- a/src/libcamera/software_isp/debayer_egl.cpp > +++ b/src/libcamera/software_isp/debayer_egl.cpp > @@ -251,15 +251,15 @@ int DebayerEGL::initBayerShaders(PixelFormat inputFormat, PixelFormat outputForm > break; > }; > > - if (egl_.compileVertexShader(vertexShaderId_, vertexShaderData.data(), > - vertexShaderData.size(), shaderEnv)) { > + if (egl_.compileVertexShader(vertexShaderId_, vertexShaderData, > + shaderEnv)) { > LOG(Debayer, Error) << "Compile vertex shader fail"; > return -ENODEV; > } > utils::scope_exit vShaderGuard([&] { glDeleteShader(vertexShaderId_); }); > > - if (egl_.compileFragmentShader(fragmentShaderId_, fragmentShaderData.data(), > - fragmentShaderData.size(), shaderEnv)) { > + if (egl_.compileFragmentShader(fragmentShaderId_, fragmentShaderData, > + shaderEnv)) { > LOG(Debayer, Error) << "Compile fragment shader fail"; > return -ENODEV; > }
diff --git a/include/libcamera/internal/egl.h b/include/libcamera/internal/egl.h index f7bfb28d4e9a..523a62aa89d5 100644 --- a/include/libcamera/internal/egl.h +++ b/include/libcamera/internal/egl.h @@ -112,11 +112,9 @@ public: void pushEnv(std::vector<std::string> &shaderEnv, const char *str); void makeCurrent(); - int compileVertexShader(GLuint &shaderId, const unsigned char *shaderData, - unsigned int shaderDataLen, + int compileVertexShader(GLuint &shaderId, Span<const unsigned char> shaderData, Span<const std::string> shaderEnv); - int compileFragmentShader(GLuint &shaderId, const unsigned char *shaderData, - unsigned int shaderDataLen, + int compileFragmentShader(GLuint &shaderId, Span<const unsigned char> shaderData, Span<const std::string> shaderEnv); int linkProgram(GLuint &programId, GLuint fragmentshaderId, GLuint vertexshaderId); void dumpShaderSource(GLuint shaderId); @@ -135,8 +133,7 @@ private: EGLSurface surface_ = EGL_NO_SURFACE; static EGLDisplay probeDisplay(); - int compileShader(int shaderType, GLuint &shaderId, const unsigned char *shaderData, - unsigned int shaderDataLen, + int compileShader(int shaderType, GLuint &shaderId, Span<const unsigned char> shaderData, Span<const std::string> shaderEnv); int createDMABufTexture2D(eGLImage &eglImage, int fd, bool output); diff --git a/src/libcamera/egl.cpp b/src/libcamera/egl.cpp index f03abb8ae07d..b5a5dc60b110 100644 --- a/src/libcamera/egl.cpp +++ b/src/libcamera/egl.cpp @@ -463,8 +463,7 @@ void eGL::pushEnv(std::vector<std::string> &shaderEnv, const char *str) /** * \brief Compile a vertex shader * \param[out] shaderId OpenGL shader object ID - * \param[in] shaderData Pointer to shader source code - * \param[in] shaderDataLen Length of shader source in bytes + * \param[in] shaderData Shader source code * \param[in] shaderEnv Span of preprocessor definitions to prepend * * Compiles a vertex shader from source code with optional preprocessor @@ -472,18 +471,17 @@ void eGL::pushEnv(std::vector<std::string> &shaderEnv, const char *str) * * \return 0 on success, or -EINVAL on compilation failure */ -int eGL::compileVertexShader(GLuint &shaderId, const unsigned char *shaderData, - unsigned int shaderDataLen, +int eGL::compileVertexShader(GLuint &shaderId, + Span<const unsigned char> shaderData, Span<const std::string> shaderEnv) { - return compileShader(GL_VERTEX_SHADER, shaderId, shaderData, shaderDataLen, shaderEnv); + return compileShader(GL_VERTEX_SHADER, shaderId, shaderData, shaderEnv); } /** * \brief Compile a fragment shader * \param[out] shaderId OpenGL shader object ID - * \param[in] shaderData Pointer to shader source code - * \param[in] shaderDataLen Length of shader source in bytes + * \param[in] shaderData Shader source code * \param[in] shaderEnv Span of preprocessor definitions to prepend * * Compiles a fragment shader from source code with optional preprocessor @@ -491,19 +489,18 @@ int eGL::compileVertexShader(GLuint &shaderId, const unsigned char *shaderData, * * \return 0 on success, or -EINVAL on compilation failure */ -int eGL::compileFragmentShader(GLuint &shaderId, const unsigned char *shaderData, - unsigned int shaderDataLen, +int eGL::compileFragmentShader(GLuint &shaderId, + Span<const unsigned char> shaderData, Span<const std::string> shaderEnv) { - return compileShader(GL_FRAGMENT_SHADER, shaderId, shaderData, shaderDataLen, shaderEnv); + return compileShader(GL_FRAGMENT_SHADER, shaderId, shaderData, shaderEnv); } /** * \brief Compile a shader of specified type * \param[in] shaderType GL_VERTEX_SHADER or GL_FRAGMENT_SHADER * \param[out] shaderId OpenGL shader object ID - * \param[in] shaderData Pointer to shader source code - * \param[in] shaderDataLen Length of shader source in bytes + * \param[in] shaderData Shader source code * \param[in] shaderEnv Span of preprocessor definitions to prepend * * Internal helper function for shader compilation. Prepends environment @@ -511,8 +508,8 @@ int eGL::compileFragmentShader(GLuint &shaderId, const unsigned char *shaderData * * \return 0 on success, or -EINVAL on compilation failure */ -int eGL::compileShader(int shaderType, GLuint &shaderId, const unsigned char *shaderData, - unsigned int shaderDataLen, +int eGL::compileShader(int shaderType, GLuint &shaderId, + Span<const unsigned char> shaderData, Span<const std::string> shaderEnv) { GLint success; @@ -531,8 +528,8 @@ int eGL::compileShader(int shaderType, GLuint &shaderId, const unsigned char *sh } // Now the main body of the shader program - shaderSourceData[i] = reinterpret_cast<const GLchar *>(shaderData); - shaderDataLengths[i] = shaderDataLen; + shaderSourceData[i] = reinterpret_cast<const GLchar *>(shaderData.data()); + shaderDataLengths[i] = shaderData.size(); // And create the shader shaderId = glCreateShader(shaderType); diff --git a/src/libcamera/software_isp/debayer_egl.cpp b/src/libcamera/software_isp/debayer_egl.cpp index 1f5fc6a4466d..56545816ead5 100644 --- a/src/libcamera/software_isp/debayer_egl.cpp +++ b/src/libcamera/software_isp/debayer_egl.cpp @@ -251,15 +251,15 @@ int DebayerEGL::initBayerShaders(PixelFormat inputFormat, PixelFormat outputForm break; }; - if (egl_.compileVertexShader(vertexShaderId_, vertexShaderData.data(), - vertexShaderData.size(), shaderEnv)) { + if (egl_.compileVertexShader(vertexShaderId_, vertexShaderData, + shaderEnv)) { LOG(Debayer, Error) << "Compile vertex shader fail"; return -ENODEV; } utils::scope_exit vShaderGuard([&] { glDeleteShader(vertexShaderId_); }); - if (egl_.compileFragmentShader(fragmentShaderId_, fragmentShaderData.data(), - fragmentShaderData.size(), shaderEnv)) { + if (egl_.compileFragmentShader(fragmentShaderId_, fragmentShaderData, + shaderEnv)) { LOG(Debayer, Error) << "Compile fragment shader fail"; return -ENODEV; }
The compileVertexShader() and compileFragmentShader() functions take a pointer and length to the shader source code as separate arguments. This is an error-prone practice. Replace them with a Span<>. Suggested-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- include/libcamera/internal/egl.h | 9 +++---- src/libcamera/egl.cpp | 29 ++++++++++------------ src/libcamera/software_isp/debayer_egl.cpp | 8 +++--- 3 files changed, 20 insertions(+), 26 deletions(-)