kcat / openal-soft

OpenAL Soft is a software implementation of the OpenAL 3D audio API.
Other
2.2k stars 532 forks source link

[not a bug] small heads up for a little openal tool and licensing question / question for an addition of a header file feature #801

Open hypatia-of-sva opened 1 year ago

hypatia-of-sva commented 1 year ago

Hello,

I don't know if this is right place to write a message, but I wanted to let you know I've built a small openal-loader tool (similar to glad) that works with openal-soft (I tested and made it on Debian 11 and it finds the driver just fine, I haven't yet tested it on Windows) and is constructed from the header, and I wanted to ask if LGPL2 was the correct license (its written in the alext.h header so I thought that one was correct, but there was also BSD-3 in the repo so I wanted to make sure I wasn't getting that wrong). Here is the link to the repo:

https://github.com/hypatia-of-sva/alad

Hypatia of Sva.

hypatia-of-sva commented 1 year ago

Hello kcat,

a small follow up. I was pondering this question and realized what's different here to, let's say, a vulkan loader, where I wouldn't see the licensing issue, and I think I might have an addition here that could be useful in general if you would be willing to add this to the library. Specifically i would add two things:

  1. ifndef guards in al.h and alc.h that would prevent the definition of any function prototypes if AL_NO_PROTOTYPES is defined (alext.h and efx.h are fine the way they are as they don't define any functions by default)

  2. A symbol AL_HEADER_REVISION that's set to a number and numerically increased any time one of the headers is changed, especially when new extensions and / or function pointer types are added, either in one of those headers or in a new al_revision_nr.h header. This is necessary to be able to use the function pointer types for extension functions without having to check for each extension on its own if it is present (which is possible, but I think such a numbering scheme is simple enough and might be also otherwise useful).

If these two things were added, I wouldn't have to copy the header information into my own, and could include the official headers instead with the AL_NO_PROTOTYPES macro set and check the version to fail compilation if the header is too old for my use case. This also would mean I wouldn't have to duplicate information about the constant values etc, which would probably be for the better. (Both of these ideas aren't new either, they're basically taken straight from the Vulkan SDK, but I've found them quite useful there so I would like them to be present here as well.)

Would you think those would be good additions to the headers, or are there some complications with this that went over my head? I'm not really that deeply involved in OpenAL and just want to be thorough with setting up all dependencies for my projects, so there might be other context I haven't found or read up on about.

Hypatia of Sva

kcat commented 1 year ago

As far as the license goes, it really should be CC0 for those public headers (alc.h, al.h, and alext.h). I don't see the point in having any restrictions on them since it's just declaring the interface for an open API, and anything that could be copyrightable is so minor that there'd be no purpose to limit it. Maybe some comments in alc.h and al.h could still be Creative's instead of mine, so I'd need to go through and rewrite what may be from the original import before I could change those files' license from LGPL, but if you're stripping out the comments anyway, I can't imagine there'd be anything left under that license.

The license notice in alext.h was added in error and should be removed. That's a file I created after the initial import and wrote myself, so there should be no problem at all with that one.

Regarding AL_HEADER_REVISION, OpenAL Soft's headers aren't the official OpenAL headers, so anything that relied on such a macro would only work with OpenAL Soft's headers. Checking for the extension macros is the intended way to check what's available at compile time. It's more robust than checking a header version since a non-standard version value doesn't guarantee anything, while a given extension macro indicates the extension is known regardless of what headers it's from. A revision value could only be for building with OpenAL Soft's headers, until the standard can be updated to define what should be included with a given revision.

That said, the tool itself does look interesting. A simple way to dynamically load OpenAL so it's not a runtime dependency, along with some extension handling. I do notice some issues though, depending on what it's use target is.

At least on Linux, the default library name should be libopenal.so.1. libopenal.so is a development file that's a symlink to libopenal.so.X (where X is the ABI version the dev environment is set up for) that the resulting binary gains as a dependency. If it ever updates to libopenal.so.2 or later, it's not guaranteed to be compatible with programs compiled for libopenal.so.1. Not that I foresee that happening any time soon, but it's not impossible. Though I do believe some BSDs avoid putting the ABI version on the filename and just use libopenal.so, so it may be needed as a fallback there. Also, macOS uses libopenal.1.dylib, I believe (or maybe just libopenal.dylib? I'm not sure, Mac/iOS isn't my area of expertise).

A more significant issue is the way it uses alGetProcAddress. It can technically only be called when a context is made current, and the returned functions are only guaranteed to work with that context. If you use OpenAL Soft directly, it will work since it doesn't do any checks for the current context and it only has one set of functions to return, but other implementations can possibly have issues. It's best to use GetProcAddress/dlsym to get the standard/non-extension function pointers. The functions you get that way are guaranteed to work with any normal device handle and context, as those are the same functions you'd have if you linked directly to the library. If using the router, these functions will correctly forward the call the appropriate driver function given the device or current context.

Handling extensions would be a bit trickier. The functions returned by alcGetProcAddress are only guaranteed to work with the same device handle used to get the function (or any contexts created from that device). And functions returned by alGetProcAddress are only guaranteed to work with the current context. As far as OpenAL Soft itself is concerned, it's all the same since there's only one implementation for all devices and the function list is shared, but the router will be more strict since different devices and contexts can be from different drivers, and it doesn't handle all extensions itself (e.g. the router has no knowledge of AL_SOFT_callback_buffer, so calling alBufferCallbackSOFT = alGetProcAddress("alBufferCallbackSOFT"); will fail if there's no current context, or the current context doesn't support the extension).

So, if you only care about using OpenAL Soft directly (no routers or other implementations), it's fine as it is. Other implementations may also be fine, but some may not be; the standard makes no guarantee. It will have problems if used with the router (even if you open an OpenAL Soft device) since the router won't handle all extensions itself. If you want to be safe with various implementations and routers, it's best to use GetProcAddress/dlsym to get the standard/core functions in aladLoadAL, then have aladUpdateALCPointers use alcGetProcAddress with a device handle to (re)load device extension pointers for that device, which the user needs to call if the device is changed and they want to use extensions. And have a new aladUpdateALPointers function use alGetProcAddress to (re)load context extension pointers for the current context, which the user needs to call when the current context is changed if they want to use extensions.

hypatia-of-sva commented 1 year ago

Hello kcat,

thanks for the long answer, I'll just write a few things that I can easily answer and will see that I can change the code in the next days and report if anything else occurs there.

  1. Good for the notice on the license, I'll remove it then since I already removed all the comments. I think I'll put alad under something similar than the headerfiles then, a CC license or maybe MIT.

  2. Yes, I agree the revision macro would only work with your headers, and that was also kind of my idea, to make sure no ten year old headers from creative or the like are imported. Maybe it would make more sense calling it ALSOFT_HEADER_REVISION and to add an extra line

    #define ALSOFT_HEADER_PROVIDER "openal-soft"

to check I got the correct headers. All I'm doing here is checking that my compiler did get the correct newer header files from the repo and not old ones in /usr/include shipped with the debian stable package e.g. The use case would be something like

   #define ALSOFT_NO_PROTOTYPES
   #include <AL/alext.h>
   #if (ALSOFT_HEADER_PROVIDER != "openal-soft") || (ALSOFT_HEADER_REVISION < 0x20221224)
            #error "the AL headers are too old for this version of alad, download the correct headers here [link]"
   #endif

So yes, it's non standard, but given the standard has been there a long time unchanging I think it's fine, especially if it's a different prefix, I realized that having AL_ at the beginning may not have been a great idea here.

I do also see a use of ALSOFT_NO_PROTOTYPES that goes beyond pure reduction in alad.h: let's say you add an extension in your header, but I didn't get to update alad.h in time. In the meantime, the user could check a ALAD_HEADER_REVISION against the ALSOFT_HEADER_REVISION, and see what's missing/ the difference. However, it it's just constants / structs, it makes no sense why they shouldn't be usable with an old alad. If I can directly #include the alsoft headers, they can be updated without alad needing to be changed, as long as there is no new extension you need or a new major revision of some sort. (Maybe it would make sense then to have two codes: ALSOFT_HEADER_REVISION and ALSOFT_EXTENSION_SUPPORT_REVISION or something like that.) Basically, I'm uncomfortable to have all the definition mirrored in my header for these kinds of maintenance reasons, and obviously it would be a change to the old header files to add the #ifndef guardes, and it depends here on how these headers are in relation to the OpenAL board, if they must be the same or not. But since I saw you changed void to ALvoid etc I thought it could be possible, so that's why I made that proposal.

  1. Yes, I made the wrong choice for the ordering of primary and secondary library names, I'll switch those around. I'm also not big on apple but I will see if I can find info on the naming from other projects etc.

  2. I completely overlooked the context dependence of alGetProcAddress, maybe because I came from Vulkan and it's the other way around there. It does say in paragraph 7.2 of the specification (the openal-1.1-specification.pdf from Creative) that you're supposed to use alGetProcAddress to load core API pointers from the DLL, so if i understand that correctly, I would only need to add

    void aladUpdateALPointers(ALCcontext* context) {
            _alad_load_al_functions();
      }

to the code and not change what's currently there. However, if you're right and I cant trust any pointers from alGetProcAddress, even for core functions, then i would have to change the code in aladLoadAL, but that also would mean the spec is wrong/ inconsistent. That seems somewhat worrying to me.

Paragraph 6.3.4 however says that a NULL device handle doesn't guarantee anything, so I should throw that out. Im left with the following options:

a) load the ALC core pointers first with alGetProcAddress, and only with alcGetProcAddress later (that seems what the standard wants me to do) b) load first with dlsym and then with alcGetProcAddress (your idea) c) load first with dlsym, then update with alcGetProcAddress and then with alGetProcAddress (because technically, the context belongs to the device, so if alGetProcAddress is context specific, it is more specific then loading per device, right?) This would also mean to switch to a different context on the same device (i.e. switching to a different listener) i would reload with alcGetProcAddress(...,device) first and then switch to the new context, because the alGetProcAddress-loaded function would be context specific, including alcMakeContextCurrent.

I hope i got this right, but it is a bit difficult for me to understand how it should work. I reckon it would be better style to load alGetProcAddress with alcGetProcAddress, if its context specific and you then get the context dispatcher of a specific device, but the spec says to use alGetProcAddress first, as that's supposedly fine for core functions.

I will definitely add the reload for the context though and will add a notice to the description to not use any extension functions before reloading, and also change the initial load from alcGetProcAddress(...,NULL) to alGetProcAddress, since the first one is obviously wrong and the other one at least agrees with the spec. If you could provide me with any document that says i should use dlsym instead, even for core functions, i would be fine with changing that as well, I'm just not quite sure what to make of section 7.2 of the spec then, especially the last paragraph.

  1. I think i may even restructure the interface to split between aladLoadAL and aladLoadALExt, just so you dont accidentally load extensions without context. A good way to do this would be to have a dummy ALCcontext* parameter, like in the snippet above, to be checked against NULL or something, so as to only load extensions with a context. This would basically artificially enforce not loading extensions without context even on alsoft. I don't know if that would be really useful, but it would at least be making this a lot more explicit in the API.

Again, thanks for the extensive reply between the holidays and I hope I'll be getting on revising my code sooner than later.

Hypatia of Sva.

hypatia-of-sva commented 1 year ago

Hello kcat,

i fixed the bug and added an (yet untested) apple option. I also changed the loading behaviour and made it effectively possible to use all three options of loading (except loading alGetProcAddress from alcGetProcAddress, I still don't know about that one, I currently don't reload alGetProcAddress and alcGetProcAddress at all). It would be good though to know which is the default, so I can recommend which to use in most cases, because I currently don't know what I should recommend in the readme / top comment.

Hypatia of Sva

kcat commented 1 year ago
  1. Yes, I agree the revision macro would only work with your headers, and that was also kind of my idea, to make sure no ten year old headers from creative or the like are imported. Maybe it would make more sense calling it ALSOFT_HEADER_REVISION and to add an extra line
    #define ALSOFT_HEADER_PROVIDER "openal-soft"

to check I got the correct headers. All I'm doing here is checking that my compiler did get the correct newer header files from the repo and not old ones in /usr/include shipped with the debian stable package e.g. The use case would be something like

#define ALSOFT_NO_PROTOTYPES
#include <AL/alext.h>
#if (ALSOFT_HEADER_PROVIDER != "openal-soft") || (ALSOFT_HEADER_REVISION < 0x20221224)
         #error "the AL headers are too old for this version of alad, download the correct headers here [link]"
#endif

It doesn't look valid to do string compares in the preprocessor like that. Though if you're checking ALSOFT_HEADER_REVISION anyway, the header that provides it is (if not OpenAL Soft) at least trying to have compatibility with it, making the actual provider unnecessary here.

So yes, it's non standard, but given the standard has been there a long time unchanging I think it's fine, especially if it's a different prefix, I realized that having AL_ at the beginning may not have been a great idea here.

Perhaps. Would such a revision value need to be specific per-file? In case, for example, someone mixes al.h and alc.h from different versions/providers. I don't imagine something like that would happen on accident, though, so maybe it's not worth worrying about.

  1. I completely overlooked the context dependence of alGetProcAddress, maybe because I came from Vulkan and it's the other way around there. It does say in paragraph 7.2 of the specification (the openal-1.1-specification.pdf from Creative) that you're supposed to use alGetProcAddress to load core API pointers from the DLL [...] [...] However, if you're right and I cant trust any pointers from alGetProcAddress, even for core functions, then i would have to change the code in aladLoadAL, but that also would mean the spec is wrong/ inconsistent. That seems somewhat worrying to me.

Being that OpenAL is modeled heavily after OpenGL, where ALC takes the place of WGL/GLX/etc system layers, I think the behavior of alcGetProcAddress is supposed to mirror that of wgl/glXGetProcAddress. I'm not completely sure what purpose alGetProcAddress would serve in that instance though, since there isn't an equivalent glGetProcAddress.

What I can say is that Creative's "generic" implementation, at least the last version they had open source, is a mess here. For both OpenAL Soft and OSX's open source code I have, alcGetProcAddress and alGetProcAddress are completely equivalent and will return the same thing regardless of what device or context (if any) is used. And when using the router, alGetProcAddress uses the current context to know which driver to dispatch the real alGetProcAddress call to (and alcGetProcAddress uses the given device handle to know which driver to dispatch the real alcGetProcAddress call to). So with the router, the returned functions will be device-dependent (either the given device handle for alcGetProcAddress, or the current context's device for alGetProcAddress).

With Creative's generic wrapper driver (wrap_oal.dll, Generic Hardware/Software devices) however, alcGetProcAddress only returns alc functions and alGetProcAddress only returns al (not alc) functions. It's possible this changed after it went closed source, but I can't test right now. I also don't know how their hardware-specific driver (ct_oal.dll) behaves, or how the Sensaura driver (sens_oal.dll) they've started to use behaves.

Despite the language in the spec, it doesn't make much sense to have context-specific function pointers. There's no equivalent to OpenGL, I can't see a reason for them, and I don't know of any implementation that uses them. Device-specific function pointers make sense though, since even in a single driver like ct_oal.dll, there could be different functions for the different devices it supports; essentially acting as a self-contained router for built-in hardware-specific implementations. And with a router, different devices can be handled by different drivers, so a function returned for one device can't be guaranteed to work with a different device (or different device's context).

So at minimum, what I'd do is have aladLoadAL load the library, then get the ALC and AL core functions using GetProcAddress/dlsym. The retrieved functions no longer need to be changed afterward since they'll work with all devices and contexts (just as if you linked it at build-time), until aladTerminate is called and aladLoadAL is called again where the library may end up at a different memory address. Consequently, there's no need for an initial_loader parameter. This should work for everything and get you core functionality.

What to do next depends on whether or not you want to put up with wrap_oal.dll's behavior of splitting alcGetProcAddress and alGetProcAddress, if it hasn't been fixed. If not, then you only need an aladUpdateExtensionPointers(ALCdevice *device) function that loads ALC and AL extension (not core) functions with alcGetProcAddress and the given device handle, which the user calls after opening a new device. Then the user can start checking/using ALC extensions right away, create/set a context with that device, and check/use AL extensions without any further work.

If wrap_oal.dll does still have that issue, and you care about supporting it, then you'll need to split up the function. An aladUpdateALCExtensionPointers(ALCdevice *device) function would be largely the same as the previous option, something the user calls after opening a new device that uses alcGetProcAddress and the given device handle, except it only loads ALC (not AL) extension functions. And also an aladUpdateALExtensionPointers(void) function that the user calls after setting a different context as current, and that uses alGetProcAddress to load AL (not ALC) extension functions (there's no need for it to take a ALCcontext* parameter since it uses the current context).

hypatia-of-sva commented 1 year ago

Hello kcat,

I've implemented your idea in a new simplified interface, now its just

void aladLoadAL(); void aladUpdateAL(); void aladTerminate();

where the second function updates based on the device of the current context and the first loads from the shared library. I've left all the other options (which now are quite many) in the manual interface, because the code is quite small and it might be interesting for diagnostics to be able to print out the function pointers, although its not recommended for anything else (so its more a troubleshooting option).

And yes, obviously checking for string equality is a mistake in C, as you said it's unnecessary anyway if the symbol ALSOFT_HEADER_REVISION is defined. I don't think it has to be in every file either. I'd put it in alext.h only, because the other files haven't changed much, and all the symbols you defined in the extensions are there anyway, so thats the most natural place to put it in, maybe in the line above including al.h and alc.h. The bigger change I was talking about would be the #ifndef guards though, since they actually have to be put in al.h and alc.h to prevent the prototypes polluting the namespace, so that's where it's more of a question if that is in some conflict with the standard.

Hypatia of Sva.

hypatia-of-sva commented 1 year ago

Hello kcat,

I've seen you added the AL_NO_PROTOTYPES and ALC_NO_PROTOTYPES macros, thanks for that! I updated alad with it and it all works now without doubling the definition of the enum values. Even without the versioning macro, it's all fine for now, because with old headers alad will fail compilation due to double definition anyways, so thats more an open thing for when there are new extensions. Until then, it seems all fine. It's been great to see how this all went along, and I think the loader is now where I wanted it to be.

Hypatia of Sva.

hypatia-of-sva commented 1 year ago

Hello kcat,

one small addendum. While testing my compile options, I noticed an error with the AL headers on -Wpedantic. alGetProcAddress and alcGetProcAddress should return function pointers, but they return void pointers and leave the warning "ISO C forbids conversion of object pointer to function pointer type [-Wpedantic]". This means code using these functions just wouldn't compile under -Wpedantic -Werror. I've "fixed it" in alad by recasting alGetProcAddress and alcGetProcAddress as well as dlsym (because the loader library has the same problem) to an appropriate compatibility typedef and it works fine now. I just wanted you to know in case that was unintentional.

Hypatia of Sva.

kcat commented 1 year ago

That's how the headers have always been, and unfortunately can't be changed without breaking source compatibility since existing code can assign the immediate result to a void*. The way around it in C99 is to use an in-place union cast (and C++ just needs a reinterpret_cast), which can be wrapped in a macro like:

#ifdef __cplusplus
#define FUNCTION_CAST(T, ptr) reinterpret_cast<T>(ptr)
#elif __STDC_VERSION__ >= 199901L
#define FUNCTION_CAST(T, ptr) ((union{void *p; T f;}){ptr}.f)
#else
/* Fallback to a regular cast if nothing else. */
#define FUNCTION_CAST(T, ptr) ((T)(ptr))
#endif

Then do the cast through the macro:

alGenFilters = FUNCTION_CAST(LPALGENFILTERS, alGetProcAddress("alGenFilters"));
hypatia-of-sva commented 1 year ago

Hello kcat,

good to know there is a general workaround. I suspected it was a general problem when libdl had the problem too, it just comes with old C libraries I guess

Edit: while writing this I accidentally closed the issue. I guess there is nothing much open about it anyway, so if you're okay with it I'll keep it at that and will write in a new issue if there is anything new to discuss.

Hypatia of Sva.