Fix push constants with ext sampler (VK)

[anishmgoyal]

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]

@poweifeng @phoenixxxx @pixelflinger @rafadevai

[rafadevai]

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

[anishmgoyal]

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.

[poweifeng]

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

[anishmgoyal]

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

[poweifeng]

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.

[anishmgoyal]

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?

[poweifeng]

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.

[anishmgoyal]

added exactly that comment, sums it up fairly well.

[pixelflinger]

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.

[poweifeng]

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

[anishmgoyal]

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

[pixelflinger]

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

[anishmgoyal]

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).

[pixelflinger]

I meant, in the comment :-)

[pixelflinger]

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

[anishmgoyal]

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.

[pixelflinger]

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.

[poweifeng]

lgtm

[anishmgoyal]

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