[gl] Push constants are mapped to incompatible uniform types

[simonrepp]

Version affected: master as of the current state in the 0.5.0 release (and probably before that already)

In my project (which runs fine with the vulkan backend but does not yet work with the gl backend) I'm using a few push constants, e.g.:

cmd_buffer.push_graphics_constants(
    &self.some_pipeline_layout,
    ShaderStageFlags::VERTEX,
    0,
    &[some_f32.to_bits(), some_f32.to_bits()]
);

In the shader this maps just to an unspectacular vec2:

layout (push_constant) uniform Translation {
    vec2 vector;
} translation;

In my efforts to get the gl backend working I dug around in gfx-hal a few hours and discovered that the above call triggers a mapping here:

https://github.com/gfx-rs/gfx/blob/a9e2c7ed5974844f4dc8c1f0d59b151a8260ba5c/src/backend/gl/src/command.rs#L1453-L1465

Now the issue with this seems to be that if there are uniforms present (they are taken from the CommandBuffer's cache field) the code simply picks and returns the first one regardless of its type, which in my project sometimes is a UniformDesc of type glow::SAMPLER_2D, sometimes of type glow::UNSIGNED_INT, but so far has never been a type that is actually fitting, therefore at some later point we run into the panic!("Unsupported uniform datatype!") branch of this:

https://github.com/gfx-rs/gfx/blob/a9e2c7ed5974844f4dc8c1f0d59b151a8260ba5c/src/backend/gl/src/queue.rs#L890-L901

It feels like the solution here might be that instead of taking the first uniform found, the list is searched for the first one that is compatible (which might have its own complexity given that the uniform could be a struct of different types, unless we can just simply treat it as bytes and don't care (?)) and that a new compatible uniform is created, appended to the cache and used if there is no existing one available.

If this approach is at all plausible and doable in a reasonable timeframe I am gladly willing to give this a try, but I'd be happy about some feedback from your side, as I feel I might be going on a wild goose chase based on false assumptions here due to my limited insight into the project. ;)

Thanks for your time, gfx-hal is really cool stuff! :+1:

[kvark]

Thank you for filing a very detailed issue! The code you posted from command.rs doesn't ring a bell for me: it just looks for the uniform at a specific offset given, and it assumes no intersections. So there isn't really a choice to look for compatible ones (and neither there should be one).

However, looking at the place where these uniforms are populated makes me suspicious: https://github.com/gfx-rs/gfx/blob/a9e2c7ed5974844f4dc8c1f0d59b151a8260ba5c/src/backend/gl/src/device.rs#L862-L885

It may come down to how SPIRV-Cross generates that GLSL code. If you run with RUST_LOG=gfx_backend_gl=debug and you have something like env_logger::init() in your program, you'll see the proded GLSL source dumped, and we can go from there.

[simonrepp]

I see what you're getting at, so basically command.rs:1456 is not incorrectly interacting with its uniforms context, but the uniforms context it gets passed is faulty to begin with, if I understood you correctly.

Luckily you just gave me the pointers for the logging stuff on matrix ;) so I'll gladly follow up with your suggestions - I'll give it a try and post the results then!

[kvark]

I'm not well versed into this logic, I wasn't one implementing it. Just trying to make sense of the code myself. Let's see what the generated shader looks like.

[simonrepp]

There's quite a few shaders involved so I'll post just the comparison for the smallest shader, and another one for the one shader I definitely know to be involved in the breakage described above (although I suspect more are affected from my debugging results so far)

So here's the smallest shader (icon.frag) in its original form:

#version 450

layout (push_constant) uniform Icon {
    layout (offset = 8) uint code;
} type;

layout(location = 0) in vec4 v_rgba;

layout(location = 0) out vec4 f_rgba;

void main() {
    f_rgba = v_rgba;
}

... and the smallest shader (icon.frag) in its cross-compiled form:

#version 460

struct Icon
{
    uint code;
};

uniform Icon type;

layout(location = 0) out vec4 f_rgba;
layout(location = 0) in vec4 v_rgba;

void main()
{
    f_rgba = v_rgba;
}

[kvark]

This looks wrong. The binding code will look for something at offset 8, while populating the uniforms starting with offset 0, which means the code will be registered at offset 0. Perhaps, we need to make SPIRV-Cross insert some padding in place of the empty space inside the uniform structs.

[simonrepp]

And here's xy_uv_translate.vert (the one definitely involved in the error context) in its original form:

#version 450

layout (push_constant) uniform Translation {
    vec2 vector;
} translation;

layout (set = 0, binding = 0) uniform UniformBufferObject {
    vec3 background;
    vec4 widget_surface;
    vec4 widget_shadow;
    vec4 text;
    vec4 playhead;
    vec4 waveform;
    vec4 waveform_centerline;
    vec2 dimensions;
} interface_globals;

layout(location = 0) in vec2 a_xy;
layout(location = 1) in vec2 a_uv;

layout(location = 0) out vec2 v_xy_translated;
layout(location = 1) out vec2 v_uv;

out gl_PerVertex {
    vec4 gl_Position;
};

void main() {
    vec2 a_xy_translated = a_xy + translation.vector;

    // Transform from window coordinate space (0..width, 0..height, top left origin)
    // to vulkan coordinate space (-1..1, -1..1, top left origin)
    gl_Position = vec4(a_xy_translated * 2 / interface_globals.dimensions - 1, 0, 1);

    v_xy_translated = a_xy_translated;
    v_uv = a_uv;
}

... and xy_uv_translate.vert in its cross-compiled form:

#version 460

layout(binding = 0, std140) uniform UniformBufferObject
{
    vec3 background;
    vec4 widget_surface;
    vec4 widget_shadow;
    vec4 text;
    vec4 playhead;
    vec4 waveform;
    vec4 waveform_centerline;
    vec2 dimensions;
} interface_globals;

struct Translation
{
    vec2 vector;
};

uniform Translation translation;

layout(location = 0) in vec2 a_xy;
layout(location = 0) out vec2 v_xy_translated;
layout(location = 1) out vec2 v_uv;
layout(location = 1) in vec2 a_uv;

void main()
{
    vec2 a_xy_translated = a_xy + translation.vector;
    gl_Position = vec4(((a_xy_translated * 2.0) / interface_globals.dimensions) - vec2(1.0), 0.0, 1.0);
    v_xy_translated = a_xy_translated;
    v_uv = a_uv;
    gl_Position.y = -gl_Position.y;
}

[simonrepp]

This looks wrong. The binding code will look for something at offset 8, while populating the uniforms starting with offset 0, which means the code will be registered at offset 0. Perhaps, we need to make SPIRV-Cross insert some padding in place of the empty space inside the uniform structs.

Unless the emulating magic gfx-hal performs splits this into two separate uniforms for the gl backend I tend to agree - in Vulkan the push constant here is basically 12 bytes split into the vec2 translation at offset 0 and the uint code at offset 8, the translation being only accessed in the vertex shader, the code only accessed in the fragment shader (but vulkan naturally doesn't care because both get the same block of memory regardless).

[simonrepp]

I have another suspicion, that maybe although shaderc and SPIRV-Cross do not complain about my shader definitions, something about them makes the latter trip up:

If one of my vertex shaders defines this:

layout (push_constant) uniform Translation {
    vec2 vector;
} translation;

... and one of my fragment shaders defines this:

layout (push_constant) uniform Icon {
    layout (offset = 8) uint code;
} type;

... it's arguably a bit hard to figure out that they, and how they, interlink without looking at the two of them combined - maybe this trips up SPIRV-Cross here?

[simonrepp]

Or in other words, could it be that for SPIRV-Cross to make sense of this I have to explicitly account for the 4 bytes at offset 8 as well (e.g. with padding or the explicit unused uint declaration as well) to indicate that more stuff will be put in here that is meant only for the fragment shader?

[kvark]

This is where the pipeline layout comes into play. In other backends, pipeline layout translates into a set of overrides in SPIRV-Cross options to place the resource bindings (and push constants) correctly. We need to double check the way GL backend does that, whether it considers the pipeline layout properly, whether it talks to SPIRV-Cross properly, and whether that one does a good job with the GLSL backend.

[kvark]

https://github.com/gfx-rs/gfx/blob/a9e2c7ed5974844f4dc8c1f0d59b151a8260ba5c/src/backend/gl/src/device.rs#L186-L191 heh, looks like something needs to be here :)

[simonrepp]

I see, thanks for the explanation! I have somewhat of an idea what's the issue and thanks to your last comment also where to look. I can offer to play around with this a bit in the next days if you like (but of course feel free to just fix it, not territorial here haha :)), I don't know yet if I'm capable of tackling this anyhow, but I'm certainly willing to give it a shot if I can be of help here. Thanks already for all the assistance, much appreciated!

[kristoff3r]

I've hit this issue as well on some machines, but not on others, with the same codebase. After looking at it for a bit, I've found at least one problem:

https://github.com/gfx-rs/gfx/blob/91a4d0e1ba15b90218e9f86ab5599cdd077339ba/src/backend/gl/src/device.rs#L873-L882

The comment is wrong, when I get the crash there is indeed a Sampler2D uniform at that location, and if I filter it away the crash goes away (but I get horrible fps, which isn't a problem on the machines without the crash). It might have something to do with the Sampler2D being created from the combine_separate_images_and_samplers function?

[kvark]

I too wonder why the comment says Sampler2D will not show up.