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: Add wrapper for EGLSync #1646

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

i509VCB
Copy link
Contributor

@i509VCB i509VCB commented Nov 8, 2023

  • 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
Copy link
Contributor Author

i509VCB commented Nov 11, 2023

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

@i509VCB i509VCB marked this pull request as ready for review November 11, 2023 07:24
Comment on lines +17 to +22
// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't we use device platform for that?

Comment on lines +148 to +151
pub(super) struct Inner {
pub(super) inner: egl::types::EGLSyncKHR,
pub(super) display: Arc<DisplayInner>,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These casts prevent messing with mutability though.

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

/// 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> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

- Add `api::egl::sync::Sync` to wrap `EGLSync`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return Err(super::check_error().err().unwrap());
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pub struct Sync(pub(super) Arc<Inner>);

impl Sync {
/// Insert this sync into the currently bound context.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +268 to +270
/// 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

// Successful import means EGL assumes ownership of the file descriptor.
mem::forget(import);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
mem::forget(import);

Comment on lines +293 to +296
// SAFETY:
// - The EGL implementation advertised EGL_ANDROID_native_fence_sync
// - The fd being imported is an OwnedFd, meaning ownership is transfered to
// EGL.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These casts prevent messing with mutability though.

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

//
// 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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


// 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");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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