egl: Add wrapper for EGLSync #1646

i509VCB · 2023-11-08T22:32:16Z

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

WIP:

Add way to create a Sync

i509VCB · 2023-11-11T07:24:41Z

Still need to write an example for sync and then one for import/export, but largely is done

kchibisov · 2023-11-12T07:26:52Z

glutin_examples/examples/egl_sync.rs

+        // We won't be displaying anything, but we will use winit to get
+        // access to some sort of platform display.
+        let event_loop = EventLoop::new().unwrap();
+
+        // Create the display for the platform.
+        let display = unsafe { Display::new(event_loop.raw_display_handle()) }.unwrap();


can't we use device platform for that?

kchibisov · 2023-11-12T07:28:43Z

glutin/src/api/egl/sync.rs

+pub(super) struct Inner {
+    pub(super) inner: egl::types::EGLSyncKHR,
+    pub(super) display: Arc<DisplayInner>,
+}


only use pub(crate), no pub(super).

kchibisov · 2023-11-12T07:29:13Z

glutin/src/api/egl/sync.rs

+                *self.0.display.raw,
+                self.0.inner,
+                attrib as EGLint,
+                result.as_mut_ptr().cast(),


remove this cast(), use direct casting with explicit types.

These casts prevent messing with mutability though.

If you want an extra type (which I can relate to), use a turbofish.

kchibisov · 2023-11-12T07:31:22Z

glutin/src/api/egl/sync.rs

+    /// This is available on Android and Linux when the
+    /// `EGL_ANDROID_native_fence_sync` extension is available.
+    #[cfg(unix)]
+    pub fn export_sync_fd(&self) -> Result<std::os::unix::prelude::OwnedFd> {


Suggested change

pub fn export_sync_fd(&self) -> Result<std::os::unix::prelude::OwnedFd> {

pub fn export_sync_fd(&self) -> Result<OwnedFd> {

I did a fully qualified path to avoid cfg'd imports

kchibisov · 2023-11-12T07:31:45Z

CHANGELOG.md

@@ -1,5 +1,8 @@
 # Unreleased

+- Add `api::egl::sync::Sync` to wrap `EGLSync`


Suggested change

- Add `api::egl::sync::Sync` to wrap `EGLSync`

- Add `api::egl::sync::Sync` wrapping `EGLSync`

kchibisov · 2023-11-12T07:47:05Z

glutin/src/api/egl/display.rs

+            return Err(super::check_error().err().unwrap());
+        }
+
+        // Successful import means EGL assumes ownership of the file descriptor.


Suggested change

// Successful import means EGL assumes ownership of the file descriptor.

// Successful import means EGL took ownership of the file descriptor.

kchibisov · 2023-11-12T07:52:37Z

glutin/src/api/egl/sync.rs

+    }
+
+    /// Get a raw handle to the `EGLSync`.
+    pub fn raw_device(&self) -> *const c_void {


Suggested change

pub fn raw_device(&self) -> *const c_void {

pub fn raw_sync(&self) -> *const c_void {

kchibisov · 2023-11-12T07:53:10Z

glutin/src/api/egl/sync.rs

+
+impl Drop for Inner {
+    fn drop(&mut self) {
+        // SAFETY: The Sync owns the underlying EGLSyncKHR


Suggested change

// SAFETY: The Sync owns the underlying EGLSyncKHR

// SAFETY: The Sync owns the underlying EGLSyncKHR.

kchibisov · 2023-11-12T07:56:24Z

glutin/src/api/egl/sync.rs

+
+    /// Block and wait for the sync object to be signalled.
+    ///
+    /// A timeout of [`None`] will wait forever. If the timeout is [`Some`], the


Suggested change

/// A timeout of [`None`] will wait forever. If the timeout is [`Some`], the

/// A timeout of [`None`] will until signalled. If the timeout is [`Some`], the

kchibisov · 2023-11-12T07:58:34Z

glutin/src/api/egl/sync.rs

+pub struct Sync(pub(super) Arc<Inner>);
+
+impl Sync {
+    /// Insert this sync into the currently bound context.


The wording is a bit tricky for this one, because context could assume glutin's context.

Suggested change

/// Insert this sync into the currently bound context.

/// Insert this sync into the currently bound *server* context.

MarijnS95 · 2023-12-22T07:10:15Z

glutin/src/api/egl/display.rs

+    /// Glutin will duplicate the sync fd being imported since EGL assumes
+    /// ownership of the file descriptor. If the file descriptor could not
+    /// be cloned, then [`ErrorKind::BadParameter`] is returned.


Why is glutin thinking for the user? Like other wrappers, model this directly in the API by taking an OwnedFd so that there is no possibly-failing dup() if the user already has an OwnedFd exclusively for an EGL fence.

If they really need to duplicate their FD, they can call try_clone() themselves.

MarijnS95 · 2023-12-22T07:49:23Z

glutin/src/api/egl/display.rs

+        let import = fd.try_clone_to_owned().map_err(|_| ErrorKind::BadParameter)?;
+
+        let attrs: [EGLint; 3] =
+            [egl::SYNC_NATIVE_FENCE_FD_ANDROID as EGLint, import.as_raw_fd(), egl::NONE as EGLint];


You should at the very least document that you're not using into_raw_fd() here and seemingly leaving Rust to close the FD again, so that you can close the FD when the function returns an error and does not consume the FD.

I'd personally prefer an into_raw_fd() anyway, and then a special-case unsafe OwnedFd::from_raw_fd() } in the error.

MarijnS95 · 2023-12-22T07:49:29Z

glutin/src/api/egl/display.rs

+        }
+
+        // Successful import means EGL assumes ownership of the file descriptor.
+        mem::forget(import);


Suggested change

mem::forget(import);

MarijnS95 · 2023-12-22T07:52:06Z

glutin/src/api/egl/display.rs

+        // SAFETY:
+        // - The EGL implementation advertised EGL_ANDROID_native_fence_sync
+        // - The fd being imported is an OwnedFd, meaning ownership is transfered to
+        //   EGL.


Glutin doesn't really have these comments elsewhere... Though there is a lot of unsafe :)

MarijnS95 · 2023-12-22T07:58:36Z

glutin/src/api/egl/sync.rs

+                *self.0.display.raw,
+                self.0.inner,
+                attrib as EGLint,
+                result.as_mut_ptr().cast(),


These casts prevent messing with mutability though.

If you want an extra type (which I can relate to), use a turbofish.

MarijnS95 · 2023-12-22T08:01:00Z

glutin_examples/examples/egl_sync.rs

+        //
+        // However we aren't recording any commands so the fence would already be
+        // signalled. Effecitvely it isn't useful to test the signalled value here.
+        sync.is_signalled().unwrap();


For an example, I was hoping to see how you'd use these fences rather than just showing that the functions can be called.

Maybe repurpose an existing example and showcase when the fence is being signalled or where it makes sense to wait on one?

MarijnS95 · 2023-12-22T08:07:23Z

glutin_examples/examples/egl_sync.rs

+
+                // To show that an exported sync fd can be imported, we will reimport the sync
+                // fd we just exported.
+                let _imported_sync = display.import_sync(sync_fd.as_fd()).expect("Import failed");


Prime example. You just received an OwnedFd from an API. Now you borrow it and this function immediately dup()s it even though you could have transferred the OwnedFd straight away.

i509VCB marked this pull request as ready for review November 11, 2023 07:24

i509VCB force-pushed the egl-sync branch from 3f10cc9 to 9e5997e Compare November 12, 2023 06:26

egl: Add wrapper for EGLSync

c24d34a

i509VCB force-pushed the egl-sync branch from 9e5997e to c24d34a Compare November 12, 2023 06:36

kchibisov requested changes Nov 12, 2023

View reviewed changes

kchibisov mentioned this pull request Dec 16, 2023

Design considerations for HDR colorspace support #1653

Open

MarijnS95 requested changes Dec 22, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

egl: Add wrapper for EGLSync #1646

egl: Add wrapper for EGLSync #1646

	pub fn export_sync_fd(&self) -> Result<std::os::unix::prelude::OwnedFd> {
	pub fn export_sync_fd(&self) -> Result<OwnedFd> {

		@@ -1,5 +1,8 @@
		# Unreleased

		- Add `api::egl::sync::Sync` to wrap `EGLSync`

	- Add `api::egl::sync::Sync` to wrap `EGLSync`
	- Add `api::egl::sync::Sync` wrapping `EGLSync`

	// Successful import means EGL assumes ownership of the file descriptor.
	// Successful import means EGL took ownership of the file descriptor.

	pub fn raw_device(&self) -> *const c_void {
	pub fn raw_sync(&self) -> *const c_void {

	// SAFETY: The Sync owns the underlying EGLSyncKHR
	// SAFETY: The Sync owns the underlying EGLSyncKHR.

	/// A timeout of [`None`] will wait forever. If the timeout is [`Some`], the
	/// A timeout of [`None`] will until signalled. If the timeout is [`Some`], the

	/// Insert this sync into the currently bound context.
	/// Insert this sync into the currently bound server context.

egl: Add wrapper for EGLSync #1646

Are you sure you want to change the base?

egl: Add wrapper for EGLSync #1646

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment