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

egl: Use EGL_KHR_display_reference #1609

Closed
wants to merge 1 commit into from

Conversation

i509VCB
Copy link
Contributor

@i509VCB i509VCB commented Jun 28, 2023

This resolves a past issue where glutin would not be able to terminate a display due to risk of two displays being created from the same native display.

  • 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

This resolves a past issue where glutin would not be able to terminate a display due to risk of two displays being created from the same native display.
@kchibisov
Copy link
Member

I think you'd need this to make it work from how I read the extension.

diff --git a/glutin/src/api/egl/display.rs b/glutin/src/api/egl/display.rs
index 43d043a..94c3dbd 100644
--- a/glutin/src/api/egl/display.rs
+++ b/glutin/src/api/egl/display.rs
@@ -125,10 +125,11 @@ impl Display {
             .into());
         }
 
-        let mut attrs = Vec::<EGLint>::with_capacity(2);
+        let mut attrs = Vec::<EGLint>::with_capacity(3);
 
         if extensions.contains("EGL_KHR_display_reference") {
             attrs.push(egl::TRACK_REFERENCES_KHR as _);
+            attrs.push(egl::TRUE as _);
         }
 
         // TODO: Some extensions exist like EGL_EXT_device_drm which allow specifying
@@ -198,7 +199,7 @@ impl Display {
         let extensions = NO_DISPLAY_EXTENSIONS.get().unwrap();
 
         // Preallocate space for the terminator and to track references.
-        let mut attrs = Vec::<EGLAttrib>::with_capacity(2);
+        let mut attrs = Vec::<EGLAttrib>::with_capacity(3);
         let (platform, mut display) = match display {
             #[cfg(wayland_platform)]
             RawDisplayHandle::Wayland(handle)
@@ -227,6 +228,7 @@ impl Display {
 
         if extensions.contains("EGL_KHR_display_reference") {
             attrs.push(egl::TRACK_REFERENCES_KHR as _);
+            attrs.push(egl::TRUE as _);
         }
 
         // Be explicit here.
@@ -251,7 +253,7 @@ impl Display {
         let extensions = NO_DISPLAY_EXTENSIONS.get().unwrap();
 
         // Preallocate space for the terminator and to track references.
-        let mut attrs = Vec::<EGLint>::with_capacity(2);
+        let mut attrs = Vec::<EGLint>::with_capacity(3);
         let (platform, mut display) = match display {
             #[cfg(wayland_platform)]
             RawDisplayHandle::Wayland(handle)
@@ -286,6 +288,7 @@ impl Display {
 
         if extensions.contains("EGL_KHR_display_reference") {
             attrs.push(egl::TRACK_REFERENCES_KHR as _);
+            attrs.push(egl::TRUE as _);
         }
 
         // Be explicit here.
@@ -509,15 +512,21 @@ impl Drop for DisplayInner {
         if self.client_extensions.contains("EGL_KHR_display_reference") {
             let mut track_references = MaybeUninit::<u32>::uninit();
             unsafe {
-                self.egl.QueryDisplayAttribEXT(
-                    *self.raw,
-                    egl::TRACK_REFERENCES_KHR as _,
-                    track_references.as_mut_ptr().cast(),
-                );
-
-                if track_references.assume_init() == egl::TRUE {
+                if match self.raw {
+                    EglDisplay::Khr(khr) => self.egl.QueryDisplayAttribKHR(
+                        khr,
+                        egl::TRACK_REFERENCES_KHR as _,
+                        track_references.as_mut_ptr().cast(),
+                    ),
+                    EglDisplay::Ext(ext) => self.egl.QueryDisplayAttribEXT(
+                        ext,
+                        egl::TRACK_REFERENCES_KHR as _,
+                        track_references.as_mut_ptr().cast(),
+                    ),
+                    EglDisplay::Legacy(_) => return,
+                } == egl::TRUE
+                {
                     self.egl.Terminate(*self.raw);
-                    return;
                 }
             }
         }

@kchibisov
Copy link
Member

@MarijnS95 could you test this? The extension is not present for me with mesa, but from what I've see it was default on Android.

With my patch applied likely and test that it actually calls eglTerminate in the drop?

@MarijnS95
Copy link
Member

Indeed, the documentation seems rather clear on requiring a TRUE/FALSE argument, so I'm curious how this was "tested on all platforms changed".

Unfortunately my Android 13 phone does not advertise EGL_KHR_display_reference, even though the docs mention that on EGL_KHR_platform_android the default value is EGL_TRUE (but I assume that only counts when the extension is there).

I bluntly tried to retrieve the value, but since eglQueryDisplayAttribKHR is provided by this extension (and eglQueryDisplayAttribEXT by EGL_EXT_device_query which isn't supported either), both panic as they cannot be loaded.

Strangely there's no BadAttribute error when adding the attribute (with or without boolean) to GetPlatformDisplay.

@kchibisov
Copy link
Member

Yeah, it'll need more work in general.

How did you test it @i509VCB , do you have nvidia card? mesa doesn't provide this extension from what I can see.

@MarijnS95
Copy link
Member

Indeed, Mesa doesn't even support it on my Intel laptop nor on Freedreno for one of the many "mainlined" phones I have lining around (there's a PR open, though...).

It seems NVidia should support it?

@i509VCB
Copy link
Contributor Author

i509VCB commented Jun 28, 2023

Yeah, it'll need more work in general.

How did you test it @i509VCB , do you have nvidia card? mesa doesn't provide this extension from what I can see.

I plan to try to test on some nvidia hardware I have, I need to install a new distro on the machine though

@kchibisov
Copy link
Member

See #1632.

@kchibisov kchibisov closed this Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants