[PATCH 0/3] MR5264: winewayland.drv: part 13.2: More OpenGL functionality

[Alexandros Frantzis (＠afrantzis)]

This MR adds support for some more OpenGL functionality:

* wglCreateContextAttribsARB
* wglShareLists
* wgl(Get)SwapIntervalEXT

I originally intended to also include a fix for the translucency issue seen in some compositors, by using EGL_EXT_present_opaque. However, I'll hold off for now since I am seeing some issues with certain hardware and pixel formats when using this extension. Initial investigation indicates it is a problem in Mesa. For anyone who is affected by the translucency issue and wants to try out the patch in their local build anyway, it is at: https://gitlab.winehq.org/afrantzis/wine/-/commit/0685d1d2cfc126e2bb546fe2cd...

-- 

A question for the upcoming work:

In the next OpenGL part I would like to focus on `WGL_ARB_pixel_format`. I was thinking that if a driver implements `wglGetPixelFormatAttribivARB` then it would be possible, in theory at least, to have a default implementation of `wglChoosePixelFormatARB` (and `wglGetPixelFormatAttribfvARB`) in `opengl32`. I am tempted to do this, instead of adding one more driver specific full implementation, although I am not yet sure if there any driver specific considerations or behaviors that would pose issues with such approach. Thoughts?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5264

[PATCH 1/3] winewayland.drv: Implement wglCreateContextAttribsARB.

[Alexandros Frantzis]

From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com>

---
 dlls/winewayland.drv/opengl.c | 96 +++++++++++++++++++++++++++++++++--
 1 file changed, 92 insertions(+), 4 deletions(-)

diff --git a/dlls/winewayland.drv/opengl.c b/dlls/winewayland.drv/opengl.c
index 4692d2f1e15..5f52ae25b67 100644
--- a/dlls/winewayland.drv/opengl.c
+++ b/dlls/winewayland.drv/opengl.c
@@ -24,6 +24,7 @@
 
 #include "config.h"
 
+#include <assert.h>
 #include <dlfcn.h>
 #include <stdlib.h>
 #include <string.h>
@@ -92,6 +93,7 @@ struct wgl_context
     EGLConfig config;
     EGLContext context;
     struct wayland_gl_drawable *draw, *read, *new_draw, *new_read;
+    EGLint attribs[16];
 };
 
 /* lookup the existing drawable for a window, gl_object_mutex must be held */
@@ -320,6 +322,68 @@ static BOOL wgl_context_make_current(struct wgl_context *ctx, HWND draw_hwnd,
     return ret;
 }
 
+static BOOL wgl_context_populate_attribs(struct wgl_context *ctx, const int *attribs)
+{
+    EGLint *egl_attribs = ctx->attribs;
+
+    if (!attribs) goto out;
+
+    for (; attribs[0] != 0; attribs += 2)
+    {
+        EGLint name;
+
+        TRACE("%#x %#x\n", attribs[0], attribs[1]);
+
+        /* Find the EGL attribute names corresponding to the WGL names.
+         * For all of the attributes below, the values match between the two
+         * systems, so we can use them directly. */
+        switch (attribs[0])
+        {
+        case WGL_CONTEXT_MAJOR_VERSION_ARB:
+            name = EGL_CONTEXT_MAJOR_VERSION_KHR;
+            break;
+        case WGL_CONTEXT_MINOR_VERSION_ARB:
+            name = EGL_CONTEXT_MINOR_VERSION_KHR;
+            break;
+        case WGL_CONTEXT_FLAGS_ARB:
+            name = EGL_CONTEXT_FLAGS_KHR;
+            break;
+        case WGL_CONTEXT_OPENGL_NO_ERROR_ARB:
+            name = EGL_CONTEXT_OPENGL_NO_ERROR_KHR;
+            break;
+        case WGL_CONTEXT_PROFILE_MASK_ARB:
+            if (attribs[1] & WGL_CONTEXT_ES2_PROFILE_BIT_EXT)
+            {
+                ERR("OpenGL ES contexts are not supported\n");
+                return FALSE;
+            }
+            name = EGL_CONTEXT_OPENGL_PROFILE_MASK_KHR;
+            break;
+        default:
+            name = EGL_NONE;
+            FIXME("Unhandled attributes: %#x %#x\n", attribs[0], attribs[1]);
+        }
+
+        if (name != EGL_NONE)
+        {
+            EGLint *dst = ctx->attribs;
+            /* Check if we have already set the same attribute and replace it. */
+            for (; dst != egl_attribs && *dst != name; dst += 2) continue;
+            /* Our context attribute array should have enough space for all the
+             * attributes we support (we merge repetitions), plus EGL_NONE. */
+            assert(dst - ctx->attribs <= ARRAY_SIZE(ctx->attribs) - 3);
+            dst[0] = name;
+            dst[1] = attribs[1];
+            if (dst == egl_attribs) egl_attribs += 2;
+        }
+    }
+
+out:
+    *egl_attribs = EGL_NONE;
+    return TRUE;
+}
+
+
 static void wgl_context_refresh(struct wgl_context *ctx)
 {
     BOOL refresh = FALSE;
@@ -381,7 +445,8 @@ static BOOL set_pixel_format(HDC hdc, int format, BOOL internal)
     return TRUE;
 }
 
-static struct wgl_context *create_context(HDC hdc)
+static struct wgl_context *create_context(HDC hdc, struct wgl_context *share,
+                                          const int *attribs)
 {
     struct wayland_gl_drawable *gl;
     struct wgl_context *ctx;
@@ -394,8 +459,17 @@ static struct wgl_context *create_context(HDC hdc)
         goto out;
     }
 
+    if (!wgl_context_populate_attribs(ctx, attribs))
+    {
+        ctx->attribs[0] = EGL_NONE;
+        goto out;
+    }
+
+    /* For now only OpenGL is supported. */
+    p_eglBindAPI(EGL_OPENGL_API);
     ctx->context = p_eglCreateContext(egl_display, EGL_NO_CONFIG_KHR,
-                                      EGL_NO_CONTEXT, NULL);
+                                      share ? share->context : EGL_NO_CONTEXT,
+                                      ctx->attribs);
 
     pthread_mutex_lock(&gl_object_mutex);
     list_add_head(&gl_contexts, &ctx->entry);
@@ -427,8 +501,15 @@ static BOOL wayland_wglCopyContext(struct wgl_context *src,
 static struct wgl_context *wayland_wglCreateContext(HDC hdc)
 {
     TRACE("hdc=%p\n", hdc);
-    p_eglBindAPI(EGL_OPENGL_API);
-    return create_context(hdc);
+    return create_context(hdc, NULL, NULL);
+}
+
+static struct wgl_context *wayland_wglCreateContextAttribsARB(HDC hdc,
+                                                              struct wgl_context *share,
+                                                              const int *attribs)
+{
+    TRACE("hdc=%p share=%p attribs=%p\n", hdc, share, attribs);
+    return create_context(hdc, share, attribs);
 }
 
 static BOOL wayland_wglDeleteContext(struct wgl_context *ctx)
@@ -623,6 +704,11 @@ static BOOL init_opengl_funcs(void)
     opengl_funcs.ext.p_wglGetCurrentReadDCARB = (void *)1;  /* never called */
     opengl_funcs.ext.p_wglMakeContextCurrentARB = wayland_wglMakeContextCurrentARB;
 
+    register_extension("WGL_ARB_create_context");
+    register_extension("WGL_ARB_create_context_no_error");
+    register_extension("WGL_ARB_create_context_profile");
+    opengl_funcs.ext.p_wglCreateContextAttribsARB = wayland_wglCreateContextAttribsARB;
+
     return TRUE;
 }
 
@@ -751,6 +837,8 @@ static void init_opengl(void)
         if (!has_extension(egl_exts, #ext)) \
             { ERR("Failed to find required extension %s\n", #ext); goto err; } \
     } while(0)
+    REQUIRE_EXT(EGL_KHR_create_context);
+    REQUIRE_EXT(EGL_KHR_create_context_no_error);
     REQUIRE_EXT(EGL_KHR_no_config_context);
 #undef REQUIRE_EXT
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/5264

[PATCH 2/3] winewayland.drv: Implement wglShareLists.

[Alexandros Frantzis]

From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com>

EGL works similarly to GLX, in that it only allows sharing on context
creation, so we adapt the approach used by winex11.
---
 dlls/winewayland.drv/opengl.c | 47 +++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/dlls/winewayland.drv/opengl.c b/dlls/winewayland.drv/opengl.c
index 5f52ae25b67..ac9c0ca1dad 100644
--- a/dlls/winewayland.drv/opengl.c
+++ b/dlls/winewayland.drv/opengl.c
@@ -94,6 +94,8 @@ struct wgl_context
     EGLContext context;
     struct wayland_gl_drawable *draw, *read, *new_draw, *new_read;
     EGLint attribs[16];
+    BOOL has_been_current;
+    BOOL sharing;
 };
 
 /* lookup the existing drawable for a window, gl_object_mutex must be held */
@@ -306,6 +308,7 @@ static BOOL wgl_context_make_current(struct wgl_context *ctx, HWND draw_hwnd,
         ctx->draw = draw;
         ctx->read = read;
         ctx->new_draw = ctx->new_read = NULL;
+        ctx->has_been_current = TRUE;
         NtCurrentTeb()->glContext = ctx;
     }
     else
@@ -635,6 +638,49 @@ static BOOL wayland_wglSetPixelFormatWINE(HDC hdc, int format)
     return set_pixel_format(hdc, format, TRUE);
 }
 
+static BOOL wayland_wglShareLists(struct wgl_context *org, struct wgl_context *dest)
+{
+    struct wgl_context *keep, *clobber;
+
+    TRACE("(%p, %p)\n", org, dest);
+
+    /* Sharing of display lists works differently in EGL and WGL. In case of EGL
+     * it is done at context creation time but in case of WGL it is done using
+     * wglShareLists. We create an EGL context in wglCreateContext /
+     * wglCreateContextAttribsARB and when a program requests sharing we
+     * recreate the destination or source context if it hasn't been made current
+     * and it hasn't shared display lists before. */
+
+    if (!dest->has_been_current && !dest->sharing)
+    {
+        keep = org;
+        clobber = dest;
+    }
+    else if (!org->has_been_current && !org->sharing)
+    {
+        keep = dest;
+        clobber = org;
+    }
+    else
+    {
+        ERR("Could not share display lists because both of the contexts have "
+            "already been current or shared\n");
+        return FALSE;
+    }
+
+    p_eglDestroyContext(egl_display, clobber->context);
+    clobber->context = p_eglCreateContext(egl_display, EGL_NO_CONFIG_KHR,
+                                          keep->context, clobber->attribs);
+    TRACE("re-created context (%p) for Wine context %p (%p) "
+          "sharing lists with ctx %p (%p)\n",
+          clobber->context, clobber, clobber->config,
+          keep->context, keep->config);
+
+    org->sharing = TRUE;
+    dest->sharing = TRUE;
+    return TRUE;
+}
+
 static BOOL wayland_wglSwapBuffers(HDC hdc)
 {
     struct wgl_context *ctx = NtCurrentTeb()->glContext;
@@ -870,6 +916,7 @@ static struct opengl_funcs opengl_funcs =
         .p_wglGetProcAddress = wayland_wglGetProcAddress,
         .p_wglMakeCurrent = wayland_wglMakeCurrent,
         .p_wglSetPixelFormat = wayland_wglSetPixelFormat,
+        .p_wglShareLists = wayland_wglShareLists,
         .p_wglSwapBuffers = wayland_wglSwapBuffers,
     }
 };
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/5264

[PATCH 3/3] winewayland.drv: Implement wgl(Get)SwapIntervalEXT.

[Alexandros Frantzis]

From: Alexandros Frantzis <alexandros.frantzis(a)collabora.com>

---
 dlls/winewayland.drv/opengl.c | 69 ++++++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/dlls/winewayland.drv/opengl.c b/dlls/winewayland.drv/opengl.c
index ac9c0ca1dad..d8f64aea8f7 100644
--- a/dlls/winewayland.drv/opengl.c
+++ b/dlls/winewayland.drv/opengl.c
@@ -69,6 +69,7 @@ DECL_FUNCPTR(eglInitialize);
 DECL_FUNCPTR(eglMakeCurrent);
 DECL_FUNCPTR(eglQueryString);
 DECL_FUNCPTR(eglSwapBuffers);
+DECL_FUNCPTR(eglSwapInterval);
 DECL_FUNCPTR(glClear);
 #undef DECL_FUNCPTR
 
@@ -85,6 +86,8 @@ struct wayland_gl_drawable
     struct wl_egl_window *wl_egl_window;
     EGLSurface surface;
     LONG resized;
+    int swap_interval;
+    BOOL refresh_swap_interval;
 };
 
 struct wgl_context
@@ -157,6 +160,7 @@ static struct wayland_gl_drawable *wayland_gl_drawable_create(HWND hwnd, int for
 
     gl->ref = 1;
     gl->hwnd = hwnd;
+    gl->swap_interval = 1;
 
     /* Get the client surface for the HWND. If don't have a wayland surface
      * (e.g., HWND_MESSAGE windows) just create a dummy surface to act as the
@@ -227,7 +231,15 @@ static void wayland_update_gl_drawable(HWND hwnd, struct wayland_gl_drawable *ne
 
     if ((old = find_drawable_for_hwnd(hwnd))) list_remove(&old->entry);
     if (new) list_add_head(&gl_drawables, &new->entry);
-    if (old && new) update_context_drawables(new, old);
+    if (old && new)
+    {
+        update_context_drawables(new, old);
+        if (new->swap_interval != old->swap_interval)
+        {
+           new->swap_interval = old->swap_interval;
+           new->refresh_swap_interval = TRUE;
+        }
+    }
 
     pthread_mutex_unlock(&gl_object_mutex);
 
@@ -409,6 +421,11 @@ static void wgl_context_refresh(struct wgl_context *ctx)
         refresh = TRUE;
     }
     if (refresh) p_eglMakeCurrent(egl_display, ctx->draw, ctx->read, ctx->context);
+    if (ctx->draw && ctx->draw->refresh_swap_interval)
+    {
+        p_eglSwapInterval(egl_display, ctx->draw->swap_interval);
+        ctx->draw->refresh_swap_interval = FALSE;
+    }
 
     pthread_mutex_unlock(&gl_object_mutex);
 
@@ -602,6 +619,21 @@ static PROC wayland_wglGetProcAddress(LPCSTR name)
     return (PROC)p_eglGetProcAddress(name);
 }
 
+static int wayland_wglGetSwapIntervalEXT(void)
+{
+    struct wgl_context *ctx = NtCurrentTeb()->glContext;
+
+    if (!ctx || !ctx->draw)
+    {
+        WARN("No GL drawable found, returning swap interval 0\n");
+        return 0;
+    }
+
+    /* It's safe to read the value without a lock, since only
+     * the current thread can write to it. */
+    return ctx->draw->swap_interval;
+}
+
 static BOOL wayland_wglMakeContextCurrentARB(HDC draw_hdc, HDC read_hdc,
                                              struct wgl_context *ctx)
 {
@@ -699,6 +731,36 @@ static BOOL wayland_wglSwapBuffers(HDC hdc)
     return TRUE;
 }
 
+static BOOL wayland_wglSwapIntervalEXT(int interval)
+{
+    struct wgl_context *ctx = NtCurrentTeb()->glContext;
+    BOOL ret;
+
+    TRACE("(%d)\n", interval);
+
+    if (interval < 0)
+    {
+        RtlSetLastWin32Error(ERROR_INVALID_DATA);
+        return FALSE;
+    }
+
+    if (!ctx || !ctx->draw)
+    {
+        RtlSetLastWin32Error(ERROR_DC_NOT_FOUND);
+        return FALSE;
+    }
+
+    pthread_mutex_lock(&gl_object_mutex);
+    if ((ret = p_eglSwapInterval(egl_display, interval)))
+        ctx->draw->swap_interval = interval;
+    else
+        RtlSetLastWin32Error(ERROR_DC_NOT_FOUND);
+    ctx->draw->refresh_swap_interval = FALSE;
+    pthread_mutex_unlock(&gl_object_mutex);
+
+    return ret;
+}
+
 static BOOL has_extension(const char *list, const char *ext)
 {
     size_t len = strlen(ext);
@@ -755,6 +817,10 @@ static BOOL init_opengl_funcs(void)
     register_extension("WGL_ARB_create_context_profile");
     opengl_funcs.ext.p_wglCreateContextAttribsARB = wayland_wglCreateContextAttribsARB;
 
+    register_extension("WGL_EXT_swap_control");
+    opengl_funcs.ext.p_wglGetSwapIntervalEXT = wayland_wglGetSwapIntervalEXT;
+    opengl_funcs.ext.p_wglSwapIntervalEXT = wayland_wglSwapIntervalEXT;
+
     return TRUE;
 }
 
@@ -860,6 +926,7 @@ static void init_opengl(void)
     LOAD_FUNCPTR_EGL(eglInitialize);
     LOAD_FUNCPTR_EGL(eglMakeCurrent);
     LOAD_FUNCPTR_EGL(eglSwapBuffers);
+    LOAD_FUNCPTR_EGL(eglSwapInterval);
 #undef LOAD_FUNCPTR_EGL
 
     egl_display = p_eglGetPlatformDisplay(EGL_PLATFORM_WAYLAND_KHR,
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/5264

[Rémi Bernon]

> In the next OpenGL part I would like to focus on `WGL_ARB_pixel_format`. I was thinking that if a driver implements `wglGetPixelFormatAttribivARB` then it would be possible, in theory at least, to have a default implementation of `wglChoosePixelFormatARB` (and `wglGetPixelFormatAttribfvARB`) in `opengl32`. I am tempted to do this, instead of adding one more driver specific full implementation, although I am not yet sure if there any driver specific considerations or behaviors that would pose issues with such approach. Thoughts?

In term of potential issues, I see that winex11 wglChoosePixelFormatARB relies on glXChooseFBConfig while winemac implements it entirely itself. I don't know which one is better, we care about how this function behaves on Windows and in this case it might also be driver specific. Probably re-implemting the logic ourselves is okay, as long as we mimic whatever happens on Windows. It would be nice to confirm that the behavior is not completely different across drivers (or even if it is, maybe that means that it doesn't matter), ideally with tests.

Then, in theory, anything that factors code between drivers is good to have, the question might then be how to do it. There's several places where this can be done, opengl32 is one but there still, it could be on the PE or the unix side, or it could also be factored in win32u.

For this particular function, calling into the drivers wglGetPixelFormatAttribivARB for every attribute of every pixel format doesn't look very appealing if this has to cross the PE-unix boundaries each time. I can also imagine that the entire list of pixel formats and their attributes could be passed right away during GL initialization - assuming it doesn't change - and cached somewhere, in which case we could factor even wglGetPixelFormatAttribivARB.

Note that currently it's also not very convenient to do this in opengl32, because we may have two different GL drivers loaded at the same time. One from OSMesa, for memory DC, and another from the display drivers for display DCs. I think it would be nice if we could get rid of OSMesa and use the same GL driver for both, not completely sure how feasible this is but in the meantime it means that factoring things in opengl32 might still be a bit awkward.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64023

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/opengl.c:
> goto out;
>      }
>
> +    if (!wgl_context_populate_attribs(ctx, attribs))
> +    {
> +        ctx->attribs[0] = EGL_NONE;
> +        goto out;
> +    }
> +
> +    /* For now only OpenGL is supported. */
> +    p_eglBindAPI(EGL_OPENGL_API);

Ah yeah I forgot about that before but isn't something that's supposed to be called on every thread that is going to issue GL calls? Shouldn't we do that in wglMakeCurrent?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64028

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/opengl.c:
> return ret;
>  }
>
> +static BOOL wgl_context_populate_attribs(struct wgl_context *ctx, const int *attribs)
> +{
> +    EGLint *egl_attribs = ctx->attribs;

I would prefer something like `attribs_end` which would more clearly indicate that this points to the end of the list.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64029

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/opengl.c:
> return set_pixel_format(hdc, format, TRUE);
>  }
>
> +static BOOL wayland_wglShareLists(struct wgl_context *org, struct wgl_context *dest)

```suggestion:-0+0
static BOOL wayland_wglShareLists(struct wgl_context *orig, struct wgl_context *dest)
```

Changing org to orig feels at least more readable, but I don't think the names are even nearly correct. Maybe it should instead follow the client API naming share / source. (And yes, I know winex11 is like that already)

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64030

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/opengl.c:
> if ((old = find_drawable_for_hwnd(hwnd))) list_remove(&old->entry);
>      if (new) list_add_head(&gl_drawables, &new->entry);
> -    if (old && new) update_context_drawables(new, old);
> +    if (old && new)
> +    {
> +        update_context_drawables(new, old);
> +        if (new->swap_interval != old->swap_interval)
> +        {
> +           new->swap_interval = old->swap_interval;

What about moving that to update_context_drawables?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64031

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/opengl.c:
> refresh = TRUE;
>      }
>      if (refresh) p_eglMakeCurrent(egl_display, ctx->draw, ctx->read, ctx->context);
> +    if (ctx->draw && ctx->draw->refresh_swap_interval)
> +    {
> +        p_eglSwapInterval(egl_display, ctx->draw->swap_interval);
> +        ctx->draw->refresh_swap_interval = FALSE;
> +    }

Any reason for having that flag? Could we just call `p_eglSwapInterval(egl_display, ctx->draw->swap_interval);` withing the `if (refresh)`?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64032

[Rémi Bernon]

Rémi Bernon (@rbernon) commented about dlls/winewayland.drv/opengl.c:
> +        return FALSE;
> +    }
> +
> +    if (!ctx || !ctx->draw)
> +    {
> +        RtlSetLastWin32Error(ERROR_DC_NOT_FOUND);
> +        return FALSE;
> +    }
> +
> +    pthread_mutex_lock(&gl_object_mutex);
> +    if ((ret = p_eglSwapInterval(egl_display, interval)))
> +        ctx->draw->swap_interval = interval;
> +    else
> +        RtlSetLastWin32Error(ERROR_DC_NOT_FOUND);
> +    ctx->draw->refresh_swap_interval = FALSE;
> +    pthread_mutex_unlock(&gl_object_mutex);
> Not sure to see why you need the lock here then if it's indeed safe to read it without a lock.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/5264#note_64033