GNOME Bugzilla – Bug 789213
Two Windows with GLAreas go horribly wrong
Last modified: 2018-05-02 19:20:29 UTC
Created attachment 361892 [details] screenshot of how it looks like Opening more than one window with a GLArea leads to /really/ bad rendering artifacts (see attachment). Things that appear in the main window also appear in the newly opened window. Multiple GLAreas within one window work fine.
Created attachment 361893 [details] test case Test script for triggering the bug
OS is Windows 10, GPU is an nvidia GTX660 Ti, Driver Version 21.21.13.7653 This bug is reproducible on a ~10 year old laptop with an nvidia GPU as well
Created attachment 361899 [details] A testcase reproducing the problem (in C) Here's roughly the same testcase, but in C. I see the bug you're describing (AMD RX 480 here), so let's hope that fanc is around to fix it (fanc does everything related to GL in W32 GDK backend).
Seems like the gdk backend doesn't like having two GL paint contexts around. When staring any gtk application with GDK_GL=always, just opening two windows immediately crashes it. I haven't been able to find out where exactly the crash occurs since gdb thinks the stack is corrupted, windbg thinks that the segfault occurs in gdk_frame_timings_get_refresh_interval and x64dbg just breaks all over the place...
I did some further debugging with GDK_GL=always and found out that the segfault originates from wglGetProcAddress("glActiveTexture") returning 0 after opening another window. GetLastError() returns ERROR_INVALID_HANDLE. So it looks like something isn't quite right with the opengl contexts.
Any comments on this, fanc?
Looks like I there are two issues present: https://git.gnome.org/browse/gtk+/tree/gdk/win32/gdkglcontext-win32.c#n793 To me, display_win32->gl_hdc and context_win32->gl_hdc should be swapped. Gdk seems to lose track of the current context: https://git.gnome.org/browse/gtk+/tree/gdk/gdkglcontext.c#n856 With disabling " if (current == context)" and swapping display_win32->gl_hdc and context_win32->gl_hdc the issue seems to be gone.
Created attachment 362460 [details] [review] proposed patch (somewhat) unrelated, but caught my eye
Created attachment 362462 [details] [review] proposed patch Looked like the culprit was _gdk_win32_gl_context_realize changing the current context and _gdk_win32_display_make_gl_context_current mixing up display and context hdc. Why is there a per-display hdc in the first place?
Review of attachment 362460 [details] [review]: Hi Lukas, Doh..., thanks for catching this. With blessings, thank you!
Review of attachment 362460 [details] [review]: Hi Lukas, I pushed the patch as: gtk-3-22: 5b8a3ba master: 15491cf With blessings, thank you!
Created attachment 362515 [details] [review] gdk/win32: Fix Win32 GL Context switching (for 3.22.x) Hi Lukas, The display_win32->gl_hdc was initially added to aid with bookkeeping, but as you have pointed out, it actually caused confusion when switching GL contexts, so I think we can just get rid of it. Thanks for letting me know. It turns out that we need to restore the current HDC and GL contexts because of the numerous dummy GL contexts that we must create, along with dummy HDCs, because there are operations that can be only called one time per HDC, so I think your approach here is right, so that the current GL context and HDC do not end up in limbo. However, I don't think it is right for us to do context switching in _gdk_win32_gl_context_realize(), since in the other GDK backends it is not done in their respective gdk_gl_context_realize() methods, and in GDK we could well be calling gdk_display_make_gl_context_current(), which will in turn wglMakeCurrent on the HDC and GL contexts passed to it via the GDKGLContext that was just created/realized in gdk_gl_context_realize(). So, I did the wglMakeCurrent() calls in the respective functions that _gdk_win32_gl_context_realize() calls that make use of dummy GL contexts and HDCs, and so we don't have to call wglMakeCurrent() that often. My take at this, and my version of the patch. With blessings, thank you!
Hi, thanks for merging my first patch! > However, I don't think it is right for us to do context switching in > _gdk_win32_gl_context_realize(), since in the other GDK backends it is not > done in their respective gdk_gl_context_realize() methods, and in GDK we > could well be calling gdk_display_make_gl_context_current(), which will in > turn wglMakeCurrent on the HDC and GL contexts passed to it via the > GDKGLContext that was just created/realized in gdk_gl_context_realize(). My idea was to make sure that _gdk_win32_gl_context_realize doesn't change the current context, just like with the other backends. Simply calling gdk_display_make_gl_context_current may not work since this one keeps track of the current Gdk GL context and does nothing if it thinks the Gdk's GL context hasn't changed. > So, I did the wglMakeCurrent() calls in the respective functions that > _gdk_win32_gl_context_realize() calls that make use of dummy GL contexts and > HDCs, and so we don't have to call wglMakeCurrent() that often. https://bugzilla.gnome.org/attachment.cgi?id=362515&action=diff#a/gdk/win32/gdkglcontext-win32.c_sec4 Shouldn't we call wglMakeCurrent after _destroy_dummy_gl_context since the latter may change the current context as well? Kind Regards, Lukas
Hi Lukas, (In reply to Lukas from comment #13) > > thanks for merging my first patch! :) > My idea was to make sure that _gdk_win32_gl_context_realize doesn't change > the current context, just like with the other backends. Simply calling > gdk_display_make_gl_context_current may not work since this one keeps track > of the current Gdk GL context and does nothing if it thinks the Gdk's GL > context hasn't changed. I understand you, but the problem is that on Windows we need to create dummy/legacy GL contexts in the process of creating the actual (system/"WGL") GL contexts that we need, because on Windows a legacy GL context is required to create GL 3.3+ contexts, or when using attributes to create GL contexts, plus a few more things. Because of that, we need to make the legacy or dummy GL context that we use to create the "advanced" contexts (or other operations) current before we can use it, which is why the current contexts shift. So, matching up these wglMakeCurrent() calls is the goal here, so we want to get these right in the proper places. There are points where this is not done, as I don't think it is necessary. > > Shouldn't we call wglMakeCurrent after _destroy_dummy_gl_context since the > latter may change the current context as well? > That was removed in my patch. With blessings, thank you!
Hi Lukas, Does my patch fix the issue for you? A few more clarifications to make things a bit clearer... note the following is wordy... (In reply to Fan, Chun-wei from comment #14) > I understand you, but the problem is that on Windows we need to create > dummy/legacy GL contexts in the process of creating the actual > (system/"WGL") GL contexts that we need... The previous 3.3+ should read 3.2+... Note that this is the process that is carried out in gdk_gl_context_realize() (i.e. which in turn calls _gdk_win32_gl_context_realize()). Also note that when "use" or "involve" a GL context is mentioned, that means wglMakeCurrent() is called to make it current. In GDK the actual GL context that we use are carried out in two steps, one in _gdk_win32_window_create_gl_context() (which calls _gdk_win32_display_init_gl() to query features/functions provided by the driver-supplied opengl32.dll--which means we create and use a dummy GL context, as we want to see whether that opengl32.dll supports WGL_ARB_create_context, which is used to create the profile GL (3.2+) contexts using attributes, and WGL_ARB_pixel_format, which helps us to better determine the best pixel format number). No actual GL contexts that we are using other than querying features are created at this point (hence wglMakeCurrent (NULL, NULL) is called after the dummy GL contexts are used). When we get to gdk_gl_context_realize(), this is when we actually create the actual GL context we want to use. We need to first acquire a pixel format (number) (via _get_wgl_pfd()), which makes use of a dummy GL context if WGL_ARB_pixel_format is supported, because it is needed for wglChoosePixelFormatARB(). So, after we get the pixel format, we bind it to the HDC we have using SetPixelFormat(), which can be done only once per HDC. In the patch, we switch back to the GL context and HDC at the end of _get_wgl_pfd(), that we saved up in the beginning, if WGL_ARB_pixel_format is found. We then create a legacy context using wglCreateContext() in _create_gl_context(), and if WGL_ARB_create_context() is found, we need to make use of the legacy context that we just created, because it is required for wglCreateContextAttribsARB(), for our profile GL context (or, for fallback legacy contexts that we still create using wglCreateContextAttribsARB()). In the patch, we save up the current GL context and HDC in the beginning of _create_gl_context() and restore them if the actual GL context is successfully created at the end. > > > > Shouldn't we call wglMakeCurrent after _destroy_dummy_gl_context since the > > latter may change the current context as well? > > > > That was removed in my patch. > This means that we don't do wglMakeCurrent() in _destroy_dummy_gl_context(), but do it as needed. With blessings, thank you!
Hi, > Does my patch fix the issue for you? I don't have access to my windows test machine right now, I'll check on Wednesday or Thursday. Thanks for the detailed explanation of what's going on! Kind Regards, Lukas
In the meantime, how about adding something like this to the gdkgears test?
Hi Lukas, (In reply to Lukas from comment #17) > In the meantime, how about adding something like this to the gdkgears test? I don't think I am following you here, can you say how? The GL context creation in the gdkgears test use the GtkGLArea/GdkGLArea APIs to acquire the GL contexts needed, so that things will work on all supported platforms (given that they have the OS-specific GL bits implemented). Everything I mentioned about the wglMakeCurrent() calls in comments 12, 14 and 15 goes on internally in the Win32 backend of GDK, as this is Windows-specific, i.e. gdk/win32/gdkglcontext-win32.c, so the fix should be done there (internally) as well, not in items such as the gdkgears test. At least for me, the issues with multiple windows with GLAreas and for using GDK_GL=always are fixed with my patch, at least on my system (Windows 10 64-bit, nVidia GTX 960m with the latest nVidia stable/release drivers, 388.13). With blessings, and cheers!
Yeah, this bug is fixed, but who knows if similar issues are lingering in other backends. By having an easy way of testing wether the backend can handle two windows with OpenGL in them, we might be able to avoid this class of bugs in the first place.
Created attachment 362845 [details] [review] gdk/win32: Fix Win32 GL Context switching (for the master branch) Hi LRN/Nacho, Can you take a look at this patch to see whether this patch (for master) and attachment 362515 [details] [review] is ok to go in (a separate patch is needed for master as the original-for-3.22.x patch needs to be rebased). Thanks a bunch! --- Hi Lukas, I don't think we can do what you mentioned in the gdkgears test/sample program, because applications aren't supposed to have access to the underlying, lower-level/system-level/per-backend implementation (details) for GL support (which my patches are meant to fix on the Windows backend, for this case), and I am not even certain how OpenGL is supposed to work on these other backends, so this is certainly out of my reach. Hope this clears things up. --- With blessings, and cheers!
Maybe I didn't get that across. My ideas was to simply make the gdkgears demo open two windows with a glarea in it since this would have triggered this bug.
> > Does my patch fix the issue for you? > I don't have access to my windows test machine right now, I'll check on > Wednesday or Thursday. I've tested your patch it works just fine as well!
(In reply to Fan, Chun-wei from comment #20) > Created attachment 362845 [details] [review] [review] > gdk/win32: Fix Win32 GL Context switching (for the master branch) > > Hi LRN/Nacho, > > Can you take a look at this patch to see whether this patch (for master) and > attachment 362515 [details] [review] [review] is ok to go in (a separate patch is > needed for master as the original-for-3.22.x patch needs to be rebased). > Thanks a bunch! > I only looked at attachment 362515 [details] [review] (assuming that attachment 362845 [details] [review] is the same, but for master). The patch seems OK, although i'm not sure why you still use wglMakeCurrent (NULL, NULL); in the !success branch in _create_gl_context(). Also, the display_win32 -> context_win32 change (the one that Lukas originally found) is not reflected in the commit message.
(In reply to Lukas from comment #21) > Maybe I didn't get that across. My ideas was to simply make the gdkgears > demo open two windows with a glarea in it since this would have triggered > this bug. Sounds good to me to have such a test
Created attachment 362875 [details] [review] gdk/win32: Fix Win32 GL context switching (for 3.22.x) [take ii] Hi LRN, This is the same patch for 3.22.x with updated commit message, hopefully it is clearer in there now. The reason why in _create_gl_context() in the !success branch I did not touch the "wglMakeCurrent (NULL, NULL);" part is because the legacy GL (WGL) context (hglrc_base) could not be set up (or switched to) properly when we want to use it for our purposes directly, or when we need to make use of it to create our more "advanced" contexts. This is why the legacy context is being deleted there as well. This means our actual WGL context creation failed, and so we don't want to make it usable, as a result, for our purposes, hence NULL is returned and the GdkGLContext will not have a valid WGL context in it for GdkGLArea/GtkGLArea to use. The patch for master does the same as the 3.22.x one, but is adapted to the state of the code in the master branch. With blessings, thank you!
Created attachment 362876 [details] [review] Fix Win32 GL Context switching (for the master branch) [take ii] Hi, These are the same fixes for the master branch. With blessings, thank you!
feel free to push these
Hi Matthias, Thanks for the ack! I have pushed the patches as follows: attachment 362875 [details] [review] (to gtk-3-22): d38a148 attachment 362876 [details] [review] (to master): 3228149 Let's keep this bug open for the additional test in gdkgears. With blessings, thank you!
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gtk/issues/954.