Skip to content

[Web] Fix checking for OpenGL extensions with Emscripten 3.1.51 and later #93560

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.

Sign up for GitHub

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

Merged
akien-mga merged 1 commit into from
Jun 25, 2024
Merged

[Web] Fix checking for OpenGL extensions with Emscripten 3.1.51 and later #93560

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions drivers/gles3/storage/config.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@
#include "../rasterizer_gles3.h"
#include "texture_storage.h"

#ifdef WEB_ENABLED
#include <emscripten/html5_webgl.h>
#endif

using namespace GLES3;

#define _GL_MAX_TEXTURE_MAX_ANISOTROPY_EXT 0x84FF
Expand All @@ -44,6 +48,23 @@ Config *Config::singleton = nullptr;
Config::Config() {
singleton = this;

#ifdef WEB_ENABLED
// Starting with Emscripten 3.1.51, glGetStringi(GL_EXTENSIONS, i) will only ever return
// a fixed list of extensions, regardless of what additional extensions are enabled. This
// isn't very useful for us in determining which extensions we can rely on here. So, instead
// we use emscripten_webgl_get_supported_extensions() to get all supported extensions, which
// is what Emscripten 3.1.50 and earlier do.
{
char *extension_array_string = emscripten_webgl_get_supported_extensions();
Copy link
Member

@clayjohn clayjohn Jun 24, 2024

Choose a reason for hiding this comment

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

Do we have any guarantee that this will actually list all supported extensions?

The code you linked in the issue report (https://github.com/emscripten-core/emscripten/blob/3.1.61/src/library_webgl.js#L145) has me a little concerned. it says:

// Restrict the list of advertised extensions to those that we actually
// support.

Which makes me think that the authors of Emscripten don't even realize what extensions they do and don't support.

I've tried to follow the history of this breaking change on Github and it is a little confusing. One of the maintainers links this document which says that "OVR_multiview2" is not supported ("OVR_multiview" and "OCULUS_multiview" are not even listed).

It looks like they are just cherrypicking extensions to the allow list now (seemingly at random?) emscripten-core/emscripten#21185

I can't find any documentation of emscripten_webgl_get_supported_extensions() including what it promises to return. I suspect at some point the devs of emscripten are going to break emscripten_webgl_get_supported_extensions() the same way they broke glGetStringi()

Copy link
Contributor Author

@dsnopek dsnopek Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Do we have any guarantee that this will actually list all supported extensions?
I suspect at some point the devs of emscripten are going to break emscripten_webgl_get_supported_extensions() the same way they broke glGetStringi()

I guess there are no guarantees, they can change anything at any time.

But emscripten_webgl_get_supported_extensions() has always (since it was introduced in June 2020) simply called GLctx.getSupportedExtensions(). That's 4 years of being a simple one-line forwarding call to the underlying WebGL function.

So, I feel more confident about them not breaking this function, than glGetStringi(GL_EXTENSIONS, i) which is an attempt to emulate the OpenGL API, and hence a little open to interpretation.

Which makes me think that the authors of Emscripten don't even realize what extensions they do and don't support.

Yeah, it depends on their definition of "support", which is varying. They do "support" almost all WebGL extensions, in that they'll forward calls to them. But they only "support" deeply emulating the OpenGL API of a much smaller set of extensions.

I've tried to follow the history of this breaking change on Github and it is a little confusing. One of the maintainers links this document which says that "OVR_multiview2" is not supported ("OVR_multiview" and "OCULUS_multiview" are not even listed).

Only OVR_multiview2 is a standardized WebGL extension. OVR_multiview (without the 2) is only available on OpenGL (not WebGL). And OCULUS_multiview is a non-standard extension only implemented by the Meta Browser (which does the same thing as OVR_multiview2 plus support for MSAA, which the standard extension doesn't have).

In any case, the latest version of Emscripten does "support" OVR_multiview2, but we're not using that in order to be compatible with older versions of Emscripten, and instead have our own thing over in platforms/web/js/libs/library_godot_webgl2.js. (But, even with their "support" of it, they still won't report it as enabled via glGetStringi(GL_EXTENSIONS, i) in Emscripten 3.1.51 or later.)

Anyway, this bit is kind of a tangent! But hopefully that helps clear up the confusion :-)

Copy link
Member

@clayjohn clayjohn Jun 24, 2024

Choose a reason for hiding this comment

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

Thank you for your answer. I am left a little bit more confused on one point:

But emscripten_webgl_get_supported_extensions() has always (since it was emscripten-core/emscripten#11400) simply called GLctx.getSupportedExtensions(). That's 4 years of being a simple one-line forwarding call to the underlying WebGL function.

getSupportedExtensions() is defined immediately under the line you linked earlier. It explicitly only returns that limited list of extensions that emscripten says it supports. Which means that it won't report support for OVR_multiview2, OVR_multiview, or OCULUS_multiview.

https://github.com/emscripten-core/emscripten/blob/67fa4c16496b157a7fc3377afd69ee0445e8a6e3/src/library_webgl.js#L198

If that is right, then it doesn't seem to do anything different from glGetStringi(GL_EXTENSIONS, i)

Is the original problem coming from glGetStringi(GL_EXTENSIONS, i) reporting support for an extension that shouldn't be enabled? Or do we have some other fallback code that I am unaware of?

Copy link
Contributor Author

@dsnopek dsnopek Jun 24, 2024

Choose a reason for hiding this comment

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

You know, if we wanted to, we could add our own version of emscripten_webgl_get_supported_extensions() over in platforms/web/js/libs/library_godot_webgl2.js?

Then Emscripten can't change it, or take it away.

However, I guess I'd prefer to wait to do that until they actually do change it or take it away, because if we do it for that one, it begs the question: why don't we do that for all Emscription functions? This one in particular doesn't seem more likely to break than any other.

But we do have that option available, either for now or for the future!

Copy link
Contributor Author

@dsnopek dsnopek Jun 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

getSupportedExtensions() is defined immediately under the line you linked earlier. It explicitly only returns that limited list of extensions that emscripten says it supports.

That line isn't defining getSupportedExtensions() (which is a standard WebGL function, it's not from Emscripten). What it's doing is calling getSupportedExtensions() (which will return all supported extensions) but then filtering it to remove all but the extensions in the fixed list of "supported" extensions from above.

So, in the example of OVR_multiview2, even if the browser reports it as supported, it will get removed because it's not on Emscripten's list.

The definition of emscripten_webgl_get_supported_extensions() is actually here:

https://github.com/emscripten-core/emscripten/blob/67fa4c16496b157a7fc3377afd69ee0445e8a6e3/src/library_html5_webgl.js#L422-L426

Which is just one-liner, calling to the standard WebGL getSupportedExtensions() function.

Is the original problem coming from glGetStringi(GL_EXTENSIONS, i) reporting support for an extension that shouldn't be enabled? Or do we have some other fallback code that I am unaware of?

No, it's that glGetStringi(GL_EXTENSIONS, i) isn't reporting all extensions that are actually supported. It's filtering the list of supported extensions to remove all but a fixed list of extensions that Emscripten has arbitrarily decided should be listed as supported.

Copy link
Member

@clayjohn clayjohn Jun 24, 2024

Choose a reason for hiding this comment

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

Ooooh, thank you for the explanation. That makes more sense now!

It's too bad that we are having to add a hack just to do the thing they are trying to stop us from doing.

PackedStringArray extension_array = String((const char *)extension_array_string).split(" ");
extensions.reserve(extension_array.size() * 2);
for (const String &s : extension_array) {
extensions.insert(s);
extensions.insert("GL_" + s);
}
free(extension_array_string);
}
#else
{
GLint max_extensions = 0;
glGetIntegerv(GL_NUM_EXTENSIONS, &max_extensions);
Expand All @@ -55,6 +76,7 @@ Config::Config() {
extensions.insert((const char *)s);
}
}
#endif

bptc_supported = extensions.has("GL_ARB_texture_compression_bptc") || extensions.has("EXT_texture_compression_bptc");
astc_supported = extensions.has("GL_KHR_texture_compression_astc") || extensions.has("GL_OES_texture_compression_astc") || extensions.has("GL_KHR_texture_compression_astc_ldr") || extensions.has("GL_KHR_texture_compression_astc_hdr");
Expand Down