metal uniform buffer size issue

[m4b]

so i'm seeing this issue on metal iOS:

SIZEOF UNIFORM: 204
validateFunctionArguments:3478: failed assertion `Vertex Function(main0): argument uniforms[0] from buffer(0) with offset(0) and length(204) has space for 204 bytes, but argument has a length(208).'

when i print my uniform std::mem::size_of above, it's 204, but metal refuses to run with this assertion error, so something is adding 4 bytes to the uniform buffer argument? if i add 4 to the size here it proceeds without error (presumably because it has enough space for the 208 byte argument now):

        let uniform_buf = device.create_buffer(
            &wgpu::BufferDescriptor {
                size: uniform_size + 4,
                usage: wgpu::BufferUsage::UNIFORM | wgpu::BufferUsage::TRANSFER_DST,
            }
        );

this doesn't seem to happen on macOS only iOS; i'm guessing something is attempting to add padding so its mod sizeof(ptr) ?

[kvark]

Looks like the Rust disagrees with SPIRV/MSL about the size of some struct? Can you provide the struct itself on both sides?

[m4b]

Sorry I’m on phone but on both macos, Linux and iOS sizeof always reports 204; I’ll send you the actual struct tonight.

But basically it was some arrays of floats for a world, view and proj, plus a camera pos iirc?

[m4b]

#[repr(C)]
#[derive(Debug, Copy, Clone)]
struct Uniform {
    world: Matrix4<f32>,
    view: Matrix4<f32>,
    proj: Matrix4<f32>,
    light: [f32; 3],
}

Here is the uniform; note I also just ran this on macOS using a Xcode project and it exhibits the behavior, but not run i run a pure rust cli version, which is.. weird.

[kvark]

I think we should make sure that at some point during SPIRV sanitization we round up the constant buffer sizes to 16 bytes, and have the Rust code taking that into account when allocating buffer sizes.

@Kangz we might need to bring this up with the working group (re: padding the uniform buffers to 16 byte alignment at SPIRV level).

[Kangz]

Yeah we're going to have that problem in various places like vertex buffers too that will need to be rounded to 4 bytes etc. We could open a gpuweb issue and tag @dneto0.

[seivan]

@m4b How did you get the size of the struct?
uniform_size?

[m4b]

std::mem::size_of

[seivan]

for iOS

This worked for me

#[derive(Clone, Debug)]
struct Vertex {
    pos: [f32; 4],
    tex: [f32; 2],
}

#[derive(Clone, Debug)]
struct QuadInstance {
    coord: [f32; 16],
}

For the matrices, I ended up doing &model.data.as_ref()[0..16]);

[m4b]

Your uniforms already 16 byte aligned though, no?

[seivan]

How is your Matrix4 interpreted when you cast_slice?

[m4b]

It’s repr c, but I don’t think it’s relevant. Seems pretty clear metal expects 16 byte alignment for uniforms and 204 sizeof isn’t 16 byte aligned.

[seivan]

I barely know this stuff but looking at the metal docs, I don't know if it expects 16 byte alignment for uniforms, I suspect it expects the biggest alignment that fits the total size.

So if your size is 204 that doesn't fit % 16 but 208 does.

204 % 16  = 12
204 + 16 - 12  = 208

That will give you the correct size with appropriate alignment.

Can't we pass alignment and size to the buffer descriptor so it can sort it out internally?

@kvark would something like this work?

match buffer_size % alignment  {
      0 => { buffer_size },
      error => { buffer_size + alignment - error }
}

[seivan]

Also is there a way to get stride and alignment out of a struct?
Swift has this

So an appropriate way to count the buffer size would be something like

UnsafeMutableRawPointer.allocate(
        bytes: count * MemoryLayout<Uniform>.stride,
        alignedTo: MemoryLayout<Uniform>.alignment)

This could be added on the BufferDescriptor to take in an alignment and size and use the snippet pasted above for calculating the correct size.

[kvark]

Whatever solution we do, it seems like getting gpuweb/gpuweb#678 first would benefit it. It will force us to consider the shader-reflected size.

[cwfitzgerald]

As we haven't seen any reports due to this, going to close it due to the new backends.