address more comments

filament/backend/include/backend/DriverEnums.h

@@ -336,7 +336,7 @@ enum class UniformType : uint8_t {
 /**
  * Supported constant parameter types
  */
- enum class ConstantType : uint8_t {
+enum class ConstantType : uint8_t {
   INT,
   FLOAT,
   BOOL

filament/backend/include/backend/Program.h

@@ -40,6 +40,8 @@ class Program {
     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
@@ -117,8 +119,13 @@ class Program {
     Program& specializationConstants(
             utils::FixedCapacityVector<SpecializationConstant> specConstants) noexcept;
 
+    struct PushConstant {
+        utils::CString name;
+        ConstantType type;
+    };
+
     Program& pushConstants(ShaderStage stage,
-            utils::FixedCapacityVector<char const*> constants) noexcept;
+            utils::FixedCapacityVector<PushConstant> constants) noexcept;
 
     Program& cacheId(uint64_t cacheId) noexcept;
 
@@ -151,12 +158,12 @@ class Program {
         return mSpecializationConstants;
     }
 
-    utils::FixedCapacityVector<char const*> const& getPushConstants(
+    utils::FixedCapacityVector<PushConstant> const& getPushConstants(
             ShaderStage stage) const noexcept {
         return mPushConstants[static_cast<uint8_t>(stage)];
     }
 
-    utils::FixedCapacityVector<char const*>& getPushConstants(ShaderStage stage) noexcept {
+    utils::FixedCapacityVector<PushConstant>& getPushConstants(ShaderStage stage) noexcept {
         return mPushConstants[static_cast<uint8_t>(stage)];
     }
 
@@ -177,7 +184,7 @@ class Program {
     uint64_t mCacheId{};
     utils::Invocable<utils::io::ostream&(utils::io::ostream& out)> mLogger;
     utils::FixedCapacityVector<SpecializationConstant> mSpecializationConstants;
-    std::array<utils::FixedCapacityVector<char const*>, SHADER_TYPE_COUNT> mPushConstants;
+    std::array<utils::FixedCapacityVector<PushConstant>, SHADER_TYPE_COUNT> mPushConstants;
     utils::FixedCapacityVector<std::pair<utils::CString, uint8_t>> mAttributes;
     std::array<UniformInfo, Program::UNIFORM_BINDING_COUNT> mBindingUniformInfo;
     CompilerPriorityQueue mPriorityQueue = CompilerPriorityQueue::HIGH;

filament/backend/src/Program.cpp

@@ -92,8 +92,8 @@ Program& Program::specializationConstants(
 }
 
 Program& Program::pushConstants(ShaderStage stage,
-        utils::FixedCapacityVector<char const*> constants) noexcept {
-    mPushConstants[static_cast<uint8_t>(stage)] = constants;
+        utils::FixedCapacityVector<PushConstant> constants) noexcept {
+    mPushConstants[static_cast<uint8_t>(stage)] = std::move(constants);
     return *this;
 }
 

filament/backend/src/opengl/OpenGLDriver.cpp

@@ -285,8 +285,30 @@ void OpenGLDriver::bindSampler(GLuint unit, GLuint sampler) noexcept {
     mContext.bindSampler(unit, sampler);
 }
 
-void OpenGLDriver::setPushConstant(backend::ShaderStage stage, uint8_t index,
+void OpenGLDriver::setPushConstant(backend::ShaderStage, uint8_t index,
         backend::PushConstantVariant value) {
+    assert_invariant(mCurrentPushConstants &&
+                     "Calling setPushConstant() when program does not define push constants");
+
+    // Note that stage is not applicable to the GL backend.
+
+    auto const& constants = *mCurrentPushConstants;
+    ASSERT_PRECONDITION(index < constants.size(), "Push constant index=%d is out-of-bounds", index);
+    auto const& constant = constants[index];
+
+    if (std::holds_alternative<bool>(value)) {
+        assert_invariant(constant.type == ConstantType::BOOL);
+        bool const bval = std::get<bool>(value);
+        glUniform1i(constant.location, bval ? 1 : 0);
+    } else if (std::holds_alternative<float>(value)) {
+        assert_invariant(constant.type == ConstantType::FLOAT);
+        float const fval = std::get<float>(value);
+        glUniform1f(constant.location, fval);
+    } else {
+        assert_invariant(constant.type == ConstantType::INT);
+        int const ival = std::get<int>(value);
+        glUniform1i(constant.location, ival);
+    }
 }
 
 void OpenGLDriver::bindTexture(GLuint unit, GLTexture const* t) noexcept {
@@ -3806,6 +3828,7 @@ 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();
 }
 
 void OpenGLDriver::bindRenderPrimitive(Handle<HwRenderPrimitive> rph) {

filament/backend/src/opengl/OpenGLDriver.h

@@ -70,6 +70,8 @@ struct TargetBufferInfo;
 class OpenGLProgram;
 class TimerQueryFactoryInterface;
 
+struct PushConstantBundle;
+
 class OpenGLDriver final : public DriverBase {
     inline explicit OpenGLDriver(OpenGLPlatform* platform,
             const Platform::DriverConfig& driverConfig) noexcept;
@@ -375,6 +377,9 @@ class OpenGLDriver final : public DriverBase {
     // for ES2 sRGB support
     GLSwapChain* mCurrentDrawSwapChain = nullptr;
     bool mRec709OutputColorspace = false;
+
+    utils::FixedCapacityVector<PushConstantBundle> const* mCurrentPushConstants =
+            nullptr;
 };
 
 // ------------------------------------------------------------------------------------------------

filament/backend/src/opengl/OpenGLProgram.cpp

@@ -30,6 +30,7 @@
 #include <utils/Systrace.h>
 
 #include <array>
+#include <sstream>
 #include <string_view>
 #include <utility>
 #include <new>
@@ -46,6 +47,7 @@ struct OpenGLProgram::LazyInitializationData {
     Program::UniformBlockInfo uniformBlockInfo;
     Program::SamplerGroupInfo samplerGroupInfo;
     std::array<Program::UniformInfo, Program::UNIFORM_BINDING_COUNT> bindingUniformInfo;
+    utils::FixedCapacityVector<Program::PushConstant> pushConstants;
 };
 
 
@@ -62,6 +64,11 @@ 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));
+
     ShaderCompilerService& compiler = gld.getShaderCompilerService();
     mToken = compiler.createProgram(name, std::move(program));
 
@@ -203,6 +210,29 @@ void OpenGLProgram::initializeProgramState(OpenGLContext& context, GLuint progra
         }
     }
     mUsedBindingsCount = usedBindingCount;
+
+    // Push constant initialization
+    auto& 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,
+                    };
+                });
+    }
 }
 
 void OpenGLProgram::updateSamplers(OpenGLDriver* const gld) const noexcept {

filament/backend/src/opengl/OpenGLProgram.h

@@ -38,6 +38,13 @@ 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;
+};
+
 class OpenGLProgram : public HwProgram {
 public:
 
@@ -78,6 +85,13 @@ class OpenGLProgram : public HwProgram {
         GLuint program = 0;
     } gl;                                               // 4 bytes
 
+    utils::FixedCapacityVector<PushConstantBundle> const* getPushConstants() {
+        if (mVertexPushConstants.empty()) {
+            return nullptr;
+        }
+        return &mVertexPushConstants;
+    }
+
 private:
     // keep these away from of other class attributes
     struct LazyInitializationData;
@@ -97,7 +111,6 @@ class OpenGLProgram : public HwProgram {
     uint8_t mUsedBindingsCount = 0u;                    // 1 byte
     UTILS_UNUSED uint8_t padding[3] = {};               // 3 bytes
 
-
     // only needed for ES2
     GLint mRec709Location = -1; // 4 bytes
     using LocationInfo = utils::FixedCapacityVector<GLint>;
@@ -108,10 +121,12 @@ class OpenGLProgram : public HwProgram {
         mutable uint16_t age = std::numeric_limits<uint16_t>::max();
     };
     UniformsRecord const* mUniformsRecords = nullptr;
+
+    utils::FixedCapacityVector<PushConstantBundle> mVertexPushConstants;
 };
 
 // if OpenGLProgram is larger tha 64 bytes, it'll fall in a larger Handle bucket.
-static_assert(sizeof(OpenGLProgram) <= 64); // currently 48 bytes
+static_assert(sizeof(OpenGLProgram) <= 64); // currently 54 bytes
 
 } // namespace filament::backend
 

filament/backend/src/vulkan/VulkanHandles.cpp

@@ -142,14 +142,22 @@ VulkanDescriptorSetLayout::~VulkanDescriptorSetLayout() {
 PushConstantDescription::PushConstantDescription(backend::Program const& program) noexcept {
     mRangeCount = 0;
     for (auto stage : { ShaderStage::VERTEX, ShaderStage::FRAGMENT, ShaderStage::COMPUTE }) {
-        auto const& names = program.getPushConstants(stage);
-        if (names.empty()) {
+        auto const& constants = program.getPushConstants(stage);
+        if (constants.empty()) {
             continue;
         }
+
+        // We store the type of the constant for type-checking when writing.
+        auto& types = mTypes[(uint8_t) stage];
+        types.reserve(constants.size());
+        std::for_each(constants.cbegin(), constants.cend(), [&types] (Program::PushConstant t) {
+            types.push_back(t.type);
+        });
+
         mRanges[mRangeCount++] = {
             .stageFlags = getVkStage(stage),
             .offset = 0,
-            .size = (uint32_t) names.size() * 4,
+            .size = (uint32_t) constants.size() * ENTRY_SIZE,
         };
     }
 }
@@ -158,23 +166,28 @@ void PushConstantDescription::write(VulkanCommands* commands, VkPipelineLayout l
         backend::ShaderStage stage, uint8_t index, backend::PushConstantVariant const& value) {
     VulkanCommandBuffer* cmdbuf = &(commands->get());
     uint32_t binaryValue = 0;
+    UTILS_UNUSED_IN_RELEASE auto const& types = mTypes[(uint8_t) stage];
     if (std::holds_alternative<bool>(value)) {
+        assert_invariant(types[index] == ConstantType::BOOL);
         bool const bval = std::get<bool>(value);
         binaryValue = static_cast<uint32_t const>(bval ? VK_TRUE : VK_FALSE);
     } else if (std::holds_alternative<float>(value)) {
+        assert_invariant(types[index] == ConstantType::FLOAT);
         float const fval = std::get<float>(value);
         binaryValue = *reinterpret_cast<uint32_t const*>(&fval);
     } else {
+        assert_invariant(types[index] == ConstantType::INT);
         int const ival = std::get<int>(value);
         binaryValue = *reinterpret_cast<uint32_t const*>(&ival);
     }
-    vkCmdPushConstants(cmdbuf->buffer(), layout, getVkStage(stage), index * 4, 4, &binaryValue);
+    vkCmdPushConstants(cmdbuf->buffer(), layout, getVkStage(stage), index * ENTRY_SIZE, ENTRY_SIZE,
+            &binaryValue);
 }
 
 VulkanProgram::VulkanProgram(VkDevice device, Program const& builder) noexcept
     : HwProgram(builder.getName()),
       VulkanResource(VulkanResourceType::PROGRAM),
-      mInfo(new PipelineInfo(builder)),
+      mInfo(new(std::nothrow) PipelineInfo(builder)),
       mDevice(device) {
 
     constexpr uint8_t UBO_MODULE_OFFSET = (sizeof(UniformBufferBitmask) * 8) / MAX_SHADER_MODULES;

filament/backend/src/vulkan/VulkanHandles.h

@@ -184,6 +184,7 @@ using PushConstantNameArray = utils::FixedCapacityVector<char const*>;
 using PushConstantNameByStage = std::array<PushConstantNameArray, Program::SHADER_TYPE_COUNT>;
 
 struct PushConstantDescription {
+
     explicit PushConstantDescription(backend::Program const& program) noexcept;
 
     VkPushConstantRange const* getVkRanges() const noexcept { return mRanges; }
@@ -194,6 +195,9 @@ struct PushConstantDescription {
             uint8_t index, backend::PushConstantVariant const& value);
 
 private:
+    static constexpr uint32_t ENTRY_SIZE = sizeof(uint32_t);
+
+    utils::FixedCapacityVector<backend::ConstantType> mTypes[Program::SHADER_TYPE_COUNT];
     VkPushConstantRange mRanges[Program::SHADER_TYPE_COUNT];
     uint32_t mRangeCount;
 };

filament/backend/src/vulkan/caching/VulkanPipelineLayoutCache.h

@@ -45,9 +45,11 @@ class VulkanPipelineLayoutCache {
     struct PipelineLayoutKey {
         using DescriptorSetLayoutArray = std::array<VkDescriptorSetLayout,
                 VulkanDescriptorSetLayout::UNIQUE_DESCRIPTOR_SET_COUNT>;
-        DescriptorSetLayoutArray descSetLayouts = {};
-        PushConstantKey pushConstant[Program::SHADER_TYPE_COUNT] = {};
+        DescriptorSetLayoutArray descSetLayouts = {};                   // 8 * 3
+        PushConstantKey pushConstant[Program::SHADER_TYPE_COUNT] = {};  // 2 * 3
+        uint16_t padding = 0;
     };
+    static_assert(sizeof(PipelineLayoutKey) == 32);
 
     VulkanPipelineLayoutCache(VulkanPipelineLayoutCache const&) = delete;
     VulkanPipelineLayoutCache& operator=(VulkanPipelineLayoutCache const&) = delete;

filament/src/details/Material.cpp

@@ -933,7 +933,13 @@ 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.
-    // mPushConstants[(uint8_t) ShaderStage::VERTEX] = PUSH_CONSTANT_NAMES;
+    // 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] });
+    // }
 }
 
 void FMaterial::processDepthVariants(FEngine& engine, MaterialParser const* const parser) {

filament/src/details/Material.h

@@ -315,15 +315,14 @@ class FMaterial : public Material {
     SamplerBindingToNameMap mSamplerBindingToNameMap;
     // Constants defined by this Material
     utils::FixedCapacityVector<MaterialConstant> mMaterialConstants;
-
     // A map from the Constant name to the mMaterialConstant index
     std::unordered_map<std::string_view, uint32_t> mSpecializationConstantsNameToIndex;
-
     // current specialization constants for the HwProgram
     utils::FixedCapacityVector<backend::Program::SpecializationConstant> mSpecializationConstants;
 
     // current push constants for the HwProgram
-    std::array<utils::FixedCapacityVector<char const*>, backend::Program::SHADER_TYPE_COUNT>
+    std::array<utils::FixedCapacityVector<backend::Program::PushConstant>,
+            backend::Program::SHADER_TYPE_COUNT>
             mPushConstants;
 
 #if FILAMENT_ENABLE_MATDBG

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

@@ -73,22 +73,22 @@ enum class ReservedSpecializationConstants : uint8_t {
 };
 
 
-using PushConstantType = backend::ConstantType;
 
 // 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;
+
 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";
-
 // 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;