search

libxmp / libxmp

Libxmp is a library that renders module files to PCM data.
314 stars 70 forks source link

Pre-built libxmp Emscripten builds? #771

Open AliceLR opened 1 week ago

AliceLR commented 1 week ago

Should we be uploading pre-built Emscripten packages with the source releases?

See #769, a bug report for libxmp 4.4.1 caused by their upstream not being updated recently: https://bitbucket.org/wothke/libxmp-4.4.1/src/master/.

Looking into packaging 4.6.0 for the reporter now, but we'll see how that goes.

edit: looks like we're linking Emscripten as a .a instead of a .js module, which will need to be resolved. edit: that libxmp fork also exports a completely different interface to ours.

AliceLR commented 1 week ago

Patch to add jsmodule Makefile target to 4.6.0: https://github.com/libxmp/libxmp/compare/libxmp-4.6.0...AliceLR:jsmodule-4.6.0

The xmp.h butchery was required to get EMSCRIPTEN_KEEPALIVE, I haven't investigated it beyond that.

edit: note that this isn't guaranteed to be usable. The extra shims may very well be necessary.

sezero commented 1 week ago

The xmp.h butchery was required to get EMSCRIPTEN_KEEPALIVE, I haven't investigated it beyond that.

It really is butchery. Does it not get EMSCRIPTEN_KEEPALIVE without that? Does it help at all if we change EMSCRIPTEN check to __EMSCRIPTEN__ instead?

AliceLR commented 1 week ago

It really is butchery. Does it not get EMSCRIPTEN_KEEPALIVE without that? Does it help at all if we change EMSCRIPTEN check to __EMSCRIPTEN__ instead?

After a little more experimentation, only moving the Emscripten EMSCRIPTEN_KEEPALIVE handler before the __GNUC__ handler is required to get around this. Regardless of whether I configure static or shared, link with LOBJS or ALL_OBJS, I get an empty module unless I make this change. (Patch updated to reflect this.)

edit: aside from this xmp.h hack there's another blocker here I think, which is that the Emscripten module might need its own API shims. IIRC when I made my zip module, some types of passing data just did not work correctly and I had to add shims to allocate return buffers around extraction calls. This is annoying to test either way and the aforementioned user didn't want to use the module I prepared regardless, so I'm going to delay this to next release for now.

sezero commented 1 week ago

It really is butchery. Does it not get EMSCRIPTEN_KEEPALIVE without that? Does it help at all if we change EMSCRIPTEN check to __EMSCRIPTEN__ instead?

After a little more experimentation, only moving the Emscripten EMSCRIPTEN_KEEPALIVE handler before the __GNUC__ handler is required to get around this. Regardless of whether I configure static or shared, link with LOBJS or ALL_OBJS, I get an empty module unless I make this change. (Patch updated to reflect this.)

edit: aside from this xmp.h hack there's another blocker here I think, which is that the Emscripten module might need its own API shims. IIRC when I made my zip module, some types of passing data just did not work correctly and I had to add shims to allocate return buffers around extraction calls. This is annoying to test either way and the aforementioned user didn't want to use the module I prepared regardless, so I'm going to delay this to next release for now.

The EMSCRIPTEN_KEEPALIVE thing was discussed previously in https://github.com/libxmp/libxmp/pull/245 (CC: @Wohlstand). The thing is, looking at SDL, it only relies on visibility attributes and doesn't special-case Emscripten. Do we truly need to special-case Emscripten here and do we not need visibility attributes for it? The only difference between SDL and libxmp is that libxmp has two data exports whereas SDL does not: do we need EMSCRIPTEN_KEEPALIVE only for data exports? (Sorry that I don't know Emscripten myself..)

AliceLR commented 1 week ago

Speaking off the cuff and without knowledge of whatever SDL is doing specifically: I suspect the difference here is SDL is being built as a library (similar to what libxmp currently supports) which can be statically linked into another Emscripten project using -s USE_SDL=2 etc.

EMSCRIPTEN_KEEPALIVE, on the other hand, is for exporting a JavaScript module that can be used by JavaScript rather than by a program compiled with Emscripten. This macro guarantees that, when a module is exported, all functions marked with it are also exported. If this macro is NOT used, the functions will be optimized out during export, and you'll end up with a small and mostly empty module. (I'm not sure how this macro affects linking via Emscripten, but I don't think it hurts.)

The 4.4.1 fork I linked above uses the other feature you can use to keep functions alive during module export, which is -s EXPORTED_FUNCTIONS=[...]. This requires that you explicitly pass a list of functions to $(LD), which is less desirable.

IIRC when a module is used, certain features such as structs and passing a buffer to be written to are not supported, hence why shims may be necessary. My knowledge might be outdated here (assuming I wasn't just wrong to begin with).

AliceLR commented 1 week ago

Example: