[Mesa-dev] [PATCH 04/15] egl: import headers from Khronos EGL registry

[Chad Versace]

On Thu 14 May 2015, Emil Velikov wrote:
> Hi Marek,
> On 12/05/15 22:54, Marek Olšák wrote:
> > From: Marek Olšák <marek.olsak at amd.com>
> > 
> > with the extension of keeping:
> >     #define KHRONOS_APICALL __attribute__((visibility("default")))
> > 
> > And don't include mesa headers in egl.h.
> Can we do this more gradually (like below). It will ease the conflicts
> that this patch might cause.
> 
>  - egl.h - should have no side effects
>  - eglext.h and eglmesaext.h - will clearly illustrate the parts that
> have been moved from the latter to the former.
>  - eglplatform.h and khrplatform.h - there are a few controversial
> changes which might cause breakage.

I want to see this patch broken up too. Currently, the patch contains
a straightforward header update layered on top of Marek's header
modifications. The two should be split into separate patches so it's
clear which changes are which.

> > ---
> >  include/EGL/egl.h          | 562 +++++++++++++++++++++------------------------
> >  include/EGL/eglext.h       | 259 +++++++++++++++++++--
> >  include/EGL/eglplatform.h  |  45 +---
> >  include/KHR/khrplatform.h  |  25 +-
> >  src/egl/main/egltypedefs.h |   2 +
> >  5 files changed, 540 insertions(+), 353 deletions(-)
> > 
> 
> > diff --git a/include/EGL/eglplatform.h b/include/EGL/eglplatform.h
> > index 2eb6865..1284089 100644
> > --- a/include/EGL/eglplatform.h
> > +++ b/include/EGL/eglplatform.h
> 
> > -#elif defined(__WINSCW__) || defined(__SYMBIAN32__)  /* Symbian */
> > +#elif defined(__APPLE__) || defined(__WINSCW__) || defined(__SYMBIAN32__)  /* Symbian */
> >  
> >  typedef int   EGLNativeDisplayType;
> >  typedef void *EGLNativeWindowType;
> >  typedef void *EGLNativePixmapType;
> >  
> > -#elif defined(WL_EGL_PLATFORM)
> > +#elif defined(__ANDROID__) || defined(ANDROID)
> >  
> > -typedef struct wl_display     *EGLNativeDisplayType;
> > -typedef struct wl_egl_pixmap  *EGLNativePixmapType;
> > -typedef struct wl_egl_window  *EGLNativeWindowType;
> > +#include <android/native_window.h>
> >  
> > -#elif defined(__GBM__)
> > -
> > -typedef struct gbm_device  *EGLNativeDisplayType;
> > -typedef struct gbm_bo      *EGLNativePixmapType;
> > -typedef void               *EGLNativeWindowType;
> > -
> > -#elif defined(ANDROID) /* Android */
> > -
> > -struct ANativeWindow;
> >  struct egl_native_pixmap_t;
> >  
> > -typedef struct ANativeWindow        *EGLNativeWindowType;
> > -typedef struct egl_native_pixmap_t  *EGLNativePixmapType;
> > -typedef void                        *EGLNativeDisplayType;
> > +typedef struct ANativeWindow*           EGLNativeWindowType;
> > +typedef struct egl_native_pixmap_t*     EGLNativePixmapType;
> > +typedef void*                           EGLNativeDisplayType;
> >  
> >  #elif defined(__unix__)
> >  
> > -#if defined(MESA_EGL_NO_X11_HEADERS)
> > -
> > -typedef void            *EGLNativeDisplayType;
> > -typedef khronos_uintptr_t EGLNativePixmapType;
> > -typedef khronos_uintptr_t EGLNativeWindowType;
> > -
> > -#else
> > -
> >  /* X11 (tentative)  */
> >  #include <X11/Xlib.h>
> >  #include <X11/Xutil.h>
> > @@ -122,18 +103,8 @@ typedef Display *EGLNativeDisplayType;
> >  typedef Pixmap   EGLNativePixmapType;
> >  typedef Window   EGLNativeWindowType;
> >  
> > -#endif /* MESA_EGL_NO_X11_HEADERS */
> > -
> > -#elif __HAIKU__
> > -#include <kernel/image.h>
> > -typedef void				*EGLNativeDisplayType;
> > -typedef khronos_uintptr_t	 EGLNativePixmapType;
> > -typedef khronos_uintptr_t	 EGLNativeWindowType;
> > -
> Upon closer look, one could get away with the above changes, although
> there may be something more subtle to it.
> 
> Kristian, Chad,
> 
> Would you have any suggestions for/against nuking the Wayland/GBM/other
> typedefs ? With an extra eye on the Haiku changes, what would it take to
> get the current eglplatform.h (or equivalent) accepted with Khronos ?

I'm against. Khronos intended eglplatform.h specifically for this
purpose. To quote the Khronos API Implementer's Guide:

    Implementers may need to modify eglplatform.h. In particular the
    eglNativeDisplayType, eglNativeWindowType, and eglNativePixmapType
    typedefs must be defined as appropriate for the platform (typically,
    they will be typedef'ed to corresponding types in the native window
    system). Developer documentation should mention the correspondence so
    that developers know what parameters to pass to eglCreateWindowSurface,
    eglCreatePixmapSurface, and eglCopyBuffers. Documentation should also
    describe the format of the display_id parameter to eglGetDisplay, since
    this is a platform-specific identifier. 

    [https://www.khronos.org/registry/implementers_guide.html]

And the EGL 1.5 spec says this about eglplatform.h:

    All platform-specific types, values, and macros used in egl.h are
    partitioned into a platform header, eglplatform.h, which is
    automatically included by egl.h. [...] Implementers should need to
    modify only eglplatform.h, never egl.h 2 .

Therefore eglplatform.h should keep the typedefs for Wayland, GBM,
Haiku, and Android. The specs say the vendor should provide the
typedefs, and other projects are already relying on them. The header
needs to keep MESA_EGL_NO_X11_HEADERS block too, because that block was
added specifically to fix compilation on Linux systems that lacked X11.

> > diff --git a/include/KHR/khrplatform.h b/include/KHR/khrplatform.h
> > index 4479539..faa0ed7 100644
> > --- a/include/KHR/khrplatform.h
> > +++ b/include/KHR/khrplatform.h
> 
> >  #if defined(_WIN32) && !defined(__SCITECH_SNAP__)
> > -#   if defined(KHRONOS_DLL_EXPORTS)
> > -#      define KHRONOS_APICALL __declspec(dllexport)
> > -#   else
> > -#      define KHRONOS_APICALL __declspec(dllimport)
> > -#   endif
> > +#   define KHRONOS_APICALL __declspec(dllimport)
> This might cause a problem with our Windows/SCons build. On the surface
> it seems to rely on KHRONOS_DLL_EXPORTS. Although we do have module
> definition files, which set the correct symbols as external.
> 
> Perhaps Jose/Brian might be able to confirm if things are good or go
> pear-shaped ?
> 
> 
> > diff --git a/src/egl/main/egltypedefs.h b/src/egl/main/egltypedefs.h
> > index e90959a..2430033 100644
> > --- a/src/egl/main/egltypedefs.h
> > +++ b/src/egl/main/egltypedefs.h
> > @@ -35,6 +35,8 @@
> >  
> >  #include <EGL/egl.h>
> >  #include <EGL/eglext.h>
> > +#include <EGL/eglextchromium.h>
> > +#include <EGL/eglmesaext.h>
> >  
> If my memory recalls correctly there was a discussion that
> additional/third party extension headers should be included from within
> eglext.h. Although I'm struggling to find the quote plus was never a fan
> of it.

> This will require that we check all possible users of the remaining
> mesa/chromium extensions and update them :\

My interpretation of the Khronos API Implementer's Guide says otherwise:

    Functions and enumerants for unregistered implementer extensions should
    be declared and defined in an implementer's own header file.

But, from a pragmatic standpoint, I don't wish to break existing user's
builds over this small technicality. I prefer to continue including
eglext{vendor}.h from eglext.h unless there is a practical reason not
to.