VideoCommon: Fix SSBO layout and remove associated "bug"

[TellowKrinkle]

Bug was ours, not AMD's

If you don't specify a layout, the driver's free to do whatever it wants and we're supposed to ask it what it did. AMD probably went for std140 layout (where int[4] would have each int padded to 16 bytes)

I don't see any reason we'd want an SSBO without std430, so I stuck it into the macro, but we could put it outside like we do for UBOs if you'd prefer that

[Pokechu22]

Unfortunately my old AMD laptop is dead, so I don't have the ability to test this (unlike I did with #9764). This sounds plausible though.

[TellowKrinkle]

I can confirm that on a Radeon Pro 5600M using AMD's 20.45.40.15 driver (which is the newest offered to Intel Macs running windows), removing the bug entry breaks bbox and adding std430 fixes it

[iwubcode]

I can confirm this works on my AMD card on Windows.

Code wise, my only concern is that the std430 layout seems to require OpenGL 4.3. Our minimum supported version is 3.0 AFAICT. So there are quite a few places this would cause a problem.

As an aside, I'd love to update our minimum requirement to something newer..

[mbc07]

Wouldn't be possible to use the std430 layout by default but somehow fall back to the int4 approach if OpenGL 4.3 isn't available?

[iwubcode]

Wouldn't be possible to use the std430 layout by default but somehow fall back to the int4 approach if OpenGL 4.3 isn't available?

Yeah something like that would be possible, it's not being done though.

The current way we handle this sort of scenario is adding a flag that says whether the feature is available. Then that flag would be set to true if OpenGL 4.3 or higher exists. If it's true you'd use the std430, if it wasn't then you'd use the other approach.

We already have many examples of this. It's just very klunky to have so many checks around the code base. That's why it would be great if we could update our minimum version. But that won't likely happen in this PR.

EDIT:

For reference, OpenGL 4.3 apparently came out in 2012! TEN years ago. Our minimum required version 3.0, came out in 2008..

[TellowKrinkle]

The document on the ssbo extension specifically mentions std430, so I assume anything that supports ssbos also supports std430 layout?

[iwubcode]

@TellowKrinkle - yes, however for OGL, we only enable that extension if bSupportsFragmentStoresAndAtomics is defined. That also sets the flag on whether we support BBOX which I believe is where we use SSBOs.

So I imagine we only want to set std430 if that flag is set. Not sure what would happen if we don't on those systems, maybe a GLSL compiler error?

For Vulkan, I think we're fine with the current assumption.

[TellowKrinkle]

bSupportsFragmentStoresAndAtomics is the "SSBO supported" flag

dolphin/Source/Core/VideoBackends/OGL/OGLRender.cpp

Lines 424 to 425 in 23ed611

	g_Config.backend_info.bSupportsFragmentStoresAndAtomics =
	GLExtensions::Supports("GL_ARB_shader_storage_buffer_object");

Therefore

1. If a driver doesn't support SSBOs, then bSupportsFragmentStoresAndAtomics will be false
2. If bSupportsFragmentStoresAndAtomics is false, bbox will be disabled
3. If bbox is disabled, we will not put SSBOs into any glsl shaders
4. If we don't put SSBOs into any glsl shaders, SSBO_BINDING(n) will also not appear in any shaders (where n is a number)
5. If SSBO_BINDING(n) does not appear in any shaders, the #define SSBO_BINDING(x) layout(std430) will never be expanded anywhere
6. If the define isn't expanded, the glsl compiler will never be asked to compile a shader that uses layout(std430)

[iwubcode]

I had assumed that just having the std430 in the shader header would be enough to be problematic but reading the spec, the #define operates similarly to a #define in C, so like you say it won't be expanded.

[iwubcode]

Sorry @TellowKrinkle , I should have followed my train of thought all the way through but I was running out the door. Anyway, this LGTM and it's been tested.

[TellowKrinkle]

I had assumed that just having the std430 in the shader header would be enough to be problematic

I mean, for all the compiler knows, I'm going to expand it into a function after uint std430 = 4;
Unless they've reserved all identifiers starting with std or something, that should be perfectly acceptable