GNOME Bugzilla – Bug 773528
Improve GL support on Windows
Last modified: 2016-10-28 09:34:10 UTC
Hi, As we enhanced GL support for GTK+ to include support for legacy GL contexts, this patch series is done to improve the GL support on GDK/Win32 so that we can be closer in terms of features of the GL support offered by X11/Wayland. I will attach a patch for this purpose shortly. With blessings, thank you!
Created attachment 338499 [details] [review] GDK/Win32: Improve GL support Hi, This is my attempt to improve the GL support on Windows, by: -Enabling multi-sampling support, used for anti-aliasing. This means we get the needed pixel format using wglChoosePixelFormatARB(), and request a format that supports WGL_SAMPLE_BUFFERS_ARB when available. A temporary GL context will be required for this. -Enable legacy GL context support, like what we have for X11. Note that it is likely that we won't get GLES support on Windows as the platform does not support this directly, but only via other means such as ANGLE, which AFAIK is C++. Please also note that this patchset is done upon the patch on 768081, so if this patch can go in earlier and rebasing is preferred, please let me know so that I can get this done. This patch is for 3.x and 4.x, ATM. With blessings, thank you!
Review of attachment 338499 [details] [review]: This looks generally okay, to me; sadly, I can't test it. The questions I can ask are: * does it work? * does it default to a core GL profile? * does it fall back to a legacy GL profile if needed/on failure? if all three questions have a "yes" answer, then: ship it! :-)
Some thoughts on general code clarity: * The pixelAttribs/ pixelAttribsNoAlpha saga is needlessly verbose. First, we should probably allocate it on the stack, not on the heap (g_malloc()), as its maximum needed size (6 + 2 = 8 pairs, which means 16 elements at most) is known in advance. Second, we don't need 2 arrays (alpha and non-alpha), we just need one, with alpha, and we need to remember the index of WGL_ALPHA_BITS_ARB, so that in case of failure we can zero it out and try again, without alpha. This is something to improve once we know that this code is, in fact, working as intended. * The comment > /* give another chance if need_alpha_bits is FALSE, > * meaning we prefer to have an alpha channel anyways > */ seems misleading (we give it another chance is need_alpha_bits is TRUE, not FALSE). That, or you've missed an '!' to invert the condition. * There are probably style nitpicking opportunities for nacho here. * The _create_gl_context() argument "is_legacy" is a pointer to boolean, you should check dereference it when checking the boolean That's all i can tell by just reading the code right now, the rest is GL stuff that i don't understand.
Review of attachment 338499 [details] [review]: See my nitpicks ::: gdk/win32/gdkglcontext-win32.c @@ +279,3 @@ static gint +_gdk_init_dummy_context (GdkWGLDummy *dummy, + const gboolean need_alpha_bits); align params also not sure why the boolean is const @@ +284,1 @@ _get_wgl_pfd (HDC hdc, let's align all these params @@ +533,3 @@ +/* Setup the legacy context after creating it */ +static gboolean +_ensure_legacy_gl_context (HDC hdc, align params @@ +554,2 @@ static HGLRC _create_gl_context (HDC hdc, align params @@ +569,3 @@ + /* if we have no wglCreateContextAttribsARB(), return the legacy context when all is set */ + if (is_legacy && !hasWglARBCreateContext) + return (_ensure_legacy_gl_context (hdc, hglrc_base, share) ? hglrc_base : NULL); no need to add the () @@ +580,3 @@ + WGL_CONTEXT_CORE_PROFILE_BIT_ARB; + + gint attribs[] = { I don't think wglCreateContextAttribsARB takes a gint, it probably takes an int? @@ +619,3 @@ static gboolean _set_pixformat_for_hdc (HDC hdc, gint *best_idx, align params @@ +681,3 @@ + + /* A legacy context cannot be shared with core profile ones, so this means we + must stick to a legacy context if the shared context is a legacy context */ multi line comments should be as: /* * */ @@ +694,1 @@ glver_major, glver_minor, wrong align here
Created attachment 338594 [details] [review] GDK/Win32: Improve GL support (take ii) Hi LRN, (some comments shortened, I hope you don't mind :) ) (In reply to LRN from comment #3) > * The pixelAttribs/ pixelAttribsNoAlpha saga is needlessly verbose... This is something to improve once we know that this > code is, in fact, working as intended. The code should look better now in terms of this. > > * The comment > > /* give another chance if need_alpha_bits is FALSE, > > * meaning we prefer to have an alpha channel anyways > > */ > seems misleading (we give it another chance is need_alpha_bits is TRUE, not > FALSE). That, or you've missed an '!' to invert the condition. Ooops, fixed it. > > * The _create_gl_context() argument "is_legacy" is a pointer to boolean, you > should check dereference it when checking the boolean Double oops, fixed. > That's all i can tell by just reading the code right now, the rest is GL > stuff that i don't understand. Thanks very much! --- Hi Nacho, (In reply to Ignacio Casal Quinteiro (nacho) from comment #4) > ::: gdk/win32/gdkglcontext-win32.c > @@ +279,3 @@ > static gint > +_gdk_init_dummy_context (GdkWGLDummy *dummy, > + const gboolean need_alpha_bits); > > align params also not sure why the boolean is const Fixed. I thought that boolean would not change in its course so I had it const. > > @@ +284,1 @@ > _get_wgl_pfd (HDC hdc, > > let's align all these params Aligned. > > @@ +533,3 @@ > +/* Setup the legacy context after creating it */ > +static gboolean > +_ensure_legacy_gl_context (HDC hdc, > > align params Aligned. > > @@ +554,2 @@ > static HGLRC > _create_gl_context (HDC hdc, > > align params Aligned. > > @@ +569,3 @@ > + /* if we have no wglCreateContextAttribsARB(), return the legacy context > when all is set */ > + if (is_legacy && !hasWglARBCreateContext) > + return (_ensure_legacy_gl_context (hdc, hglrc_base, share) ? hglrc_base > : NULL); I updated the logic a bit here, so shouldn't have this line anymore. Wanted to be more tolerant against WGL_ARB_Create_Context (or so) failures. > > @@ +580,3 @@ > + WGL_CONTEXT_CORE_PROFILE_BIT_ARB; > + > + gint attribs[] = { > > I don't think wglCreateContextAttribsARB takes a gint, it probably takes an > int? Changed it anyways, though on Windows it is gint == int. > > @@ +619,3 @@ > static gboolean > _set_pixformat_for_hdc (HDC hdc, > gint *best_idx, > > align params Aligned. > > @@ +681,3 @@ > + > + /* A legacy context cannot be shared with core profile ones, so this > means we > + must stick to a legacy context if the shared context is a legacy > context */ > > multi line comments should be as: > /* > * > */ Got that. > > @@ +694,1 @@ > glver_major, glver_minor, > > wrong align here Fixed that as well. --- Thanks for the reviews on the stuff. It turns out that these updates are needed for GTKGL stuff to run on Windows for 4.x, and this should work for the 3.x codebase as well. With blessings, thank you, and cheers!
Created attachment 338669 [details] [review] GDK/Win32/4.0: Improve GL support (In reply to Fan, Chun-wei from comment #5) > ... and this should work for the 3.x codebase as well. It seems that I need to take back this comment for 4.x, so this is the patch for 4.x as we don't have visuals that we need to deal with here anymore. So there is now separate patches for 3.x and 4.x With blessings, thank you!
Review of attachment 338669 [details] [review]: I think you have the OK from all of us :)
Review of attachment 338669 [details] [review]: Hi Nacho, Thanks, I pushed the patch as b960008 in master. :) Should I try to push the one for GTK+-3.22.x? With blessings, thank you!
if you feel confident about it why not? :)
Hi Nacho, Thanks :) pushed it as b67a1c7f3 in gtk-3-22. With blessings, thank you!