Allow libopengl.so to be used when GLX_LIB is missing #257

batesj · 2021-06-17T18:39:30Z

This maintains compatibility with previous behavior of
always using GLX_LIB if it is found. The only change is
when there is no GLX_LIB.

Previous behavior when no GLX_LIB:

abort.

New behavior when no GLX_LIB:

Try to load libOpenGL.so as gl_handle (glx_handle remains NULL).
Else, abort.

batesj · 2021-06-17T18:45:06Z

@ya-isakov @tomeuv @ebassi

PTAL. Also, if you have a repro with one of the known downstream failures caused by #229, it would help to have someone test one of those cases.

batesj · 2021-06-22T19:54:48Z

ping.

ya-isakov · 2021-06-22T20:07:07Z

@batesj Checked it on my libGL-free system - it works:

openat(AT_FDCWD, "/usr/lib64/libGLdispatch.so.0", O_RDONLY|O_CLOEXEC) = 16
....
openat(AT_FDCWD, "/usr/lib64/libGL.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/lib64/libOpenGL.so.0", O_RDONLY|O_CLOEXEC) = 21
openat(AT_FDCWD, "/lib64/libGLX.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/lib64/libGLX.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/lib64/libGL.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/lib64/libGL.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/lib64/libGLX.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/lib64/libGLX.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/lib64/libGL.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/lib64/libGL.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)

(spam about libGLX.so is caused by lack of check, which I had in one of my other patches)
Without your patch, vanilla libepoxy:

openat(AT_FDCWD, "/usr/lib64/libGLdispatch.so.0", O_RDONLY|O_CLOEXEC) = 16
openat(AT_FDCWD, "/usr/lib64/libOpenGL.so.0", O_RDONLY|O_CLOEXEC) = 21
....
openat(AT_FDCWD, "/usr/lib64/haswell/libGL.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/lib64/x86_64/libGL.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/usr/lib64/libGL.so.1", O_RDONLY|O_CLOEXEC) = -1 ENOENT (No such file or directory)
write(2, "Couldn't open libGL.so.1: libGL."..., 96Couldn't open libGL.so.1: libGL.so.1: cannot open shared object file: No such file or directory

But previous one worked for me, too, so, somebody with Xorg should also check...

batesj · 2021-06-25T23:53:33Z

I've tested this extensively now in Chrome OS and confirmed that it works to enable the use of libOpenGL.so while not changing behavior of existing libGLX expectations. For example, confirmed that this libepoxy change does not break crostini apps when libopengl0 is installed.

batesj · 2021-06-28T16:55:18Z

@ebassi ptal

ebassi · 2021-06-28T17:20:02Z

Let's see if the people having issues in #252 and #240 can test this: @Ernest1338 and @evelikov

As an aside: there's no point in pinging me. I receive all bug mail for libepoxy.

kev009 · 2021-06-28T20:43:01Z

LGTM from code inspection, we didn't experience any known crashes on FreeBSD that I am aware of but just switched to libglvnd everywhere so would like to drop the direct GLX dependency.

evelikov · 2021-06-30T11:17:21Z

From a quick look libGL -> libOpenGL fallback sounds like the right thing to do. Thank you.

I would suggest a couple of things:

elaborate/reference the inline "existing behaviour" comment - either add concrete details how, or simply reference the issues/broken workloads Draw calls missing in apitrace (regression) #240 and Upgrading libepoxy from 1.5.5 to 1.5.7 results in Xorg crashing on boot #252
adding an in-tree test, with trivial reproducer of said bugs - so it is even harder to accidentally break in the future

Doubt I can test it at the moment @linkmauve - do you have some time to check this on your end?

batesj · 2021-06-30T23:51:37Z

Does the test need to be in this pull request or can it be a followup? The nature of this change is at the level of attempting to open libGL.so.1 and libOpenGL.so.0 so a test could have have dummy libraries that confirm libGL.so.1 is loaded if it exists, and then fall back on libOpenGL.so.0.

Ernest1338 · 2021-07-01T14:56:23Z

I'm on vacation at the moment, I will test this when I get home.

evelikov · 2021-07-01T15:16:15Z

Does the test need to be in this pull request or can it be a followup? The nature of this change is at the level of attempting to open libGL.so.1 and libOpenGL.so.0 so a test could have have dummy libraries that confirm libGL.so.1 is loaded if it exists, and then fall back on libOpenGL.so.0.

I don't have a preference if tests come here or as follow-up.

The test you outlined doesn't sound super useful. Tests which do functional testing, similar to the bugs #240 #252 and even #253 would be much more valuable. Mind you I did not trace each use-case, so it might be that they all follow the exact same problematic pattern.

Note: the tests are a suggestion, ultimately it's not my call to make.

batesj · 2021-07-01T18:43:51Z

Perhaps there's an easy github workflow that would repro some of the original integration bugs in #240, #252 and/or #253. There is some investigation to be done for that, so would prefer to not block this CL -- filed #258 for that.

In the mean time this CL fixes the bug that @ya-isakov described in #229 (comment). To reiterate the motivation here: without this fix, it is impossible for a wayland-only system to use OpenGL via libepoxy.

kev009 · 2021-07-01T18:57:18Z

From the outside it seems like this is an acceptable workaround to an issue that is not very well understood. With my cursory inspection it seems to require non-trivial knowledge of how these handles are used everywhere (which might affect library-consumers?) and whether there are known or unknown limitations with libOpenGL in a GLX setup. Until the issue is better understood, it is difficult to prescribe a test plan, and the salient change is to preserve old known good behavior for GLX.

This is probably obvious to most but I'll just write it here for completeness: libGL == libOpenGL + libGLX.

batesj · 2021-07-01T20:37:53Z

elaborate/reference the inline "existing behaviour" comment - either add concrete details how, or simply reference the issues/broken workloads Draw calls missing in apitrace (regression) #240 and Upgrading libepoxy from 1.5.5 to 1.5.7 results in Xorg crashing on boot #252

Done.

src/dispatch_common.c

evelikov · 2021-07-02T11:15:15Z

@batesj I'm aware how annoying libGL.so can be. In case it's unclear - I'm not suggesting we should block this MR. I did said "suggestion" and not "requirement" exactly for that reason ;-)

batesj · 2021-07-02T18:49:40Z

Thanks for the review, @evelikov!

This maintains compatibility with previous behavior of always using GLX_LIB if it is found. The only change is when there is no GLX_LIB. Previous behavior when no GLX_LIB: - abort. New behavior when no GLX_LIB: - Try to load libOpenGL.so as gl_handle (glx_handle remains NULL). - Else, abort. Reviewed-by: Emil Velikov <[email protected]>

Ernest1338 · 2021-07-09T20:25:19Z

So I tested this, and found no issues (related to Xorg at least). ;)

batesj · 2021-07-12T15:19:50Z

Thanks for testing @Ernest1338, sounds like we can land this then.

ebassi · 2021-07-12T15:49:36Z

I agree. Thanks to everyone who tested this, it's very much appreciated.

Thanks @batesj for working on this, as well.

batesj · 2021-07-12T16:50:17Z

Thanks @ebassi, let me know when you think we can release version 1.5.9 with this change.

batesj marked this pull request as ready for review June 25, 2021 23:50

ebassi mentioned this pull request Jun 28, 2021

dispatch: fix loading of GL without GLX library #232

Closed

batesj mentioned this pull request Jul 1, 2021

Add github CI workflow and script to test integration of libepoxy, apitrace, Xorg, etc #258

Open

batesj force-pushed the allow_libopengl branch from 8919ece to 5c90080 Compare July 1, 2021 20:36

evelikov reviewed Jul 2, 2021

View reviewed changes

src/dispatch_common.c Outdated Show resolved Hide resolved

batesj force-pushed the allow_libopengl branch from 5c79f50 to 7975061 Compare July 2, 2021 18:58

ebassi merged commit ecfa8e0 into anholt:master Jul 12, 2021

batesj deleted the allow_libopengl branch July 15, 2021 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow libopengl.so to be used when GLX_LIB is missing #257

Allow libopengl.so to be used when GLX_LIB is missing #257

batesj commented Jun 17, 2021

batesj commented Jun 17, 2021

batesj commented Jun 22, 2021

ya-isakov commented Jun 22, 2021 •

edited

Loading

batesj commented Jun 25, 2021

batesj commented Jun 28, 2021

ebassi commented Jun 28, 2021

kev009 commented Jun 28, 2021

evelikov commented Jun 30, 2021 •

edited

Loading

batesj commented Jun 30, 2021

Ernest1338 commented Jul 1, 2021

evelikov commented Jul 1, 2021

batesj commented Jul 1, 2021

kev009 commented Jul 1, 2021 •

edited

Loading

batesj commented Jul 1, 2021

evelikov commented Jul 2, 2021

batesj commented Jul 2, 2021

Ernest1338 commented Jul 9, 2021

batesj commented Jul 12, 2021

ebassi commented Jul 12, 2021

batesj commented Jul 12, 2021

Allow libopengl.so to be used when GLX_LIB is missing #257

Allow libopengl.so to be used when GLX_LIB is missing #257

Conversation

batesj commented Jun 17, 2021

batesj commented Jun 17, 2021

batesj commented Jun 22, 2021

ya-isakov commented Jun 22, 2021 • edited Loading

batesj commented Jun 25, 2021

batesj commented Jun 28, 2021

ebassi commented Jun 28, 2021

kev009 commented Jun 28, 2021

evelikov commented Jun 30, 2021 • edited Loading

batesj commented Jun 30, 2021

Ernest1338 commented Jul 1, 2021

evelikov commented Jul 1, 2021

batesj commented Jul 1, 2021

kev009 commented Jul 1, 2021 • edited Loading

batesj commented Jul 1, 2021

evelikov commented Jul 2, 2021

batesj commented Jul 2, 2021

Ernest1338 commented Jul 9, 2021

batesj commented Jul 12, 2021

ebassi commented Jul 12, 2021

batesj commented Jul 12, 2021

ya-isakov commented Jun 22, 2021 •

edited

Loading

evelikov commented Jun 30, 2021 •

edited

Loading

kev009 commented Jul 1, 2021 •

edited

Loading