From 7ec3bb91b72ba249dff6077c1816bb2b4b626ca9 Mon Sep 17 00:00:00 2001 From: Kirill Chibisov Date: Mon, 2 Sep 2024 14:44:28 +0300 Subject: [PATCH 1/3] ci: use winit ci setup --- .github/workflows/ci.yml | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 515c125a4e..d24ef46c1c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -18,18 +18,23 @@ jobs: name: Check formatting runs-on: ubuntu-latest steps: - - uses: actions/checkout@v3 - - uses: hecrj/setup-rust-action@v1 + - uses: taiki-e/checkout-action@v1 + - uses: dtolnay/rust-toolchain@nightly with: - rust-version: nightly components: rustfmt - uses: taiki-e/install-action@v2 with: tool: typos-cli - name: Check Formatting - run: cargo +nightly fmt --all -- --check - - name: Run Typos + run: cargo fmt --all -- --check + - name: run typos run: typos + - name: Typos info + if: failure() + run: | + echo 'To fix typos, please run `typos -w`' + echo 'To check for a diff, run `typos`' + echo 'You can find typos here: https://crates.io/crates/typos' tests: name: Tests From 8ee26006f54be3521e569a8f05b90180024f4b74 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Thu, 5 Sep 2024 13:41:17 +0200 Subject: [PATCH 2/3] egl/display: Pass *pointer to* output variable when querying `DEVICE_EXT` By casting a `null_mut()` ptr and passing it directly _by value_ to `QueryDisplayAttribEXT()`, the function ignores writing the output as it didn't receive an address (it received `NULL`) to write the device to. Instead we should take the _pointer address of this `null_mut()` pointer_ and provide that to the function instead so that it can _overwrite_ it with a pointer to the requested `eglDevice`. This is even more clear when allocating a `MaybeUninit` in which the pointer will be returned, which has a convenient `.as_mut_ptr()` member to pass its pointer value directly into `QueryDisplayAttribEXT()`. --- CHANGELOG.md | 1 + glutin/src/api/egl/display.rs | 13 ++++++++++--- 2 files changed, 11 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f43ae9fe5..6b6fde86b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ - Fixed EGL's `Device::query_devices()` being too strict about required extensions - Fixed crash in `EglGetProcAddress` on Win32-x86 platform due to wrong calling convention +- Fixed EGL's `Display::device()` always returning an error due to invalid pointer-argument passing inside # Version 0.32.0 diff --git a/glutin/src/api/egl/display.rs b/glutin/src/api/egl/display.rs index 0cae10d9b4..0e2ee62e38 100644 --- a/glutin/src/api/egl/display.rs +++ b/glutin/src/api/egl/display.rs @@ -2,11 +2,11 @@ use std::collections::HashSet; use std::ffi::{self, CStr}; +use std::fmt; use std::mem::MaybeUninit; use std::ops::Deref; use std::os::raw::c_char; use std::sync::Arc; -use std::{fmt, ptr}; use glutin_egl_sys::egl; use glutin_egl_sys::egl::types::{EGLAttrib, EGLDisplay, EGLint}; @@ -193,12 +193,12 @@ impl Display { .into()); } - let device = ptr::null_mut(); + let mut device = MaybeUninit::uninit(); if unsafe { self.inner.egl.QueryDisplayAttribEXT( *self.inner.raw, egl::DEVICE_EXT as EGLint, - device as *mut _, + device.as_mut_ptr(), ) } == egl::FALSE { @@ -211,6 +211,13 @@ impl Display { })); } + let device = unsafe { device.assume_init() } as egl::types::EGLDeviceEXT; + debug_assert_ne!( + device, + egl::NO_DEVICE_EXT, + "eglQueryDisplayAttribEXT(EGL_DEVICE_EXT) should never return EGL_NO_DEVICE_EXT on \ + success" + ); Device::from_ptr(self.inner.egl, device) } From 6eded55ce4c15802e310a65e6cea244cb53ddd0e Mon Sep 17 00:00:00 2001 From: Kirill Chibisov Date: Thu, 5 Sep 2024 14:48:51 +0300 Subject: [PATCH 3/3] examples: clean up examples to not recreate display It was incorrectly ported that the display was recreated, though, given that passing the same arguments to it will yield the same results it was still working as expected. --- glutin_examples/examples/egl_device.rs | 2 +- glutin_examples/src/lib.rs | 226 +++++++++++++++---------- 2 files changed, 137 insertions(+), 91 deletions(-) diff --git a/glutin_examples/examples/egl_device.rs b/glutin_examples/examples/egl_device.rs index 191eb657eb..c615c31b94 100644 --- a/glutin_examples/examples/egl_device.rs +++ b/glutin_examples/examples/egl_device.rs @@ -112,7 +112,7 @@ mod example { } let path = Path::new(IMG_PATH); - let file = OpenOptions::new().write(true).truncate(true).open(path).unwrap(); + let file = OpenOptions::new().create(true).write(true).truncate(true).open(path).unwrap(); let mut encoder = png::Encoder::new(file, 1280, 720); encoder.set_depth(png::BitDepth::Eight); diff --git a/glutin_examples/src/lib.rs b/glutin_examples/src/lib.rs index e997dd01d3..e5b75e17bf 100644 --- a/glutin_examples/src/lib.rs +++ b/glutin_examples/src/lib.rs @@ -7,10 +7,11 @@ use gl::types::GLfloat; use raw_window_handle::HasWindowHandle; use winit::application::ApplicationHandler; use winit::event::{KeyEvent, WindowEvent}; +use winit::event_loop::ActiveEventLoop; use winit::keyboard::{Key, NamedKey}; -use winit::window::Window; +use winit::window::{Window, WindowAttributes}; -use glutin::config::{Config, ConfigTemplateBuilder}; +use glutin::config::{Config, ConfigTemplateBuilder, GetGlConfig}; use glutin::context::{ ContextApi, ContextAttributesBuilder, NotCurrentContext, PossiblyCurrentContext, Version, }; @@ -28,10 +29,6 @@ pub mod gl { } pub fn main(event_loop: winit::event_loop::EventLoop<()>) -> Result<(), Box> { - let window_attributes = Window::default_attributes() - .with_transparent(true) - .with_title("Glutin triangle gradient example (press Escape to exit)"); - // The template will match only the configurations supporting rendering // to windows. // @@ -43,7 +40,7 @@ pub fn main(event_loop: winit::event_loop::EventLoop<()>) -> Result<(), Box) -> Result<(), Box ok, - Err(e) => { - self.exit_state = Err(e); - event_loop.exit(); - return; - }, - }; - - println!("Picked a config with {} samples", gl_config.num_samples()); - - let raw_window_handle = window - .as_ref() - .and_then(|window| window.window_handle().ok()) - .map(|handle| handle.as_raw()); - - // XXX The display could be obtained from any object created by it, so we can - // query it from the config. - let gl_display = gl_config.display(); - - // The context creation part. - let context_attributes = ContextAttributesBuilder::new().build(raw_window_handle); - - // Since glutin by default tries to create OpenGL core context, which may not be - // present we should try gles. - let fallback_context_attributes = ContextAttributesBuilder::new() - .with_context_api(ContextApi::Gles(None)) - .build(raw_window_handle); - - // There are also some old devices that support neither modern OpenGL nor GLES. - // To support these we can try and create a 2.1 context. - let legacy_context_attributes = ContextAttributesBuilder::new() - .with_context_api(ContextApi::OpenGl(Some(Version::new(2, 1)))) - .build(raw_window_handle); - - // Reuse the uncurrented context from a suspended() call if it exists, otherwise - // this is the first time resumed() is called, where the context still - // has to be created. - let not_current_gl_context = self.not_current_gl_context.take().unwrap_or_else(|| unsafe { - gl_display.create_context(&gl_config, &context_attributes).unwrap_or_else(|_| { - gl_display.create_context(&gl_config, &fallback_context_attributes).unwrap_or_else( - |_| { - gl_display - .create_context(&gl_config, &legacy_context_attributes) - .expect("failed to create context") + fn resumed(&mut self, event_loop: &ActiveEventLoop) { + let (window, gl_config) = match &self.gl_display { + // We just created the event loop, so initialize the display, pick the config, and + // create the context. + GlDisplayCreationState::Builder(display_builder) => { + let (window, gl_config) = match display_builder.clone().build( + event_loop, + self.template.clone(), + gl_config_picker, + ) { + Ok((window, gl_config)) => (window.unwrap(), gl_config), + Err(err) => { + self.exit_state = Err(err); + event_loop.exit(); + return; }, - ) - }) - }); + }; - #[cfg(android_platform)] - println!("Android window available"); + println!("Picked a config with {} samples", gl_config.num_samples()); - let window = window.take().unwrap_or_else(|| { - let window_attributes = Window::default_attributes() - .with_transparent(true) - .with_title("Glutin triangle gradient example (press Escape to exit)"); - glutin_winit::finalize_window(event_loop, window_attributes, &gl_config).unwrap() - }); + // Mark the display as initialized to not recreate it on resume, since the + // display is valid until we explicitly destroy it. + self.gl_display = GlDisplayCreationState::Init; + + // Create gl context. + self.gl_context = + Some(create_gl_context(&window, &gl_config).treat_as_possibly_current()); + + (window, gl_config) + }, + GlDisplayCreationState::Init => { + println!("Recreating window in `resumed`"); + // Pick the config which we already use for the context. + let gl_config = self.gl_context.as_ref().unwrap().config(); + match glutin_winit::finalize_window(event_loop, window_attributes(), &gl_config) { + Ok(window) => (window, gl_config), + Err(err) => { + self.exit_state = Err(err.into()); + event_loop.exit(); + return; + }, + } + }, + }; let attrs = window .build_surface_attributes(Default::default()) @@ -123,41 +100,42 @@ impl ApplicationHandler for App { let gl_surface = unsafe { gl_config.display().create_window_surface(&gl_config, &attrs).unwrap() }; - // Make it current. - let gl_context = not_current_gl_context.make_current(&gl_surface).unwrap(); - // The context needs to be current for the Renderer to set up shaders and // buffers. It also performs function loading, which needs a current context on // WGL. - self.renderer.get_or_insert_with(|| Renderer::new(&gl_display)); + let gl_context = self.gl_context.as_ref().unwrap(); + gl_context.make_current(&gl_surface).unwrap(); + + self.renderer.get_or_insert_with(|| Renderer::new(&gl_config.display())); // Try setting vsync. if let Err(res) = gl_surface - .set_swap_interval(&gl_context, SwapInterval::Wait(NonZeroU32::new(1).unwrap())) + .set_swap_interval(gl_context, SwapInterval::Wait(NonZeroU32::new(1).unwrap())) { eprintln!("Error setting vsync: {res:?}"); } - assert!(self.state.replace(AppState { gl_context, gl_surface, window }).is_none()); + assert!(self.state.replace(AppState { gl_surface, window }).is_none()); } - fn suspended(&mut self, _event_loop: &winit::event_loop::ActiveEventLoop) { + fn suspended(&mut self, _event_loop: &ActiveEventLoop) { // This event is only raised on Android, where the backing NativeWindow for a GL // Surface can appear and disappear at any moment. println!("Android window removed"); // Destroy the GL Surface and un-current the GL Context before ndk-glue releases // the window back to the system. - let gl_context = self.state.take().unwrap().gl_context; - assert!(self - .not_current_gl_context - .replace(gl_context.make_not_current().unwrap()) - .is_none()); + self.state = None; + + // Make context not current. + self.gl_context = Some( + self.gl_context.take().unwrap().make_not_current().unwrap().treat_as_possibly_current(), + ); } fn window_event( &mut self, - event_loop: &winit::event_loop::ActiveEventLoop, + event_loop: &ActiveEventLoop, _window_id: winit::window::WindowId, event: WindowEvent, ) { @@ -167,12 +145,14 @@ impl ApplicationHandler for App { // Notable platforms here are Wayland and macOS, other don't require it // and the function is no-op, but it's wise to resize it for portability // reasons. - if let Some(AppState { gl_context, gl_surface, window: _ }) = self.state.as_ref() { + if let Some(AppState { gl_surface, window: _ }) = self.state.as_ref() { + let gl_context = self.gl_context.as_ref().unwrap(); gl_surface.resize( gl_context, NonZeroU32::new(size.width).unwrap(), NonZeroU32::new(size.height).unwrap(), ); + let renderer = self.renderer.as_ref().unwrap(); renderer.resize(size.width as i32, size.height as i32); } @@ -186,8 +166,26 @@ impl ApplicationHandler for App { } } - fn about_to_wait(&mut self, _event_loop: &winit::event_loop::ActiveEventLoop) { - if let Some(AppState { gl_context, gl_surface, window }) = self.state.as_ref() { + fn exiting(&mut self, _event_loop: &ActiveEventLoop) { + // NOTE: The handling below is only needed due to nvidia on Wayland to not crash + // on exit due to nvidia driver touching the Wayland display from on + // `exit` hook. + let _gl_display = self.gl_context.take().unwrap().display(); + + // Clear the window. + self.state = None; + #[cfg(egl_backend)] + #[allow(irrefutable_let_patterns)] + if let glutin::display::Display::Egl(display) = _gl_display { + unsafe { + display.terminate(); + } + } + } + + fn about_to_wait(&mut self, _event_loop: &ActiveEventLoop) { + if let Some(AppState { gl_surface, window }) = self.state.as_ref() { + let gl_context = self.gl_context.as_ref().unwrap(); let renderer = self.renderer.as_ref().unwrap(); renderer.draw(); window.request_redraw(); @@ -197,23 +195,72 @@ impl ApplicationHandler for App { } } +fn create_gl_context(window: &Window, gl_config: &Config) -> NotCurrentContext { + let raw_window_handle = window.window_handle().ok().map(|wh| wh.as_raw()); + + // The context creation part. + let context_attributes = ContextAttributesBuilder::new().build(raw_window_handle); + + // Since glutin by default tries to create OpenGL core context, which may not be + // present we should try gles. + let fallback_context_attributes = ContextAttributesBuilder::new() + .with_context_api(ContextApi::Gles(None)) + .build(raw_window_handle); + + // There are also some old devices that support neither modern OpenGL nor GLES. + // To support these we can try and create a 2.1 context. + let legacy_context_attributes = ContextAttributesBuilder::new() + .with_context_api(ContextApi::OpenGl(Some(Version::new(2, 1)))) + .build(raw_window_handle); + + // Reuse the uncurrented context from a suspended() call if it exists, otherwise + // this is the first time resumed() is called, where the context still + // has to be created. + let gl_display = gl_config.display(); + + unsafe { + gl_display.create_context(gl_config, &context_attributes).unwrap_or_else(|_| { + gl_display.create_context(gl_config, &fallback_context_attributes).unwrap_or_else( + |_| { + gl_display + .create_context(gl_config, &legacy_context_attributes) + .expect("failed to create context") + }, + ) + }) + } +} + +fn window_attributes() -> WindowAttributes { + Window::default_attributes() + .with_transparent(true) + .with_title("Glutin triangle gradient example (press Escape to exit)") +} + +enum GlDisplayCreationState { + /// The display was not build yet. + Builder(DisplayBuilder), + /// The display was already created for the application. + Init, +} + struct App { template: ConfigTemplateBuilder, - display_builder: DisplayBuilder, - exit_state: Result<(), Box>, - not_current_gl_context: Option, renderer: Option, // NOTE: `AppState` carries the `Window`, thus it should be dropped after everything else. state: Option, + gl_context: Option, + gl_display: GlDisplayCreationState, + exit_state: Result<(), Box>, } impl App { fn new(template: ConfigTemplateBuilder, display_builder: DisplayBuilder) -> Self { Self { template, - display_builder, + gl_display: GlDisplayCreationState::Builder(display_builder), exit_state: Ok(()), - not_current_gl_context: None, + gl_context: None, state: None, renderer: None, } @@ -221,7 +268,6 @@ impl App { } struct AppState { - gl_context: PossiblyCurrentContext, gl_surface: Surface, // NOTE: Window should be dropped after all resources created using its // raw-window-handle.