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

RFC: EGL counterpart to GLX_EXT_swap_control_tear #113

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

nwnk
Copy link
Contributor

@nwnk nwnk commented Sep 9, 2020

There's no EGL analogue to GLX_EXT_swap_control_tear, which seems like an easy oversight to fix.

I don't have this quite wired up for Mesa yet, and there's some details to address. The GLX extensions let you name a drawable explicitly (meh) and also query the actually-set swap interval and whether tears can be requested. I'm not convinced that's useful for EGL, since you can't import an EGL surface the way you can a GLX drawable, but maybe we're orthogonality enthusiasts. Feedback welcome.

@flibitijibibo
Copy link

I was looking at this same issue for ANGLE, where the Vulkan backend could use FIFO_RELAXED to implement the feature:

https://bugs.chromium.org/p/angleproject/issues/detail?id=4257

Incidentally, the NVIDIA binary driver already accepts negative values for eglSwapInterval, so the extension is somewhat supported already despite not being advertised at all.

@ShabbyX
Copy link

ShabbyX commented Sep 14, 2020

Looks useful. IIUC, FIFO_RELAXED is the equivalent of eglSwapInternal(-1) per this extension, so ANGLE would have to set EGL_MIN_SWAP_INTERVAL to -1.

I'm not sure how problematic that is though for applications. What if an application just uses the value of EGL_MIN_SWAP_INTERVAL knowing that it's >= 0 per spec, but then suddenly that's not true anymore? Then they silently go from no-vsync to vsync mode.

What if a new symbol like EGL_MIN_NEGATIVE_SWAP_INTERVAL is introduced for this purpose so that EGL_MIN_SWAP_INTERVAL can retain its value with or without this extension?

@stonesthrow
Copy link
Contributor

EGL Work Group ping'd for comment.

@dkartch-nv
Copy link
Contributor

Incidentally, the NVIDIA binary driver already accepts negative values for eglSwapInterval, so the extension is somewhat supported already despite not being advertised at all.

As per the specification for eglSwapInterval, the NVIDIA EGL driver silently clamps negative values to EGL_MIN_SWAP_INTERVAL, which is 0. The feature described here is NOT supported.

@dkartch-nv
Copy link
Contributor

I'm not sure how problematic that is though for applications. What if an application just uses the value of EGL_MIN_SWAP_INTERVAL knowing that it's >= 0 per spec, but then suddenly that's not true anymore? Then they silently go from no-vsync to vsync mode.

What if a new symbol like EGL_MIN_NEGATIVE_SWAP_INTERVAL is introduced for this purpose so that EGL_MIN_SWAP_INTERVAL can retain its value with or without this extension?

I agree that is a potential problem which will cause backwards compatibility issues. I think it probably wasn't a consideration for GLX because it only exposes a constant for MAX interval, not MIN.

It seems to be that a better way to support this feature would be as a separate flag, rather than overloading the swap interval. But if there's a need to make this mirror the GLX extension, then perhaps a compromise is that it requires explicitly setting a new surface attribute to enable the feature, and then eglSwapInterval will accept negative values for that surface.

@flibitijibibo
Copy link

flibitijibibo commented Sep 21, 2020

Are we aware of any scenario where the max swap interval would not be possible as a negative swap interval? If not, the attribute might be enough, without having to worry for something like MIN_NEGATIVE_INTERVAL - having a new flag would be a bit fussy for us using SDL, for example, since we only have SDL_GL_SetSwapInterval available to us, so any new EGL functionality would have to be tacked onto the EGL backend for that call.

@nwnk
Copy link
Contributor Author

nwnk commented Sep 22, 2020

In Mesa the "max" swap interval is just INT_MAX. Presumably the max is there to accommodate implementations where the limit is imposed by the hardware, but if any hardware actually exposes that kind of interval control we don't take advantage of it in any of the existing drivers (and I don't think we ever did for any of the drivers we've dropped).

Internally Mesa uses the negative-interval convention since it's unambiguous and matched what GLX did. I don't have any particular need for the EGL API to do the same, and given the existing language about clamping to EGL_MIN_SWAP_INTERVAL I'm leaning towards a new surface attribute so apps would need to opt in:

eglSurfaceAttrib(dpy, surface, EGL_LATE_SWAPS_TEAR_XXX, EGL_TRUE);

SDL could still honor the negative-interval convention if it wanted, and Mesa would likely convert that to a negative interval internally, but anybody else that happened to be accidentally setting a negative interval would see no change.

The next question would be whether you'd need to have EGL_LATE_SWAPS_MAY_TEAR_XXX as an EGLConfig attribute. Maybe? For Mesa it's a property of the EGLDisplay not the EGLConfig, and the feature's availability would be a function of having the extension string in the display extensions list; if we made it a property of the EGLConfig it'd just be either set for all or for none. I don't have a preference, but I guess I can imagine hardware where it'd be a property of the EGLConfig.

@flibitijibibo
Copy link

An opt-in for eglSurfaceAttrib would be great for us. We do already have some backends where we set an error without even calling SwapInterval when interval < 0, so we could do the same for the EGL backend if the function returns EGL_FALSE (or EGL_BAD_ATTRIBUTE if it's an old setup).

@ShabbyX might have a better idea of where to put EGL_LATE_SWAPS_MAY_TEAR_XXX if needed, as the ANGLE implementation did have some issues with feature queries for this iirc.

@ShabbyX
Copy link

ShabbyX commented Sep 23, 2020

In the Vulkan backend of ANGLE, we use swap interval to determine the swapchain's VkPresentModeKHR. That in turn selects from a list of eligible present modes queried from the surface. Given that vkGetPhysicalDeviceSurfacePresentModesKHR depends on the surface, I would say EGL_LATE_SWAPS_MAY_TEAR_XXX may or may not have any effect as technically Vulkan is allowed to return a different set of supported present modes per surface configuration. The same is true for eglSwapInterval.

Given that eglSwapInterval applies to the display, EGL_LATE_SWAPS_MAY_TEAR_XXX should also be a display property. In other words, no to EGLConfig attribute IMO.

FYI @null77

@nwnk
Copy link
Contributor Author

nwnk commented Sep 23, 2020

Given that eglSwapInterval applies to the display, EGL_LATE_SWAPS_MAY_TEAR_XXX should also be a display property. In other words, no to EGLConfig attribute IMO.

Just to be clear, when I said "For Mesa it's a property of the EGLDisplay not the EGLConfig", I was describing how the implementation works, not the object where the state is attached. The swap interval is a property of the surface. From the spec (emphasis mine):

The function
EGLBoolean eglSwapInterval(EGLDisplay dpy, EGLint interval);
specifies the minimum number of video frame periods per buffer swap for the draw surface of the current context, for the current rendering API.

That said, in trying to sketch out the spec language adding an EGLConfig attribute and documenting the interactions with the other clever-posting-semantics extensions, I'm not loving it. I'm considering making it only a surface attribute, with some language a la EGL_KHR_mutable_render_buffer or EGL_NV_post_sub_buffer to allow the winsys to just throw errors at you. Something like: eglQuerySurface will return true/false for SWAPS_MAY_TEAR if that's configurable for the surface, and raise EGL_BAD_MATCH otherwise. (This also lets me punt on the issue of what it means to post a tearing swap to a stream or a double-buffered pbuffer.)

@ShabbyX
Copy link

ShabbyX commented Sep 24, 2020

I'm considering making it only a surface attribute

In case I wasn't clear before, that's what ANGLE prefers as well.

@nwnk nwnk force-pushed the egl-mesa-swap-control-tear branch from 89019ca to 925b349 Compare September 29, 2020 18:45
@nwnk
Copy link
Contributor Author

nwnk commented Sep 29, 2020

New revision adds the surface query, some extension interactions, and a few more open issues, and provisionally allocates an enum from Mesa's range. I'm hoping to have it wired up for Mesa later this week, see the Mesa merge request for updates on the code part.

nwnk added 2 commits October 1, 2020 15:07
When we switched to using SPDX identifiers instead of explicit Apache
2.0 text, the resulting generated headers ended up with a nested /*
inside the license comment.
@nwnk nwnk force-pushed the egl-mesa-swap-control-tear branch from 925b349 to f1f1c15 Compare October 1, 2020 19:42
@nwnk
Copy link
Contributor Author

nwnk commented Oct 1, 2020

Updated (atop #114), now with updated XML and headers.

@stonesthrow
Copy link
Contributor

Need a few MESA supporters to review.

@Zamundaaa
Copy link

Working on a Wayland protocol to allow clients to request asynchronous buffer swaps revealed the problem that EGL does not have a way to differentiate between mailbox and immediate presentation modes (borrowing the Vulkan terms) - swap interval of 0 means mailbox by default (at least on Wayland, and it can't be changed without breaking applications).

This extension seems to provide the desired functionality; interpreting the text literally, a swap interval of 0 means that all frames are late and thus should always tear. Should that maybe be stated explicitly in the extension text?

XZiar pushed a commit to XZiar/EGL-Registry that referenced this pull request Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants