From 26a0b3d3ed502495cb2bacc0e6650f7803ed12d7 Mon Sep 17 00:00:00 2001 From: Powei Feng Date: Tue, 7 May 2024 22:46:03 -0700 Subject: [PATCH] address more comments #2 --- filament/backend/include/backend/Program.h | 3 - filament/backend/src/opengl/OpenGLDriver.cpp | 36 +++++++----- filament/backend/src/opengl/OpenGLDriver.h | 3 +- filament/backend/src/opengl/OpenGLProgram.cpp | 58 ++++++++++--------- filament/backend/src/opengl/OpenGLProgram.h | 44 +++++++++----- filament/src/details/Material.cpp | 28 ++++++--- .../include/private/filament/EngineEnums.h | 10 +--- libs/filamat/src/shaders/CodeGenerator.cpp | 12 ++-- 8 files changed, 114 insertions(+), 80 deletions(-) diff --git a/filament/backend/include/backend/Program.h b/filament/backend/include/backend/Program.h index a50582ef55d3..5258d86f0250 100644 --- a/filament/backend/include/backend/Program.h +++ b/filament/backend/include/backend/Program.h @@ -39,9 +39,6 @@ class Program { static constexpr size_t SHADER_TYPE_COUNT = 3; static constexpr size_t UNIFORM_BINDING_COUNT = CONFIG_UNIFORM_BINDING_COUNT; static constexpr size_t SAMPLER_BINDING_COUNT = CONFIG_SAMPLER_BINDING_COUNT; - - static constexpr char const* PUSH_CONSTANT_STRUCT_VAR_NAME = "pushConstants"; - struct Sampler { utils::CString name = {}; // name of the sampler in the shader uint32_t binding = 0; // binding point of the sampler in the shader diff --git a/filament/backend/src/opengl/OpenGLDriver.cpp b/filament/backend/src/opengl/OpenGLDriver.cpp index f2d50db477e3..40b090ad7d59 100644 --- a/filament/backend/src/opengl/OpenGLDriver.cpp +++ b/filament/backend/src/opengl/OpenGLDriver.cpp @@ -265,6 +265,10 @@ void OpenGLDriver::terminate() { assert_invariant(mGpuCommandCompleteOps.empty()); #endif + if (mCurrentPushConstants) { + delete mCurrentPushConstants; + } + mContext.terminate(); mPlatform.terminate(); @@ -285,29 +289,30 @@ void OpenGLDriver::bindSampler(GLuint unit, GLuint sampler) noexcept { mContext.bindSampler(unit, sampler); } -void OpenGLDriver::setPushConstant(backend::ShaderStage, uint8_t index, +void OpenGLDriver::setPushConstant(backend::ShaderStage stage, uint8_t index, backend::PushConstantVariant value) { assert_invariant(mCurrentPushConstants && - "Calling setPushConstant() when program does not define push constants"); + "Calling setPushConstant() before binding a pipeline"); - // Note that stage is not applicable to the GL backend. + auto const& [location, type] = mCurrentPushConstants->get(stage, index); - auto const& constants = *mCurrentPushConstants; - ASSERT_PRECONDITION(index < constants.size(), "Push constant index=%d is out-of-bounds", index); - auto const& constant = constants[index]; + // This push constant wasn't found in the shader. It's ok to return without error-ing here. + if (location < 0) { + return; + } if (std::holds_alternative(value)) { - assert_invariant(constant.type == ConstantType::BOOL); + assert_invariant(type == ConstantType::BOOL); bool const bval = std::get(value); - glUniform1i(constant.location, bval ? 1 : 0); + glUniform1i(location, bval ? 1 : 0); } else if (std::holds_alternative(value)) { - assert_invariant(constant.type == ConstantType::FLOAT); + assert_invariant(type == ConstantType::FLOAT); float const fval = std::get(value); - glUniform1f(constant.location, fval); + glUniform1f(location, fval); } else { - assert_invariant(constant.type == ConstantType::INT); + assert_invariant(type == ConstantType::INT); int const ival = std::get(value); - glUniform1i(constant.location, ival); + glUniform1i(location, ival); } } @@ -3828,7 +3833,12 @@ void OpenGLDriver::bindPipeline(PipelineState state) { gl.polygonOffset(state.polygonOffset.slope, state.polygonOffset.constant); OpenGLProgram* const p = handle_cast(state.program); mValidProgram = useProgram(p); - mCurrentPushConstants = p->getPushConstants(); + + if (!mCurrentPushConstants) { + mCurrentPushConstants = new (std::nothrow) PushConstantBundle{p->getPushConstants()}; + } else { + (*mCurrentPushConstants) = p->getPushConstants(); + } } void OpenGLDriver::bindRenderPrimitive(Handle rph) { diff --git a/filament/backend/src/opengl/OpenGLDriver.h b/filament/backend/src/opengl/OpenGLDriver.h index 8a1824a8201c..a3b863cfa5c0 100644 --- a/filament/backend/src/opengl/OpenGLDriver.h +++ b/filament/backend/src/opengl/OpenGLDriver.h @@ -378,8 +378,7 @@ class OpenGLDriver final : public DriverBase { GLSwapChain* mCurrentDrawSwapChain = nullptr; bool mRec709OutputColorspace = false; - utils::FixedCapacityVector const* mCurrentPushConstants = - nullptr; + PushConstantBundle* mCurrentPushConstants = nullptr; }; // ------------------------------------------------------------------------------------------------ diff --git a/filament/backend/src/opengl/OpenGLProgram.cpp b/filament/backend/src/opengl/OpenGLProgram.cpp index ff47f811a01e..fcf08f2ce457 100644 --- a/filament/backend/src/opengl/OpenGLProgram.cpp +++ b/filament/backend/src/opengl/OpenGLProgram.cpp @@ -30,7 +30,6 @@ #include #include -#include #include #include #include @@ -48,14 +47,13 @@ struct OpenGLProgram::LazyInitializationData { Program::SamplerGroupInfo samplerGroupInfo; std::array bindingUniformInfo; utils::FixedCapacityVector pushConstants; + uint8_t pushConstantFragmentStageOffset; }; - OpenGLProgram::OpenGLProgram() noexcept = default; OpenGLProgram::OpenGLProgram(OpenGLDriver& gld, Program&& program) noexcept : HwProgram(std::move(program.getName())) { - auto* const lazyInitializationData = new(std::nothrow) LazyInitializationData(); lazyInitializationData->samplerGroupInfo = std::move(program.getSamplerGroupInfo()); if (UTILS_UNLIKELY(gld.getContext().isES2())) { @@ -64,10 +62,28 @@ OpenGLProgram::OpenGLProgram(OpenGLDriver& gld, Program&& program) noexcept lazyInitializationData->uniformBlockInfo = std::move(program.getUniformBlockBindings()); } - // We only keep the push constants for vertex stage because we'd like to keep this class a - // certain size. - lazyInitializationData->pushConstants = - std::move(program.getPushConstants(ShaderStage::VERTEX)); + lazyInitializationData->pushConstantFragmentStageOffset = 0; + + auto& vertexConstants = program.getPushConstants(ShaderStage::VERTEX); + auto& fragmentConstants = program.getPushConstants(ShaderStage::FRAGMENT); + + size_t const totalConstantCount = vertexConstants.size() + fragmentConstants.size(); + if (totalConstantCount > 0) { + auto& allConstants = lazyInitializationData->pushConstants; + + if (vertexConstants.size() > 0) { + allConstants = std::move(vertexConstants); + lazyInitializationData->pushConstantFragmentStageOffset = vertexConstants.size(); + } + + allConstants.reserve(totalConstantCount); + allConstants.resize(totalConstantCount); + + std::for_each(fragmentConstants.cbegin(), fragmentConstants.cend(), + [&allConstants](Program::PushConstant const& constant) { + allConstants.push_back(constant); + }); + } ShaderCompilerService& compiler = gld.getShaderCompilerService(); mToken = compiler.createProgram(name, std::move(program)); @@ -96,7 +112,6 @@ OpenGLProgram::~OpenGLProgram() noexcept { } void OpenGLProgram::initialize(OpenGLDriver& gld) { - SYSTRACE_CALL(); assert_invariant(gl.program == 0); @@ -212,25 +227,16 @@ void OpenGLProgram::initializeProgramState(OpenGLContext& context, GLuint progra mUsedBindingsCount = usedBindingCount; // Push constant initialization - auto& constants = lazyInitializationData.pushConstants; + mPushConstantFragmentStageOffset = lazyInitializationData.pushConstantFragmentStageOffset; + auto const& constants = lazyInitializationData.pushConstants; if (!constants.empty()) { - mVertexPushConstants.reserve(constants.size()); - mVertexPushConstants.resize(constants.size()); - // The constants are defined in a struct of name PUSH_CONSTANT_STRUCT_VAR_NAME. We prepend - // the prefix here to avoid string manipulation in a the draw loop. - std::stringbuf constantPrefix; - constantPrefix.sputn(Program::PUSH_CONSTANT_STRUCT_VAR_NAME, - strlen(Program::PUSH_CONSTANT_STRUCT_VAR_NAME)); - constantPrefix.sputn(".", 1); - - std::transform(constants.begin(), constants.end(), mVertexPushConstants.begin(), - [&constantPrefix, program](Program::PushConstant& constant) -> PushConstantBundle { - constant.name.insert(0, utils::CString(constantPrefix.str().c_str())); - auto const loc = glGetUniformLocation(program, constant.name.c_str()); - return { - .location = loc, - .type = constant.type, - }; + mPushConstants.reserve(constants.size()); + mPushConstants.resize(constants.size()); + + std::transform(constants.cbegin(), constants.cend(), mPushConstants.begin(), + [program](Program::PushConstant const& constant) -> std::pair { + GLint const loc = glGetUniformLocation(program, constant.name.c_str()); + return {loc, constant.type}; }); } } diff --git a/filament/backend/src/opengl/OpenGLProgram.h b/filament/backend/src/opengl/OpenGLProgram.h index c62506da8b74..a0a50e0cc0c4 100644 --- a/filament/backend/src/opengl/OpenGLProgram.h +++ b/filament/backend/src/opengl/OpenGLProgram.h @@ -38,11 +38,22 @@ namespace filament::backend { class OpenGLDriver; -// Holds information needed to write to push constants. This struct cannot nest within OpenGLProgram -// due to the need for forward declaration. struct PushConstantBundle { - GLint location = -1; - ConstantType type; + uint8_t fragmentStageOffset = 0; + utils::FixedCapacityVector> const* constants = nullptr; + + inline std::pair const& get(ShaderStage stage, uint8_t index) const { + assert_invariant(stage == ShaderStage::VERTEX ||stage == ShaderStage::FRAGMENT); + + // Either we're asking for a fragment stage constant or we're asking for a vertex stage + // constant and the number of vertex stage constants is greater than 0. + assert_invariant(stage == ShaderStage::FRAGMENT || fragmentStageOffset > 0); + + uint8_t const offset = (stage == ShaderStage::VERTEX ? 0 : fragmentStageOffset) + index; + + assert_invariant(offset < constants->size()); + return (*constants)[offset]; + } }; class OpenGLProgram : public HwProgram { @@ -85,11 +96,11 @@ class OpenGLProgram : public HwProgram { GLuint program = 0; } gl; // 4 bytes - utils::FixedCapacityVector const* getPushConstants() { - if (mVertexPushConstants.empty()) { - return nullptr; - } - return &mVertexPushConstants; + PushConstantBundle getPushConstants() { + return { + .fragmentStageOffset = mPushConstantFragmentStageOffset, + .constants = &mPushConstants, + }; } private: @@ -109,10 +120,14 @@ class OpenGLProgram : public HwProgram { ShaderCompilerService::program_token_t mToken{}; // 16 bytes uint8_t mUsedBindingsCount = 0u; // 1 byte - UTILS_UNUSED uint8_t padding[3] = {}; // 3 bytes + UTILS_UNUSED uint8_t padding[2] = {}; // 2 byte + + // Push constant array offset for fragment stage constants. + uint8_t mPushConstantFragmentStageOffset = 0u; // 1 byte // only needed for ES2 - GLint mRec709Location = -1; // 4 bytes + GLint mRec709Location = -1; // 4 bytes + using LocationInfo = utils::FixedCapacityVector; struct UniformsRecord { Program::UniformInfo uniforms; @@ -120,13 +135,14 @@ class OpenGLProgram : public HwProgram { mutable GLuint id = 0; mutable uint16_t age = std::numeric_limits::max(); }; - UniformsRecord const* mUniformsRecords = nullptr; + UniformsRecord const* mUniformsRecords = nullptr; // 8 bytes - utils::FixedCapacityVector mVertexPushConstants; + // Store [location, type] pairs. + utils::FixedCapacityVector> mPushConstants; // 16 bytes }; // if OpenGLProgram is larger tha 64 bytes, it'll fall in a larger Handle bucket. -static_assert(sizeof(OpenGLProgram) <= 64); // currently 54 bytes +static_assert(sizeof(OpenGLProgram) <= 64); // currently 64 bytes } // namespace filament::backend diff --git a/filament/src/details/Material.cpp b/filament/src/details/Material.cpp index 396badbdc2c3..6b97c433ec7a 100644 --- a/filament/src/details/Material.cpp +++ b/filament/src/details/Material.cpp @@ -933,13 +933,27 @@ void FMaterial::processSpecializationConstants(FEngine& engine, Material::Builde void FMaterial::processPushConstants(FEngine& engine, Material::Builder const& builder) { // TODO: for testing and illustrating push constants. To be removed. - // auto& vertexPushConstant = mPushConstants[(uint8_t) ShaderStage::VERTEX]; - // vertexPushConstant.reserve(PUSH_CONSTANT_NAMES.size()); - // assert_invariant(PUSH_CONSTANT_NAMES.size() == PUSH_CONSTANT_TYPES.size()); - // for (size_t i = 0; i < PUSH_CONSTANT_NAMES.size(); i++) { - // vertexPushConstant.push_back( - // { utils::CString(PUSH_CONSTANT_NAMES[i]), PUSH_CONSTANT_TYPES[i] }); - // } + auto& vertexPushConstant = mPushConstants[(uint8_t) ShaderStage::VERTEX]; + vertexPushConstant.reserve(PUSH_CONSTANTS.size()); + vertexPushConstant.resize(PUSH_CONSTANTS.size()); + + constexpr size_t PREFIX_LEN = sizeof(PUSH_CONSTANT_STRUCT_VAR_NAME); + constexpr size_t MAX_VAR_LEN = 30; + constexpr size_t TOTAL_NAME_LEN = + PREFIX_LEN + MAX_VAR_LEN + 2; // additional chars for '.' and for the ending 0. + char buf[TOTAL_NAME_LEN]; + + std::transform(PUSH_CONSTANTS.cbegin(), PUSH_CONSTANTS.cend(), vertexPushConstant.begin(), + [&buf](std::pair const& constant) + -> backend::Program::PushConstant { + assert_invariant(strlen(constant.first) <= MAX_VAR_LEN); + // The following will be pass to Program, where the "name" is needed for GL to + // identify the location of the uniform. We prepend the struct name here to save + // some work in the backend. + snprintf(buf, TOTAL_NAME_LEN, "%s.%s", PUSH_CONSTANT_STRUCT_VAR_NAME, + constant.first); + return { utils::CString(buf), constant.second }; + }); } void FMaterial::processDepthVariants(FEngine& engine, MaterialParser const* const parser) { diff --git a/libs/filabridge/include/private/filament/EngineEnums.h b/libs/filabridge/include/private/filament/EngineEnums.h index 26238ad70415..0b845426d88b 100644 --- a/libs/filabridge/include/private/filament/EngineEnums.h +++ b/libs/filabridge/include/private/filament/EngineEnums.h @@ -72,8 +72,6 @@ enum class ReservedSpecializationConstants : uint8_t { CONFIG_STEREO_EYE_COUNT = 8, // don't change (hardcoded in ShaderCompilerService.cpp) }; - - // Note that the following enum/arrays should be ordered so that the ids correspond to indices in // the two vectors. enum class PushConstantIds { @@ -82,11 +80,9 @@ enum class PushConstantIds { using PushConstantType = backend::ConstantType; -const utils::FixedCapacityVector PUSH_CONSTANT_NAMES = { - "morphingBufferOffset", -}; -const utils::FixedCapacityVector PUSH_CONSTANT_TYPES = { - PushConstantType::INT, +constexpr char const PUSH_CONSTANT_STRUCT_VAR_NAME[] = "pushConstants"; +const utils::FixedCapacityVector> PUSH_CONSTANTS = { + { "morphingBufferOffset", PushConstantType::INT }, }; // This value is limited by UBO size, ES3.0 only guarantees 16 KiB. diff --git a/libs/filamat/src/shaders/CodeGenerator.cpp b/libs/filamat/src/shaders/CodeGenerator.cpp index bd4559bfc268..cb6fef4280e7 100644 --- a/libs/filamat/src/shaders/CodeGenerator.cpp +++ b/libs/filamat/src/shaders/CodeGenerator.cpp @@ -18,10 +18,8 @@ #include "MaterialInfo.h" -#include "backend/Program.h" #include "generated/shaders.h" #include "private/filament/EngineEnums.h" -#include "utils/Panic.h" #include @@ -59,18 +57,16 @@ io::sstream& generatePushConstantImpl(size_t const layoutLocation, bool const ou out << "struct " << STRUCT_NAME << " {\n"; } - for (size_t i = 0; i < PUSH_CONSTANT_NAMES.size(); ++i) { - char const* type = getType(PUSH_CONSTANT_TYPES[i]); - out << type << " " << PUSH_CONSTANT_NAMES[i] << ";\n"; - i++; + for (auto const& [name, type] : PUSH_CONSTANTS) { + out << getType(type) << " " << name << ";\n"; } if (outputSpirv) { - out << "} " << backend::Program::PUSH_CONSTANT_STRUCT_VAR_NAME << ";\n"; + out << "} " << PUSH_CONSTANT_STRUCT_VAR_NAME << ";\n"; } else { out << "};\n"; out << "LAYOUT_LOCATION(" << static_cast(layoutLocation) << ") uniform " << STRUCT_NAME - << " " << backend::Program::PUSH_CONSTANT_STRUCT_VAR_NAME << ";\n"; + << " " << PUSH_CONSTANT_STRUCT_VAR_NAME << ";\n"; } return out; }