eglplatform.h: add EGL_NO_PLATFORM_SPECIFIC_TYPES flag

[caramelli]

Like X11, Wayland (WL_EGL_PLATFORM) or DRM (__GBM__) also correspond to unix-based platforms.
This change allows the use of primitive types regardless of the unix-based platform.
Something like EGL_KHRONOS_TYPES might be more explicit than EGL_NO_X11, but that's a detail.

Nicolas Caramelli

[stonesthrow]

The effect is not clear to me. I have asked the EGL WG to review since we have MESA, X11, Wayland and GBM implementors. Looking for verification that this is good.

[caramelli]

Ok, I'm waiting for the review from the EGL WG. Thanks.

[cubanismo]

Discussed internally here. Looks good to NV.

Additionally, we would be in favor of making EGL_NO_X11 the default in a future change.

[fooishbar]

I would be in favour of EGL_NO_X11 becoming the default behaviour, but not in favour of this MR.

The only reason to select one of the Wayland or GBM platform tokens is specifically to change the declared prototype signature for eglGetDisplay and eglCreateWindowSurface; if they are used, then I think it makes sense to respect that rather than ignore it (even if people should of course be using the platform functions).

What's the particular reason you have for effectively ignoring WL_EGL_PLATFORM?

[caramelli]

In practice, generic types for EGLNativeDisplayType, EGLNativePixmapType, EGLNativeWindowType can be used and work for all platforms.

If the Mesa library on your system supports multiple platforms (such as Wayland, DRM or X11), it's possible to dynamically choose which platform support to use by setting EGL_PLATFORM variable.
Here is the idea:

#include <wayland-egl.h>
#include <gbm.h>
#include <X11/Xlib.h>
#include <EGL/egl.h>

...

struct wl_display *wl_dpy = wl_display_connect()
struct wl_egl_window *wl_win = wl_egl_window_create()
struct gbm_device *drm_dpy = gbm_create_device()
struct gbm_surface *drm_win = gbm_surface_create()
Display *x11_dpy = XOpenDisplay()
Window x11_win = XCreateWindow()

EGLDisplay egl_dpy;
EGLSurface egl_win;
EGLConfig egl_config;

if (!strcmp(getenv("EGL_PLATFORM"), "wayland")) {
egl_dpy = eglGetDisplay((EGLNativeDisplayType)wl_dpy)
egl_win = eglCreateWindowSurface(egl_dpy, egl_config, (EGLNativeWindowType)wl_win, NULL);
}

if (!strcmp(getenv("EGL_PLATFORM"), "drm")) {
egl_dpy = eglGetDisplay((EGLNativeDisplayType)drm_dpy);
egl_win = eglCreateWindowSurface(egl_dpy, egl_config, (EGLNativeWindowType)drm_win, NULL);
}

if (!strcmp(getenv("EGL_PLATFORM"), "x11")) {
egl_dpy = eglGetDisplay((EGLNativeDisplayType)x11_dpy);
egl_win = eglCreateWindowSurface(egl_dpy, egl_config, (EGLNativeWindowType)x11_win, NULL);
}

Without the proposed change, since WL_EGL_PLATFORM comes first in eglplatform.h, EGLNativeDisplayType is defined with "wl_display *" when compiling this code. But given the code above, we have no reason to use this definition instead of another platform's definition.

With the proposed change, the common type "void *" is simply used when setting EGL_NO_X11 flag, without making a preferential choice. And EGL is then completely "independent of definitions and concepts specific to any platform or rendering API."

Nicolas Caramelli

[fooishbar]

Two things - firstly, you should really be using the platform methods, which allow you to explicitly specify the platform as part of the function signature, instead of using the EGL_PLATFORM environment variable. That variable is Mesa-specific and not portable ... as well as quite ugly. You can see an example in Weston which preferentially uses the platform interfaces, falling back to the legacy API if it is not available.

Secondly, I can see the issue here - which is that WL_EGL_PLATFORM is defined by wayland-egl, which I somehow hadn't realised. That was almost certainly a mistake. I'm not sure if we'd be able to fix it, so in that case, perhaps EGL_NO_X11 should win, but in that case it should probably be called EGL_REALLY_NO_PLATFORM_SPECIFIC_TYPES.

[caramelli]

Yes, EGL_PLATFORM is specific to Mesa, but this was only an example to illustrate the proposed change. And yes, using eglGetPlatformDisplayEXT() and eglCreatePlatformWindowSurfaceEXT() is portable, but as we can see in weston/shared/platform.h and as explained, EGLNativeDisplayType will be defined with "wl_display *". The purpose of the initial proposed modification is to have the possibility of using a generic type.

Yes, WL_EGL_PLATFORM is defined in wayland-egl.h
Note that the same goes for the DRM platform, __GBM__ is defined in gbm.h

The proposed change is perhaps the simplest but yes it would be more explicit to have a test expression with EGL_REALLY_NO_PLATFORM_SPECIFIC_TYPES (and to leave EGL_NO_X11 in its place).
In this case, what about defining the following types in order to be aligned with the "void *" used in EGL_EXT_platform_base:

typedef void *EGLNativeDisplayType;
typedef void *EGLNativePixmapType;
typedef void *EGLNativeWindowType;

So I have updated the pull request.

Nicolas Caramelli

[stonesthrow]

@fooishbar, yours to approve if resolves your reservations.

[caramelli]

As suggested in the review, EGL_REALLY_NO_PLATFORM_SPECIFIC_TYPES has been introduced. Is there anything missing from the proposed changes?

Nicolas Caramelli

[stonesthrow]

My suggestion: drop "REALLY_" from token name, and put the types at the end of the if-elif-end chain.

[fooishbar]

Yes sorry, this is really embarrassing. I made that suggestion as a figure of speech, not meaning it to be taken literally.

[caramelli]

Thanks for your comments, the idea is there and the code review is constructive.

OK, I dropped "REALLY_" in the flag name.
But because WL_EGL_PLATFORM is defined in wayland-egl.h and __GBM__ is defined in gbm.h, the order is essential (to put at least before WL_EGL_PLATFORM). For example, if wayland-egl.h is included before egl.h (and therefore before eglplatform.h), the flag will have no effect if we put the types at the end of the if-elif-end chain.