-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
[Web] Fix checking for OpenGL extensions with Emscripten 3.1.51 and later #93560
Conversation
d12c686
to
7342ba4
Compare
7342ba4
to
51f9df0
Compare
// 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(); |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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 breakemscripten_webgl_get_supported_extensions()
the same way they brokeglGetStringi()
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 :-)
There was a problem hiding this comment.
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
.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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.
Thanks! |
Fixes #93480
As explained here, with Emscripten 3.1.51 and later, the usual OpenGL method for determining which extensions are enabled will always return a fixed list of extensions, regardless of what additional extensions may be enabled.
This makes our checks for which extensions we can rely on in
drivers/gles3/storage/config.cpp
pretty useless! (Including the checks for the multiview extensions needed by WebXR, which is what causes issue #93480.)This PR switches to using
emscripten_webgl_get_supported_extensions()
to get the list of extensions in the same way as Emscripten 3.1.50 and earlier used to.(The code here should work just fine for Emscripten 3.1.50 and earlier too, so it's fine to use it regardless of the Emscripten version.)