From f8c97baf1df48afdbba0eb3640ce165aaacb4687 Mon Sep 17 00:00:00 2001 From: Chandler Newman Date: Thu, 16 Apr 2020 13:44:32 +0100 Subject: [PATCH] Prevent display being destroyed until all resources have been dropped --- src/backend/egl/context.rs | 71 ++++----- src/backend/egl/display.rs | 308 +++++++++++++++++++------------------ src/backend/egl/mod.rs | 55 +++---- src/backend/egl/surface.rs | 53 +++---- 4 files changed, 229 insertions(+), 258 deletions(-) diff --git a/src/backend/egl/context.rs b/src/backend/egl/context.rs index 6e1489a..dc8e8fd 100644 --- a/src/backend/egl/context.rs +++ b/src/backend/egl/context.rs @@ -1,17 +1,17 @@ //! EGL context related structs use super::{ffi, Error}; -use crate::backend::egl::display::EGLDisplay; +use crate::backend::egl::display::{EGLDisplay, EGLDisplayHandle}; use crate::backend::egl::native::NativeSurface; use crate::backend::egl::{native, EGLSurface}; use crate::backend::graphics::{PixelFormat, SwapBuffersError}; use std::ptr; -use std::sync::{Arc, Weak}; +use std::sync::Arc; /// EGL context for rendering pub struct EGLContext { - context: Arc, - display: Weak, + context: ffi::egl::types::EGLContext, + display: Arc, config_id: ffi::egl::types::EGLConfig, pixel_format: PixelFormat, } @@ -94,7 +94,7 @@ impl EGLContext { // TODO: Support shared contexts let context = unsafe { ffi::egl::CreateContext( - *display.display, + **display.display, config_id, ptr::null(), context_attributes.as_ptr(), @@ -111,8 +111,8 @@ impl EGLContext { info!(log, "EGL context created"); Ok(EGLContext { - context: Arc::new(context as _), - display: Arc::downgrade(&display.display), + context, + display: display.display.clone(), config_id, pixel_format, }) @@ -132,26 +132,17 @@ impl EGLContext { where N: NativeSurface, { - if let Some(display) = self.display.upgrade() { - let surface_ptr = surface.surface.get(); + let surface_ptr = surface.surface.get(); - let ret = ffi::egl::MakeCurrent( - (*display) as *const _, - surface_ptr as *const _, - surface_ptr as *const _, - (*self.context) as *const _, - ); + let ret = ffi::egl::MakeCurrent(**self.display, surface_ptr, surface_ptr, self.context); - if ret == 0 { - match ffi::egl::GetError() as u32 { - ffi::egl::CONTEXT_LOST => Err(SwapBuffersError::ContextLost), - err => panic!("eglMakeCurrent failed (eglGetError returned 0x{:x})", err), - } - } else { - Ok(()) + if ret == 0 { + match ffi::egl::GetError() as u32 { + ffi::egl::CONTEXT_LOST => Err(SwapBuffersError::ContextLost), + err => panic!("eglMakeCurrent failed (eglGetError returned 0x{:x})", err), } } else { - Err(SwapBuffersError::ContextLost) + Ok(()) } } @@ -162,32 +153,21 @@ impl EGLContext { /// This function is marked unsafe, because the context cannot be made current /// on multiple threads. pub unsafe fn make_current(&self) -> ::std::result::Result<(), SwapBuffersError> { - if let Some(display) = self.display.upgrade() { - let surface_ptr = ptr::null(); + let ret = ffi::egl::MakeCurrent(**self.display, ptr::null(), ptr::null(), self.context); - let ret = ffi::egl::MakeCurrent( - (*display) as *const _, - surface_ptr as *const _, - surface_ptr as *const _, - (*self.context) as *const _, - ); - - if ret == 0 { - match ffi::egl::GetError() as u32 { - ffi::egl::CONTEXT_LOST => Err(SwapBuffersError::ContextLost), - err => panic!("eglMakeCurrent failed (eglGetError returned 0x{:x})", err), - } - } else { - Ok(()) + if ret == 0 { + match ffi::egl::GetError() as u32 { + ffi::egl::CONTEXT_LOST => Err(SwapBuffersError::ContextLost), + err => panic!("eglMakeCurrent failed (eglGetError returned 0x{:x})", err), } } else { - Err(SwapBuffersError::ContextLost) + Ok(()) } } /// Returns true if the OpenGL context is the current one in the thread. pub fn is_current(&self) -> bool { - unsafe { ffi::egl::GetCurrentContext() == (*self.context) as *const _ } + unsafe { ffi::egl::GetCurrentContext() == self.context as *const _ } } /// Returns the egl config for this context @@ -204,11 +184,12 @@ impl EGLContext { impl Drop for EGLContext { fn drop(&mut self) { unsafe { - // we don't call MakeCurrent(0, 0) because we are not sure that the context - // is still the current one - if let Some(display) = self.display.upgrade() { - ffi::egl::DestroyContext((*display) as *const _, (*self.context) as *const _); + // We need to ensure the context is unbound, otherwise it egl stalls the destroy call + if ffi::egl::GetCurrentContext() == self.context as *const _ { + ffi::egl::MakeCurrent(ptr::null(), ptr::null(), ptr::null(), ptr::null()); } + + ffi::egl::DestroyContext(**self.display, self.context); } } } diff --git a/src/backend/egl/display.rs b/src/backend/egl/display.rs index 4fc73d6..ff1b8a1 100644 --- a/src/backend/egl/display.rs +++ b/src/backend/egl/display.rs @@ -5,7 +5,7 @@ use crate::backend::egl::EGLGraphicsBackend; use crate::backend::egl::{ ffi, get_proc_address, native, BufferAccessError, EGLContext, EGLImages, EGLSurface, Error, Format, }; -use std::sync::{Arc, Weak}; +use std::sync::Arc; use std::ptr; @@ -25,10 +25,34 @@ use std::ffi::{CStr, CString}; use std::marker::PhantomData; use std::mem::MaybeUninit; +use std::ops::Deref; + +/// Wrapper around [`ffi::EGLDisplay`](ffi::egl::types::EGLDisplay) to ensure display is only destroyed +/// once all resources bound to it have been dropped. +pub(crate) struct EGLDisplayHandle { + handle: ffi::egl::types::EGLDisplay, +} + +impl Deref for EGLDisplayHandle { + type Target = ffi::egl::types::EGLDisplay; + + fn deref(&self) -> &Self::Target { + &self.handle + } +} + +impl Drop for EGLDisplayHandle { + fn drop(&mut self) { + unsafe { + ffi::egl::Terminate(self.handle); + } + } +} + /// [`EGLDisplay`] represents an initialised EGL environment pub struct EGLDisplay> { native: RefCell, - pub(crate) display: Arc, + pub(crate) display: Arc, pub(crate) egl_version: (i32, i32), pub(crate) extensions: Vec, logger: slog::Logger, @@ -126,7 +150,7 @@ impl> EGLDisplay { Ok(EGLDisplay { native: RefCell::new(native), - display: Arc::new(display as *const _), + display: Arc::new(EGLDisplayHandle { handle: display }), egl_version, extensions, logger: log, @@ -259,7 +283,7 @@ impl> EGLDisplay { let mut num_configs = MaybeUninit::uninit(); if unsafe { ffi::egl::ChooseConfig( - *self.display, + **self.display, descriptor.as_ptr(), config_id.as_mut_ptr(), 1, @@ -285,7 +309,7 @@ impl> EGLDisplay { ($display:expr, $config:expr, $attr:expr) => {{ let mut value = MaybeUninit::uninit(); let res = ffi::egl::GetConfigAttrib( - *$display, + **$display, $config, $attr as ffi::egl::types::EGLint, value.as_mut_ptr(), @@ -345,7 +369,7 @@ impl> EGLDisplay { })?; EGLSurface::new( - &self.display, + self.display.clone(), pixel_format, double_buffer, config, @@ -388,14 +412,6 @@ impl> EGLDisplay { } } -impl> Drop for EGLDisplay { - fn drop(&mut self) { - unsafe { - ffi::egl::Terminate((*self.display) as *const _); - } - } -} - #[cfg(feature = "use_system_lib")] impl> EGLGraphicsBackend for EGLDisplay { /// Binds this EGL display to the given Wayland display. @@ -414,12 +430,12 @@ impl> EGLGraphicsBackend for EGL if !self.extensions.iter().any(|s| s == "EGL_WL_bind_wayland_display") { return Err(Error::EglExtensionNotSupported(&["EGL_WL_bind_wayland_display"])); } - let res = unsafe { ffi::egl::BindWaylandDisplayWL(*self.display, display.c_ptr() as *mut _) }; + let res = unsafe { ffi::egl::BindWaylandDisplayWL(**self.display, display.c_ptr() as *mut _) }; if res == 0 { return Err(Error::OtherEGLDisplayAlreadyBound); } Ok(EGLBufferReader::new( - Arc::downgrade(&self.display), + self.display.clone(), display.c_ptr(), &self.extensions, )) @@ -431,7 +447,7 @@ impl> EGLGraphicsBackend for EGL /// Can be created by using [`EGLGraphicsBackend::bind_wl_display`]. #[cfg(feature = "use_system_lib")] pub struct EGLBufferReader { - display: Weak, + display: Arc, wayland: *mut wl_display, #[cfg(feature = "renderer_gl")] gl: gl_ffi::Gles2, @@ -441,11 +457,7 @@ pub struct EGLBufferReader { #[cfg(feature = "use_system_lib")] impl EGLBufferReader { - fn new( - display: Weak, - wayland: *mut wl_display, - extensions: &[String], - ) -> Self { + fn new(display: Arc, wayland: *mut wl_display, extensions: &[String]) -> Self { #[cfg(feature = "renderer_gl")] let gl = gl_ffi::Gles2::load_with(|s| get_proc_address(s) as *const _); @@ -470,105 +482,101 @@ impl EGLBufferReader { &self, buffer: WlBuffer, ) -> ::std::result::Result { - if let Some(display) = self.display.upgrade() { - let mut format: i32 = 0; - if unsafe { - ffi::egl::QueryWaylandBufferWL( - *display, - buffer.as_ref().c_ptr() as *mut _, - ffi::egl::EGL_TEXTURE_FORMAT, - &mut format as *mut _, - ) == 0 - } { - return Err(BufferAccessError::NotManaged(buffer)); - } - let format = match format { - x if x == ffi::egl::TEXTURE_RGB as i32 => Format::RGB, - x if x == ffi::egl::TEXTURE_RGBA as i32 => Format::RGBA, - ffi::egl::TEXTURE_EXTERNAL_WL => Format::External, - ffi::egl::TEXTURE_Y_UV_WL => Format::Y_UV, - ffi::egl::TEXTURE_Y_U_V_WL => Format::Y_U_V, - ffi::egl::TEXTURE_Y_XUXV_WL => Format::Y_XUXV, - _ => panic!("EGL returned invalid texture type"), - }; - - let mut width: i32 = 0; - if unsafe { - ffi::egl::QueryWaylandBufferWL( - *display, - buffer.as_ref().c_ptr() as *mut _, - ffi::egl::WIDTH as i32, - &mut width as *mut _, - ) == 0 - } { - return Err(BufferAccessError::NotManaged(buffer)); - } - - let mut height: i32 = 0; - if unsafe { - ffi::egl::QueryWaylandBufferWL( - *display, - buffer.as_ref().c_ptr() as *mut _, - ffi::egl::HEIGHT as i32, - &mut height as *mut _, - ) == 0 - } { - return Err(BufferAccessError::NotManaged(buffer)); - } - - let mut inverted: i32 = 0; - if unsafe { - ffi::egl::QueryWaylandBufferWL( - *display, - buffer.as_ref().c_ptr() as *mut _, - ffi::egl::WAYLAND_Y_INVERTED_WL, - &mut inverted as *mut _, - ) != 0 - } { - inverted = 1; - } - - let mut images = Vec::with_capacity(format.num_planes()); - for i in 0..format.num_planes() { - let mut out = Vec::with_capacity(3); - out.push(ffi::egl::WAYLAND_PLANE_WL as i32); - out.push(i as i32); - out.push(ffi::egl::NONE as i32); - - images.push({ - let image = unsafe { - ffi::egl::CreateImageKHR( - *display, - ffi::egl::NO_CONTEXT, - ffi::egl::WAYLAND_BUFFER_WL, - buffer.as_ref().c_ptr() as *mut _, - out.as_ptr(), - ) - }; - if image == ffi::egl::NO_IMAGE_KHR { - return Err(BufferAccessError::EGLImageCreationFailed); - } else { - image - } - }); - } - - Ok(EGLImages { - display: Arc::downgrade(&display), - width: width as u32, - height: height as u32, - y_inverted: inverted != 0, - format, - images, - buffer, - #[cfg(feature = "renderer_gl")] - gl: self.gl.clone(), - #[cfg(feature = "renderer_gl")] - egl_to_texture_support: self.egl_to_texture_support, - }) - } else { - Err(BufferAccessError::ContextLost) + let mut format: i32 = 0; + if unsafe { + ffi::egl::QueryWaylandBufferWL( + **self.display, + buffer.as_ref().c_ptr() as _, + ffi::egl::EGL_TEXTURE_FORMAT, + &mut format, + ) == 0 + } { + return Err(BufferAccessError::NotManaged(buffer)); } + let format = match format { + x if x == ffi::egl::TEXTURE_RGB as i32 => Format::RGB, + x if x == ffi::egl::TEXTURE_RGBA as i32 => Format::RGBA, + ffi::egl::TEXTURE_EXTERNAL_WL => Format::External, + ffi::egl::TEXTURE_Y_UV_WL => Format::Y_UV, + ffi::egl::TEXTURE_Y_U_V_WL => Format::Y_U_V, + ffi::egl::TEXTURE_Y_XUXV_WL => Format::Y_XUXV, + _ => panic!("EGL returned invalid texture type"), + }; + + let mut width: i32 = 0; + if unsafe { + ffi::egl::QueryWaylandBufferWL( + **self.display, + buffer.as_ref().c_ptr() as _, + ffi::egl::WIDTH as i32, + &mut width, + ) == 0 + } { + return Err(BufferAccessError::NotManaged(buffer)); + } + + let mut height: i32 = 0; + if unsafe { + ffi::egl::QueryWaylandBufferWL( + **self.display, + buffer.as_ref().c_ptr() as _, + ffi::egl::HEIGHT as i32, + &mut height, + ) == 0 + } { + return Err(BufferAccessError::NotManaged(buffer)); + } + + let mut inverted: i32 = 0; + if unsafe { + ffi::egl::QueryWaylandBufferWL( + **self.display, + buffer.as_ref().c_ptr() as _, + ffi::egl::WAYLAND_Y_INVERTED_WL, + &mut inverted, + ) != 0 + } { + inverted = 1; + } + + let mut images = Vec::with_capacity(format.num_planes()); + for i in 0..format.num_planes() { + let mut out = Vec::with_capacity(3); + out.push(ffi::egl::WAYLAND_PLANE_WL as i32); + out.push(i as i32); + out.push(ffi::egl::NONE as i32); + + images.push({ + let image = unsafe { + ffi::egl::CreateImageKHR( + **self.display, + ffi::egl::NO_CONTEXT, + ffi::egl::WAYLAND_BUFFER_WL, + buffer.as_ref().c_ptr() as *mut _, + out.as_ptr(), + ) + }; + if image == ffi::egl::NO_IMAGE_KHR { + return Err(BufferAccessError::EGLImageCreationFailed); + } else { + image + } + }); + } + + Ok(EGLImages { + display: self.display.clone(), + width: width as u32, + height: height as u32, + y_inverted: inverted != 0, + format, + images, + buffer, + #[cfg(feature = "renderer_gl")] + gl: self.gl.clone(), + #[cfg(feature = "renderer_gl")] + egl_to_texture_support: self.egl_to_texture_support, + }) } /// Try to receive the dimensions of a given [`WlBuffer`]. @@ -576,46 +584,40 @@ impl EGLBufferReader { /// In case the buffer is not managed by EGL (but e.g. the [`wayland::shm` module](::wayland::shm)) or the /// context has been lost, `None` is returned. pub fn egl_buffer_dimensions(&self, buffer: &WlBuffer) -> Option<(i32, i32)> { - if let Some(display) = self.display.upgrade() { - let mut width: i32 = 0; - if unsafe { - ffi::egl::QueryWaylandBufferWL( - *display, - buffer.as_ref().c_ptr() as *mut _, - ffi::egl::WIDTH as i32, - &mut width as *mut _, - ) == 0 - } { - return None; - } - - let mut height: i32 = 0; - if unsafe { - ffi::egl::QueryWaylandBufferWL( - *display, - buffer.as_ref().c_ptr() as *mut _, - ffi::egl::HEIGHT as i32, - &mut height as *mut _, - ) == 0 - } { - return None; - } - - Some((width, height)) - } else { - None + let mut width: i32 = 0; + if unsafe { + ffi::egl::QueryWaylandBufferWL( + **self.display, + buffer.as_ref().c_ptr() as _, + ffi::egl::WIDTH as _, + &mut width, + ) == 0 + } { + return None; } + + let mut height: i32 = 0; + if unsafe { + ffi::egl::QueryWaylandBufferWL( + **self.display, + buffer.as_ref().c_ptr() as _, + ffi::egl::HEIGHT as _, + &mut height, + ) == 0 + } { + return None; + } + + Some((width, height)) } } #[cfg(feature = "use_system_lib")] impl Drop for EGLBufferReader { fn drop(&mut self) { - if let Some(display) = self.display.upgrade() { - if !self.wayland.is_null() { - unsafe { - ffi::egl::UnbindWaylandDisplayWL(*display, self.wayland as *mut _); - } + if !self.wayland.is_null() { + unsafe { + ffi::egl::UnbindWaylandDisplayWL(**self.display, self.wayland as _); } } } diff --git a/src/backend/egl/mod.rs b/src/backend/egl/mod.rs index b2a31dc..dce4a85 100644 --- a/src/backend/egl/mod.rs +++ b/src/backend/egl/mod.rs @@ -42,8 +42,9 @@ pub mod surface; pub use self::surface::EGLSurface; #[cfg(feature = "use_system_lib")] use crate::backend::egl::display::EGLBufferReader; +use crate::backend::egl::display::EGLDisplayHandle; use std::ffi::CString; -use std::sync::Weak; +use std::sync::Arc; /// Error that can happen on optional EGL features #[derive(Debug, Clone, PartialEq)] @@ -167,7 +168,7 @@ impl Format { /// Images of the EGL-based [`WlBuffer`]. #[cfg(feature = "wayland_frontend")] pub struct EGLImages { - display: Weak, + display: Arc, /// Width in pixels pub width: u32, /// Height in pixels @@ -204,41 +205,35 @@ impl EGLImages { plane: usize, tex_id: c_uint, ) -> ::std::result::Result<(), TextureCreationError> { - if self.display.upgrade().is_some() { - if !self.egl_to_texture_support { - return Err(TextureCreationError::GLExtensionNotSupported("GL_OES_EGL_image")); - } - - let mut old_tex_id: i32 = 0; - self.gl.GetIntegerv(gl_ffi::TEXTURE_BINDING_2D, &mut old_tex_id); - self.gl.BindTexture(gl_ffi::TEXTURE_2D, tex_id); - self.gl.EGLImageTargetTexture2DOES( - gl_ffi::TEXTURE_2D, - *self - .images - .get(plane) - .ok_or(TextureCreationError::PlaneIndexOutOfBounds)?, - ); - let res = match ffi::egl::GetError() as u32 { - ffi::egl::SUCCESS => Ok(()), - err => Err(TextureCreationError::TextureBindingFailed(err)), - }; - self.gl.BindTexture(gl_ffi::TEXTURE_2D, old_tex_id as u32); - res - } else { - Err(TextureCreationError::ContextLost) + if !self.egl_to_texture_support { + return Err(TextureCreationError::GLExtensionNotSupported("GL_OES_EGL_image")); } + + let mut old_tex_id: i32 = 0; + self.gl.GetIntegerv(gl_ffi::TEXTURE_BINDING_2D, &mut old_tex_id); + self.gl.BindTexture(gl_ffi::TEXTURE_2D, tex_id); + self.gl.EGLImageTargetTexture2DOES( + gl_ffi::TEXTURE_2D, + *self + .images + .get(plane) + .ok_or(TextureCreationError::PlaneIndexOutOfBounds)?, + ); + let res = match ffi::egl::GetError() as u32 { + ffi::egl::SUCCESS => Ok(()), + err => Err(TextureCreationError::TextureBindingFailed(err)), + }; + self.gl.BindTexture(gl_ffi::TEXTURE_2D, old_tex_id as u32); + res } } #[cfg(feature = "wayland_frontend")] impl Drop for EGLImages { fn drop(&mut self) { - if let Some(display) = self.display.upgrade() { - for image in self.images.drain(..) { - unsafe { - ffi::egl::DestroyImageKHR(*display, image); - } + for image in self.images.drain(..) { + unsafe { + ffi::egl::DestroyImageKHR(**self.display, image); } } self.buffer.release(); diff --git a/src/backend/egl/surface.rs b/src/backend/egl/surface.rs index 05df432..1efe67c 100644 --- a/src/backend/egl/surface.rs +++ b/src/backend/egl/surface.rs @@ -1,9 +1,10 @@ //! EGL surface related structs use super::{ffi, native, Error}; +use crate::backend::egl::display::EGLDisplayHandle; use crate::backend::graphics::{PixelFormat, SwapBuffersError}; use nix::libc::c_int; -use std::sync::{Arc, Weak}; +use std::sync::Arc; use std::{ cell::Cell, ops::{Deref, DerefMut}, @@ -11,7 +12,7 @@ use std::{ /// EGL surface of a given EGL context for rendering pub struct EGLSurface { - display: Weak, + display: Arc, native: N, pub(crate) surface: Cell, config_id: ffi::egl::types::EGLConfig, @@ -34,7 +35,7 @@ impl DerefMut for EGLSurface { impl EGLSurface { pub(crate) fn new( - display: &Arc, + display: Arc, pixel_format: PixelFormat, double_buffered: Option, config: ffi::egl::types::EGLConfig, @@ -76,7 +77,7 @@ impl EGLSurface { } Ok(EGLSurface { - display: Arc::downgrade(display), + display, native, surface: Cell::new(surface), config_id: config, @@ -90,34 +91,28 @@ impl EGLSurface { let surface = self.surface.get(); if !surface.is_null() { - if let Some(display) = self.display.upgrade() { - let ret = unsafe { ffi::egl::SwapBuffers((*display) as *const _, surface as *const _) }; + let ret = unsafe { ffi::egl::SwapBuffers(**self.display, surface as *const _) }; - if ret == 0 { - match unsafe { ffi::egl::GetError() } as u32 { - ffi::egl::CONTEXT_LOST => return Err(SwapBuffersError::ContextLost), - err => return Err(SwapBuffersError::Unknown(err)), - }; - } else { - self.native.swap_buffers()?; - } + if ret == 0 { + match unsafe { ffi::egl::GetError() } as u32 { + ffi::egl::CONTEXT_LOST => return Err(SwapBuffersError::ContextLost), + err => return Err(SwapBuffersError::Unknown(err)), + }; } else { - return Err(SwapBuffersError::ContextLost); + self.native.swap_buffers()?; } }; if self.native.needs_recreation() || surface.is_null() { - if let Some(display) = self.display.upgrade() { - self.native.recreate(); - self.surface.set(unsafe { - ffi::egl::CreateWindowSurface( - *display, - self.config_id, - self.native.ptr(), - self.surface_attributes.as_ptr(), - ) - }); - } + self.native.recreate(); + self.surface.set(unsafe { + ffi::egl::CreateWindowSurface( + **self.display, + self.config_id, + self.native.ptr(), + self.surface_attributes.as_ptr(), + ) + }); } Ok(()) @@ -144,10 +139,8 @@ impl EGLSurface { impl Drop for EGLSurface { fn drop(&mut self) { - if let Some(display) = self.display.upgrade() { - unsafe { - ffi::egl::DestroySurface((*display) as *const _, self.surface.get() as *const _); - } + unsafe { + ffi::egl::DestroySurface(**self.display, self.surface.get() as *const _); } } }