Opened 5 months ago

Last modified 4 months ago

#16402 reopened enhancement

OpenGL context profiles

Reported by: captainhorst Owned by: VZ
Priority: normal Milestone:
Component: OpenGL Version: dev-latest
Keywords: OpenGL context profile core compatibility Cc: raysatiro@…
Blocked By: Blocking:
Patch: yes

Description

Hi,
I created a patch to allow for explicitly creating a compatibility or a core (new) OpenGL context. It is inspired by the way SDL2 handles this. I checked out trunk using svn and created this patch against revision r77003. I tested this patch with my version of the PS3 emulator rpcs3 (https://github.com/DHrpcs3/rpcs3) and it works.

Why is this necessary?
When using the compatibility profile, OS X only provides an OpenGL 2.1 feature set. When using core though, features up to OpenGL version 4.1 are available.

This patch currently only contains the OS X/Cocoa implementation. Windows and Linux variants aren't yet implemented. For Windows it could be done as this article suggests: http://www.lighthouse3d.com/2012/07/working-with-core-opengl-in-wx-wigets/#more-3568

Kind regards,
Fabio

Attachments (6)

opengl_context_profiles.patch download (3.4 KB) - added by captainhorst 5 months ago.
Patch against svn trunk revision r77003
opengl_context_profiles_win_linux.patch download (17.3 KB) - added by captainhorst 5 months ago.
OpenGL core profile for Windows and Linux (untested)
opengl_context_profiles3.patch download (19.4 KB) - added by captainhorst 5 months ago.
included fix for windows and simple test on the OpenGL cube sample
opengl_core_profile_winmaclin.patch download (16.0 KB) - added by captainhorst 4 months ago.
Final working patch. Tested under Linux (nvidia, nouveau), OS X (nvidia) and Windows (nvidia 331.65)
opengl_core_linux_sample.patch download (25.7 KB) - added by captainhorst 4 months ago.
OpenGL 3 core renderer for the cube sample. glxContextAttributes aren't static anymore
msw_glcanvas_skip_null_attriblist.patch download (1.4 KB) - added by raysatiro 4 months ago.
Avoid null deref of attribList parameter in MSW wxGLCanvas. Refer to comment #15.

Download all attachments as: .zip

Change History (25)

Changed 5 months ago by captainhorst

Patch against svn trunk revision r77003

comment:1 Changed 5 months ago by vadz

  • Status changed from new to confirmed

Thanks, this would certainly be useful but I'm not sure why do we need to add WX_GL_CONTEXT_{MAJ,MIN}OR_VERSION when they're not used/implemented anywhere? Instead of having them and WX_GL_CONTEXT_PROFILE_MASK I'd prefer to have a single WX_GL_CONTEXT_MIN_VERSION with values like WX_GL_VERSION_3_2 (which would be the only one defined for now presumably). What do you think?

A couple of other random comments:

  • We don't need to check for OS X 10.7 in the trunk, it's required there now.
  • Please add @since 3.1.0 to the documentation of the new constants.

And, of course, while if you could implement this for Windows and Linux too, it would be perfect, we could apply a Mac-only patch too, but in this case the documentation should mention this.

Changed 5 months ago by captainhorst

OpenGL core profile for Windows and Linux (untested)

comment:2 Changed 5 months ago by captainhorst

I tried to add Windows and Linux support now. I didn't test this yet since I currently have no access to a Windows nor a Linux machine. Will do tomorrow.
I also added the @since 3.1.0 tags to the new constants and removed the unnecessary comment about 10.7.

Now Windows and Linux are similar in that their {glX,wgl}CreateContextAttribsARB functions take an attribute array consisting of the following 8 integers (+1 zero termination):
GL_MAJOR_VERSION, x,
GL_MINOR_VERSION, y,
GL_CONTEXT_FLAGS, {DEBUG_BIT, FORWARD_COMPATIBILITY_BIT},
GL_CONTEXT_PROFILE_MASK, {CORE, COMPATIBILITY}

(I guess FORWARD_COMPATIBILITY_BIT should be set by default.)
This is quite a bit different from the OS X version. On OS X you specify the context profile in the pixel format attributes. This is why for now I only implemented the context profile option.
On the other platforms you pass these attributes directly to the context creation function. To achieve this I created a static attribute array which gets populated in the wxGLCanvas constructor (which gets passed the pixel format attributes containing this information for now). The wxGLContext constructor accesses this static array then which isn't ideal but shouldn't be a problem because of the calling convention mentioned in the documentation (http://docs.wxwidgets.org/3.0/classwx_g_l_canvas.html).

A possible solution would be to pass a separate context attributes array to the wxGLContext constructor. Which in turn wouldn't work on OS X since these attributes are passed with the pixel format during canvas creation...

SDL2 handles this using a private gl_config struct which is basically the same as the static array just organized nicely.

Again I only implemented the GL_CONTEXT_CORE_PROFILE part. So shall I continue to implement the other three options? I think a debug context could certainly be useful. As for merging major and minor version numbers to single constants: I think individual numbers fit more nicely to the Windows/Linux approach.

Last edited 5 months ago by captainhorst (previous) (diff)

comment:3 Changed 5 months ago by captainhorst

Just tested the Windows version.
On line 120 in src/msw/glcanvas.cpp
typedef HGLRC(APIENTRYP PFNWGLCREATECONTEXTATTRIBSARBPROC)

(HDC hDC, HGLRC hShareContext, const int *attribList);

needs to be replaced with:
typedef HGLRC(WINAPI * PFNWGLCREATECONTEXTATTRIBSARBPROC)

(HDC hDC, HGLRC hShareContext, const int *attribList);

It will compile and execute wglCreateContextAttribs but on my system with an NVIDIA GeForce GTX 560 Ti driver version 331.65 requesting the core profile has no effect (glGetError() doesn't report anything on deprecated gl calls). Maybe someone with an AMD card could test this?

Something else:
It seams the core profile is also required for the open source Linux graphics drivers nouveaux and radeon to use OpenGL 3.x and above. So this patch doesn't just affect OS X users.

I don't think I have time to test Linux yet.

comment:4 follow-up: Changed 5 months ago by vadz

Could you please add the code allowing to test this functionality to the cube sample? It would make it simpler for people to test it.

FWIW I still remain unconvinced by the introduction of all these new (public) constants when we basically use just one of them: boolean "request core profile" flag. Let's add WX_GL_CONTEXT_MAJOR_VERSION and friends later if needed. For now a single WX_GL_CORE_PROFILE could well be all we need.

Changed 5 months ago by captainhorst

included fix for windows and simple test on the OpenGL cube sample

comment:5 in reply to: ↑ 4 Changed 5 months ago by captainhorst

Replying to vadz:

Could you please add the code allowing to test this functionality to the cube sample? It would make it simpler for people to test it.

FWIW I still remain unconvinced by the introduction of all these new (public) constants when we basically use just one of them: boolean "request core profile" flag. Let's add WX_GL_CONTEXT_MAJOR_VERSION and friends later if needed. For now a single WX_GL_CORE_PROFILE could well be all we need.

Please see my latest patch. It also contains the previous two patches.

I removed WX_GL_CONTEXT_M{AJ,IN}OR_VERSION in the previous patch. As this isn't used yet. As for the debug context I would really like to see this functionality. But we can handle this as a separate issue, do this at a later time and just focus on the context profile.

I used a mask for the context profile because there are bits for core, compatibility, OpenGL ES1 and OpenGL ES2. These can be arbitrarily combined. ES1 and ES2 can be added at a later time.
But given the limited functionality of my current implementation I agree that a single a WX_GL_CONTEXT_CORE_PROFILE (or WX_GL_CONTEXT_PROFILE_CORE?) could be sufficient.

What's left is testing on AMD and Linux and changing the mask to a single boolean if this is requested.

comment:6 Changed 5 months ago by captainhorst

So I did some testing on Linux with nvidia and nouveau now.
Turns out one needs to specify the requested OpenGL version to retrieve a core profile at all. So I entered 3.0 by default since this is the minimum GL core version and set the forward compatibility bit. nouveau gave me a 3.3 core profile context as expected. But nvidia gave me a 3.0 core profile context (I didn't test whether features of higher versions work with this one due to forward compatibility) or basically any valid version I requested.
All this is not an issue with compatibility contexts since glx will return the highest version available.

In conclusion we need to expose control over the OpenGL version number for context creation since the exact version will be created. Being stuck with 3.0 is pretty stupid... rpcs3 for example needs version 3.3.

Now we could mimic the exact attributes of {glX,wgl}CreateContextAttribsARB functions only with GLX and WGL replaced by WX_GL or doing something like:

WX_GL_VERSION_MAJOR (followed by an integer)
WX_GL_VERSION_MINOR (followed by an integer)
WX_GL_DEBUG_CONTEXT (as boolean) (again we don't need to add this yet)
WX_GL_CORE_PROFILE (as boolean)

appended to the wxGL_FLAGS enum which I think might be nicer and easier to grasp. Forward compatibility would be enabled by default.

Changed 4 months ago by captainhorst

Final working patch. Tested under Linux (nvidia, nouveau), OS X (nvidia) and Windows (nvidia 331.65)

comment:7 Changed 4 months ago by captainhorst

I created a final patch. I tested this on all platforms available to me (sadly no AMD). I forgot to mention in the patch description I also tested this on OS X with integrated Intel Iris Pro.

The patch introduces three new wxGL_FLAGS: WX_GL_CORE_PROFILE, WX_GL_MAJOR_VERSION and WX_GL_MINOR_VERSION all marked with @since 3.1

The changes to the cube sample shouldn't be committed since they produce OpenGL errors (as expected with deprecated gl calls and a core profile).
For testing the sample also reports the current OpenGL version now. It should report a core profile.

If someone could test this on AMD/Intel, that would be great. :)

comment:8 Changed 4 months ago by vadz

I do get

Using OpenGL version: 3.2.12217 Core Profile Forward-Compatible Context 12.104.0.0

under Windows with ATI Radeon HD 5670, so I guess it works, doesn't it?

BTW, if anybody feels like providing a sample using modern OpenGL API, it would certainly be nice to have one (even if a very simple one).

I'll try to review and apply the patch a.s.a.p., thanks for working on this!

comment:9 Changed 4 months ago by mmarsan

I'd like to mention that OGL once changed the profile, adding the new 'core' profile. If they did it once, we can reasonable think that they would do again (say in some years).

IMHO the WX_CORE_PROFILE flag shoud not be boolean, but a constant, allowing future new profiles to be added to wx hopelfully without ABI breaking.

comment:10 Changed 4 months ago by VZ

  • Owner set to VZ
  • Resolution set to fixed
  • Status changed from confirmed to closed

In 77018:

Allow requesting core OpenGL profile and explicit OpenGL version.

Add WX_GL_CORE_PROFILE and WX_GL_{MAJOR,MINOR}_VERSION attributes which can be
used to use modern OpenGL with wxGLCanvas.

Closes #16402.

comment:11 follow-ups: Changed 4 months ago by vadz

Actually this looks relatively straightforward, so I just went and applied it with only minor changes, thanks again!

One thing I dislike is the use of static s_wglContextAttribs in MSW and Unix code. This code can be used from the main thread only, of course, but it's still easily possible to imagine creating 2 wxGLCanvas objects before creating wxGLContexts for them and things will break in a surprising way if one of them uses modern OpenGL and the other one doesn't. Why can't we have wxGLContext query wxGLCanvas for the context attributes instead of storing them as globals? This looks very straightforward to me, am I missing something? If not, it would be great if you could please change this.

TIA!

comment:12 in reply to: ↑ 11 Changed 4 months ago by captainhorst

Replying to vadz:

Actually this looks relatively straightforward, so I just went and applied it with only minor changes, thanks again!

One thing I dislike is the use of static s_wglContextAttribs in MSW and Unix code. This code can be used from the main thread only, of course, but it's still easily possible to imagine creating 2 wxGLCanvas objects before creating wxGLContexts for them and things will break in a surprising way if one of them uses modern OpenGL and the other one doesn't. Why can't we have wxGLContext query wxGLCanvas for the context attributes instead of storing them as globals? This looks very straightforward to me, am I missing something? If not, it would be great if you could please change this.

TIA!

Thanks for applying the patch! :)

You're right that's annoying. I didn't add a get{GLX,WGL}ContextAttribs() method to wxGLCanvas, because I thought it would be exposed to the user. But now I understand "interface" isn't just for documentation purposes and actually the public API...

Replying to mmarsen:

I'd like to mention that OGL once changed the profile, adding the new 'core' profile. If they did it once, we can reasonable think that they would > do again (say in some years).

IMHO the WX_CORE_PROFILE flag shoud not be boolean, but a constant, allowing future new profiles to be added to wx hopelfully without ABI breaking.

This would fit my first patch where I used a profile mask with bits for compatibility, core, OpenGL ES and OpenGL ES2 profiles as available under Windows and Linux now where any combination might be possible.
The core profile right now means "use this exact OpenGL version with no backwards compatibility" (with forward compatibility if the respective bit is set).
But you're right, having a WX_GL_CONTEXT_PROFILE and constants for every valid profile combination like WX_GL_CORE_PROFILE would ensure binary compatibility in the future.

As for debug contexts, which I would like to add later, this is just a flag to the context creation right now. So a boolean will be fine, I guess.

I have to go to work now. I will try to prepare another patch for these suggestions this evening.

comment:13 Changed 4 months ago by vadz

We can make the GetContextAttribs() method private and let wxGLContext (only) access it by declaring it as a friend. Or, simpler, just rely on common sense and leave it public but put a comment saying "This is used for implementation purposes only" before the method declaration (and, of course, do not document it in the interface file). I wouldn't worry about exposing it to the user.

As for more precise profile selection, I don't really know what makes more sense: a single attribute with a value or several attributes without a value. How many of them do we risk having? As long as it's just a few, I think the latter (i.e. the current code) is just fine. Also, the two approaches are exactly equivalent from ABI perspective AFAICS.

Changed 4 months ago by captainhorst

OpenGL 3 core renderer for the cube sample. glxContextAttributes aren't static anymore

comment:14 Changed 4 months ago by captainhorst

In the latest patch I added the GetGLXContextAttribs method to wxGLCanvasX11 and started work on a core renderer for the OpenGL cube sample. I underestimated the work quite a bit, because there's no GLEW and glm. So I have to manually load all the function pointers and probably have to write some matrix math functions. I have yet to add lighting, perspective transform and x-axis rotation.
As I'm without a Windows/Linux machine over the weekend this has to wait.

comment:15 in reply to: ↑ 11 Changed 4 months ago by raysatiro

  • Resolution fixed deleted
  • Status changed from closed to reopened

Following the changes made to msw/glcanvas.cpp I get a null pointer dereference in wxGLCanvas::Create() via the attribList parameter which is NULL by default.
https://github.com/wxWidgets/wxWidgets/commit/3c7ba39#diff-245483e6f988bb4f063ca536eb25e0c9R324

I fixed this in my own build by adding an if( attribList ) which I will attach as a patch.

Replying to vadz:

One thing I dislike is the use of static s_wglContextAttribs in MSW and Unix code. This code can be used from the main thread only, of course, but it's still easily possible to imagine creating 2 wxGLCanvas objects before creating wxGLContexts for them and things will break in a surprising way if one of them uses modern OpenGL and the other one doesn't.

It looks as though this is still an issue, no? Since this master attribute list is static it can end up being changed from one creation to the next, unless that is what is intended?

Changed 4 months ago by raysatiro

Avoid null deref of attribList parameter in MSW wxGLCanvas. Refer to comment #15.

comment:16 Changed 4 months ago by vadz

@raysatiro, thanks for the fix, will apply soon.

@captainorst: I'll apply the changes to Linux implementation getting rid of the static array, but not the sample changes because I think the cube sample is becoming too confusing. I really liked the idea to add another sample ("pyramid") showing modern OpenGL, could this please be done instead? TIA!

comment:17 Changed 4 months ago by VZ

In 77072:

Fix creating wxGLCanvas without any attributes in wxMSW.

This was broken by the changes of r77018, see #16402.

Just check that we do have the attributes before examining them.

comment:18 Changed 4 months ago by VZ

In 77073:

Refactor Unix OpenGL code to avoid using static attributes array.

Put the context attributes in wxGLCanvasX11 itself instead.

See #16402.

comment:19 Changed 4 months ago by raysatiro

  • Cc raysatiro@… added
Note: See TracTickets for help on using tickets.