Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 644732 - media-libs/libsdl2: vulkan support
Summary: media-libs/libsdl2: vulkan support
Status: RESOLVED FIXED
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Current packages (show other bugs)
Hardware: All Linux
: Normal normal with 1 vote (vote)
Assignee: Gentoo Games
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-16 04:16 UTC by Niklas Haas
Modified: 2018-11-03 11:33 UTC (History)
7 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
SDL_x11vulkan.c hack patch (file_644732.txt,4.94 KB, patch)
2018-01-22 10:36 UTC, Niklas Haas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Niklas Haas 2018-01-16 04:16:32 UTC
Upstream has added support for vulkan in libSDL2, which is currently hard-disabled by the ebuild. I got it running on my end, but I encountered an odd issue:

Just adding the USE flag (and dependency on media-libs/vulkan-loader) did not work, because of this weird quirk in ./src/video/SDL_vulkan_internal.h:

    #if defined(SDL_LOADSO_DISABLED)
    #undef SDL_VIDEO_VULKAN
    #define SDL_VIDEO_VULKAN 0
    #endif

I'm not entirely sure what's going on here - for some reason, LOADSO_DISABLED being 1 hard-disables vulkan support from SDL2. But on the other hand, the vulkan ebuild always disables loadso except when `kernel_Winnt` is set? I'm not sure what's going on here, but it would be nice if somebody could investigate.

At any rate, removing the `$(use_enable kernel_Winnt loadso)` from the ebuild made it work, but this seems like a hack. (Why is that use_enable there to begin with?)
Comment 1 Jonas Stein gentoo-dev 2018-01-16 16:13:39 UTC
The bug tracker is not good for discussions. Best would be to join the IRC channel #gentoo-games and #gentoo-dev-help to discuss and solve things there and report back the solution.
Comment 2 James Le Cuirot gentoo-dev 2018-01-21 22:43:51 UTC
(In reply to Jonas Stein from comment #1)
> The bug tracker is not good for discussions.
Here works for me.

loadso was previously disabled entirely. It's for loading arbitrary libraries dynamically at runtime (using dlopen on Linux), which is sometimes best avoided in distribution packages for QA reasons and it's also a potential attack vector. It's not clear to me exactly why SDL needs this but it was needed for OpenGL support on Windows. Evidently it's also needed for Vulkan support. The option is normally enabled by default so I guess it would be best for us to do that now.

As for Vulkan support, Polynomial-C disabled it unconditionally but I imagine this is because he didn't have the means to test it. I have used Vulkan in games such as The Talos Principle so I can test it. It doesn't link to any libraries at build time (probably because of loadso) but it does use some Vulkan headers. These have been bundled under src/video/khronos/vulkan, which is probably bad, so I suggest we remove these in favour of media-libs/vulkan-loader and patch src/video/SDL_vulkan_internal.h accordingly.

I have already tried this and it builds for me. With a little persuasion, I was also able to run the testvulkan program, which fades between pretty colours. I'm not sure if that's what it's supposed to do but hey. :)
Comment 3 Niklas Haas 2018-01-21 23:12:07 UTC
Sure, that seems like a more reasonable approach. It might also be possible to patch SDL to just link against media-libs/vulkan-loader as well, instead of relying on dlopen at runtime. Then we can drop the loadso requirement for vulkan.
Comment 4 Lars Wendler (Polynomial-C) (RETIRED) gentoo-dev 2018-01-22 09:34:30 UTC
(In reply to James Le Cuirot from comment #2)
> As for Vulkan support, Polynomial-C disabled it unconditionally but I
> imagine this is because he didn't have the means to test it.

That is true. I don't have vulkan enabled yet on my dev machine and when I tried to add vulkan support to libsdl2 I ran into build issues. So I decided to disable vulkan support and leave its addition to someone knowing vulkan better than me.
Comment 5 Lars Wendler (Polynomial-C) (RETIRED) gentoo-dev 2018-01-22 09:47:31 UTC
(In reply to Niklas Haas from comment #3)
> Sure, that seems like a more reasonable approach. It might also be possible
> to patch SDL to just link against media-libs/vulkan-loader as well, instead
> of relying on dlopen at runtime. Then we can drop the loadso requirement for
> vulkan.

+1

We should avoid dlopen as much as possible.
Comment 6 Niklas Haas 2018-01-22 10:10:09 UTC
I had a short glance at the SDL2 code base and I noticed that, in addition to the SDL_vulkan code, the SDL_opengl code *also* seems to rely on loadso support in order to load "libGL.so". This strikes me as very odd considering the fact that OpenGL support apparently works fine even with loadso disabled?

It's a bit weird, but even stepping through this code in gdb confirms that it's using dlopen() even for OpenGL loading, and in particular it's using this series of definitions in SDL_x11opengl.c:

    #if defined(OPENGL_REQUIRES_DLOPEN) && defined(SDL_LOADSO_DLOPEN)
    #include <dlfcn.h>
    #define GL_LoadObject(X)    dlopen(X, (RTLD_NOW|RTLD_GLOBAL))
    #define GL_LoadFunction     dlsym
    #define GL_UnloadObject     dlclose
    #else
    ...

So that means that SDL_LOADSO_DLOPEN is set - but at the same time, SDL_LOADSO_DISABLED is also set? Does this make sense to anybody at all? And how come the SDL_vulkan code can't use SDL_LOADSO_DLOPEN here as well?

Anyway, it seems working around this weirdness might be non-trivial - and would also have to be done for OpenGL - but I'll give it a try nonetheless
Comment 7 Niklas Haas 2018-01-22 10:35:48 UTC
I had a quick glance at it and working around the loadso is somewhat nontrivial. The hacks involved are crude, and I have no idea how to use autotools so it's probably broken. But I managed to produce a libSDL2.so that was able to successfully create an X11 vulkan context with --loadso-disabled. Based on the invasive nature of the changes to the code, I don't think this is the right path forwards, though, unless we can upstream the changes.

I've attached the hack-patch I was using at the time of producing the above result, but it's rough around the edges. Just so you can get an idea of how invasive the change it.
Comment 8 Niklas Haas 2018-01-22 10:36:25 UTC
Created attachment 515818 [details, diff]
SDL_x11vulkan.c hack patch
Comment 9 Niklas Haas 2018-01-22 10:41:46 UTC
I've also reported this issue upstream: https://bugzilla.libsdl.org/show_bug.cgi?id=4061
Comment 10 James Le Cuirot gentoo-dev 2018-01-22 10:49:46 UTC
(In reply to Niklas Haas from comment #6)
> It's a bit weird, but even stepping through this code in gdb confirms that
> it's using dlopen() even for OpenGL loading, and in particular it's using
> this series of definitions in SDL_x11opengl.c:
I saw that too and was confused as well. However, I think it's normal for things to not explicitly link against libGL.so? I don't know why, I've just found that to be the case. Maybe it's because libGL.so has many different implementations, hence the push for GLVND. Vulkan has already taken the vendor-neutral approach.

Good work with the patch. I can handle Autotools so I'll take a look later and I'll CC to the bug.
Comment 11 Lars Wendler (Polynomial-C) (RETIRED) gentoo-dev 2018-01-22 11:04:30 UTC
(In reply to James Le Cuirot from comment #10)
> 
> Good work with the patch. I can handle Autotools so I'll take a look later
> and I'll CC to the bug.

While being at that, please ask upstream to rename their configure.in file to configure.ac :)
Comment 12 Niklas Haas 2018-01-22 11:15:03 UTC
Poking and prodding the code some more: Seems like for OpenGL, what SDL2 is basically doing is bypassing the internal SDL_LoadLibrary for X11+libGL.so in particular - and the only reason I can come up with as to why it would want to do this is because the `dlopen` code from SDL_x11opengl.c uses RTLD_GLOBAL whereas the generic SDL_LoadLibrary from the `loadso` subsystem is uses RTLD_LOCAL.

Perhaps some SDL2 developer ended up realizing that libGL.so needs to be loaded globally instead of locally when using GLX in particular (I don't know why?), and therefore wrote this check - and the net result of it was that OpenGL was "accidentally" supported for libSDL2+OpenGL+X11 even though we compile it with --disable-loadso, and nobody noticed it until now because it was working.

In particular, if my analysis is correct, then libSDL2+GLES+Wayland would fail to work on Gentoo, since that configuration doesn't include the dlopen() hack and falls back to the regular SDL_LoadLibrary; although for some reason, that particular file does *not* check for SDL_LOADSO_DISABLED - so it would probably just produce an SDL error at runtime? Can somebody with wayland support test this? :p

So if we want to get rid of loadso entirely, we'll also have to patch out all of the code that hard-codes dlopen(), as well as all code based on OpenGL or OpenGL ES. This seems like a much more difficult endeavor than I initially anticipated.

Alternatively, we could just enable loadso, call it a day, and hope upstream eventually removes the need to do this.
Comment 13 James Le Cuirot gentoo-dev 2018-01-22 11:43:06 UTC
(In reply to Niklas Haas from comment #12)
> In particular, if my analysis is correct, then libSDL2+GLES+Wayland would
> fail to work on Gentoo, since that configuration doesn't include the
> dlopen() hack and falls back to the regular SDL_LoadLibrary; although for
> some reason, that particular file does *not* check for SDL_LOADSO_DISABLED -
> so it would probably just produce an SDL error at runtime? Can somebody with
> wayland support test this? :p
Pretty sure I tried this combo already with Neverball on my ARM box a few months ago. I didn't notice this issue.
Comment 14 Mark Jacobson 2018-09-04 00:16:09 UTC
I was looking into this as I was building vkquake from source, and it looks like upstream closed out this bug, per https://bugzilla.libsdl.org/show_bug.cgi?id=4061#c3
Comment 15 James Le Cuirot gentoo-dev 2018-09-04 09:37:09 UTC
(In reply to Mark Jacobson from comment #14)
> I was looking into this as I was building vkquake from source, and it looks
> like upstream closed out this bug, per
> https://bugzilla.libsdl.org/show_bug.cgi?id=4061#c3

I guess things that could be better but aren't really broken don't get much priority. Certainly the case for me!
Comment 16 Mark Jacobson 2018-09-05 17:19:57 UTC
(In reply to James Le Cuirot from comment #15)
> (In reply to Mark Jacobson from comment #14)
> > I was looking into this as I was building vkquake from source, and it looks
> > like upstream closed out this bug, per
> > https://bugzilla.libsdl.org/show_bug.cgi?id=4061#c3
> 
> I guess things that could be better but aren't really broken don't get much
> priority. Certainly the case for me!

Thank you for your remark earlier regarding your response to conversing about this in IRC. Researching this issue was really just a minor thing for me, as I was just playing around with building some vulkan programs from source. Everything I want to do still works, but as things move on it looks like upstream doesn't want to improve or change their approach on this. 

It also looks like an issue for Ubuntu as they have a similar bug, and the solution so far was for someone to make a PPA that has loadso enabled. https://bugs.launchpad.net/ubuntu/+source/libsdl2/+bug/1740517 

If a dev could close this bug out with something definitive I'd sure appreciate it. As Niklas Haas pointed out, if the decision not to allow dlopen in this case is made, we go down the road of "whatabout"-isms and it looks like far more extensive changes need to be made across several other packages.

I very much appreciate everyone's time here though, and whichever decision is made, thanks regardless. This really is a minor trifle.
Comment 17 Larry the Git Cow gentoo-dev 2018-11-03 11:33:43 UTC
The bug has been closed via the following commit(s):

https://gitweb.gentoo.org/repo/gentoo.git/commit/?id=046604e293c61b449639b42f7f09da1bdc38c209

commit 046604e293c61b449639b42f7f09da1bdc38c209
Author:     James Le Cuirot <chewi@gentoo.org>
AuthorDate: 2018-11-03 11:31:55 +0000
Commit:     James Le Cuirot <chewi@gentoo.org>
CommitDate: 2018-11-03 11:33:20 +0000

    media-libs/libsdl2: Add vulkan support via USE flag
    
    This now enables the "loadso" option unconditionally, partly because
    both Vulkan and Windows require it, and partly because upstream insist
    that it should be enabled. It is feasible that Vulkan could be made to
    work without it and upstream have shown interest so if someone is
    prepared to do the work then we can revisit this.
    
    Closes: https://bugs.gentoo.org/644732
    Signed-off-by: James Le Cuirot <chewi@gentoo.org>
    Package-Manager: Portage-2.3.51, Repoman-2.3.11

 media-libs/libsdl2/libsdl2-2.0.9.ebuild | 19 ++++++++++++++-----
 media-libs/libsdl2/metadata.xml         |  1 +
 2 files changed, 15 insertions(+), 5 deletions(-)