address more comments #2

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

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<bool>(value)) {
-        assert_invariant(constant.type == ConstantType::BOOL);
+        assert_invariant(type == ConstantType::BOOL);
         bool const bval = std::get<bool>(value);
-        glUniform1i(constant.location, bval ? 1 : 0);
+        glUniform1i(location, bval ? 1 : 0);
     } else if (std::holds_alternative<float>(value)) {
-        assert_invariant(constant.type == ConstantType::FLOAT);
+        assert_invariant(type == ConstantType::FLOAT);
         float const fval = std::get<float>(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<int>(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<OpenGLProgram*>(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<HwRenderPrimitive> rph) {

filament/backend/src/opengl/OpenGLDriver.h

@@ -378,8 +378,7 @@ class OpenGLDriver final : public DriverBase {
     GLSwapChain* mCurrentDrawSwapChain = nullptr;
     bool mRec709OutputColorspace = false;
 
-    utils::FixedCapacityVector<PushConstantBundle> const* mCurrentPushConstants =
-            nullptr;
+    PushConstantBundle* mCurrentPushConstants = nullptr;
 };
 
 // ------------------------------------------------------------------------------------------------

filament/backend/src/opengl/OpenGLProgram.cpp

@@ -30,7 +30,6 @@
 #include <utils/Systrace.h>
 
 #include <array>
-#include <sstream>
 #include <string_view>
 #include <utility>
 #include <new>
@@ -48,14 +47,13 @@ struct OpenGLProgram::LazyInitializationData {
     Program::SamplerGroupInfo samplerGroupInfo;
     std::array<Program::UniformInfo, Program::UNIFORM_BINDING_COUNT> bindingUniformInfo;
     utils::FixedCapacityVector<Program::PushConstant> 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.begin(), fragmentConstants.end(),
+                [&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, ConstantType> {
+                    GLint const loc = glGetUniformLocation(program, constant.name.c_str());
+                    return {loc, constant.type};
                 });
     }
 }

filament/backend/src/opengl/OpenGLProgram.h

@@ -38,13 +38,26 @@ 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<std::pair<GLint, ConstantType>> const* constants = nullptr;
+
+    inline std::pair<GLint, ConstantType> 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];
+    }
 };
 
+static_assert(sizeof(PushConstantBundle) <= 24);
+
 class OpenGLProgram : public HwProgram {
 public:
 
@@ -85,14 +98,14 @@ class OpenGLProgram : public HwProgram {
         GLuint program = 0;
     } gl;                                               // 4 bytes
 
-    utils::FixedCapacityVector<PushConstantBundle> const* getPushConstants() {
-        if (mVertexPushConstants.empty()) {
-            return nullptr;
-        }
-        return &mVertexPushConstants;
+    PushConstantBundle getPushConstants() {
+        return {
+            .fragmentStageOffset = mPushConstantFragmentStageOffset,
+            .constants = &mPushConstants,
+        };
     }
 
-private:
+//private:
     // keep these away from of other class attributes
     struct LazyInitializationData;
 
@@ -109,24 +122,29 @@ 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
+
+    // Offset into the array indexing to 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<GLint>;
     struct UniformsRecord {
         Program::UniformInfo uniforms;
         LocationInfo locations;
         mutable GLuint id = 0;
         mutable uint16_t age = std::numeric_limits<uint16_t>::max();
     };
-    UniformsRecord const* mUniformsRecords = nullptr;
+    UniformsRecord const* mUniformsRecords = nullptr;               // 8 bytes
 
-    utils::FixedCapacityVector<PushConstantBundle> mVertexPushConstants;
+    // Store [location, type] pairs.
+    utils::FixedCapacityVector<std::pair<GLint, ConstantType>> 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
 

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<char const*, ConstantType> 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) {

libs/filabridge/include/private/filament/EngineEnums.h

@@ -82,11 +82,9 @@ enum class PushConstantIds {
 
 using PushConstantType = backend::ConstantType;
 
-const utils::FixedCapacityVector<char const*> PUSH_CONSTANT_NAMES = {
-    "morphingBufferOffset",
-};
-const utils::FixedCapacityVector<PushConstantType> PUSH_CONSTANT_TYPES = {
-    PushConstantType::INT,
+constexpr char const PUSH_CONSTANT_STRUCT_VAR_NAME[] = "pushConstants";
+const utils::FixedCapacityVector<std::pair<char const*, PushConstantType>> PUSH_CONSTANTS = {
+    { "morphingBufferOffset", PushConstantType::INT },
 };
 
 // This value is limited by UBO size, ES3.0 only guarantees 16 KiB.

libs/filamat/src/shaders/CodeGenerator.cpp

@@ -59,18 +59,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<int>(layoutLocation) << ") uniform " << STRUCT_NAME
-            << " " << backend::Program::PUSH_CONSTANT_STRUCT_VAR_NAME << ";\n";
+            << " " << PUSH_CONSTANT_STRUCT_VAR_NAME << ";\n";
     }
     return out;
 }