From d7d0f4d5a09d23488d4905bd5ae6b3e0f04d17a9 Mon Sep 17 00:00:00 2001 From: Laszlo Agocs Date: Sat, 12 Sep 2020 22:02:05 +0200 Subject: [PATCH] rhi: gl: Eliminate duplicate glUniform calls This can happen when there is a uniform block in the Vulkan shader with an instance name, both in the vertex and fragment shader. The members are mapped to members of a struct uniform in GLSL since we do not use uniform buffers there. Now if there is, for example, "ubuf.opacity" both in the vertex and fragment shader reflection info, then it's still just one single uniform in the GL program, using one location. Registering it twice in our 'uniforms' list is harmless, but wasteful, since it means the uniform value will be set twice upon each setShaderResources(). Change-Id: Ib646eaec333522560d631b3b81800ef610f09319 Reviewed-by: Andy Nichols --- src/gui/rhi/qrhigles2.cpp | 28 +++++++++++++++++++++------- src/gui/rhi/qrhigles2_p_p.h | 6 ++++-- 2 files changed, 25 insertions(+), 9 deletions(-) diff --git a/src/gui/rhi/qrhigles2.cpp b/src/gui/rhi/qrhigles2.cpp index a06cec2b389..72a8cc539b5 100644 --- a/src/gui/rhi/qrhigles2.cpp +++ b/src/gui/rhi/qrhigles2.cpp @@ -3447,6 +3447,7 @@ void QRhiGles2::registerUniformIfActive(const QShaderDescription::BlockVariable int binding, int baseOffset, GLuint program, + QSet *activeUniformLocations, QGles2UniformDescriptionVector *dst) { if (var.type == QShaderDescription::Struct) { @@ -3457,8 +3458,14 @@ void QRhiGles2::registerUniformIfActive(const QShaderDescription::BlockVariable QGles2UniformDescription uniform; uniform.type = var.type; const QByteArray name = namePrefix + var.name; + // Here we expect that the OpenGL implementation has proper active uniform + // handling, meaning that a uniform that is declared but not accessed + // elsewhere in the code is reported as -1 when querying the location. If + // that is not the case, it won't break anything, but we'll generate + // unnecessary glUniform* calls then. uniform.glslLocation = f->glGetUniformLocation(program, name.constData()); - if (uniform.glslLocation >= 0) { + if (uniform.glslLocation >= 0 && !activeUniformLocations->contains(uniform.glslLocation)) { + activeUniformLocations->insert(uniform.glslLocation); if (var.arrayDims.count() > 1) { qWarning("Array '%s' has more than one dimension. This is not supported.", var.name.constData()); @@ -3474,6 +3481,7 @@ void QRhiGles2::registerUniformIfActive(const QShaderDescription::BlockVariable void QRhiGles2::gatherUniforms(GLuint program, const QShaderDescription::UniformBlock &ub, + QSet *activeUniformLocations, QGles2UniformDescriptionVector *dst) { QByteArray prefix = ub.structName + '.'; @@ -3485,7 +3493,7 @@ void QRhiGles2::gatherUniforms(GLuint program, if (blockMember.arrayDims.isEmpty()) { for (const QShaderDescription::BlockVariable &structMember : blockMember.structMembers) registerUniformIfActive(structMember, structPrefix + ".", ub.binding, - baseOffset, program, dst); + baseOffset, program, activeUniformLocations, dst); } else { if (blockMember.arrayDims.count() > 1) { qWarning("Array of struct '%s' has more than one dimension. Only the first " @@ -3498,12 +3506,12 @@ void QRhiGles2::gatherUniforms(GLuint program, for (int di = 0; di < dim; ++di) { const QByteArray arrayPrefix = structPrefix + '[' + QByteArray::number(di) + ']' + '.'; for (const QShaderDescription::BlockVariable &structMember : blockMember.structMembers) - registerUniformIfActive(structMember, arrayPrefix, ub.binding, elemOffset, program, dst); + registerUniformIfActive(structMember, arrayPrefix, ub.binding, elemOffset, program, activeUniformLocations, dst); elemOffset += elemSize; } } } else { - registerUniformIfActive(blockMember, prefix, ub.binding, 0, program, dst); + registerUniformIfActive(blockMember, prefix, ub.binding, 0, program, activeUniformLocations, dst); } } } @@ -4328,11 +4336,16 @@ bool QGles2GraphicsPipeline::create() rhiD->trySaveToDiskCache(program, diskCacheKey); } + // Use the same work area for the vertex & fragment stages, thus ensuring + // that we will not do superfluous glUniform calls for uniforms that are + // present in both shaders. + QSet activeUniformLocations; + for (const QShaderDescription::UniformBlock &ub : vsDesc.uniformBlocks()) - rhiD->gatherUniforms(program, ub, &uniforms); + rhiD->gatherUniforms(program, ub, &activeUniformLocations, &uniforms); for (const QShaderDescription::UniformBlock &ub : fsDesc.uniformBlocks()) - rhiD->gatherUniforms(program, ub, &uniforms); + rhiD->gatherUniforms(program, ub, &activeUniformLocations, &uniforms); for (const QShaderDescription::InOutVariable &v : vsDesc.combinedImageSamplers()) rhiD->gatherSamplers(program, v, &samplers); @@ -4403,8 +4416,9 @@ bool QGles2ComputePipeline::create() rhiD->trySaveToDiskCache(program, diskCacheKey); } + QSet activeUniformLocations; for (const QShaderDescription::UniformBlock &ub : csDesc.uniformBlocks()) - rhiD->gatherUniforms(program, ub, &uniforms); + rhiD->gatherUniforms(program, ub, &activeUniformLocations, &uniforms); for (const QShaderDescription::InOutVariable &v : csDesc.combinedImageSamplers()) rhiD->gatherSamplers(program, v, &samplers); diff --git a/src/gui/rhi/qrhigles2_p_p.h b/src/gui/rhi/qrhigles2_p_p.h index 93c8f82f8d6..9392254a785 100644 --- a/src/gui/rhi/qrhigles2_p_p.h +++ b/src/gui/rhi/qrhigles2_p_p.h @@ -813,9 +813,11 @@ public: bool linkProgram(GLuint program); void registerUniformIfActive(const QShaderDescription::BlockVariable &var, const QByteArray &namePrefix, int binding, int baseOffset, - GLuint program, QGles2UniformDescriptionVector *dst); + GLuint program, + QSet *activeUniformLocations, + QGles2UniformDescriptionVector *dst); void gatherUniforms(GLuint program, const QShaderDescription::UniformBlock &ub, - QGles2UniformDescriptionVector *dst); + QSet *activeUniformLocations, QGles2UniformDescriptionVector *dst); void gatherSamplers(GLuint program, const QShaderDescription::InOutVariable &v, QGles2SamplerDescriptionVector *dst); bool isProgramBinaryDiskCacheEnabled() const;