Fix push constants with ext sampler (VK) #9445

anishmgoyal · 2025-11-19T21:39:51Z

Currently, in Vulkan, we set the pipeline layout when binding the pipeline, which doesn't happen until draw time when external samplers are present. In cases like that, we push constants with the layout VK_NULL_HANDLE, which is not valid. Instead, we can wait until the pipeline layout is known, and then flush all push constants.

anishmgoyal · 2025-11-19T21:40:07Z

@poweifeng @phoenixxxx @pixelflinger @rafadevai

rafadevai · 2025-11-19T22:15:43Z

filament/backend/src/vulkan/VulkanDriver.cpp

        }
+        // If we're doing bind in draw, it's possible there are some push constants that weren't
+        // written out because we didn't have the pipelineLayout yet. Push them now.
+        program->flushPushConstants(pipelineLayout);


this should be inside the above if, only need to do this when we bind a new pipeline

technically we probably shouldn't even have that if-statement, because it's guaranteed that condition will be true with the code written as it is today. that said, i'm happy to move this into the if block.

I do think that if block is still necessary, because you could have multiple draw calls for one pipeline. It's just an optomization.

ahhhhh ok - then in that case, yes, this absolutely should be in the if block.

sorry, thinking about it some more. I think it needs to be outside the conditional. This is also valid

bindPipeline setPushConstant draw setPushConstant draw

We need to be able do that second update of push constants.

in the second case, we wouldn't have the push constants in a list that needs to be flushed. we're only storing values if the layout is null. in the second case, the layout should not be null. unless i'm missing something?

Ah yes you're right. The mPipelinState.pipelineLayout thing. Can you add a comment to that effect above the program->flushPushConstants(pipelineLayout);? Something like, "Further push constant updates after the pipeline is bound will immediately set the constants because the pipeline is known (see VulkanProgram::writePushConstant)".

Because this logic is getting harder to follow with intertwining paths.

added exactly that comment, sums it up fairly well.

pixelflinger · 2025-11-20T06:17:35Z

filament/backend/src/vulkan/VulkanDriver.cpp

    auto const& [doBindInDraw, bundle] = mPipelineState.bindInDraw;

    fvkutils::DescriptorSetMask setsWithExternalSamplers = {};
    if (doBindInDraw) {


is this expected to be the common case? If not, it should be UTILS_UNLIKELY and in fact I think this should all be moved into a separate function.

it's common if you have externally sampled textures, otherwise will never be hit. So not sure how one would quantify that with UTILS_LIKELY/UNLIKELY

i think this is out of scope for this change, and a much broader question

pixelflinger · 2025-11-20T06:19:59Z

filament/backend/src/vulkan/VulkanHandles.h

+        // It's possible that we don't have the layout yet. But, we don't want to "forget"
+        // about the push constant! We can flush the constants later, once the layout is set.


can you explain in which scenario we have or don't have a layout?

for external samplers, in bindPipeline(), there's an early return - prior to setting the layout. push constants are then set after that, and finally, the layout is selected and bound in draw2() (called after binding push constants in the render pass).

I meant, in the comment :-)

pixelflinger · 2025-11-20T06:20:57Z

filament/backend/src/vulkan/VulkanHandles.h

+
    PipelineInfo* mInfo;
    VkDevice mDevice = VK_NULL_HANDLE;
+    std::vector<PushConstantInfo> mQueuedPushConstants;


should we do a "reasonable" .reserve() for this, so we avoid too many heap allocations?

i did consider that, but it's hard to know what a reasonable number is. the driver api takes one push constant at a time, and only the frontend knows how many push constants will be pushed. it's possible that actually, only the app knows about the push constants.

Actually, we have MAX_PUSH_CONSTANT_COUNT (32). So I think we can be pragmatic here and maybe do a reserve with 8 for instance.

can we just use an array with the fixed MAX_PUSH_CONSTANT_COUNT size?

Currently, in Vulkan, we set the pipeline layout when binding the pipeline, which doesn't happen until draw time when external samplers are present. In cases like that, we push constants with the layout VK_NULL_HANDLE, which is not valid. Instead, we can wait until the pipeline layout is known, and then flush all push constants.

Some of the logic related to external samplers is getting to be a bit convoluted.

poweifeng

lgtm

anishmgoyal · 2025-11-24T13:47:10Z

@pixelflinger please re-review when you get a chance!

rafadevai · 2025-12-10T18:40:54Z

filament/backend/src/vulkan/VulkanDriver.cpp

+            // added to a list of pending constants, so we'll flush them now.
+            // Further push constant updates after the pipeline is bound will immediately set
+            // the constants because the layout is non-null (see VulkanProgram::writePushConstant).
+            program->flushPushConstants(pipelineLayout);


On a different approach and this for me is better, is to just use the RGB layout of the pipeline to set the push constants?

I expect the pipeline layouts to be compatible as described here https://registry.khronos.org/VulkanSC/specs/1.0-extensions/html/vkspec.html#descriptorsets-compatibility

vkCmdPushConstants takes layout as a parameter. i don't see it implied in the docs that it's sufficient to use a compatible layout instead of the actual layout that the constants are being pushed for, so i don't believe we should make that change.

while they technically might currently have the same push constant layouts, it'd be fairly flimsy. any change where, say, we intro a push constant specific to external samplers (for some reason we can't anticipate today), and we'd likely be existing in the realm of undefined behavior.

Youre right, it does say it have to identically layout, with all the creation parameters the same, which is not true for external samplers.

Too bad :(

poweifeng approved these changes Nov 19, 2025

View reviewed changes

rafadevai reviewed Nov 19, 2025

View reviewed changes

pixelflinger requested changes Nov 20, 2025

View reviewed changes

anishmgoyal force-pushed the fix_external_samplers branch from 5a5cb7e to 91a9b26 Compare November 21, 2025 18:03

Address PR comment, add clarifying comment

191ed71

Some of the logic related to external samplers is getting to be a bit convoluted.

anishmgoyal force-pushed the fix_external_samplers branch from 8035247 to 191ed71 Compare November 21, 2025 19:12

poweifeng approved these changes Nov 21, 2025

View reviewed changes

rafadevai reviewed Dec 10, 2025

View reviewed changes

		// It's possible that we don't have the layout yet. But, we don't want to "forget"
		// about the push constant! We can flush the constants later, once the layout is set.

Fix push constants with ext sampler (VK) #9445

Are you sure you want to change the base?

Fix push constants with ext sampler (VK) #9445

Conversation

anishmgoyal commented Nov 19, 2025

Uh oh!

anishmgoyal commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anishmgoyal Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

poweifeng left a comment

Choose a reason for hiding this comment

Uh oh!

anishmgoyal commented Nov 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anishmgoyal Dec 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anishmgoyal Nov 21, 2025 •

edited

Loading

anishmgoyal Dec 15, 2025 •

edited

Loading