Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Align validation with greatest common requirement #776

Merged
merged 1 commit into from
Jan 24, 2023

Conversation

nmr8acme
Copy link
Contributor

While the Metal backend will accept a larger buffer neither the GL nor the DirectX backend will. By making validation stricter we prevent code developed with the Metal backend from crashing when run on GL or DX.

See e.g. validation for GL:

sokol/sokol_gfx.h

Line 7985 in b3aecb2

SOKOL_ASSERT(_sg.gl.cache.cur_pipeline->shader->cmn.stage[stage_index].uniform_blocks[ub_index].size == data->size);

Also, it may only crash with validation enabled.

@floooh
Copy link
Owner

floooh commented Jan 20, 2023

Hmmm..... I think this should be solved differently, already in the validation function for sg_make_shader(), the uniform block sizes provided there should be checked against a common max size (not sure at the moment what this size is, either 16 or 64 KBytes I'd guess).

The other option would be to make the max uniform block size a runtime variable exposed via sg_query_features() sg_query_limts(), this wouldn't help much for writing portable code, but would allow code to take advantage of bigger limits in some 3D backends.

PS: for the GL backend, this would need to be computed from GL_MAX_VERTEX_UNIFORM_VECTORS I guess - and this can be pretty small on older mobile GPUs, so I guess it's better to make it a runtime value, because otherwise this would be a harsh restriction for people who don't even care about older mobile GPUs).

@floooh
Copy link
Owner

floooh commented Jan 20, 2023

I wrote a ticket for now: #777

@nmr8acme
Copy link
Contributor Author

nmr8acme commented Jan 20, 2023

Maybe I am misunderstanding, or maybe my fix is wrong, but it sounds like you're talking about a separate issue.

The issue I identified and tried to fix doesn't relate to max UB size, but rather validating that the sg_range argument passed to sg_apply_uniforms is properly validated size wise. The GL backend validates that the range is == the expected size, but the Metal backend, as well as _sg_validate_apply_uniforms, both accept ranges >= the expected size. This leads to too-large buffers passed to sg_apply_uniforms to succeed on Metal, but fail validation on GL. See what I mean?

edit: grammar

@floooh
Copy link
Owner

floooh commented Jan 20, 2023

Doh... github just threw away my carefully researched reply :D

TL;DR: yes, it was a misunderstanding and your PR makes sense :)

GL and D3D11 both have an assert in their apply-uniforms backend function which checks for the exact size:

  • sokol/sokol_gfx.h

    Line 7985 in b3aecb2

    SOKOL_ASSERT(_sg.gl.cache.cur_pipeline->shader->cmn.stage[stage_index].uniform_blocks[ub_index].size == data->size);
  • sokol/sokol_gfx.h

    Line 9938 in b3aecb2

    SOKOL_ASSERT(data->size == _sg.d3d11.cur_pipeline->shader->cmn.stage[stage_index].uniform_blocks[ub_index].size);

...but for reasons lost in history, in the Metal backend this is a <=:

sokol/sokol_gfx.h

Line 11705 in b3aecb2

SOKOL_ASSERT(data->size <= _sg.mtl.state_cache.cur_pipeline->shader->cmn.stage[stage_index].uniform_blocks[ub_index].size);

...I checked via git blame, and it's always been like that, so it's most likely just an oversight (although I'm not 100% sure if I had some problems with padding bytes at the end of uniform block structs in Metal which then resulted in some sort of size mismatch, but in any case that should no longer be an issue.

One request, while you're at it, can you maybe also fix the assert in _sg_mtl_apply_uniforms() from <= to == (here:

sokol/sokol_gfx.h

Line 11705 in b3aecb2

SOKOL_ASSERT(data->size <= _sg.mtl.state_cache.cur_pipeline->shader->cmn.stage[stage_index].uniform_blocks[ub_index].size);
).

Apart from that, the ticket I wrote still makes sense, even if it's an entirely different topic, heh.

@nmr8acme
Copy link
Contributor Author

RE lost reply: Ugh, I hate it when that happens.

RE validation consistency in _sg_mtl_apply_uniforms: Yep, will do

While the Metal backend will accept a larger buffer neither
the GL nor the DirectX backend will. By making validation
stricter we prevent code developed with the Metal backend
from crashing when run on GL or DX.
@nmr8acme
Copy link
Contributor Author

Should be good to go, LMK if you see anything I missed.

@floooh
Copy link
Owner

floooh commented Jan 23, 2023

I'll try to test and merge in the next few days. Just wrapping up the sokol_audio.h update today.

@floooh floooh merged commit 185ec80 into floooh:master Jan 24, 2023
@floooh
Copy link
Owner

floooh commented Jan 24, 2023

Ok, merged. Many thanks! I did some randomly picked tests in the sokol-samples, my emulators, and v6502r, and they all work fine.

@nmr8acme
Copy link
Contributor Author

Many thanks to you, I would have been stuck on OpenGL ES 2 for a lot longer without sokol_gfx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants