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
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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`

- Add ability to import and export an a sync fd from a `api::egl::sync::Sync`
Copy link
Member

Choose a reason for hiding this comment

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

don't understand what you're trying to say with an a sync


# Version 0.31.1

- Fixed `CGLContextObj` having an invalid encoding on newer macOS versions.
Expand Down
85 changes: 85 additions & 0 deletions glutin/src/api/egl/display.rs
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,91 @@ impl Display {
}
}

/// Create a sync.
///
/// Creating a sync fence requires that an EGL context is currently bound.
/// Otherwise [`ErrorKind::BadMatch`] is returned.
///
/// This function returns [`Err`] if the EGL version is not at least 1.5
/// or `EGL_KHR_fence_sync` is not available. An error is also returned if
/// native fences are not supported.
pub fn create_sync(&self, native: bool) -> Result<super::sync::Sync> {
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 explain what native is supposed to do.

Comment on lines +234 to +242
Copy link
Member

Choose a reason for hiding this comment

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

In general such methods in their modules iirc. So you should move it to sync module ad do impl Display there.

if self.inner.version < super::VERSION_1_5
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
if self.inner.version < super::VERSION_1_5
if self.inner.version < VERSION_1_5

Do a proper crate style import.

&& !self.inner.display_extensions.contains("EGL_KHR_fence_sync")
{
return Err(ErrorKind::NotSupported("Sync objects are not supported").into());
}

if native && !self.inner.display_extensions.contains("EGL_ANDROID_native_fence_sync") {
return Err(ErrorKind::NotSupported("Native fences are not supported").into());
}

let ty = if native { egl::SYNC_NATIVE_FENCE_ANDROID } else { egl::SYNC_FENCE_KHR };
let sync = unsafe { self.inner.egl.CreateSyncKHR(*self.inner.raw, ty, ptr::null()) };

if sync == egl::NO_SYNC {
return Err(super::check_error().err().unwrap());
}

Ok(super::sync::Sync(Arc::new(super::sync::Inner {
inner: sync,
display: self.inner.clone(),
})))
}

/// Import a sync fd into EGL.
///
/// 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.
Comment on lines +268 to +270
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.

///
/// This function returns [`ErrorKind::NotSupported`] if the
/// `EGL_ANDROID_native_fence_sync` extension is not available.
#[cfg(unix)]
pub fn import_sync(
&self,
fd: std::os::unix::prelude::BorrowedFd<'_>,
) -> Result<super::sync::Sync> {
use std::mem;
use std::os::unix::prelude::AsRawFd;

// The specification states that EGL_KHR_fence_sync must be available,
// and therefore does not need to be tested.
if !self.inner.display_extensions.contains("EGL_ANDROID_native_fence_sync") {
return Err(ErrorKind::NotSupported("Importing a sync fd is not supported").into());
}

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.


// SAFETY:
// - The EGL implementation advertised EGL_ANDROID_native_fence_sync
// - The fd being imported is an OwnedFd, meaning ownership is transfered to
// EGL.
Comment on lines +293 to +296
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 :)

let sync = unsafe {
self.inner.egl.CreateSyncKHR(
*self.inner.raw,
egl::SYNC_NATIVE_FENCE_ANDROID,
attrs.as_ptr(),
)
};

if sync == egl::NO_SYNC {
// Drop will implicitly close the duplicated file descriptor.
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.

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);


Ok(super::sync::Sync(Arc::new(super::sync::Inner {
inner: sync,
display: self.inner.clone(),
})))
}

fn get_platform_display(egl: &Egl, display: RawDisplayHandle) -> Result<EglDisplay> {
if !egl.GetPlatformDisplay.is_loaded() {
return Err(ErrorKind::NotSupported("eglGetPlatformDisplay is not supported").into());
Expand Down
4 changes: 4 additions & 0 deletions glutin/src/api/egl/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ use libloading::os::unix as libloading_os;
#[cfg(windows)]
use libloading::os::windows as libloading_os;

use crate::context::Version;
use crate::error::{Error, ErrorKind, Result};
use crate::lib_loading::{SymLoading, SymWrapper};

Expand All @@ -27,6 +28,7 @@ pub mod context;
pub mod device;
pub mod display;
pub mod surface;
pub mod sync;

pub(crate) static EGL: Lazy<Option<Egl>> = Lazy::new(|| {
#[cfg(windows)]
Expand All @@ -41,6 +43,8 @@ pub(crate) static EGL: Lazy<Option<Egl>> = Lazy::new(|| {
type EglGetProcAddress = unsafe extern "C" fn(*const ffi::c_void) -> *const ffi::c_void;
static EGL_GET_PROC_ADDRESS: OnceCell<libloading_os::Symbol<EglGetProcAddress>> = OnceCell::new();

const VERSION_1_5: Version = Version { major: 1, minor: 5 };

pub(crate) struct Egl(pub SymWrapper<egl::Egl>);

unsafe impl Sync for Egl {}
Expand Down
166 changes: 166 additions & 0 deletions glutin/src/api/egl/sync.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,166 @@
//! EGL Sync Fences.

use std::ffi::c_void;
use std::mem::MaybeUninit;
use std::sync::Arc;
use std::time::Duration;

use glutin_egl_sys::egl::types::{EGLenum, EGLint};

use super::display::DisplayInner;
use super::{egl, ErrorKind, VERSION_1_5};
use crate::error::Result;

/// EGL sync object.
#[derive(Debug, Clone)]
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.

///
/// If the EGL version is not at least 1.5 or the `EGL_KHR_wait_sync`
/// extension is not available, this returns [`ErrorKind::NotSupported`].
///
/// This will return [`ErrorKind::BadParameter`] if there is no currently
/// bound context.
pub fn wait(&self) -> Result<()> {
if self.0.display.version < VERSION_1_5
&& !self.0.display.display_extensions.contains("EGL_KHR_wait_sync")
{
return Err(ErrorKind::NotSupported(
"Sync::wait is not supported if EGL_KHR_wait_sync isn't available",
)
.into());
}

if unsafe { self.0.display.egl.WaitSyncKHR(*self.0.display.raw, self.0.inner, 0) }
== egl::FALSE as EGLint
{
return Err(super::check_error().err().unwrap());
}

Ok(())
}

/// Query if the sync is already
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
/// Query if the sync is already
/// Query if the sync is already.

pub fn is_signalled(&self) -> Result<bool> {
let status = unsafe { self.get_attrib(egl::SYNC_STATUS) }? as EGLenum;
Ok(status == egl::SIGNALED)
}

/// 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

/// maximum timeout is [`u64::MAX`] - 1 nanoseconds. Anything larger will be
/// truncated. If the timeout is reached this function returns [`false`].
///
/// If `flush` is [`true`], the currently bound context is flushed.
pub fn client_wait(&self, timeout: Option<Duration>, flush: bool) -> Result<bool> {
let flags = if flush { egl::SYNC_FLUSH_COMMANDS_BIT } else { 0 };
Copy link
Member

Choose a reason for hiding this comment

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

is it 0 or egl::NONE? I'd assume 0.

let timeout = timeout
.as_ref()
.map(Duration::as_nanos)
.map(|nanos| nanos.max(u128::from(u64::MAX)) as u64)
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
.map(|nanos| nanos.max(u128::from(u64::MAX)) as u64)
.map(|nanos| nanos.min(u128::from(u64::MAX)) as u64)

That's how it should be to match with your comment.

.unwrap_or(egl::FOREVER);

let result = unsafe {
self.0.display.egl.ClientWaitSyncKHR(
*self.0.display.raw,
self.0.inner,
flags as EGLint,
timeout,
)
} as EGLenum;

match result {
egl::FALSE => Err(super::check_error().err().unwrap()),
egl::TIMEOUT_EXPIRED => Ok(false),
egl::CONDITION_SATISFIED => Ok(true),
_ => unreachable!(),
}
}

/// Export the fence's underlying sync fd.
///
/// Returns [`ErrorKind::NotSupported`] if the sync is not a native fence.
///
/// # Availability
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
/// # Availability
/// ## Availability

I think glutin uses that amount of ##?

///
/// 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

// Invariants:
// - EGL_KHR_fence_sync must be supported if a Sync is creatable.
use std::os::unix::prelude::FromRawFd;
Copy link
Member

Choose a reason for hiding this comment

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

could you change prelude to io as it should be in all places?

Copy link
Contributor Author

@i509VCB i509VCB Nov 12, 2023

Choose a reason for hiding this comment

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

If you are fine with an MSRV bump to 1.66

Confusion with std::os::fd, ignore the comment


// Check the type of the fence to see if it can be exported.
let ty = unsafe { self.get_attrib(egl::SYNC_TYPE) }?;

// SAFETY: GetSyncAttribKHR was successful.
if ty as EGLenum != egl::SYNC_NATIVE_FENCE_ANDROID {
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 cast the other part.

return Err(ErrorKind::NotSupported("The sync is not a native fence").into());
}

// SAFETY: The fence type is SYNC_NATIVE_FENCE_ANDROID, making it possible to
// export the native fence.
let fd = unsafe {
self.0.display.egl.DupNativeFenceFDANDROID(*self.0.display.raw, self.0.inner)
};

if fd == egl::NO_NATIVE_FENCE_FD_ANDROID {
return Err(super::check_error().err().unwrap());
}

// SAFETY:
// - The file descriptor from EGL is valid if the return value is not
// NO_NATIVE_FENCE_FD_ANDROID.
// - The EGL implemention duplicates the underlying file descriptor and
// transfers ownership to the application.
Ok(unsafe { std::os::unix::prelude::OwnedFd::from_raw_fd(fd) })
Copy link
Member

Choose a reason for hiding this comment

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

full import inside the function.

}

/// 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 {

self.0.inner
}

unsafe fn get_attrib(&self, attrib: EGLenum) -> Result<EGLint> {
let mut result = MaybeUninit::<EGLint>::uninit();

if unsafe {
self.0.display.egl.GetSyncAttribKHR(
*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.

)
} == egl::FALSE
{
return Err(super::check_error().err().unwrap());
};

Ok(unsafe { result.assume_init() })
}
}

#[derive(Debug)]
pub(super) struct Inner {
pub(super) inner: egl::types::EGLSyncKHR,
pub(super) display: Arc<DisplayInner>,
}
Comment on lines +148 to +151
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).


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.

if unsafe { self.display.egl.DestroySyncKHR(*self.display.raw, self.inner) } == egl::FALSE {
// If this fails we can't do much in Drop. At least drain the error.
let _ = super::check_error();
}
}
}

// SAFETY: The Inner owns the sync and the display is valid.
unsafe impl Send for Inner {}
// SAFETY: EGL allows destroying the sync on any thread.
unsafe impl std::marker::Sync for Inner {}
87 changes: 87 additions & 0 deletions glutin_examples/examples/egl_sync.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
fn main() {
#[cfg(all(egl_backend))]
example::run();
}

#[cfg(all(egl_backend))]
mod example {
use glutin::api::egl::display::Display;
use glutin::config::ConfigTemplate;
use glutin::context::{ContextApi, ContextAttributesBuilder};
use glutin::display::{GetDisplayExtensions, GlDisplay};
use glutin::prelude::GlConfig;
use raw_window_handle::HasRawDisplayHandle;
use winit::event_loop::EventLoop;

pub fn run() {
// 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();
Comment on lines +17 to +22
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?


if !display.extensions().contains("EGL_KHR_fence_sync") {
eprintln!("EGL implementation does not support fence sync objects");
return;
}

// Now we need a context to draw to create a sync object.
let template = ConfigTemplate::default();
let config = unsafe { display.find_configs(template) }
.unwrap()
.reduce(
|config, acc| {
if config.num_samples() > acc.num_samples() {
config
} else {
acc
}
},
)
.expect("No available configs");

println!("Picked a config with {} samples", config.num_samples());

let context_attributes =
ContextAttributesBuilder::new().with_context_api(ContextApi::Gles(None)).build(None);
let not_current = unsafe { display.create_context(&config, &context_attributes) }.unwrap();

// Make the context current, since we are not rendering we can do a surfaceless
// bind.
let _context = not_current.make_current_surfaceless().unwrap();

// Now a sync object can be created.
let sync = display.create_sync(false).unwrap();

// The sync object at this point is inserted into the command stream for the GL
// context.
//
// 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?


#[cfg(unix)]
{
if display.extensions().contains("EGL_ANDROID_native_fence_sync") {
use std::os::unix::prelude::AsFd;

println!("EGL Android native fence sync is supported");

// Glutin also supports exporting a sync fence.
// Firstly the sync must be a native fence.
let sync = display.create_sync(true).unwrap();

// An exported Sync FD can then be used in many ways, including:
// - Send the Sync FD to another processe to synchronize rendering
// - Import the Sync FD into another EGL Display
// - Import the Sync FD into Vulkan using VK_KHR_external_fence_fd.
let sync_fd = sync.export_sync_fd().expect("Export failed");

// 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.

}
}
}
}
Loading