From 6bd83a9b4eacad5c10189ddf06f229860c4909bd Mon Sep 17 00:00:00 2001 From: johndoe6345789 Date: Wed, 7 Jan 2026 21:08:53 +0000 Subject: [PATCH] feat(shader): Enhance shader validation and refactor uniform handling in BgfxGuiService --- src/services/impl/bgfx_gui_service.cpp | 109 ++++++++++--- src/services/impl/bgfx_gui_service.hpp | 5 +- src/services/impl/bgfx_shader_compiler.cpp | 170 +++++++++++++++++++++ 3 files changed, 265 insertions(+), 19 deletions(-) diff --git a/src/services/impl/bgfx_gui_service.cpp b/src/services/impl/bgfx_gui_service.cpp index 93ec8d9..6e48543 100644 --- a/src/services/impl/bgfx_gui_service.cpp +++ b/src/services/impl/bgfx_gui_service.cpp @@ -298,7 +298,6 @@ void BgfxGuiService::InitializeResources() { if (configService_) { const auto& materialConfig = configService_->GetMaterialXConfig(); if (materialConfig.enabled && materialConfig.shaderKey == "gui") { - usingMaterialX = true; try { ShaderPaths generated = materialxGenerator_.Generate(materialConfig, {}); if (!generated.vertexSource.empty() && !generated.fragmentSource.empty()) { @@ -306,6 +305,7 @@ void BgfxGuiService::InitializeResources() { guiFragmentSourceOverride_ = std::move(generated.fragmentSource); vertexSource = guiVertexSourceOverride_.c_str(); fragmentSource = guiFragmentSourceOverride_.c_str(); + usingMaterialX = true; if (logger_) { logger_->Trace("BgfxGuiService", "InitializeResources", "Using MaterialX GUI shaders"); @@ -321,9 +321,13 @@ void BgfxGuiService::InitializeResources() { } } } - + + usesMaterialXShaders_ = usingMaterialX; + usesPredefinedModelViewProj_ = false; + modelViewProjUniform_ = BGFX_INVALID_HANDLE; + // Create uniforms matching the shader we're using - if (usingMaterialX) { + if (usesMaterialXShaders_) { // MaterialX shaders use separate world and viewProjection matrices worldMatrixUniform_ = bgfx::createUniform("u_worldMatrix", bgfx::UniformType::Mat4); viewProjMatrixUniform_ = bgfx::createUniform("u_viewProjectionMatrix", bgfx::UniformType::Mat4); @@ -334,13 +338,13 @@ void BgfxGuiService::InitializeResources() { ", sampler=" + std::to_string(bgfx::isValid(sampler_))); } } else { - // Built-in shader uses combined modelViewProj matrix - modelViewProjUniform_ = bgfx::createUniform("u_modelViewProj", bgfx::UniformType::Mat4); - if (logger_) { - logger_->Trace("BgfxGuiService", "InitializeResources", - "Built-in uniforms: modelViewProj=" + std::to_string(bgfx::isValid(modelViewProjUniform_)) + - ", sampler=" + std::to_string(bgfx::isValid(sampler_))); - } + modelViewProjUniform_ = BGFX_INVALID_HANDLE; + usesPredefinedModelViewProj_ = false; + } + + if (logger_) { + logger_->Trace("BgfxGuiService", "InitializeResources", + "GUI shader mode=" + std::string(usesMaterialXShaders_ ? "materialx" : "builtin")); } program_ = CreateProgram(vertexSource, fragmentSource); @@ -743,7 +747,7 @@ void BgfxGuiService::SubmitQuad(const GuiVertex& v0, "], color=[" + std::to_string(v0.r) + "," + std::to_string(v0.g) + "," + std::to_string(v0.b) + "," + std::to_string(v0.a) + "], uv=[" + std::to_string(v0.u) + "," + std::to_string(v0.v) + "]"); logger_->Trace("BgfxGuiService", "SubmitQuad", - "uniforms: mvp=" + std::to_string(bgfx::isValid(modelViewProjUniform_)) + + "uniforms: mode=" + std::string(usesMaterialXShaders_ ? "materialx" : "builtin") + ", sampler=" + std::to_string(bgfx::isValid(sampler_)) + ", program=" + std::to_string(bgfx::isValid(program_)) + ", texture=" + std::to_string(bgfx::isValid(texture)) + @@ -755,19 +759,35 @@ void BgfxGuiService::SubmitQuad(const GuiVertex& v0, std::to_string(viewProjection_[3]) + "]"); } + if (!bgfx::isValid(sampler_)) { + if (logger_) { + logger_->Error("BgfxGuiService::SubmitQuad: Sampler uniform not initialized"); + } + return; + } + SetScissor(scissor); bgfx::setTransform(identity); // Use appropriate uniforms based on shader type - if (bgfx::isValid(modelViewProjUniform_)) { - // Built-in shader: single combined matrix - bgfx::setUniform(modelViewProjUniform_, viewProjection_.data()); - } else if (bgfx::isValid(worldMatrixUniform_) && bgfx::isValid(viewProjMatrixUniform_)) { + if (usesMaterialXShaders_) { + if (!bgfx::isValid(worldMatrixUniform_) || !bgfx::isValid(viewProjMatrixUniform_)) { + if (logger_) { + logger_->Error("BgfxGuiService::SubmitQuad: MaterialX uniforms not initialized"); + } + return; + } // MaterialX shader: separate matrices bgfx::setUniform(worldMatrixUniform_, identity); bgfx::setUniform(viewProjMatrixUniform_, viewProjection_.data()); - } else if (logger_) { - logger_->Error("BgfxGuiService::SubmitQuad: No valid uniforms for shader!"); + } else if (!usesPredefinedModelViewProj_) { + if (!bgfx::isValid(modelViewProjUniform_)) { + if (logger_) { + logger_->Error("BgfxGuiService::SubmitQuad: GUI modelViewProj uniform not initialized"); + } + return; + } + bgfx::setUniform(modelViewProjUniform_, viewProjection_.data()); } bgfx::setTexture(0, sampler_, texture); bgfx::setVertexBuffer(0, &tvb, 0, 4); @@ -959,7 +979,7 @@ bgfx::TextureHandle BgfxGuiService::CreateTexture(const uint8_t* rgba, } bgfx::ProgramHandle BgfxGuiService::CreateProgram(const char* vertexSource, - const char* fragmentSource) const { + const char* fragmentSource) { if (!vertexSource || !fragmentSource) { if (logger_) { logger_->Error("BgfxGuiService::CreateProgram: null shader source"); @@ -988,6 +1008,10 @@ bgfx::ProgramHandle BgfxGuiService::CreateProgram(const char* vertexSource, } return BGFX_INVALID_HANDLE; } + + if (!usesMaterialXShaders_) { + ResolveGuiMatrixUniform(vs); + } bgfx::ProgramHandle program = bgfx::createProgram(vs, fs, true); if (!bgfx::isValid(program) && logger_) { @@ -1038,6 +1062,55 @@ bgfx::ShaderHandle BgfxGuiService::CreateShader(const std::string& label, return compiler.CompileShader(label, source, isVertex, uniforms, attributes); } +void BgfxGuiService::ResolveGuiMatrixUniform(bgfx::ShaderHandle shader) { + modelViewProjUniform_ = BGFX_INVALID_HANDLE; + usesPredefinedModelViewProj_ = false; + + if (!bgfx::isValid(shader)) { + usesPredefinedModelViewProj_ = true; + return; + } + + const uint16_t uniformCount = bgfx::getShaderUniforms(shader, nullptr, 0); + std::string modelViewProjName = "predefined"; + std::string uniformNames; + + if (uniformCount > 0) { + std::vector uniforms(uniformCount); + bgfx::getShaderUniforms(shader, uniforms.data(), uniformCount); + + for (const auto& uniform : uniforms) { + bgfx::UniformInfo info{}; + bgfx::getUniformInfo(uniform, info); + if (!uniformNames.empty()) { + uniformNames += ", "; + } + uniformNames += info.name; + if (std::strcmp(info.name, "u_modelViewProj") == 0 || + std::strcmp(info.name, "UniformBuffer.u_modelViewProj") == 0) { + modelViewProjUniform_ = uniform; + modelViewProjName = info.name; + break; + } + } + } + + if (!bgfx::isValid(modelViewProjUniform_)) { + usesPredefinedModelViewProj_ = true; + } + + if (logger_) { + logger_->Trace("BgfxGuiService", "ResolveGuiMatrixUniform", + "uniformCount=" + std::to_string(uniformCount) + + ", modelViewProj=" + modelViewProjName + + (uniformNames.empty() ? "" : ", uniforms=[" + uniformNames + "]")); + if (!uniformNames.empty() && modelViewProjName == "predefined") { + logger_->Warn("BgfxGuiService::ResolveGuiMatrixUniform: u_modelViewProj not found; uniforms=[" + + uniformNames + "]"); + } + } +} + void BgfxGuiService::PruneTextCache() { if (textCache_.size() <= maxTextCacheEntries_) { return; diff --git a/src/services/impl/bgfx_gui_service.hpp b/src/services/impl/bgfx_gui_service.hpp index ba9d489..463b11c 100644 --- a/src/services/impl/bgfx_gui_service.hpp +++ b/src/services/impl/bgfx_gui_service.hpp @@ -143,8 +143,9 @@ private: uint32_t width, uint32_t height, uint64_t flags) const; - bgfx::ProgramHandle CreateProgram(const char* vertexSource, const char* fragmentSource) const; + bgfx::ProgramHandle CreateProgram(const char* vertexSource, const char* fragmentSource); bgfx::ShaderHandle CreateShader(const std::string& label, const std::string& source, bool isVertex) const; + void ResolveGuiMatrixUniform(bgfx::ShaderHandle shader); std::shared_ptr configService_; std::shared_ptr logger_; @@ -173,6 +174,8 @@ private: uint16_t viewId_ = 1; bool initialized_ = false; bool loggedMissingResources_ = false; + bool usesMaterialXShaders_ = false; + bool usesPredefinedModelViewProj_ = false; uint64_t frameIndex_ = 0; size_t maxTextCacheEntries_ = 256; size_t maxSvgCacheEntries_ = 64; diff --git a/src/services/impl/bgfx_shader_compiler.cpp b/src/services/impl/bgfx_shader_compiler.cpp index 6ce313e..555faf4 100644 --- a/src/services/impl/bgfx_shader_compiler.cpp +++ b/src/services/impl/bgfx_shader_compiler.cpp @@ -1,6 +1,7 @@ #include "bgfx_shader_compiler.hpp" #include +#include #include #include #include @@ -39,6 +40,154 @@ const char* ShadercTargetName(bgfx::RendererType::Enum type) { } } +bool ValidateBgfxShaderBinary(const std::vector& buffer, + char expectedType, + std::string& error) { + if (buffer.size() < sizeof(uint32_t) * 3 + sizeof(uint16_t)) { + error = "buffer too small for header"; + return false; + } + + size_t offset = 0; + auto readBytes = [&](void* out, size_t size) { + if (offset + size > buffer.size()) { + return false; + } + std::memcpy(out, buffer.data() + offset, size); + offset += size; + return true; + }; + + uint32_t magic = 0; + if (!readBytes(&magic, sizeof(magic))) { + error = "failed to read magic"; + return false; + } + + const uint32_t base = magic & 0x00FFFFFFu; + const uint32_t vsh = static_cast('V') + | (static_cast('S') << 8) + | (static_cast('H') << 16); + const uint32_t fsh = static_cast('F') + | (static_cast('S') << 8) + | (static_cast('H') << 16); + const uint32_t csh = static_cast('C') + | (static_cast('S') << 8) + | (static_cast('H') << 16); + const uint32_t expectedBase = (expectedType == 'v') ? vsh : (expectedType == 'c' ? csh : fsh); + if (base != vsh && base != fsh && base != csh) { + error = "invalid magic"; + return false; + } + if (base != expectedBase) { + error = "shader type mismatch"; + return false; + } + + const uint8_t version = static_cast(magic >> 24); + const bool hasTexData = version >= 8; + const bool hasTexFormat = version >= 10; + + uint32_t hashIn = 0; + uint32_t hashOut = 0; + if (!readBytes(&hashIn, sizeof(hashIn)) || !readBytes(&hashOut, sizeof(hashOut))) { + error = "failed to read hashes"; + return false; + } + + uint16_t uniformCount = 0; + if (!readBytes(&uniformCount, sizeof(uniformCount))) { + error = "failed to read uniform count"; + return false; + } + + for (uint16_t i = 0; i < uniformCount; ++i) { + uint8_t nameSize = 0; + if (!readBytes(&nameSize, sizeof(nameSize))) { + error = "failed to read uniform name size"; + return false; + } + if (offset + nameSize > buffer.size()) { + error = "uniform name out of bounds"; + return false; + } + offset += nameSize; + + uint8_t type = 0; + uint8_t num = 0; + uint16_t regIndex = 0; + uint16_t regCount = 0; + if (!readBytes(&type, sizeof(type)) || + !readBytes(&num, sizeof(num)) || + !readBytes(®Index, sizeof(regIndex)) || + !readBytes(®Count, sizeof(regCount))) { + error = "failed to read uniform metadata"; + return false; + } + + if (hasTexData) { + uint8_t texComponent = 0; + uint8_t texDimension = 0; + if (!readBytes(&texComponent, sizeof(texComponent)) || + !readBytes(&texDimension, sizeof(texDimension))) { + error = "failed to read texture metadata"; + return false; + } + } + if (hasTexFormat) { + uint16_t texFormat = 0; + if (!readBytes(&texFormat, sizeof(texFormat))) { + error = "failed to read texture format"; + return false; + } + } + } + + uint32_t shaderSize = 0; + if (!readBytes(&shaderSize, sizeof(shaderSize))) { + error = "failed to read shader size"; + return false; + } + if (shaderSize % 4 != 0) { + error = "shader size not aligned"; + return false; + } + if (offset + shaderSize + 1 > buffer.size()) { + error = "shader code out of bounds"; + return false; + } + + if (shaderSize >= sizeof(uint32_t)) { + uint32_t spirvMagic = 0; + std::memcpy(&spirvMagic, buffer.data() + offset, sizeof(uint32_t)); + if (spirvMagic != 0x07230203u) { + error = "invalid SPIR-V magic"; + return false; + } + } + + offset += shaderSize + 1; + + uint8_t numAttrs = 0; + if (!readBytes(&numAttrs, sizeof(numAttrs))) { + error = "failed to read attribute count"; + return false; + } + if (offset + static_cast(numAttrs) * sizeof(uint16_t) > buffer.size()) { + error = "attribute list out of bounds"; + return false; + } + offset += static_cast(numAttrs) * sizeof(uint16_t); + + uint16_t constantSize = 0; + if (!readBytes(&constantSize, sizeof(constantSize))) { + error = "failed to read constant size"; + return false; + } + + return true; +} + } // namespace BgfxShaderCompiler::BgfxShaderCompiler(std::shared_ptr logger, @@ -107,6 +256,15 @@ bgfx::ShaderHandle BgfxShaderCompiler::CompileShader( ", target=" + std::string(target) + ", size=" + std::to_string(outSize)); } + std::string validationError; + if (!ValidateBgfxShaderBinary(buffer, isVertex ? 'v' : 'f', validationError)) { + compiledInMemory = false; + buffer.clear(); + if (logger_) { + logger_->Error("BgfxShaderCompiler: invalid shader binary for " + label + + " (" + validationError + ")"); + } + } } else if (logger_) { logger_->Trace("BgfxShaderCompiler", "CompileShader", "in-memory compile failed for " + label + @@ -171,6 +329,15 @@ bgfx::ShaderHandle BgfxShaderCompiler::CompileShader( remove(tempOutputPath.c_str()); } + std::string validationError; + if (!ValidateBgfxShaderBinary(buffer, isVertex ? 'v' : 'f', validationError)) { + if (logger_) { + logger_->Error("BgfxShaderCompiler: invalid shader binary for " + label + + " (" + validationError + ")"); + } + return BGFX_INVALID_HANDLE; + } + uint32_t binSize = static_cast(buffer.size()); const bgfx::Memory* mem = bgfx::copy(buffer.data(), binSize); bgfx::ShaderHandle handle = bgfx::createShader(mem); @@ -178,6 +345,9 @@ bgfx::ShaderHandle BgfxShaderCompiler::CompileShader( logger_->Error("BgfxShaderCompiler: bgfx::createShader failed for " + label + " (binSize=" + std::to_string(binSize) + ")"); } else if (logger_) { + logger_->Info("BgfxShaderCompiler: created shader " + label + + " (binSize=" + std::to_string(binSize) + + ", renderer=" + std::string(RendererTypeName(rendererType)) + ")"); logger_->Trace("BgfxShaderCompiler", "CompileShader", "label=" + label + " shader created successfully, handle=" + std::to_string(handle.idx)); }