Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some bugs in loading OpenGL/GLX/EGL libraries #229

Closed
wants to merge 4 commits into from

Conversation

ya-isakov
Copy link
Contributor

@ya-isakov ya-isakov commented Jul 29, 2020

Without additional check, even if libOpenGL was loaded, libGL.so will be loaded as well, and used both in gl_handle and glx_handle, so libglvnd libraries will not be used.

The problem is that on Wayland-only system, there will be no libGL, so epoxy_load_gl will fail even if libOpenGL was loaded.

Without additional check, even if libOpenGL was loaded, libGL.so will
be loaded as well, and used both in gl_handle and glx_handle, so
libglvnd libraries will not be used.
@ya-isakov
Copy link
Contributor Author

ya-isakov commented Jul 29, 2020

Hmm, it looks like this leads to not loading GLX library, as epoxy_conservative_glx_dlsym is using exit_if_fails as a load flag. So, every epoxy_current_context_is_glx will load GLX library again. But, I cannot find a proper way to fix this.

@ebassi
Copy link
Collaborator

ebassi commented Jul 29, 2020

I guess this is for @nwnk to review.

@ya-isakov
Copy link
Contributor Author

I've found a way to fix problem with loading libGL/libGLX on every epoxy_current_context_is_glx check - there is no point of trying epoxy_conservative_egl_dlsym if appropriate handle is NULL.
If this handle is NULL, it means that library was not loaded, and epoxy_conservative_egl_dlsym with load=exit_if_fails=false will not load it as well, and this will lead to do_dlsym also returning NULL. So, I've added checks for api.*_handle in epoxy_current_context_is_glx function.

@ya-isakov
Copy link
Contributor Author

Here is output of strace for https://github.com/ebassi/glarea-example on libglvnd-enabled wayland (libGL.so/libGLX.so still exists):
Before my fixes:
....
[pid 3520958] openat(AT_FDCWD, "/usr/lib64/libOpenGL.so.0", O_RDONLY|O_CLOEXEC) = 22
[pid 3520958] openat(AT_FDCWD, "/usr/lib64/libGL.so.1", O_RDONLY|O_CLOEXEC) = 22
[pid 3520958] openat(AT_FDCWD, "/usr/lib64/libGLX.so.0", O_RDONLY|O_CLOEXEC) = 22

After:
...
[pid 3519200] openat(AT_FDCWD, "/usr/lib64/libOpenGL.so.0", O_RDONLY|O_CLOEXEC) = 22

@ya-isakov ya-isakov changed the title If glvnd library found, do not use libGL.so in epoxy_load_gl Fix some bugs in loading OpenGL/GLX/EGL libraries Jul 29, 2020
@nwnk nwnk self-assigned this Aug 15, 2020
@nwnk
Copy link
Collaborator

nwnk commented Aug 17, 2020

One thing we might want to consider is using dlopen(RTLD_NOLOAD) to probe for things...

EDIT: Oh hey, we are doing that. Excellent.

@nwnk
Copy link
Collaborator

nwnk commented Aug 17, 2020

Patch 1 2 and 4 in this series are:

Reviewed-by: Adam Jackson <ajax@redhat.com>

Patch 3 ("Do not load symbols in epoxy_current_context_is_glx [...]") isn't great. With that change, epoxy_current_context_is_glx will fail if it's the very first epoxy library call, even if there is a current context and it is GLX. We should instead gently (RTLD_NOLOAD-style) test whether GLX libraries are already loaded, and fill in api.glx_handle and friends based on that.

@ya-isakov
Copy link
Contributor Author

ya-isakov commented Aug 18, 2020

@nwnk I'm completely fine, if you would merge only 3 commits. Third one was my best try on fixing the load of libGL.so on every symbol lookup, if EGL is used, but I'm not sure how to fix it properly.

P.S. Right now I'm using libepoxy with my fixes on system without libGL/libGLX, and everything is completely fine.

@ya-isakov
Copy link
Contributor Author

ya-isakov commented Aug 18, 2020

@nwnk Oh, and how it could be possible, to have GLX context without loading libGLX first? And to load it, application will use glXCreateNewContext, a wrapper for epoxy_glx_dlsym, which will load libGLX.so, if needed, and set api.glx_handle.

From what I've seen, epoxy_current_context_is_glx is used only for OpenGL/GLES calls, so EGL/GLX initialization should run without issues, and one cannot use GL context without EGL/GLX, right?

So, my point is, there is no "current context" without creating it via GLX/EGL, so it's completely fine to return false in this case. And if context is created via GLX/EGL, then api.glx_handle/api.egl_handle will be populated, so epoxy_current_context_is_glx will definitely return proper result.

@nwnk
Copy link
Collaborator

nwnk commented Aug 20, 2020

And to load it, application will use glXCreateNewContext, a wrapper for epoxy_glx_dlsym, which will load libGLX.so, if needed, and set api.glx_handle.

This isn't necessarily true. Imagine plugin library A gets loaded first, which links against libGL directly, creates a GLX context, and makes it current. Then plugin library B is loaded, which links against epoxy.

@tomeuv
Copy link

tomeuv commented Jan 20, 2021

@nwnk Could we get 1, 2 and 4 merged? With them, I'm able to run virglrenderer without linking to GLX.

@ebassi
Copy link
Collaborator

ebassi commented Jan 20, 2021

Patch 2 introduces a test failure in glx_alias_prefer_same_name.

I've opened #238 with patches 1 and 4.

@batesj
Copy link

batesj commented Mar 5, 2021

Any thoughts on whether #240 is a libepoxy bug or apitrace?

@ya-isakov
Copy link
Contributor Author

@batesj I would say, that it is probably a apitrace not supporting libOpenGL.so. I cannot find any mentions of it in it's code, so, maybe it's trying to override libGL.so, but with my patch, it should not be used, if libOpenGL.so is present in the system

@ya-isakov
Copy link
Contributor Author

ya-isakov commented Mar 5, 2021

@ebassi I'm fine if commit 2 (libGLX.so version fix) is omitted, it's probably different in different distributions. On GLX-free system there is no such file at all...
@nwnk Sorry, proper fixing of patch 3 is way above my skill set in C :( This patch is optional, but without it, every call, which checks for epoxy_current_context_is_glx, will try to load GLX library, if libepoxy is compiled with GLX support, but libglnvd is installed, and EGL is used (e.g. in Wayland)

@batesj
Copy link

batesj commented Apr 29, 2021

Can there be a new release to pick up this bug fix?

@ebassi
Copy link
Collaborator

ebassi commented Apr 30, 2021

@batesj I just released 1.5.6 which contains the first and last commits of this MR.

@ebassi
Copy link
Collaborator

ebassi commented Apr 30, 2021

If nobody beats me to it, I'll try to carve some time to rework the third commit to follow the approach outlined by @nwnk.

@batesj
Copy link

batesj commented Jun 15, 2021

Any thoughts on bringing these fixes back without breaking some distros? Compile option perhaps?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants