Skip to content

Conversation

binary1248
Copy link
Member

The way OpenGL entry points are loaded on Windows is unique compared to other platforms. Whereas on other platforms, the entry points returned by e.g. glXGetProcAddressARB can be used independently of the context they were queried in, the entry points returned by wglGetProcAddress should only be used in the context they were queried in. This means in theory, any OpenGL function could have multiple different addresses on Windows depending on the context that was active when the query was made.

In practice, we just have to differentiate between 2 cases on Windows, either an ICD (hardware accelerated vendor driver) is available or an ICD is not available. In the former case, wglGetProcAddress forwards the query to the ICD providing the context that is currently active and it returns the address of the function from the concrete ICD .dll. In the latter case, when an ICD is not available, Windows provides a generic GDI software rendering implementation. For whatever reason, when the generic implementation is in use, wglGetProcAddress cannot be used to query entry points available for the active context, it will always return NULL. Instead they have to be loaded from the generic implementation library (OpenGL32.dll) itself using GetProcAddress.

So in summary:

  • ICD available -> Create ICD context, activate and load using wglGetProcAddress
  • ICD not available -> Create generic context, activate and load using GetProcAddress

The current implementation in SFML isn't 100% correct since it will try to fall back to GetProcAddress even when an ICD context is active but wglGetProcAddress does not return an address for whatever reason.

This is bad for 2 reasons:

  1. We get entry points that are incompatible with the context they are supposed to be used in
  2. We get a false impression that something is supported when it really isn't

This change fixes OpenGL entry points being loaded from OpenGL32.dll instead of failing to load if they are not provided by the ICD on Windows.

This is technically still not a 100% correct implementation, since in theory there could be multiple ICDs installed on a given system, each providing their specific entry points. The entry points would have to be stored per context that is created instead of globally for the entire application. Because this scenario occurs so rarely if at all, any OpenGL entry point loader (like glad which SFML uses) assumes the same entry points can be used across any context that is created by the application and thus stores them globally. This is correct on any platform except Windows, but I guess they weren't willing to make an exception just for Windows.

The only way to test this is by running a test application on both a system that has an ICD installed and one that only provides the generic implementation and inspecting where the function pointers originate from. In the former case, they should be addresses in the ICD (e.g. atioglxx.dll for AMD, nvoglv32.dll for Nvidia) and in the latter case they should be addresses inside OpenGL32.dll.

@codecov
Copy link

codecov bot commented Mar 29, 2023

Codecov Report

Merging #2478 (1517d6c) into master (f371a99) will increase coverage by 0.01%.
The diff coverage is 94.44%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2478      +/-   ##
==========================================
+ Coverage   25.83%   25.84%   +0.01%     
==========================================
  Files         226      226              
  Lines       19420    19420              
  Branches     4715     4712       -3     
==========================================
+ Hits         5017     5020       +3     
+ Misses      13869    13866       -3     
  Partials      534      534              
Impacted Files Coverage Δ
src/SFML/Window/GlContext.cpp 72.88% <ø> (+0.21%) ⬆️
src/SFML/Window/Win32/WglContext.cpp 46.85% <94.11%> (+1.42%) ⬆️
src/SFML/Window/Win32/WglContext.hpp 100.00% <100.00%> (ø)

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f371a99...1517d6c. Read the comment docs.

@eXpl0it3r eXpl0it3r added this to the 3.0 milestone Mar 29, 2023
@binary1248 binary1248 force-pushed the bugfix/wgl_getfunction branch from bc4587b to 405e8e7 Compare March 29, 2023 07:42
@binary1248 binary1248 force-pushed the bugfix/wgl_getfunction branch from 405e8e7 to af23322 Compare March 29, 2023 20:12
@binary1248
Copy link
Member Author

Updated.

@binary1248 binary1248 force-pushed the bugfix/wgl_getfunction branch from af23322 to ee3ac1c Compare April 3, 2023 01:49
@binary1248
Copy link
Member Author

Rebased to master.

@binary1248 binary1248 force-pushed the bugfix/wgl_getfunction branch 3 times, most recently from 69e9222 to b4f6648 Compare April 4, 2023 19:11
Copy link
Member

@vittorioromeo vittorioromeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not confident on the OpenGL-related logic here but I am happy with the code changes.

…ailing to load if they are not provided by the vendor provided library on Windows.
@binary1248 binary1248 force-pushed the bugfix/wgl_getfunction branch from b4f6648 to 1517d6c Compare April 5, 2023 08:01
@eXpl0it3r eXpl0it3r merged commit 21af6fe into master Apr 5, 2023
@eXpl0it3r eXpl0it3r deleted the bugfix/wgl_getfunction branch April 5, 2023 16:09
@ChrisThrasher
Copy link
Member

#2495

This is causing crashes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants