egl: use EGL_KHR_display_reference

[Molytho]

This is a updated version of #1626 which intends to fix alacritty/alacritty#7146

• Tested on all platforms changed
• Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
• Created or updated an example program if it would help users understand this functionality

[kchibisov]

What's the reason of submitting my code again which you can't test? It's not like I'll use this over what I had, since I don't even understand what are you trying to do with your loops?

[Molytho]

First there are some notable changes in the drop/terminate path: it has to check the availability of EGL_KHR_display_reference against the client extensions and not the display extensions like it does in your version.
You also used a wrong type in the call to QueryDisplayAttribKHR/QueryDisplayAttribEXT.

Regarding the loop:
There is an issue with the egl dispatching done by libglvnd. It reports client extensions if one vendor library supports them. This can lead to display creation failure if the library exposing EGL_KHR_display_reference fails to create a display for another reason.
An example is a system with mesa and the nvidia libraries but without an nvidia card (I know this is rather theoretical but I suspect this to also happen in a nvidia+intel setup when trying to uses the intel card). I was able to confirm this issue with my laptop with intel graphics.
To address this I added this loop to try again without EGL_KHR_display_reference if it failed using it.

Regarding the testing:
I was able to test this on my ArchLinux setup through patching libglvnd as suggested here:
alacritty/alacritty#7146 (comment)
i will try to get a fix upsteamed to libglvnd soon.
Currently I have a working alacritty build which does no longer crash.

[kchibisov]

it has to check the availability of EGL_KHR_display_reference against the client extensions and not the display extensions like it does in your version.

But you've just renamed the variables, you still query extensions the way it was before and you still check things the way they were before, you just changed a name in the codebase.

You also used a wrong type in the call to QueryDisplayAttribKHR/QueryDisplayAttribEXT.

u32 which you're using is the wrong type, according to the specification. The EGLAttrib is what I've used and what spec uses, if it's not EGLAttrib the driver or the specification should be fixed. I won't be using the wrong type because that's what nvidia/libglvnd wants.
https://github.com/KhronosGroup/EGL-Registry/blob/b055c9b483e70ecd57b3cf7204db21f5a06f9ffe/extensions/KHR/EGL_KHR_display_reference.txt#L84 . The type is the same for both KHR and EXT.

Regarding the loop:
There is an issue with the egl dispatching done by libglvnd. It reports client extensions if one vendor library supports them. This can lead to display creation failure if the library exposing EGL_KHR_display_reference fails to create a display for another reason.
An example is a system with mesa and the nvidia libraries but without an nvidia card (I know this is rather theoretical but I suspect this to also happen in a nvidia+intel setup when trying to uses the intel card). I was able to confirm this issue with my laptop with intel graphics.
To address this I added this loop to try again without EGL_KHR_display_reference if it failed using it.

Sounds like the bug should go upstream and not to glutin. Adding hacks like that means that no-one will ever fix it upstream, especially when nvidia is at play, since no-one will bother. I'd
rather have a way to naturally call eglTerminate even without the extension, since I can change semantics in the 0.31.0 version.

I was able to test this on my ArchLinux setup through patching libglvnd as suggested here:

Good, that means we have something to work with, but I'd suggest to leave review comments on original PR or at least, explain what you're trying to do when submitting patches which were waiting for their testing. It's not like I can't review your comments to fix stuff, and the
way you've submitted looked like spam to me(since such things happen from time to time).

In general, I'm fine with the renaming, but not fine with the loop and use of u32.

[kchibisov]

I was able to test this on my ArchLinux setup through patching libglvnd as suggested here:

Also, if you haven't understood why I said that you haven't tested, it's because you have unchecked checkbox for 'testing stuff'. Which I have because mesa doesn't support this extension thus I can't test myself, but given that you can test and tested you should mark check it.

[Molytho]

it has to check the availability of EGL_KHR_display_reference against the client extensions and not the display extensions like it does in your version.

But you've just renamed the variables, you still query extensions the way it was before and you still check things the way they were before, you just changed a name in the codebase.

That's not true:

glutin/glutin/src/api/egl/display.rs

Line 559 in ff69874

if !CLIENT_EXTENSIONS.get().unwrap().contains("EGL_KHR_display_reference") {

https://github.com/kchibisov/glutin/blob/b39853aaf68da3c9dbb14a2d69a8de7f22fe20e7/glutin/src/api/egl/display.rs#L548C74-L548C74
I use CLIENT_EXTENSIONS while you use self.client_extensions (renamed to self.display_extensions)

You also used a wrong type in the call to QueryDisplayAttribKHR/QueryDisplayAttribEXT.

u32 which you're using is the wrong type, according to the specification. The EGLAttrib is what I've used and what spec uses, if it's not EGLAttrib the driver or the specification should be fixed. I won't be using the wrong type because that's what nvidia/libglvnd wants. https://github.com/KhronosGroup/EGL-Registry/blob/b055c9b483e70ecd57b3cf7204db21f5a06f9ffe/extensions/KHR/EGL_KHR_display_reference.txt#L84 . The type is the same for both KHR and EXT.

You misread that. You where using u32.
https://github.com/kchibisov/glutin/blob/b39853aaf68da3c9dbb14a2d69a8de7f22fe20e7/glutin/src/api/egl/display.rs#L549
I changed it to EGLAttrib

Regarding the loop:
There is an issue with the egl dispatching done by libglvnd. It reports client extensions if one vendor library supports them. This can lead to display creation failure if the library exposing EGL_KHR_display_reference fails to create a display for another reason.
An example is a system with mesa and the nvidia libraries but without an nvidia card (I know this is rather theoretical but I suspect this to also happen in a nvidia+intel setup when trying to uses the intel card). I was able to confirm this issue with my laptop with intel graphics.
To address this I added this loop to try again without EGL_KHR_display_reference if it failed using it.

Sounds like the bug should go upstream and not to glutin. Adding hacks like that means that no-one will ever fix it upstream, especially when nvidia is at play, since no-one will bother. I'd rather have a way to naturally call eglTerminate even without the extension, since I can change semantics in the 0.31.0 version.

I can drop this part but to clarify: I think this is not the fault of nvidia neither do I think this is fixable upstream. The way egl dispatching currently work in combination with EGL_KHR_display_reference being a client extension just doesn't allow libglvnd to do something else except joining all client extensions supported by any vendor.
I don't like this either but I can't think of a better solution anywhere.

I was able to test this on my ArchLinux setup through patching libglvnd as suggested here:

Good, that means we have something to work with, but I'd suggest to leave review comments on original PR or at least, explain what you're trying to do when submitting patches which were waiting for their testing. It's not like I can't review your comments to fix stuff, and the way you've submitted looked like spam to me(since such things happen from time to time).

Sorry for that. I didn't check the box because I didn't test it on windows which might also be effected by this patch.
If you prefer I can close this PR and add some comments to yours.

In general, I'm fine with the renaming, but not fine with the loop and use of u32.

[kchibisov]

I use CLIENT_EXTENSIONS while you use self.client_extensions (renamed to self.display_extensions)

This part I know that it's wrong, the way you said it sounded like something more than that was wrong. I just hasn't bothered to fix, since after checking user's eglinfo the extension wasn't there in any instance.

Also, maybe we can merge all the extensions together? Because NO_DISPLAY and regular ones have no intersection, so if we can merge them it'll be more clear what extensions are available, since they are both available from what I can say.

You misread that. You where using u32.

aye, sorry. I used that cast() thing again...

I can drop this part but to clarify: I think this is not the fault of nvidia neither do I think this is fixable upstream. The way egl dispatching currently work in combination with EGL_KHR_display_reference being a client extension just doesn't allow libglvnd to do something else except joining all client extensions supported by any vendor.
I don't like this either but I can't think of a better solution anywhere.

I don't like the loop, but if there's no other way around it, I'd suggest to add the reference stuff in the end, and if adding them fails, just pop and retry.

Sorry for that. I didn't check the box because I didn't test it on windows which might also be effected by this patch.
If you prefer I can close this PR and add some comments to yours.

Given that you're the one which can test you can continue with the patch. It was better initially to either comment or open a PR against my PR and ping me (github doesn't notify on forks by default).

[Molytho]

Also, maybe we can merge all the extensions together? Because NO_DISPLAY and regular ones have no intersection, so if we can merge them it'll be more clear what extensions are available, since they are both available from what I can say.

We could but I think this isn't a good idea. Client extensions and display extensions are intentionally kept apart by the spec as they do serve different purposes. Most client extensions are useless after display creation so it's just waste of space to store them together with the display extensions.
I also don't see an issue with the current NO_DISPLAY_EXTENSIONS/CLIENT_EXTENSIONS global.

[kchibisov]

We could but I think this isn't a good idea. Client extensions and display extensions are intentionally kept apart by the spec as they do serve different purposes. Most client extensions are useless after display creation so it's just waste of space to store them together with the display extensions.

It's not like it'll be more space, since it'll be the same space. But yeah, NO_DISPLAY stuff is pretty rarely used.

[kchibisov]

Thanks, I'd assume that we still need libglvnd update?

[Molytho]

Yes we do. I opened an issue upstream:
https://gitlab.freedesktop.org/glvnd/libglvnd/-/issues/244