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_KHR_platform_wayland: Clarify requirements of eglSwapBuffers #205

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

Conversation

jadahl
Copy link

@jadahl jadahl commented Sep 9, 2024

The EGL implementation attaching the front buffer, posting damage, and committing during the call to eglSwapBuffers(WithDamage)() is a well established requirement that EGL Wayland clients currently rely upon, thus should be explicitly documented.


I noticed EGL_EXT_platform_wayland already existing, but EGL_KHR_platform_wayland seems to be the one most recently updated. Should this perhaps be targeting that, and with the changes to the KHR variant being ported over?

@@ -93,6 +94,16 @@ New Behavior
until the next buffer swap. The rationale behind this behavior is to keep
operations result accurate until the next swap.

During the call to eglSwapBuffers wl_surface.attach will be requested with
Copy link

Choose a reason for hiding this comment

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

Should we maybe clarify that the EGL implementation is the one who will do these requests instead of just saying they "will be requested"? It seems like that's the way things like this are worded in some other extensions for clarity.

Suggested change
During the call to eglSwapBuffers wl_surface.attach will be requested with
During the call to eglSwapBuffers wl_surface.attach will be requested by the EGL implementation with

Not sure if we also need to call out things like explicit sync points being sent by the implementation as part of this commit? I guess what you have right now just specifies the minimum set of requests the implementation will do, so that could be fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems hard both to enumerate all the state ever needed and to write general language that implies the right amount of state.

Maybe for explicit sync:

"if the EGL implementation associated a wp_linux_drm_syncobj_surface_v1 object with the surface, an acquire and a release point will be associated with the surface prior to the commit as well."

For others state the implementation might need to set as part of a commit, hopefully it makes more sense to call it out in separate extensions? E.g., as an interaction with this extension in a present timing extension, an HDR extension, etc., or in the other direction for existing EGL extensions that necessitate additional wayland surface state?

Copy link
Author

Choose a reason for hiding this comment

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

I reworded things a bit while still trying to make it more explicit, because adding "by the EGL implementation" to every "requested" would have been a bit much.

Copy link
Author

Choose a reason for hiding this comment

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

Didn't add anything about explicit sync there, as it's not strictly needed, but could be added too I guess.

Copy link
Author

Choose a reason for hiding this comment

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

"if the EGL implementation associated a wp_linux_drm_syncobj_surface_v1 object with the surface, an acquire and a release point will be associated with the surface prior to the commit as well."

Added this as well, thanks.

@amshafer
Copy link

amshafer commented Sep 9, 2024

Context on the motivation for this: https://gitlab.freedesktop.org/wayland/wayland/-/merge_requests/423

During the call to eglSwapBuffers wl_surface.attach will be requested with
the new front buffer, a full surface damage is requested via
wl_surface.damage or wl_surface.damage_buffer, and a wl_surface.commit will
be requested to commit the new state.
Copy link
Contributor

Choose a reason for hiding this comment

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

It might be better to call out that this must be synchronous within eglSwapBuffers and cannot be deferred to a thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me this language already implies that, but you could say something like "Before the call to eglSwapBuffers() returns" or similar, rather than "During the call to." It could still happen in another thread, but has to happen before the call ends from the calling thread's perspective, rather than some mangled interpretation of the call's duration from the driver's point of view.

Copy link
Author

Choose a reason for hiding this comment

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

Is that better? I left out mentioning threads, because I guess it's still technically possible to offload anything to a thread, as long as the thread calling eglSwapBuffers is blocked.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like it. Thanks!

@jadahl jadahl force-pushed the wip/egl-wayland-commit-damage branch from 0980dca to cc37425 Compare September 9, 2024 21:26
@jadahl jadahl force-pushed the wip/egl-wayland-commit-damage branch from cc37425 to 1e47686 Compare September 9, 2024 21:47
The EGL implementation attaching the front buffer, posting damage, and
committing during the call to eglSwapBuffers(WithDamage)() is a well
established requirement that EGL Wayland clients currently rely upon,
thus should be explicitly documented.
@jadahl jadahl force-pushed the wip/egl-wayland-commit-damage branch from 1e47686 to fc1b211 Compare September 9, 2024 22:07
@cubanismo
Copy link
Contributor

This version looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants