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

Fix postprocessing shader support on macOS #11823

Merged
merged 1 commit into from
Feb 22, 2019
Merged

Fix postprocessing shader support on macOS #11823

merged 1 commit into from
Feb 22, 2019

Conversation

vit9696
Copy link
Contributor

@vit9696 vit9696 commented Feb 22, 2019

Check GL_ARB_shading_language_420pack availability, which SPIRV-Cross assumes
present by default, causing explicit binding location generation during shader
translation.

Check GL_ARB_shading_language_420pack availability, which SPIRV-Cross assumes
present by default, causing explicit binding location generation during shader
translation.
@@ -335,6 +335,7 @@ void CheckGLExtensions() {
gl_extensions.EXT_blend_func_extended = g_set_gl_extensions.count("GL_EXT_blend_func_extended") != 0;
gl_extensions.ARB_conservative_depth = g_set_gl_extensions.count("GL_ARB_conservative_depth") != 0;
gl_extensions.ARB_shader_image_load_store = (g_set_gl_extensions.count("GL_ARB_shader_image_load_store") != 0) || (g_set_gl_extensions.count("GL_EXT_shader_image_load_store") != 0);
gl_extensions.ARB_shading_language_420pack = (g_set_gl_extensions.count("GL_ARB_shading_language_420pack") != 0);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also set this to true down lower, see here:

// ARB_texture_storage = true;

Since AFAIK, this extension can be elided in the extensions string on 4.2+.

-[Unknown]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if it is intended. Since 4.2 it is the core functionality indeed, yet the extension support has nothing to do to it. For instance, SPIR-V cross has the following code, and it makes good sense to me:

can_use_binding = options.enable_420pack_extension || (options.version >= 420);

I am fine to change it if gl_extensions is just mislabeled and does not mean extension availability but rather OpenGL features. However, I would suggest to rename it to gl_features in this case.

@unknownbrackets unknownbrackets added this to the v1.8.0 milestone Feb 22, 2019
@hrydgard
Copy link
Owner

I think it's ok but not 100% sure of the semantics either of enabling it on post-420 versions. But let's find out I guess.

@hrydgard hrydgard merged commit 5a01bfb into hrydgard:master Feb 22, 2019
@unknownbrackets
Copy link
Collaborator

If we want to move the checks to every usage for every extension that's built into core, we should do that consistently. I don't agree with not doing it only in this case.

Although, really, it seems like we might as well just always use false since we probably get no benefit from this extension for post shaders.

-[Unknown]

@vit9696
Copy link
Contributor Author

vit9696 commented Feb 23, 2019

@unknownbrackets just in case, as I did not mention it explicitly in my previous message. Changing gl_extensions.ARB_shading_language_420pack to true when OpenGL 4.2 is supported will result in generating ARB_shading_language_420pack code in OpenGL 4.1 shaders we use.

This will not result in generation of non conformant shaders, but will just break on implementations that do not support ARB_shading_language_420pack (which is fine) and do disable OpenGL 4.2 features when targeting OpenGL 4.1.

Consequentially it seems to me that the current design may be no good in several other places you, where you took the route of checking OpenGL version. If you are concerned about consistency, I would suggest making a PR renaming gl_extensions to gl_features. An important logic change would also be passing its generator the OpenGL version targeted, which would be <= the actual supported OpenGL version on the system. I believe for PPSSPP that would be 4.1.

@unknownbrackets
Copy link
Collaborator

Whether it's called gl_features, gl_extensions, gl_extension_names_of_features_we_use, or anything else - gl_extensions is mostly a historic name. I'm not a huge fan of changing all the code on ruining blame history in this case.

We of course support anything from OpenGL 2.0 to OpenGL 4.6. So you could say we're "targeting" OpenGL 2.0. We check that various OpenGL functionality, exposed via extensions, is usable in many places across many operating systems. SPRIV cross and post-render shaders are a fraction of this usage.

For example, we check if the things exposed by ARB_copy_image are usable to know if we can use glCopyImageSubData rather than a draw. We check EXT_texture_filter_anisotropic to see if we can send GL_TEXTURE_MAX_ANISOTROPY_EXT,

To be clear, the larger % of OpenGL implementations we deal with expose ARB_shading_language_420pack already in the extensions list in newer versions. For example, on my OpenGL 4.6 Windows NVIDIA context, it is listed. This is very common, and it just means that when we write code and test it - we won't notice that it's a problem to only check for the extension, which is there 90% of the time. It's been a source of things breaking, or at least new features not working, on those drivers in the 10% in the past. I'm just not wanting to relive those problems.

I'm chiefly concerned with the runtime shaders we generate, which don't run through the spriv-cross code (although perhaps they might eventually - #11412.)

-[Unknown]

@vit9696
Copy link
Contributor Author

vit9696 commented Feb 23, 2019

Mh, but basically the same issue happens here, when 10% of the drivers behave differently from the rest. Not sure what is the correct approach.

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.

3 participants