From 89764d141f0834e96e77c2801318d88e2ddbc46c Mon Sep 17 00:00:00 2001 From: Robin Ebert Date: Mon, 9 Oct 2023 16:11:16 +0200 Subject: [PATCH 1/3] egl: Rename variables for consistency with EGL spec --- glutin/src/api/egl/context.rs | 2 +- glutin/src/api/egl/device.rs | 4 ++-- glutin/src/api/egl/display.rs | 26 +++++++++++++------------- glutin/src/api/egl/surface.rs | 6 +++--- 4 files changed, 19 insertions(+), 19 deletions(-) diff --git a/glutin/src/api/egl/context.rs b/glutin/src/api/egl/context.rs index 331029669f..437d3cf5dc 100644 --- a/glutin/src/api/egl/context.rs +++ b/glutin/src/api/egl/context.rs @@ -55,7 +55,7 @@ impl Display { }; let is_one_five = self.inner.version >= Version::new(1, 5); - if is_one_five || self.inner.client_extensions.contains("EGL_KHR_create_context") { + if is_one_five || self.inner.display_extensions.contains("EGL_KHR_create_context") { let mut flags = 0; // Add profile for the OpenGL Api. diff --git a/glutin/src/api/egl/device.rs b/glutin/src/api/egl/device.rs index 465fb6050c..498651dca7 100644 --- a/glutin/src/api/egl/device.rs +++ b/glutin/src/api/egl/device.rs @@ -9,7 +9,7 @@ use glutin_egl_sys::egl::types::EGLDeviceEXT; use crate::error::{ErrorKind, Result}; -use super::display::{extensions_from_ptr, get_extensions, NO_DISPLAY_EXTENSIONS}; +use super::display::{extensions_from_ptr, get_extensions, CLIENT_EXTENSIONS}; use super::{Egl, EGL}; /// Wrapper for `EGLDevice`. @@ -34,7 +34,7 @@ impl Device { }; let no_display_extensions = - NO_DISPLAY_EXTENSIONS.get_or_init(|| get_extensions(egl, egl::NO_DISPLAY)); + CLIENT_EXTENSIONS.get_or_init(|| get_extensions(egl, egl::NO_DISPLAY)); // Querying devices requires EGL_EXT_device_enumeration and // EGL_EXT_device_query. diff --git a/glutin/src/api/egl/display.rs b/glutin/src/api/egl/display.rs index 518a34872c..a752bb077b 100644 --- a/glutin/src/api/egl/display.rs +++ b/glutin/src/api/egl/display.rs @@ -30,7 +30,7 @@ use super::surface::Surface; use super::{Egl, EGL}; /// Extensions that don't require any display. -pub(crate) static NO_DISPLAY_EXTENSIONS: OnceCell> = OnceCell::new(); +pub(crate) static CLIENT_EXTENSIONS: OnceCell> = OnceCell::new(); /// A wrapper for the `EGLDisplay` and its supported extensions. #[derive(Debug, Clone)] @@ -54,7 +54,7 @@ impl Display { None => return Err(ErrorKind::NotFound.into()), }; - NO_DISPLAY_EXTENSIONS.get_or_init(|| get_extensions(egl, egl::NO_DISPLAY)); + CLIENT_EXTENSIONS.get_or_init(|| get_extensions(egl, egl::NO_DISPLAY)); // Create a EGL display by chaining all display creation functions aborting on // `EGL_BAD_ATTRIBUTE`. @@ -113,7 +113,7 @@ impl Display { // Okay to unwrap here because the client extensions must have been enumerated // while querying the available devices or the device was gotten from an // existing display. - let extensions = NO_DISPLAY_EXTENSIONS.get().unwrap(); + let extensions = CLIENT_EXTENSIONS.get().unwrap(); if !extensions.contains("EGL_EXT_platform_base") && !extensions.contains("EGL_EXT_platform_device") @@ -150,7 +150,7 @@ impl Display { /// This function returns [`Err`] if the `EGL_EXT_device_query` or /// `EGL_EXT_device_base` extensions are not available. pub fn device(&self) -> Result { - let no_display_extensions = NO_DISPLAY_EXTENSIONS.get().unwrap(); + let no_display_extensions = CLIENT_EXTENSIONS.get().unwrap(); // Querying the device of a display only requires EGL_EXT_device_query, but we // also check if EGL_EXT_device_base is available since @@ -190,7 +190,7 @@ impl Display { return Err(ErrorKind::NotSupported("eglGetPlatformDisplay is not supported").into()); } - let extensions = NO_DISPLAY_EXTENSIONS.get().unwrap(); + let extensions = CLIENT_EXTENSIONS.get().unwrap(); let mut attrs = Vec::::new(); let (platform, mut display) = match display { @@ -238,7 +238,7 @@ impl Display { return Err(ErrorKind::NotSupported("eglGetPlatformDisplayEXT is not supported").into()); } - let extensions = NO_DISPLAY_EXTENSIONS.get().unwrap(); + let extensions = CLIENT_EXTENSIONS.get().unwrap(); let mut attrs = Vec::::new(); let mut legacy = false; @@ -382,15 +382,15 @@ impl Display { }; // Load extensions. - let client_extensions = get_extensions(egl, *display); - let features = Self::extract_display_features(&client_extensions, version); + let display_extensions = get_extensions(egl, *display); + let features = Self::extract_display_features(&display_extensions, version); let inner = Arc::new(DisplayInner { egl, raw: display, _native_display: raw_display_handle.map(NativeDisplay), version, - client_extensions, + display_extensions, features, }); Ok(Self { inner }) @@ -458,7 +458,7 @@ impl GlDisplay for Display { impl GetDisplayExtensions for Display { fn extensions(&self) -> &HashSet<&'static str> { - &self.inner.client_extensions + &self.inner.display_extensions } } @@ -480,8 +480,8 @@ pub(crate) struct DisplayInner { /// The version of the egl library. pub(crate) version: Version, - /// Client EGL extensions. - pub(crate) client_extensions: HashSet<&'static str>, + /// Display EGL extensions. + pub(crate) display_extensions: HashSet<&'static str>, /// The features supported by the display. pub(crate) features: DisplayFeatures, @@ -496,7 +496,7 @@ impl fmt::Debug for DisplayInner { .field("raw", &self.raw) .field("version", &self.version) .field("features", &self.features) - .field("extensions", &self.client_extensions) + .field("extensions", &self.display_extensions) .finish() } } diff --git a/glutin/src/api/egl/surface.rs b/glutin/src/api/egl/surface.rs index 7bc242600a..102c74077e 100644 --- a/glutin/src/api/egl/surface.rs +++ b/glutin/src/api/egl/surface.rs @@ -270,7 +270,7 @@ impl Surface { context.inner.bind_api(); let res = unsafe { - if self.display.inner.client_extensions.contains("EGL_KHR_swap_buffers_with_damage") { + if self.display.inner.display_extensions.contains("EGL_KHR_swap_buffers_with_damage") { self.display.inner.egl.SwapBuffersWithDamageKHR( *self.display.inner.raw, self.raw, @@ -280,7 +280,7 @@ impl Surface { } else if self .display .inner - .client_extensions + .display_extensions .contains("EGL_EXT_swap_buffers_with_damage") { self.display.inner.egl.SwapBuffersWithDamageEXT( @@ -333,7 +333,7 @@ impl GlSurface for Surface { fn buffer_age(&self) -> u32 { self.display .inner - .client_extensions + .display_extensions .contains("EGL_EXT_buffer_age") .then(|| unsafe { self.raw_attribute(egl::BUFFER_AGE_EXT as EGLint) }) .unwrap_or(0) as u32 From 2b77be589f1c65419ca61eb6ad278a1406a59d5d Mon Sep 17 00:00:00 2001 From: Robin Ebert Date: Mon, 9 Oct 2023 16:52:49 +0200 Subject: [PATCH 2/3] egl: use EGL_KHR_display_reference 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. Besides not terminating EGL raises issue with nvidia on Wayland, thus the `api::egl::Display::terminate` is added to help clients call this function when calling `eglTerminate` is generally unsafe to do automatically when dropping the `Display`. Links: https://github.com/alacritty/alacritty/issues/7146 Fixes: #1588 --- CHANGELOG.md | 2 + glutin/src/api/egl/display.rs | 86 +++++++++++++++++++++++++++++------ glutin_egl_sys/build.rs | 1 + 3 files changed, 76 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 182f7e7e22..4e33eb778d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ - **Breaking:** `bitflags` which is used as a part of public API was updated to `2.0`. - **Breaking:** `.*SurfaceAccessor` traits got removed; their methods now on respective `.*GlContext` traits instead. - **Breaking:** `GlContext` trait is now a part of the `prelude`. +- Automatically cleanup the `EGLDisplay` when `EGL_KHR_display_reference` is present. +- Add `api::egl::Display::terminate` to terminate the display when glutin doesn't manage it. # Version 0.30.10 diff --git a/glutin/src/api/egl/display.rs b/glutin/src/api/egl/display.rs index a752bb077b..7d34df2727 100644 --- a/glutin/src/api/egl/display.rs +++ b/glutin/src/api/egl/display.rs @@ -2,6 +2,7 @@ use std::collections::HashSet; use std::ffi::{self, CStr}; +use std::mem::MaybeUninit; use std::ops::Deref; use std::os::raw::c_char; use std::sync::Arc; @@ -124,7 +125,12 @@ impl Display { .into()); } - let mut attrs = Vec::::with_capacity(2); + let mut attrs = Vec::::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 // which DRM master fd to use under the hood by the implementation. This would @@ -185,6 +191,25 @@ impl Display { Device::from_ptr(self.inner.egl, device) } + /// Terminate the EGL display. + /// + /// When the display is managed by glutin with the + /// `EGL_KHR_display_reference` this function does nothing and + /// `eglTerminate` will be automatically invoked during display destruction. + /// + /// # Safety + /// + /// This function will destroy the global EGL state, even the one created + /// and managed by other libraries. Use this function only when you're + /// bringing everything down. + pub unsafe fn terminate(self) { + if !self.inner.uses_display_reference() { + unsafe { + self.inner.egl.Terminate(*self.inner.raw); + } + } + } + fn get_platform_display(egl: &Egl, display: RawDisplayHandle) -> Result { if !egl.GetPlatformDisplay.is_loaded() { return Err(ErrorKind::NotSupported("eglGetPlatformDisplay is not supported").into()); @@ -192,7 +217,7 @@ impl Display { let extensions = CLIENT_EXTENSIONS.get().unwrap(); - let mut attrs = Vec::::new(); + let mut attrs = Vec::::with_capacity(5); let (platform, mut display) = match display { #[cfg(wayland_platform)] RawDisplayHandle::Wayland(handle) @@ -219,6 +244,11 @@ impl Display { }, }; + if extensions.contains("EGL_KHR_display_reference") { + attrs.push(egl::TRACK_REFERENCES_KHR as _); + attrs.push(egl::TRUE as _); + } + // Be explicit here. if display.is_null() { display = egl::DEFAULT_DISPLAY as *mut _; @@ -240,7 +270,7 @@ impl Display { let extensions = CLIENT_EXTENSIONS.get().unwrap(); - let mut attrs = Vec::::new(); + let mut attrs = Vec::::with_capacity(5); let mut legacy = false; let (platform, mut display) = match display { #[cfg(wayland_platform)] @@ -279,6 +309,11 @@ impl Display { }, }; + if extensions.contains("EGL_KHR_display_reference") { + attrs.push(egl::TRACK_REFERENCES_KHR as _); + attrs.push(egl::TRUE as _); + } + // Be explicit here. if display.is_null() { display = egl::DEFAULT_DISPLAY as *mut _; @@ -490,6 +525,35 @@ pub(crate) struct DisplayInner { pub(crate) _native_display: Option, } +impl DisplayInner { + fn uses_display_reference(&self) -> bool { + if !CLIENT_EXTENSIONS.get().unwrap().contains("EGL_KHR_display_reference") { + return false; + } + + // If the EGL_TRACK_REFERENCES_KHR attribute is true, then EGL will internally + // reference count the display. If that is the case, glutin can + // terminate the display without worry for the instance being + // reused elsewhere. + let mut track_references = MaybeUninit::::uninit(); + unsafe { + (match self.raw { + EglDisplay::Khr(khr) => self.egl.QueryDisplayAttribKHR( + khr, + egl::TRACK_REFERENCES_KHR as _, + track_references.as_mut_ptr(), + ), + EglDisplay::Ext(ext) => self.egl.QueryDisplayAttribEXT( + ext, + egl::TRACK_REFERENCES_KHR as _, + track_references.as_mut_ptr(), + ), + EglDisplay::Legacy(_) => egl::FALSE, + } == egl::TRUE) + } + } +} + impl fmt::Debug for DisplayInner { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("Display") @@ -503,6 +567,12 @@ impl fmt::Debug for DisplayInner { impl Drop for DisplayInner { fn drop(&mut self) { + if self.uses_display_reference() { + unsafe { + self.egl.Terminate(*self.raw); + } + } + // We cannot call safely call `eglTerminate`. // // This may sound confusing, but this is a result of how EGL works: @@ -545,16 +615,6 @@ impl Drop for DisplayInner { // of not dropping the display is negligible because the display will // probably be destroyed on app termination and we can let the // operating system deal with tearing down EGL instead. - // - // # Possible future work: - // - // For platform displays, we could track the use of individual raw - // window handles and display attributes (recall the "with the - // same parameters" line) and use that to determine if it is safe to - // terminate the display, but that increases maintenance burden and is - // possibly flaky to implement. - - // unsafe { self.egl.Terminate(self.raw) }; } } diff --git a/glutin_egl_sys/build.rs b/glutin_egl_sys/build.rs index ccef77db20..337458bd61 100644 --- a/glutin_egl_sys/build.rs +++ b/glutin_egl_sys/build.rs @@ -37,6 +37,7 @@ fn main() { "EGL_EXT_swap_buffers_with_damage", "EGL_KHR_create_context", "EGL_KHR_create_context_no_error", + "EGL_KHR_display_reference", "EGL_KHR_fence_sync", "EGL_KHR_platform_android", "EGL_KHR_platform_gbm", From e69c3608aea00662f69f4de44b0e10bc24a3172b Mon Sep 17 00:00:00 2001 From: Robin Ebert Date: Mon, 9 Oct 2023 18:58:27 +0200 Subject: [PATCH 3/3] egl: Fallback if EGL_KHR_display_reference not supported libglvnd reports client extensions if at least one vendor supports them. This might lead to display creation failure when a vendor library reports support but fails to create a display for another reason (like when the nvidia egl implementation is installed but no nvidia card is available). To work around this issue retry creation without EGL_KHR_display_reference. --- glutin/src/api/egl/display.rs | 143 +++++++++++++++++++++++++--------- 1 file changed, 105 insertions(+), 38 deletions(-) diff --git a/glutin/src/api/egl/display.rs b/glutin/src/api/egl/display.rs index 7d34df2727..1481eb4478 100644 --- a/glutin/src/api/egl/display.rs +++ b/glutin/src/api/egl/display.rs @@ -127,28 +127,49 @@ impl Display { let mut attrs = Vec::::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 // which DRM master fd to use under the hood by the implementation. This would // mean there would need to be an unsafe equivalent to this function. + // Push at the end so we can pop it on failure + let mut has_display_reference = extensions.contains("EGL_KHR_display_reference"); + if has_display_reference { + attrs.push(egl::TRACK_REFERENCES_KHR as _); + attrs.push(egl::TRUE as _); + } + // Push `egl::NONE` to terminate the list. attrs.push(egl::NONE as EGLint); - let display = Self::check_display_error(unsafe { - egl.GetPlatformDisplayEXT( - egl::PLATFORM_DEVICE_EXT, - device.raw_device() as *mut _, - attrs.as_ptr(), - ) - }) + // NOTE: This fallback is needed because libglvnd advertises client extensions + // if at least one vendor library supports them. This leads to creation + // failures for the vendor libraries not supporting + // EGL_KHR_display_reference. Also according to the spec creation is allowed + // to fail with EGL_KHR_display_reference set to EGL_TRUE even if + // EGL_KHR_display_reference is advertised in the client extension + // string, so just always try creation without EGL_KHR_display_reference + // if it failed using it. + let platform_display = loop { + match Self::check_display_error(unsafe { + egl.GetPlatformDisplayEXT( + egl::PLATFORM_DEVICE_EXT, + device.raw_device() as *mut _, + attrs.as_ptr(), + ) + }) { + Err(_) if has_display_reference => { + attrs.pop(); + attrs.pop(); + attrs.pop(); + attrs.push(egl::NONE as EGLint); + has_display_reference = false; + }, + platform_display => break platform_display, + } + } .map(EglDisplay::Ext)?; - Self::initialize_display(egl, display, None) + Self::initialize_display(egl, platform_display, None) } /// Get the [`Device`] the display is using. @@ -244,23 +265,45 @@ impl Display { }, }; - if extensions.contains("EGL_KHR_display_reference") { - attrs.push(egl::TRACK_REFERENCES_KHR as _); - attrs.push(egl::TRUE as _); - } - // Be explicit here. if display.is_null() { display = egl::DEFAULT_DISPLAY as *mut _; } + // Push at the end so we can pop it on failure + let mut has_display_reference = extensions.contains("EGL_KHR_display_reference"); + if has_display_reference { + attrs.push(egl::TRACK_REFERENCES_KHR as _); + attrs.push(egl::TRUE as _); + } + // Push `egl::NONE` to terminate the list. attrs.push(egl::NONE as EGLAttrib); - let display = - unsafe { egl.GetPlatformDisplay(platform, display as *mut _, attrs.as_ptr()) }; + // NOTE: This fallback is needed because libglvnd advertises client extensions + // if at least one vendor library supports them. This leads to creation + // failures for the vendor libraries not supporting + // EGL_KHR_display_reference. Also according to the spec creation is allowed + // to fail with EGL_KHR_display_reference set to EGL_TRUE even if + // EGL_KHR_display_reference is advertised in the client extension + // string, so just always try creation without EGL_KHR_display_reference + // if it failed using it. + let platform_display = loop { + match Self::check_display_error(unsafe { + egl.GetPlatformDisplay(platform, display as *mut _, attrs.as_ptr()) + }) { + Err(_) if has_display_reference => { + attrs.pop(); + attrs.pop(); + attrs.pop(); + attrs.push(egl::NONE as EGLAttrib); + has_display_reference = false; + }, + platform_display => break platform_display, + } + }; - Self::check_display_error(display).map(EglDisplay::Khr) + platform_display.map(EglDisplay::Khr) } fn get_platform_display_ext(egl: &Egl, display: RawDisplayHandle) -> Result { @@ -309,23 +352,45 @@ impl Display { }, }; - if extensions.contains("EGL_KHR_display_reference") { - attrs.push(egl::TRACK_REFERENCES_KHR as _); - attrs.push(egl::TRUE as _); - } - // Be explicit here. if display.is_null() { display = egl::DEFAULT_DISPLAY as *mut _; } + // Push at the end so we can pop it on failure + let mut has_display_reference = extensions.contains("EGL_KHR_display_reference"); + if has_display_reference { + attrs.push(egl::TRACK_REFERENCES_KHR as _); + attrs.push(egl::TRUE as _); + } + // Push `egl::NONE` to terminate the list. attrs.push(egl::NONE as EGLint); - let display = - unsafe { egl.GetPlatformDisplayEXT(platform, display as *mut _, attrs.as_ptr()) }; + // NOTE: This fallback is needed because libglvnd advertises client extensions + // if at least one vendor library supports them. This leads to creation + // failures for the vendor libraries not supporting + // EGL_KHR_display_reference. Also according to the spec creation is allowed + // to fail with EGL_KHR_display_reference set to EGL_TRUE even if + // EGL_KHR_display_reference is advertised in the client extension + // string, so just always try creation without EGL_KHR_display_reference + // if it failed using it. + let platform_display = loop { + match Self::check_display_error(unsafe { + egl.GetPlatformDisplayEXT(platform, display as *mut _, attrs.as_ptr()) + }) { + Err(_) if has_display_reference => { + attrs.pop(); + attrs.pop(); + attrs.pop(); + attrs.push(egl::NONE as EGLint); + has_display_reference = false; + }, + platform_display => break platform_display, + } + }; - Self::check_display_error(display).map(|display| { + platform_display.map(|display| { if legacy { // NOTE: For angle we use the Legacy code path, as that uses CreateWindowSurface // instead of CreatePlatformWindowSurface*. The latter somehow @@ -536,21 +601,23 @@ impl DisplayInner { // terminate the display without worry for the instance being // reused elsewhere. let mut track_references = MaybeUninit::::uninit(); - unsafe { - (match self.raw { - EglDisplay::Khr(khr) => self.egl.QueryDisplayAttribKHR( + (match self.raw { + EglDisplay::Khr(khr) => unsafe { + self.egl.QueryDisplayAttribKHR( khr, egl::TRACK_REFERENCES_KHR as _, track_references.as_mut_ptr(), - ), - EglDisplay::Ext(ext) => self.egl.QueryDisplayAttribEXT( + ) + }, + EglDisplay::Ext(ext) => unsafe { + self.egl.QueryDisplayAttribEXT( ext, egl::TRACK_REFERENCES_KHR as _, track_references.as_mut_ptr(), - ), - EglDisplay::Legacy(_) => egl::FALSE, - } == egl::TRUE) - } + ) + }, + EglDisplay::Legacy(_) => egl::FALSE, + } == egl::TRUE) } }