address comment #3

filament/backend/include/backend/Program.h

@@ -39,6 +39,7 @@ 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;
+
     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

@@ -294,7 +294,16 @@ void OpenGLDriver::setPushConstant(backend::ShaderStage stage, uint8_t index,
     assert_invariant(mCurrentPushConstants &&
                      "Calling setPushConstant() before binding a pipeline");
 
-    auto const& [location, type] = mCurrentPushConstants->get(stage, index);
+    assert_invariant(stage == ShaderStage::VERTEX || stage == ShaderStage::FRAGMENT);
+    utils::Slice<std::pair<GLint, ConstantType>> constants;
+    if (stage == ShaderStage::VERTEX) {
+        constants = mCurrentPushConstants->vertexConstants;
+    } else if (stage == ShaderStage::FRAGMENT) {
+        constants = mCurrentPushConstants->fragmentConstants;
+    }
+
+    assert_invariant(index < constants.size());
+    auto const& [location, type] = constants[index];
 
     // This push constant wasn't found in the shader. It's ok to return without error-ing here.
     if (location < 0) {

filament/backend/src/opengl/OpenGLDriver.h

@@ -66,10 +66,8 @@ namespace filament::backend {
 class OpenGLPlatform;
 class PixelBufferDescriptor;
 struct TargetBufferInfo;
-
 class OpenGLProgram;
 class TimerQueryFactoryInterface;
-
 struct PushConstantBundle;
 
 class OpenGLDriver final : public DriverBase {

filament/backend/src/opengl/OpenGLProgram.cpp

@@ -46,10 +46,11 @@ struct OpenGLProgram::LazyInitializationData {
     Program::UniformBlockInfo uniformBlockInfo;
     Program::SamplerGroupInfo samplerGroupInfo;
     std::array<Program::UniformInfo, Program::UNIFORM_BINDING_COUNT> bindingUniformInfo;
-    utils::FixedCapacityVector<Program::PushConstant> pushConstants;
-    uint8_t pushConstantFragmentStageOffset;
+    utils::FixedCapacityVector<Program::PushConstant> vertexPushConstants;
+    utils::FixedCapacityVector<Program::PushConstant> fragmentPushConstants;
 };
 
+
 OpenGLProgram::OpenGLProgram() noexcept = default;
 
 OpenGLProgram::OpenGLProgram(OpenGLDriver& gld, Program&& program) noexcept
@@ -61,30 +62,8 @@ OpenGLProgram::OpenGLProgram(OpenGLDriver& gld, Program&& program) noexcept
     } else {
         lazyInitializationData->uniformBlockInfo = std::move(program.getUniformBlockBindings());
     }
-
-    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;
-        size_t const numVertexConstants = vertexConstants.size();
-
-        if (numVertexConstants > 0) {
-            allConstants = std::move(vertexConstants);
-            lazyInitializationData->pushConstantFragmentStageOffset = numVertexConstants;
-        }
-
-        allConstants.reserve(totalConstantCount);
-        allConstants.resize(totalConstantCount);
-
-        std::for_each(fragmentConstants.cbegin(), fragmentConstants.cend(),
-                [&allConstants](Program::PushConstant const& constant) {
-                    allConstants.push_back(constant);
-                });
-    }
+    lazyInitializationData->vertexPushConstants = std::move(program.getPushConstants(ShaderStage::VERTEX));
+    lazyInitializationData->fragmentPushConstants = std::move(program.getPushConstants(ShaderStage::FRAGMENT));
 
     ShaderCompilerService& compiler = gld.getShaderCompilerService();
     mToken = compiler.createProgram(name, std::move(program));
@@ -113,6 +92,7 @@ OpenGLProgram::~OpenGLProgram() noexcept {
 }
 
 void OpenGLProgram::initialize(OpenGLDriver& gld) {
+
     SYSTRACE_CALL();
 
     assert_invariant(gl.program == 0);
@@ -227,18 +207,19 @@ void OpenGLProgram::initializeProgramState(OpenGLContext& context, GLuint progra
     }
     mUsedBindingsCount = usedBindingCount;
 
-    // Push constant initialization
-    mPushConstantFragmentStageOffset = lazyInitializationData.pushConstantFragmentStageOffset;
-    auto const& constants = lazyInitializationData.pushConstants;
-    if (!constants.empty()) {
-        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};
-                });
+    auto& vertexConstants = lazyInitializationData.vertexPushConstants;
+    auto& fragmentConstants = lazyInitializationData.fragmentPushConstants;
+
+    size_t const totalConstantCount = vertexConstants.size() + fragmentConstants.size();
+    if (totalConstantCount > 0) {
+        mPushConstants.reserve(totalConstantCount);
+        mPushConstantFragmentStageOffset = vertexConstants.size();
+        auto const transformAndAdd = [&](Program::PushConstant const& constant) {
+            GLint const loc = glGetUniformLocation(program, constant.name.c_str());
+            mPushConstants.push_back({loc, constant.type});
+        };
+        std::for_each(vertexConstants.cbegin(), vertexConstants.cend(), transformAndAdd);
+        std::for_each(fragmentConstants.cbegin(), fragmentConstants.cend(), transformAndAdd);
     }
 }
 

filament/backend/src/opengl/OpenGLProgram.h

@@ -27,6 +27,7 @@
 
 #include <utils/compiler.h>
 #include <utils/FixedCapacityVector.h>
+#include <utils/Slice.h>
 
 #include <array>
 #include <limits>
@@ -39,21 +40,8 @@ namespace filament::backend {
 class OpenGLDriver;
 
 struct PushConstantBundle {
-    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];
-    }
+    utils::Slice<std::pair<GLint, ConstantType>> vertexConstants;
+    utils::Slice<std::pair<GLint, ConstantType>> fragmentConstants;
 };
 
 class OpenGLProgram : public HwProgram {
@@ -97,9 +85,10 @@ class OpenGLProgram : public HwProgram {
     } gl;                                               // 4 bytes
 
     PushConstantBundle getPushConstants() {
+        auto fragBegin = mPushConstants.begin() + mPushConstantFragmentStageOffset;
         return {
-            .fragmentStageOffset = mPushConstantFragmentStageOffset,
-            .constants = &mPushConstants,
+            .vertexConstants = utils::Slice(mPushConstants.begin(), fragBegin),
+            .fragmentConstants = utils::Slice(fragBegin, mPushConstants.end()),
         };
     }
 
@@ -137,8 +126,9 @@ class OpenGLProgram : public HwProgram {
     };
     UniformsRecord const* mUniformsRecords = nullptr;               // 8 bytes
 
-    // Store [location, type] pairs.
-    utils::FixedCapacityVector<std::pair<GLint, ConstantType>> mPushConstants;  // 16 bytes
+    // Note that this can be replaced with a raw pointer and an uint8_t (for size) to reduce the
+    // size of the container to 9 bytes if there is a need in the future.
+    utils::FixedCapacityVector<std::pair<GLint, ConstantType>> mPushConstants;// 16 bytes
 };
 
 // if OpenGLProgram is larger tha 64 bytes, it'll fall in a larger Handle bucket.

filament/src/MaterialParser.cpp

@@ -30,6 +30,7 @@
 #include <private/filament/SubpassInfo.h>
 #include <private/filament/Variant.h>
 #include <private/filament/ConstantInfo.h>
+#include <private/filament/PushConstantInfo.h>
 
 #include <utils/CString.h>
 
@@ -225,6 +226,14 @@ bool MaterialParser::getConstants(utils::FixedCapacityVector<MaterialConstant>*
     return ChunkMaterialConstants::unflatten(unflattener, value);
 }
 
+bool MaterialParser::getPushConstants(utils::CString* structVarName,
+        utils::FixedCapacityVector<MaterialPushConstant>* value) const noexcept {
+    auto [start, end] = mImpl.mChunkContainer.getChunkRange(filamat::MaterialPushConstants);
+    if (start == end) return false;
+    Unflattener unflattener(start, end);
+    return ChunkMaterialPushConstants::unflatten(unflattener, structVarName, value);
+}
+
 bool MaterialParser::getDepthWriteSet(bool* value) const noexcept {
     return mImpl.getFromSimpleChunk(ChunkType::MaterialDepthWriteSet, value);
 }
@@ -709,4 +718,46 @@ bool ChunkMaterialConstants::unflatten(filaflat::Unflattener& unflattener,
     return true;
 }
 
+bool ChunkMaterialPushConstants::unflatten(filaflat::Unflattener& unflattener,
+        utils::CString* structVarName,
+        utils::FixedCapacityVector<MaterialPushConstant>* materialPushConstants) {
+    assert_invariant(materialPushConstants);
+
+    if (!unflattener.read(structVarName)) {
+        return false;
+    }
+
+    // Read number of constants.
+    uint64_t numConstants = 0;
+    if (!unflattener.read(&numConstants)) {
+        return false;
+    }
+
+    materialPushConstants->reserve(numConstants);
+    materialPushConstants->resize(numConstants);
+
+    for (uint64_t i = 0; i < numConstants; i++) {
+        CString constantName;
+        uint8_t constantType = 0;
+        uint8_t shaderStage = 0;
+
+        if (!unflattener.read(&constantName)) {
+            return false;
+        }
+
+        if (!unflattener.read(&constantType)) {
+            return false;
+        }
+
+        if (!unflattener.read(&shaderStage)) {
+            return false;
+        }
+
+        (*materialPushConstants)[i].name = constantName;
+        (*materialPushConstants)[i].type = static_cast<backend::ConstantType>(constantType);
+        (*materialPushConstants)[i].stage = static_cast<backend::ShaderStage>(shaderStage);
+    }
+    return true;
+}
+
 } // namespace filament

filament/src/MaterialParser.h

@@ -47,6 +47,7 @@ class BufferInterfaceBlock;
 class SamplerInterfaceBlock;
 struct SubpassInfo;
 struct MaterialConstant;
+struct MaterialPushConstant;
 
 class MaterialParser {
 public:
@@ -79,6 +80,8 @@ class MaterialParser {
     bool getSamplerBlockBindings(SamplerGroupBindingInfoList* pSamplerGroupInfoList,
             SamplerBindingToNameMap* pSamplerBindingToNameMap) const noexcept;
     bool getConstants(utils::FixedCapacityVector<MaterialConstant>* value) const noexcept;
+    bool getPushConstants(utils::CString* structVarName,
+            utils::FixedCapacityVector<MaterialPushConstant>* value) const noexcept;
 
     using BindingUniformInfoContainer = utils::FixedCapacityVector<
             std::pair<filament::UniformBindingPoints, backend::Program::UniformInfo>>;
@@ -214,6 +217,11 @@ struct ChunkMaterialConstants {
             utils::FixedCapacityVector<MaterialConstant>* materialConstants);
 };
 
+struct ChunkMaterialPushConstants {
+    static bool unflatten(filaflat::Unflattener& unflattener, utils::CString* structVarName,
+            utils::FixedCapacityVector<MaterialPushConstant>* materialPushConstants);
+};
+
 } // namespace filament
 
 #endif // TNT_FILAMENT_MATERIALPARSER_H

filament/src/details/Material.cpp

@@ -25,6 +25,7 @@
 #include <private/filament/EngineEnums.h>
 #include <private/filament/SamplerInterfaceBlock.h>
 #include <private/filament/BufferInterfaceBlock.h>
+#include <private/filament/PushConstantInfo.h>
 #include <private/filament/Variant.h>
 
 #include <filament/Material.h>
@@ -297,7 +298,7 @@ FMaterial::FMaterial(FEngine& engine, const Material::Builder& builder,
 
     processBlendingMode(parser);
     processSpecializationConstants(engine, builder, parser);
-    processPushConstants(engine, builder);
+    processPushConstants(engine, parser);
     processDepthVariants(engine, parser);
 
     // we can only initialize the default instance once we're initialized ourselves
@@ -931,29 +932,40 @@ 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_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::processPushConstants(FEngine& engine, MaterialParser const* parser) {
+    utils::FixedCapacityVector<backend::Program::PushConstant>& vertexConstants =
+            mPushConstants[(uint8_t) ShaderStage::VERTEX];
+    utils::FixedCapacityVector<backend::Program::PushConstant>& fragmentConstants =
+            mPushConstants[(uint8_t) ShaderStage::FRAGMENT];
+
+    CString structVarName;
+    utils::FixedCapacityVector<MaterialPushConstant> pushConstants;
+    parser->getPushConstants(&structVarName, &pushConstants);
+
+    vertexConstants.reserve(pushConstants.size());
+    fragmentConstants.reserve(pushConstants.size());
+
+    constexpr size_t MAX_NAME_LEN = 60;
+    char buf[MAX_NAME_LEN];
+    uint8_t vertexCount = 0, fragmentCount = 0;
+
+    std::for_each(pushConstants.cbegin(), pushConstants.cend(),
+            [&](MaterialPushConstant const& constant) {
+                snprintf(buf, sizeof(buf), "%s.%s", structVarName.c_str(), constant.name.c_str());
+
+                switch (constant.stage) {
+                    case ShaderStage::VERTEX:
+                        vertexConstants.push_back({utils::CString(buf), constant.type});
+                        vertexCount++;
+                        break;
+                    case ShaderStage::FRAGMENT:
+                        fragmentConstants.push_back({utils::CString(buf), constant.type});
+                        fragmentCount++;
+                        break;
+                    case ShaderStage::COMPUTE:
+                        break;
+                }
+            });
 }
 
 void FMaterial::processDepthVariants(FEngine& engine, MaterialParser const* const parser) {

filament/src/details/Material.h

@@ -253,7 +253,7 @@ class FMaterial : public Material {
     void processSpecializationConstants(FEngine& engine, Material::Builder const& builder,
             MaterialParser const* parser);
 
-    void processPushConstants(FEngine& engine, Material::Builder const& builder);
+    void processPushConstants(FEngine& engine, MaterialParser const* parser);
 
     void processDepthVariants(FEngine& engine, MaterialParser const* parser);
 

libs/filabridge/include/filament/MaterialChunkType.h

@@ -53,6 +53,7 @@ enum UTILS_PUBLIC ChunkType : uint64_t {
     MaterialAttributeInfo = charTo64bitNum("MAT_ATTR"),
     MaterialProperties = charTo64bitNum("MAT_PROP"),
     MaterialConstants = charTo64bitNum("MAT_CONS"),
+    MaterialPushConstants = charTo64bitNum("MAT_PCON"),
 
     MaterialName = charTo64bitNum("MAT_NAME"),
     MaterialVersion = charTo64bitNum("MAT_VERS"),

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

@@ -72,19 +72,10 @@ 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 {
     MORPHING_BUFFER_OFFSET = 0,
 };
 
-using PushConstantType = backend::ConstantType;
-
-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.
 // It's also limited by the Froxelizer's record buffer data type (uint8_t).
 constexpr size_t CONFIG_MAX_LIGHT_COUNT = 256;